AO3-5556 Don't change collections on preview - Follow up PR

[pmonfort]

Pull Request Checklist

• Have you read "How to write the perfect pull request"?
• Have you read the contributing guidelines?
• Have you added tests for any changed functionality?
• Have you added the Jira issue number as the first thing in your pull request title (e.g. AO3-1234 Fix thing)
• Do you have fewer than 5 pull requests already open? If not, please wait until they are reviewed and merged before creating new pull requests.

Issue

https://otwarchive.atlassian.net/browse/AO3-5556

Purpose

Follow-up to #4501 by @tickinginstant, adopting and completing the work from that PR.

Original PR (#4501)

• Generalize the function assign_tags_of_type to a function assign_through_association defined in ApplicationRecord that can be used for any type of association, not just tags.
• Use that function to assign collections, so that setting collection_names will only set up the changes to be performed on save instead of performing them immediately. Similarly with collections_to_add and collections_to_remove.
• Change the approve_automatically and set_anonymous_and_unrevealed callbacks in the CollectionItem class to run before_validation instead of before_save, so that we get the correct values for anonymous, unrevealed, user_approval_status, and collection_approval_status after validation.
• Make the Collectible module a concern and move it to the correct directory.
• Rewrite the Collectible function set_anon_unrevealed so that it uses the status of the collection items to compute whether the work should be anonymous/unrevealed, and make it run as an after_validation callback so that it has access to the collection item statuses set before validation.
• Rewrite BookmarksController#update to use the collection handling from Collectible, and add a function akin to WorksController#in_moderated_collection so that the message on adding a moderated collection stays the same as it was before the change.
• Stop calling save(validate: false) so much, and replace it with a reindex_item callback in the CollectionItem class. The item association on CollectionItems has touch: true, so changes to collection items will already update the work/bookmark timestamp. And indexing is the only other real purpose that calling save(validate: false) serves.

This PR

• Resolved merge conflicts with master (~2 years of divergence)
• Reorder callbacks in CollectionItem to follow Rails convention (before_validation → before_save → after_save → after_create_commit → after_destroy → after_commit)
• Fix annotation style in Collectible (# Note: → # NOTE:) per Hound/Rubocop convention
• Extract assign_through_association from ApplicationRecord into an AssociationAssignment concern, included only in Collectible and Taggable
• Extract collections_meta_tag helper in WorksHelper to replace variable assignment in works/_meta.html.erb
• Fix cucumber steps: Cancel is a link (I follow), not a button (I press)

Testing Instructions

Steps to reproduce the bug

1. Post a work
2. Edit the work
3. Add a collection to the work, especially an anonymous or unrevealed collection
4. Press "Preview"
5. Note that in the preview, the collection is listed in the work meta, and the byline is updated to "Anonymous" if the collection is anonymous (there will be no
   indication on the preview page if the collection is unrevealed)
6. Press "Cancel"
7. The collection should not be added to the work

Automated tests

• bundle exec rspec spec/models/collection_item_spec.rb spec/models/concerns/collectible_spec.rb spec/lib/works_owner_spec.rb — 65 examples, 0 failures
• bundle exec cucumber features/collections/work_preview_collections.feature — 6 scenarios, 6 passed

References

Follow-up to #4501

Credit

Pablo Monfort (he/him))

[brianjaustin]

To make sure I understand the flow here, why do we need the if condition?

[pmonfort]

It's a performance guard, without it, every save (even unrelated ones like editing tags) would trigger a query. If collection_items is loaded, there's a chance it was updated. It may be confusing though, I can remove it if you'd like.

[brianjaustin]

Nah, I think a comment for clarity would be enough 👍

[pmonfort]

@brianjaustin thanks for your code review! The code is ready for a second review, let me know your thoughts.

[brianjaustin]

Thanks! Nothing blocking from my side