Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
AO3-5033 Rails 5.0 upgrade #2958
Conversation
WendyBeth
and others
added some commits
Jun 8, 2017
WendyBeth
added some commits
Jul 6, 2017
| it_redirects_to(new_orphan_path(series_id: series)) | ||
| end | ||
| end | ||
| describe 'create' do | ||
| it 'renders new if the series is invalid' do | ||
| fake_login_known_user(user) | ||
| - post :create, series: {summary: ""} | ||
| + post :create, params: { series: {summary: ""} } |
| it_redirects_to_with_error(edit_series_path(series), \ | ||
| "Sorry, you cannot remove yourself entirely as an author of a series right now.") | ||
| end | ||
| xit 'allows you to change the pseuds associated with the series' do | ||
| fake_login_known_user(user) | ||
| new_pseud = create(:pseud) | ||
| - put :update, series: { author_attributes: { ids: [user.id] } }, id: series, pseud: { byline: new_pseud.byline } | ||
| + put :update, params: { series: { author_attributes: { ids: [user.id] } }, id: series, pseud: { byline: new_pseud.byline } } |
| @@ -804,6 +804,7 @@ def add_freeform_nominations(num) | ||
| } | ||
| } | ||
| } | ||
| + } |
houndci-bot
Jul 13, 2017
Indent the right brace the same as the start of the line where the left brace is.
| - post :create, tag_set_id: owned_tag_set.id, tag_set_nomination: { pseud_id: random_user.default_pseud.id, | ||
| - owned_tag_set_id: owned_tag_set.id } | ||
| + post :create, params: { tag_set_id: owned_tag_set.id, tag_set_nomination: { pseud_id: random_user.default_pseud.id, | ||
| + owned_tag_set_id: owned_tag_set.id } } |
| expect(response).to render_template('new') | ||
| end | ||
| it 'builds a new tag set nomination' do | ||
| - expect { post :create, tag_set_id: owned_tag_set.id, tag_set_nomination: { pseud_id: random_user.default_pseud.id, | ||
| - owned_tag_set_id: owned_tag_set.id } }. | ||
| + expect { post :create, params: { tag_set_id: owned_tag_set.id, tag_set_nomination: { pseud_id: random_user.default_pseud.id, |
houndci-bot
Jul 13, 2017
Parenthesize the param change { owned_tag_set.tag_set_nominations.count } to make sure that the block will be associated with the change method call.
Avoid using {...} for multi-line blocks.
Block body expression is on the same line as the block start.
| - expect { post :create, tag_set_id: owned_tag_set.id, tag_set_nomination: { pseud_id: random_user.default_pseud.id, | ||
| - owned_tag_set_id: owned_tag_set.id } }. | ||
| + expect { post :create, params: { tag_set_id: owned_tag_set.id, tag_set_nomination: { pseud_id: random_user.default_pseud.id, | ||
| + owned_tag_set_id: owned_tag_set.id } } }. |
houndci-bot
Jul 13, 2017
Align the elements of a hash literal if they span more than one line.
Expression at 898, 127 should be on its own line.
| - post :create, tag_set_id: owned_tag_set.id, tag_set_nomination: { pseud_id: random_user.default_pseud.id, | ||
| - owned_tag_set_id: owned_tag_set.id } | ||
| + post :create, params: { tag_set_id: owned_tag_set.id, tag_set_nomination: { pseud_id: random_user.default_pseud.id, | ||
| + owned_tag_set_id: owned_tag_set.id } } |
| - put :update, id: tag_set_nomination.id, tag_set_id: owned_tag_set.id, | ||
| - tag_set_nomination: { pseud_id: random_user.default_pseud.id } | ||
| + put :update, params: { id: tag_set_nomination.id, tag_set_id: owned_tag_set.id, | ||
| + tag_set_nomination: { pseud_id: random_user.default_pseud.id } } |
| @@ -1025,6 +1027,7 @@ def add_freeform_nominations(num) | ||
| } | ||
| } | ||
| } | ||
| + } |
houndci-bot
Jul 13, 2017
Indent the right brace the same as the start of the line where the left brace is.
| @@ -1079,6 +1082,7 @@ def add_freeform_nominations(num) | ||
| from_fandom_nomination: true } | ||
| } } | ||
| } } | ||
| + } |
houndci-bot
Jul 13, 2017
Indent the right brace the same as the start of the line where the left brace is.
| tag_set_id: owned_tag_set.id, | ||
| id: tag_set_nomination.id, | ||
| tag_set_nomination: { pseud_id: tag_nominator_pseud.id, | ||
| owned_tag_set_id: owned_tag_set.id } | ||
| + } |
houndci-bot
Jul 13, 2017
Indent the right brace the same as the start of the line where the left brace is.
| @@ -43,7 +43,7 @@ | ||
| end | ||
| it "should redirect to the wrangle action for that tag" do | ||
| - expect(put :mass_update, id: @fandom1.name, show: 'freeforms', status: 'unwrangled'). | ||
| + expect(put :mass_update, params: { id: @fandom1.name, show: 'freeforms', status: 'unwrangled' }). |
houndci-bot
Jul 13, 2017
Add parentheses to nested method call put :mass_update, params: { id: @fandom1.name, show: 'freeforms', status: 'unwrangled' }.
| @@ -86,7 +86,7 @@ | ||
| context "with a canonical fandom in the fandom string, a selected unwrangled character, and the same character to be made canonical" do | ||
| it "should be successful" do | ||
| - put :mass_update, id: @fandom1.name, show: 'characters', status: 'unwrangled', fandom_string: "#{@fandom1.name}", selected_tags: [@character1.id], canonicals: [@character1.id] | ||
| + put :mass_update, params: { id: @fandom1.name, show: 'characters', status: 'unwrangled', fandom_string: "#{@fandom1.name}", selected_tags: [@character1.id], canonicals: [@character1.id] } |
| @@ -96,7 +96,7 @@ | ||
| context "with a canonical fandom in the fandom string, a selected synonym character, and the same character to be made canonical" do | ||
| it "should be successful" do | ||
| - put :mass_update, id: @fandom1.name, show: 'characters', status: 'unfilterable', fandom_string: "#{@fandom2.name}", selected_tags: [@character2.id], canonicals: [@character2.id] | ||
| + put :mass_update, params: { id: @fandom1.name, show: 'characters', status: 'unfilterable', fandom_string: "#{@fandom2.name}", selected_tags: [@character2.id], canonicals: [@character2.id] } |
| @@ -5,7 +5,7 @@ | ||
| include LoginMacros | ||
| include RedirectExpectationHelper | ||
| - describe "before_filter #clean_work_search_params" do | ||
| + describe "before_action #clean_work_search_params" do |
| expect(assigns(:works)).to include(@unrestricted_work_in_collection) | ||
| expect(assigns(:works)).to include(@unrestricted_work_2_in_collection) | ||
| expect(assigns(:works)).not_to include(@unrestricted_work) | ||
| expect(assigns(:works)).not_to include(@restricted_work_in_collection) | ||
| end | ||
| it "should return filtered works when search parameters are provided" do | ||
| - get :collected, { user_id: collected_user.login, work_search: { query: "fandom_ids:#{collected_fandom2.id}" }} | ||
| + get :collected, params: { user_id: collected_user.login, work_search: { query: "fandom_ids:#{collected_fandom2.id}" }} |
ariana-paris
changed the title from
Ao3 5033 rails 5 dot 0 upgrade
to
AO3-5033 Rails 5.0 upgrade
Jul 13, 2017
sarken
added
the
Awaiting review
label
Jul 16, 2017
sarken
reviewed
Jul 26, 2017
I have a few questions and some tiny nitpicks, plus there are some spots I'm asking for additional review on (some of the spots I've marked here, but others I'm asking about in the staff Slack channel), but this looks good
| @@ -511,8 +511,7 @@ def redirect_to_comment(comment, options = {}) | ||
| end | ||
| end | ||
| - def redirect_to_all_comments(commentable, options = {}) | ||
| - default_options = {anchor: "comments"} | ||
| + def redirect_to_all_comments(commentable, options = {}) default_options = {anchor: "comments"} |
WendyBeth
Aug 1, 2017
Contributor
No. Sometimes my editor has its own opinion about things. >_> Will fix.
| @@ -524,14 +524,16 @@ def import | ||
| render(:new_import) && return | ||
| end | ||
| + importing_for_others = params[:importing_for_others] != "false" && params[:importing_for_others] |
sarken
Jul 26, 2017
Owner
@ariana-paris Since you're our importing code expert, can you double check the logic changes here? The commit is here if that's easier to follow: littlelines/otwarchive@34c7583
WendyBeth
Aug 1, 2017
Contributor
I replied in the commit to @ariana-paris. Hopefully the logic is sound.
| @@ -391,7 +391,8 @@ def delete_signup_notification(user, challenge_signup) | ||
| I18n.with_locale(Locale.find(@user.preference.preferred_locale).iso) do | ||
| mail( | ||
| to: user.email, | ||
| - subject: "[#{ArchiveConfig.APP_SHORT_NAME}] Your sign-up for #{@signup.collection.title} has been deleted" | ||
| + subject: "[#{ArchiveConfig.APP_SHORT_NAME}] Your sign-up for #{@signup.collection.title} has been deleted", | ||
| + body: "" |
sarken
Jul 26, 2017
Owner
Why is it only necessary to define body for this particular mailer? Is it because there's no existing template for delete_signup_notification? (Despite the presence of this code, we don't actually seem to send an email when a user's challenge sign-up is deleted.)
WendyBeth
Aug 1, 2017
Contributor
Yes - it needs a body. It looks like it's sent in the before_destroy callback in ChallengeSignup so I'm not sure what you mean about it not being used. ?
sarken
Aug 1, 2017
Owner
I just mean I haven't gotten one of these emails when deleting challenge sign-ups in recent memory -- the last time I did it on production was April, and because this email didn't sound familiar, I tested it manually on staging shortly after this PR was submitted. It's possible Google's spam filter is eating my emails or it's been broken for a long time and we failed to notice.
| @@ -1,3 +1,3 @@ | ||
| <% content_for :message do %> | ||
| -<%= t 'user_mailer.invite_increase_notification.html', login: style_bold(@user.login), total: @total, invitation_page: style_link("your invitations page", user_invitations_url(@user)) , archive_name: style_link(ArchiveConfig.APP_SHORT_NAME, root_path) %> | ||
| +<%= t 'user_mailer.invite_increase_notification.html', login: style_bold(@user.login), total: @total, invitation_page: style_link("your invitations page", user_invitations_url(@user)) , archive_name: style_link(ArchiveConfig.APP_SHORT_NAME, root_url) %> |
| @@ -148,10 +148,10 @@ Feature: Admin Actions to Post News | ||
| Scenario: If an admin post has characters like & and < and > in the title, the escaped version will not show on the various admin post pages | ||
| Given I am logged in as an admin | ||
| When I follow "Admin Posts" | ||
| - And I follow "Post AO3 News" | ||
| + And I follow "Post AO3 News" |
sarken
Jul 26, 2017
Owner
This lost its indenting (and the When on line 154 appears to have gained an indent. Generally, we just indent anything that's an And or a But.)
| @@ -23,7 +23,7 @@ Feature: Tag Wrangling - Unsorted Tags | ||
| And I go to the unsorted_tags page | ||
| And I follow "2" | ||
| And I press "Update" | ||
| - Then I should see "2" within ".pagination span.current" | ||
| + Then I should see "2" within ".pagination .current" |
sarken
Jul 26, 2017
Owner
Just a note for my future self and/or other reviewers that I double-checked the CSS and this (the underlying loss of the span, that is) shouldn't cause a load of styling.
| @@ -5,7 +5,7 @@ require 'resque/scheduler/tasks' | ||
| namespace :resque do | ||
| task :setup do | ||
| require 'resque' | ||
| - require 'resque/scheduler' | ||
| + require 'resque/scheduler/tasks' |
sarken
Jul 26, 2017
Owner
@zz9pzza I know we've had some trouble getting this right in the past and our tests didn't notice, so can you confirm this is right?
ariana-paris
commented on app/controllers/works_controller.rb in 34c7583
Jul 28, 2017
|
I'm puzzled by the logic here, possibly because I'm more to used to seeing (I also have plans to rip some of this out and trample on it some day, so it's not a blocker either way!) |
|
If it exists but is "false" we can stop evaluating because as far as we're concerned "false" is false-y. That's why it's first, so we can short-circuit the check as soon as we have enough information to know that the value indicates we're not importing for others. The type-checking that existed on line 528 before failed with the ugprade because the new ActionController::Parameters object in Rails 5 has changed the way it processes certain values - I think it automatically reads "false" and "1" as false when the parameters are first created. I left the type-check present to avoid false positives coming from places I can't see, since it's not always obvious how the params change once they're handed to the controller. |
ariana-paris
replied
Aug 1, 2017
|
Thank you very much for the explanation, Wendy! |
| @@ -4,6 +4,19 @@ class Creatorship < ActiveRecord::Base | ||
| before_destroy :expire_caches | ||
| + validate :unique_index |
sarken
Jul 30, 2017
Owner
This code looks good, but we were wondering why it's necessary -- is there something specific that was causing creatorships to be invalid due to a non-unique index?
WendyBeth
Aug 1, 2017
Contributor
Took me a moment to remember: I used it to make debugging easier when I was trying to figure out why creatorships were being created twice in some callbacks. This had to do with the order of the callbacks being defined that referenced associated models through join models (hence why it's in the same commit that moves the import Taggable line). Instead of an error message indicating that something had failed to save, I wanted an error message that told me explicitly why, and writing this is how I found it.
Since it's valid and indicates what's required explicitly I never removed it.
| + end | ||
| + end | ||
| + | ||
| + def notify_user_of_own_comments?(user) |
sarken
Jul 31, 2017
Owner
It looks like this was a protected method when we were using an observer. Is it possible to have this (and the other formerly-protected methods) be protected here in the model? Or does moving them to the model mean we shouldn't worry about protecting them?
(Here's the full list of methods that were protected in the observer for quick reference: notify_parent_comment_owner(comment), have_different_owner?(comment, parent_comment), not_user_commenter?(parent_comment), content_too_different?(new_content, old_content), add_feedback_to_inbox(user, comment), update_feedback_in_inbox(user, comment), notify_user_by_email?(user), notify_user_by_inbox?(user), notify_user_of_own_comments?(user))
WendyBeth
Aug 1, 2017
Contributor
Whoops! I'll get these protected again. There's nothing that should prevent them from being so.
WendyBeth commentedJul 13, 2017
Pull Request Checklist
AO3-1234 Fix thing)Issue
https://otwarchive.atlassian.net/browse/AO3-5033
Purpose
What does this PR do?
Upgrades application from 4.2 to 5.0.
There are a handful of deprecations not addressed in this PR:
alias_method_chain - there is a file, lib/plugins/livevalidation/form_helpers.rb which will need to be restructured in order to remove alias_method_chain and replace it with Module#prepend. I've held off on this as I believe the test failures it causes in 5.1 will be useful in restructuring (instead of attempting to do so blind).
The dynamic :controller/:action segments in routes will not be outdated until 5.2.
Some others, left alone simply because I believe the test failures in 5.1 will make it easier to fix them.
Testing
How can the Archive's QA team verify that this is working as you intended? (If you have access, please copy this into the JIRA ticket for them!)
Due to the extensive nature of these changes, a full walkthrough in the staging environment is likely necessary to ensure everything is working as expected.
References
Are there any other relevant issues / PRs / mailing lists discussions related to this?
Credit
What name do you want us to use to credit your work in the Archive of Our Own's Release Notes?
Wendy Randquist - Littlelines
What pronouns do you prefer (she/her, he/him, zie/hir etc)?
she/her
(if you do not fill this in, we will use your GitHub account name and will use "they" to avoid making assumptions about your gender)