AO3-5556 Don't change collections on preview - Follow up PR by pmonfort · Pull Request #5820 · otwcode/otwarchive

pmonfort · 2026-05-15T23:36:17Z

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

Post a work
Edit the work
Add a collection to the work, especially an anonymous or unrevealed collection
Press "Preview"
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)
Press "Cancel"
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))

I think it's pre-existing, but it's vaguely related, so I might as well fix it.

- Extract AssociationAssignment into its own concern - Restore update_bookmarker_collections_index in Bookmarkable - Fix set_anon_unrevealed for new records in Collectible - Reorder CollectionItem callbacks per rubocop-rails 2.12.4 - Normalize locale files and add missing i18n keys

- Update specs to use item: instead of work: for CollectionItem - Fix stale tags cache in rake spec with collection.reload - Update bookmark controller spec for simplified flash message - Fix cucumber scenario: remove non-existent flash assertion

brianjaustin · 2026-06-01T20:30:10Z

+    return if collections_data.blank?
+
+    content_tag(:dt, t("works_helper.collections_meta_tag.collections"), class: "collections") +
+      "\n".html_safe +


Is there a way to join these with a newline that doesn't need html_safe? (Not super blocking if there isn't a nice way, but since we're in the neighbourhood...) Maybe content_tag(:br)?

brianjaustin · 2026-06-01T20:51:55Z

+      distinct.joins(:approved_collection_items).merge(collection.all_items)
+    }
+
+    after_validation :set_anon_unrevealed, if: -> { collection_items.loaded? }


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

brianjaustin · 2026-06-02T20:29:10Z

+
+    # approve with the current user, who is the person who has just
+    # added this item -- might be either moderator or owner
+    approve(User.current_user)


Do we still need to check if this is :false? iirc there's some legacy code that sets it to that 🥲

brianjaustin · 2026-06-02T20:40:57Z

        FactoryBot.create(:work, collection_names: @owner.name)
-        @owner.collection_items.each {|ci| ci.approve(nil); ci.save}
-        @child.collection_items.each {|ci| ci.approve(nil); ci.save} if @child
        expect(@original_cache_key).not_to eq(@owner.works_index_cache_key)


Are these deletions because the approval is automatic now? Or some other reason?

brianjaustin · 2026-06-02T20:41:43Z

-                      "Added to collection(s): #{collection.title}. " \
-                      "Please note: private bookmarks are not listed in collections."
-        it_redirects_to_with_notice(bookmark_path(bookmark), success_msg)
+        it_redirects_to_with_notice(bookmark_path(bookmark), "Bookmark was successfully updated.")


This doesn't seem to match the description of the test -- is the test useful anymore?

brianjaustin · 2026-06-02T20:42:57Z

      success: Bookmark was successfully created. It should appear in bookmark listings within the next few minutes.
      warnings:
        private_bookmark_added_to_collection: " Please note: private bookmarks are not listed in collections."
+    new_moderated_collections_message_html:


Why does this (and the other key) need _html? I don't see any HTML getting injected 🤔

brianjaustin · 2026-06-02T20:44:43Z

+
+    message = t("bookmarks.new_moderated_collections_message_html",
+                count: new_moderated_collections.length,
+                collections: view_context.safe_join(links, ", "))


Does this need to be collections_html? (Related to my question about the parent key name)

brianjaustin · 2026-06-02T20:47:58Z

-    if moderated_collections.present?
-      flash[:notice] ||= ''
-      flash[:notice] += ts(" You have submitted your work to #{moderated_collections.size > 1 ? 'moderated collections (%{all_collections}). It will not become a part of those collections' : "the moderated collection '%{all_collections}'. It will not become a part of the collection"} until it has been approved by a moderator.", all_collections: moderated_collections.map(&:title).join(', '))
+  def in_moderated_collection(flash = self.flash)


in_moderated_collection sounds like it would return a boolean to me; maybe show_moderated_collection_warning would be more accurate?

tickinginstant added 6 commits May 14, 2026 00:52

AO3-5556 Don't change collections on preview.

a66de5d

AO3-5556 For Hound.

7776fdd

AO3-5556 Add test for unmark_for_destruction.

947523a

AO3-5556 Fix bad merge.

a618435

AO3-5556 Better fix.

fc6ef09

AO3-5556 Fix intermittent test failure.

fe7fe28

I think it's pre-existing, but it's vaguely related, so I might as well fix it.

github-actions Bot added the Awaiting Review label May 15, 2026

pmonfort marked this pull request as draft May 16, 2026 01:22

pmonfort removed the Awaiting Review label May 16, 2026

pmonfort force-pushed the AO3-5556-collection-persists-on-preview-cancel branch 3 times, most recently from 0e88cf1 to 809c2e6 Compare May 17, 2026 01:15

pmonfort added 3 commits May 17, 2026 02:01

AO3-5556 Extract collections_meta_tag helper

81be419

AO3-5556 Fix test and feature failures

4ef0c3f

- Update specs to use item: instead of work: for CollectionItem - Fix stale tags cache in rake spec with collection.reload - Update bookmark controller spec for simplified flash message - Fix cucumber scenario: remove non-existent flash assertion

pmonfort force-pushed the AO3-5556-collection-persists-on-preview-cancel branch from 809c2e6 to 4ef0c3f Compare May 17, 2026 20:09

pmonfort marked this pull request as ready for review May 17, 2026 20:37

brianjaustin added the Awaiting Review label May 19, 2026

brianjaustin reviewed Jun 2, 2026

View reviewed changes

brianjaustin added Reviewed: Action Needed and removed Awaiting Review labels Jun 2, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

AO3-5556 Don't change collections on preview - Follow up PR#5820
pmonfort wants to merge 9 commits into
otwcode:masterfrom
pmonfort:AO3-5556-collection-persists-on-preview-cancel

pmonfort commented May 15, 2026

Uh oh!

brianjaustin Jun 1, 2026

Uh oh!

brianjaustin Jun 1, 2026

Uh oh!

brianjaustin Jun 2, 2026

Uh oh!

brianjaustin Jun 2, 2026

Uh oh!

brianjaustin Jun 2, 2026

Uh oh!

brianjaustin Jun 2, 2026

Uh oh!

brianjaustin Jun 2, 2026

Uh oh!

brianjaustin Jun 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

pmonfort commented May 15, 2026

Pull Request Checklist

Issue

Purpose

Original PR (#4501)

This PR

Testing Instructions

Steps to reproduce the bug

Automated tests

References

Credit

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants