-
Notifications
You must be signed in to change notification settings - Fork 467
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.
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
AO3-4808 Display an error if fandom is removed when editing tags #2808
AO3-4808 Display an error if fandom is removed when editing tags #2808
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready when Travis is happy!
app/controllers/works_controller.rb
Outdated
@@ -455,6 +455,7 @@ def update | |||
end | |||
|
|||
def update_tags | |||
set_work_tag_error_messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning will be shown if I remove all fandom tags then hit "Cancel".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you -- good catch! Taking the Ready to Merge label off and putting Needs Coder Action on.
892cd41
to
205da81
Compare
flash[:notice] = ts('Tags were successfully updated.') | ||
redirect_to(@work) | ||
else | ||
if @work.has_required_tags? && @work.invalid_tags.blank? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid more than 3 levels of block nesting.
Convert if nested inside else to elsif.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use unless @work.errors.empty?
and avoid nesting.
@@ -306,7 +306,7 @@ def create | |||
render :new && return | |||
elsif params[:cancel_button] | |||
flash[:notice] = ts('New work posting canceled.') | |||
redirect_to current_user && return | |||
redirect_to current_user and return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use && instead of and.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. This had actually broken that code path so my change fixes this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No change needed, just noting that we use redirect_to(current_user) && return
in some places to get around this warning.
205da81
to
5e13eee
Compare
5e13eee
to
715fbf9
Compare
@work.minor_version = @work.minor_version + 1 | ||
@work.save | ||
flash[:notice] = ts('Work was successfully updated.') | ||
redirect_to(@work) and return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use && instead of and.
Looks good to me. |
Issue
https://otwarchive.atlassian.net/browse/AO3-4808
Purpose
Fixes the Broken on Test issue where no message appears when using Edit Tags to remove the fandom.
Testing
See Jira.