Display feature pets on org home page [558] #660
Conversation
|
FYI tests failures and need to run the linter. |
|
@all-contributors |
|
I've put up a pull request to add @VonTeacher! 🎉 |
|
@VonTeacher let me know if you have any questions! |
|
@VonTeacher hey are you still able to finish this? Please let me know in the next couple of days. I usually re-assign issues that sit without communication for a week or more. Thanks :) |
|
Thanks for the reminder and apologies for the delay. I'll spend some time Edit: I updated the test file, taking into account your comments on the script and resolving conflicts that occurred as a result of #618. Awaiting workflow approval and any other comments you might have had on the |
|
@VonTeacher No worries at all! Just making sure you have what you need and still able to work on it. |
| @@ -12,7 +13,14 @@ class Organizations::HomeControllerTest < ActionDispatch::IntegrationTest | |||
| get home_index_path(script_name: "/#{@organization.slug}") | |||
|
|
|||
| assert_response :success | |||
| assert_select "title", text: "#{@organization.name} | \"Pet Rescue\"" | |||
| assert_select "title", text: "#{ActsAsTenant.current_tenant.name} | \"Pet Rescue\"" | |||
There was a problem hiding this comment.
This is failing. Although we assert specific dom elements in this app's test suite, I am recommending we steer away from it as it couples the test suite too tightly with the dom and leads to test failures whenever we touch the dom in the future.
I personally don't think we need to test this particular feature, but if we wanted to test something, I would just test the index action on the controller returns a collection of 4 pets that belong to the organization set in the test context. However, unless someone thinks otherwise, I vote we just remove the test changes here altogether. We can always add a test later if this feature becomes more complex.
| @@ -4,6 +4,7 @@ class Organizations::HomeControllerTest < ActionDispatch::IntegrationTest | |||
| setup do | |||
| @organization = create(:organization, :with_page_text) | |||
| @organization.save | |||
| @pets = create_list(:pet, 4, :with_image, organization_id: @organization.id) | |||
There was a problem hiding this comment.
Should we need this? The Index action should be performing a query to retrieve 4 pets from the given Org, and we should be able to access that via hitting the index action and accessing the instance var within a controller test? If we delete these test changes per my other comment, then this is a non-issue.
|
@VonTeacher let me know if you can update re my feedback above. If not, I can also finish this off if you're busy. Let me know :) |
|
@kasugaijin (and @VonTeacher) might we want to merge this PR this weekend? |
|
@maebeale yes we can it just needs the tests removing or updating. I personally don’t think m this needs to be tested. |
|
Resolved by #784 |
🔗 Issue
Display feature pets on org home page #558
✍️ Description
Organizations::HomeController#indexaction to sample from the adoptable_pets (though I'm not sure the pattern matches what is used in other controllers)./<org>/homepage with a sample of the adoptable pets from that organization.📷 Screenshots/Demos
Before:
After: