Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Milestone 8: views integration specs and fix n+1 problem on 'post#index' action #8

Merged

Conversation

ITurres
Copy link
Owner

@ITurres ITurres commented Dec 12, 2023

Pull Request Summary for Milestone 8 Completion


Added:

/spec/views/post_index_spec.rb

  • @ITurres: Integration tests for the Post index view were added according to the requirements of the project

/spec/views/post_show_spec.rb

  • @ITurres: Integration tests for the Post show view were added according to the requirements of the project.

/spec/views/user_index_spec.rb

  • @demesameneshoa: Integration tests for the User index view were added according to the requirements of the project.

/spec/views/user_show_spec.rb

  • @demesameneshoa: Integration tests for the User show view were added according to the requirements of the project.

Modified:

/README.md

  • Marked the Integration specs for Views and fixing n+1 problems task as completed.
  • Added @demesameneshoa as a contributor.

/app/models/user.rb

  • @ITurres && @demesameneshoa: The includes method was added to the User model to avoid n+1 queries at the most_recent_posts method.

📸 Screenshots of n+1 queries before and after the changes:

Before:

blog_app-posts#index-n+1-problem

After:

blog_app-posts#index-n+1-problem-fixed

/app/views/posts/_posts.html.erb

  • @ITurres: I have given a dynamic id for each post link.
    This way, we can test specific posts when we run the integration tests.

/app/views/shared/_user_card.html.erb

  • @ITurres: I have remove the capitalize method from the user.name attribute.
    Since before it was downcasing the rest of the name. For example, Arthur Emanuel was being displayed as Arthur emanuel. Making the integration tests fail, because the user.name attribute was not matching the user.name attribute from the Post model.

/db/seeds.rb

/spec/examples.txt


Thank you for reviewing this PR. Feel free to reach out on Slack as Arturo (Arthur) Emanuel Guerra Iturres for any queries or further assistance. 🌟

@ITurres ITurres added documentation Improvements or additions to documentation enhancement New feature or request i-tests RSpec/Capybara Integration Tests labels Dec 12, 2023
@ITurres ITurres marked this pull request as ready for review December 12, 2023 22:53
Copy link

@brainconnect93 brainconnect93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status: Approved ✔️

Hi @ITurres & @demesameneshoa, 👋 👋

Your project is complete! Great job for following the project requirement🥇. There is nothing else to say other than... it's time to merge it 🍾🚢 :shipit: . Congratulations! 🎉💯🌟

Good Points 👍

  • Descriptive Pull Request title & summary. ✔️
  • integration specs for views and fix n+1 problems ✔️
  • Good use of Git flow ✔️

Optional changes

Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.

Cheers..Happy coding!..and keep soaring higher! 💻 🍷 🚀

Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me @brainconnect93 in your question so I can receive the notification.

Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.


As described in the Code reviews limits policy you have 2 more limited reviews per this project. If you think that the code review was not fair, you can request a second opinion using this form.

@ITurres
Copy link
Owner Author

ITurres commented Dec 12, 2023

Hi @brainconnect93 Afolabi!

Thank you so much for having reviewed our PR 😃 🥂 and for contacting me over Zoom to check tests and snapshots. Wish you a great week and happy programming!

@ITurres ITurres merged commit 7488531 into development Dec 12, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request i-tests RSpec/Capybara Integration Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants