Skip to content

Conversation

@Yanpei-Wang
Copy link
Contributor

@Yanpei-Wang Yanpei-Wang commented Oct 19, 2025

Issue

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

Purpose

The purpose of this PR is to fix CreatorshipsController#update so that when users don’t select any Co-Creator Requests, a warning message is shown instead of an empty notice banner.

Credit

Yanpei Wang

Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, I have a few suggestions!

Comment on lines 20 to 22
selected_ids = Array.wrap(params[:selected]).reject(&:blank?)
if selected_ids.blank?
flash[:caution] = t(".must_select_request")
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be simpler and less fragile to potential changes to always call @creatorships.where(id: params[:selected]) and decide whether to show the message based on the return value of that, instead of this extra Array check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestions! They are helpful.

end

context "without selecting any requests" do
it "displays an caution" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it "displays an caution" do
it "displays a caution" do

it "displays an caution" do
fake_login_known_user(user)
put :update, params: { user_id: user.login }
expect(flash[:caution]).to eq("Please select at least one co-creator request first.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use it_redirects_to_with_caution instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants