AO3-5023 Fix collections displaying too many times for works #2940
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
June 14, 2017 10:29
…-5023_rails_4_dot_2_bugfix
houndci-bot
reviewed
Jun 15, 2017
| .where( | ||
| collection_items: { | ||
| user_approval_status: CollectionItem::REJECTED | ||
| } | ||
| ) | ||
| .references(:collection_items) |
There was a problem hiding this comment.
Place the . on the previous line, together with the method call receiver.
| .where( | ||
| collection_items: { | ||
| user_approval_status: CollectionItem::APPROVED | ||
| } | ||
| ) | ||
| .references(:collection_items) |
There was a problem hiding this comment.
Place the . on the previous line, together with the method call receiver.
| .where( | ||
| collection_items: { | ||
| user_approval_status: CollectionItem::APPROVED, | ||
| collection_approval_status: CollectionItem::APPROVED | ||
| } | ||
| ) | ||
| .references(:collection_items) |
There was a problem hiding this comment.
Place the . on the previous line, together with the method call receiver.
houndci-bot
reviewed
Jun 15, 2017
| .where( | ||
| collection_items: { | ||
| user_approval_status: CollectionItem::REJECTED | ||
| } | ||
| ) | ||
| .references(:collection_items) |
There was a problem hiding this comment.
Place the . on the previous line, together with the method call receiver.
| .where( | ||
| collection_items: { | ||
| user_approval_status: CollectionItem::APPROVED | ||
| } | ||
| ) | ||
| .references(:collection_items) |
There was a problem hiding this comment.
Place the . on the previous line, together with the method call receiver.
| .where( | ||
| collection_items: { | ||
| user_approval_status: CollectionItem::APPROVED, | ||
| collection_approval_status: CollectionItem::APPROVED | ||
| } | ||
| ) | ||
| .references(:collection_items) |
There was a problem hiding this comment.
Place the . on the previous line, together with the method call receiver.
sarken
reviewed
Jun 16, 2017
sarken
added
the
Priority: High - Broken on Test
sarken
changed the title
sarken
added
Reviewed: Ready to Merge
Priority: High - Broken on Test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist
AO3-1234 Fix thing)Issue
https://otwarchive.atlassian.net/browse/AO3-5023
Purpose
What does this PR do?
This PR fixes the issue where the same collection was showing up multiple times on a given work's collections page.
The first fix is using
.includesinstead of.joinsin collection scopes that should return collections (not collection items) based on their approval status.This raised a bug caused by queries in the collections and works controllers that use
.includes(:approved_collections), which is including a scoped set of collections based on a join table in the includes - this tends to blow up when combined with other scopes. I've fixed this by explicitly spelling out how the:approved_collectionswork - to.includes(collections: [:collection_items]).where(collection_items: { user_approval_status: CollectionItem::APPROVED, collection_approval_status: CollectionItem::APPROVED }).Work.includes(:approved_collections).references(:collection_items)will run the expected query (although that could cause some confusion because it's not precisely clear why thereferences(:collection_items)has to be there without context).Work.includes(:approved_collections).references(:item_collections).in_collection(@collection)(and other variations, rearranging the order the methods are chained in or adding different scopes like the ones present in the two controllers) blow up with the errorActiveRecord::AssociationNotFoundError: Association named 'collection_items' was not found on CollectionItem. I'm happy to talk more about this if there are concerns with the solution in this PR.Testing
How can the Archive's QA team verify that this is working as you intended? (If you have access, please copy this into the JIRA ticket for them!)
Locally -
In staging - go to http://test.archiveofourown.org/works/1064145/collections and ensure that the collection ModeratedUnrevealedTwoWorks shows up only once.
References
Are there any other relevant issues / PRs / mailing lists discussions related to this?
Credit
What name do you want us to use to credit your work in the Archive of Our Own's Release Notes?
Wendy Randquist - Littlelines
What pronouns do you prefer (she/her, he/him, zie/hir etc)?
she/her
(if you do not fill this in, we will use your GitHub account name and will use "they" to avoid making assumptions about your gender)