Skip to content

AO3-7211 Add a warning to the success message when directly adding existing bookmark to collection #5481

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.

Sign up for GitHub

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

Open
germpy wants to merge 4 commits into
base: master
Choose a base branch
from
Open

AO3-7211 Add a warning to the success message when directly adding existing bookmark to collection #5481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3d9ff43
AO3-7211 : adds warning when adding a new to collection.
germpy Nov 22, 2025
0953eb1
Added expected warning to automated tests messages.
germpy Nov 22, 2025
6ab5835
AO3-7211: Added expected warning to automated tests messages.
germpy Nov 22, 2025
2e432b0
Merge branch 'AO3-7211' of https://github.com/germpy/otwarchive into …
germpy Nov 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/controllers/collection_items_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ def create
unless new_collections.empty?
flash[:notice] = ts("Added to collection(s): %{collections}.",
collections: new_collections.collect(&:title).join(", "))
flash[:notice] += t("bookmarks.create.warnings.private_bookmark_added_to_collection")
Copy link
Member

@omerfaruk-pseud omerfaruk-pseud Nov 24, 2025

Choose a reason for hiding this comment

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

Per our internationalization standards, you should avoid reusing the exact same locale key across different contexts, and use lazy lookup. Can you change this locale key to a new one and add it to locale file?

Copy link
Member

@omerfaruk-pseud omerfaruk-pseud Nov 24, 2025

Choose a reason for hiding this comment

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

And also, in this situation the note wouldn't be added to endings of invited and unapproved collections' success messages. Can you move this line to a bit lower, just before where the message is created?

Copy link
Author

@germpy germpy Nov 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yep! Just to clarify, should I move it to replace the empty message in line 137 (which is only assigned if new_collections and unapproved_collections are empty), or after the unless statements at around line 155?

Copy link
Member

@omerfaruk-pseud omerfaruk-pseud Nov 26, 2025

Choose a reason for hiding this comment

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

The latter, line 155, (though what line 137 does is kinda the opposite of what you said.)

end
unless invited_collections.empty?
invited_collections.each do |needs_user_approval|
Expand Down
4 changes: 2 additions & 2 deletions spec/controllers/collection_items_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,15 +358,15 @@
context "when the item's creator does not allow collection invitations" do
it "adds the item anyway" do
post :create, params: params
it_redirects_to_with_notice(work, "Added to collection(s): #{collection.title}.")
it_redirects_to_with_notice(work, "Added to collection(s): #{collection.title}. Please note: private bookmarks are not listed in collections.")
expect(work.reload.collections).to include(collection)
end
end

context "when the item's creator allows collection invitations" do
it "adds the item" do
post :create, params: params
it_redirects_to_with_notice(work, "Added to collection(s): #{collection.title}.")
it_redirects_to_with_notice(work, "Added to collection(s): #{collection.title}. Please note: private bookmarks are not listed in collections.")
expect(work.reload.collections).to include(collection)
end
end
Expand Down
Loading