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-5034 Rails 5 dot 1 upgrade #2980
Conversation
WendyBeth
and others
added some commits
Jun 8, 2017
WendyBeth
added some commits
Jul 27, 2017
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! |
WendyBeth
added some commits
Jul 31, 2017
| + before_action :load_participant_and_collection, only: [:update, :destroy] | ||
| + before_action :allowed_to_promote, only: [:update] | ||
| + before_action :allowed_to_destroy, only: [:destroy] | ||
| + before_action :has_other_owners, only: [:update, :destroy] |
| + before_action :allowed_to_promote, only: [:update] | ||
| + before_action :allowed_to_destroy, only: [:destroy] | ||
| + before_action :has_other_owners, only: [:update, :destroy] | ||
| + before_action :collection_maintainers_only, only: [:index, :add, :update] |
| - before_filter :collection_owners_only, only: [:edit, :update, :destroy, :confirm_delete] | ||
| - before_filter :check_user_status, only: [:new, :create, :edit, :update, :destroy] | ||
| - before_filter :validate_challenge_type | ||
| + before_action :users_only, only: [:new, :edit, :create, :update] |
| - before_filter :check_user_status, only: [:new, :create, :edit, :update, :destroy] | ||
| - before_filter :validate_challenge_type | ||
| + before_action :users_only, only: [:new, :edit, :create, :update] | ||
| + before_action :load_collection_from_id, only: [:show, :edit, :update, :destroy, :confirm_delete] |
| - before_filter :validate_challenge_type | ||
| + before_action :users_only, only: [:new, :edit, :create, :update] | ||
| + before_action :load_collection_from_id, only: [:show, :edit, :update, :destroy, :confirm_delete] | ||
| + before_action :collection_owners_only, only: [:edit, :update, :destroy, :confirm_delete] |
| + before_action :users_only, only: [:new, :edit, :create, :update] | ||
| + before_action :load_collection_from_id, only: [:show, :edit, :update, :destroy, :confirm_delete] | ||
| + before_action :collection_owners_only, only: [:edit, :update, :destroy, :confirm_delete] | ||
| + before_action :check_user_status, only: [:new, :create, :edit, :update, :destroy] |
| @@ -1,24 +1,24 @@ | ||
| class CommentsController < ApplicationController | ||
| - skip_before_filter :store_location, except: [:show, :index, :new] | ||
| - before_filter :load_commentable, only: [ :index, :new, :create, :edit, :update, | ||
| + skip_before_action :store_location, except: [:show, :index, :new] |
| - skip_before_filter :store_location, except: [:show, :index, :new] | ||
| - before_filter :load_commentable, only: [ :index, :new, :create, :edit, :update, | ||
| + skip_before_action :store_location, except: [:show, :index, :new] | ||
| + before_action :load_commentable, only: [ :index, :new, :create, :edit, :update, |
| - before_filter :check_permission_to_review, only: [:unreviewed] | ||
| - before_filter :check_permission_to_access_single_unreviewed, only: [:show] | ||
| - before_filter :check_permission_to_moderate, only: [:approve, :reject] | ||
| + before_action :check_user_status, only: [:new, :create, :edit, :update, :destroy] |
| - before_filter :check_permission_to_access_single_unreviewed, only: [:show] | ||
| - before_filter :check_permission_to_moderate, only: [:approve, :reject] | ||
| + before_action :check_user_status, only: [:new, :create, :edit, :update, :destroy] | ||
| + before_action :load_comment, only: [:show, :edit, :update, :delete_comment, :destroy, :cancel_comment_edit, :cancel_comment_delete, :review, :approve, :reject] |
| + before_action :check_visibility, only: [:show] | ||
| + before_action :check_if_restricted | ||
| + before_action :check_tag_wrangler_access | ||
| + before_action :check_pseud_ownership, only: [:create, :update] |
| + before_action :check_if_restricted | ||
| + before_action :check_tag_wrangler_access | ||
| + before_action :check_pseud_ownership, only: [:create, :update] | ||
| + before_action :check_ownership, only: [:edit, :update, :cancel_comment_edit] |
| + before_action :check_tag_wrangler_access | ||
| + before_action :check_pseud_ownership, only: [:create, :update] | ||
| + before_action :check_ownership, only: [:edit, :update, :cancel_comment_edit] | ||
| + before_action :check_permission_to_edit, only: [:edit, :update ] |
| + before_action :check_pseud_ownership, only: [:create, :update] | ||
| + before_action :check_ownership, only: [:edit, :update, :cancel_comment_edit] | ||
| + before_action :check_permission_to_edit, only: [:edit, :update ] | ||
| + before_action :check_permission_to_delete, only: [:delete_comment, :destroy] |
| + before_action :check_ownership, only: [:edit, :update, :cancel_comment_edit] | ||
| + before_action :check_permission_to_edit, only: [:edit, :update ] | ||
| + before_action :check_permission_to_delete, only: [:delete_comment, :destroy] | ||
| + before_action :check_anonymous_comment_preference, only: [:new, :create, :add_comment_reply] |
| + before_action :check_unreviewed, only: [:add_comment_reply] | ||
| + before_action :check_permission_to_review, only: [:unreviewed] | ||
| + before_action :check_permission_to_access_single_unreviewed, only: [:show] | ||
| + before_action :check_permission_to_moderate, only: [:approve, :reject] |
| @@ -1,5 +1,5 @@ | ||
| class InviteRequestsController < ApplicationController | ||
| - before_filter :admin_only, only: [:manage, :reorder, :destroy] | ||
| + before_action :admin_only, only: [:manage, :reorder, :destroy] |
| @@ -1,5 +1,5 @@ | ||
| class LanguagesController < ApplicationController | ||
| - before_filter :check_permission, only: [:new, :create, :edit, :update] | ||
| + before_action :check_permission, only: [:new, :create, :edit, :update] |
| @@ -1,5 +1,5 @@ | ||
| class LocalesController < ApplicationController | ||
| - before_filter :check_permission, only: [:new, :create, :update, :edit] | ||
| + before_action :check_permission, only: [:new, :create, :update, :edit] |
| - before_filter :load_external_author, only: [:show, :edit, :update, :forward] | ||
| + before_action :users_only | ||
| + before_action :opendoors_only | ||
| + before_action :load_external_author, only: [:show, :edit, :update, :forward] |
| @@ -1,8 +1,8 @@ | ||
| class OrphansController < ApplicationController | ||
| # You must be logged in to orphan works - relies on current_user data | ||
| - before_filter :users_only, except: [:index, :about] | ||
| + before_action :users_only, except: [:index, :about] |
| - before_filter :load_orphans, except: [:index, :about] | ||
| + before_action :load_orphans, except: [:index, :about] |
| - before_filter :users_only, only: [ :new, :create, :nominate ] | ||
| - before_filter :moderators_only, except: [ :index, :new, :create, :show, :show_options ] | ||
| - before_filter :owners_only, only: [ :destroy ] | ||
| + before_action :load_tag_set, except: [ :index, :new, :create, :show_options ] |
| - before_filter :moderators_only, except: [ :index, :new, :create, :show, :show_options ] | ||
| - before_filter :owners_only, only: [ :destroy ] | ||
| + before_action :load_tag_set, except: [ :index, :new, :create, :show_options ] | ||
| + before_action :users_only, only: [ :new, :create, :nominate ] |
| - before_filter :owners_only, only: [ :destroy ] | ||
| + before_action :load_tag_set, except: [ :index, :new, :create, :show_options ] | ||
| + before_action :users_only, only: [ :new, :create, :nominate ] | ||
| + before_action :moderators_only, except: [ :index, :new, :create, :show, :show_options ] |
| + before_action :load_tag_set, except: [ :index, :new, :create, :show_options ] | ||
| + before_action :users_only, only: [ :new, :create, :nominate ] | ||
| + before_action :moderators_only, except: [ :index, :new, :create, :show, :show_options ] | ||
| + before_action :owners_only, only: [ :destroy ] |
| + before_action :users_only | ||
| + before_action :load_collection, except: [:index] | ||
| + before_action :load_challenge, except: [:index] | ||
| + before_action :load_prompt_from_id, only: [:show, :edit, :update, :destroy] |
| + before_action :load_collection, except: [:index] | ||
| + before_action :load_challenge, except: [:index] | ||
| + before_action :load_prompt_from_id, only: [:show, :edit, :update, :destroy] | ||
| + before_action :load_signup, except: [:index, :destroy, :show] |
| + before_action :load_signup, except: [:index, :destroy, :show] | ||
| + # before_action :promptmeme_only, except: [:index, :new] | ||
| + before_action :allowed_to_destroy, only: [:destroy] | ||
| + before_action :signup_owner_only, only: [:edit, :update] |
| + # before_action :promptmeme_only, except: [:index, :new] | ||
| + before_action :allowed_to_destroy, only: [:destroy] | ||
| + before_action :signup_owner_only, only: [:edit, :update] | ||
| + before_action :check_signup_open, only: [:new, :create, :edit, :update] |
| - before_filter :check_ownership, only: [:create, :edit, :destroy, :new, :update] | ||
| - before_filter :check_user_status, only: [:new, :create, :edit, :update] | ||
| + before_action :load_user | ||
| + before_action :check_ownership, only: [:create, :edit, :destroy, :new, :update] |
| - before_filter :check_user_status, only: [:new, :create, :edit, :update] | ||
| + before_action :load_user | ||
| + before_action :check_ownership, only: [:create, :edit, :destroy, :new, :update] | ||
| + before_action :check_user_status, only: [:new, :create, :edit, :update] |
| end | ||
| end | ||
| end | ||
| def log_change_if_login_was_edited | ||
| - create_log_item(options = { action: ArchiveConfig.ACTION_RENAME, note: "Old Username: #{login_was}; New Username: #{login}" }) if login_changed? | ||
| + create_log_item(options = { action: ArchiveConfig.ACTION_RENAME, note: "Old Username: #{login_before_last_save}; New Username: #{login}" }) if saved_change_to_login? |
| + | ||
| + def remove_stale_from_autocomplete | ||
| + Rails.logger.debug "Removing stale from autocomplete: #{autocomplete_search_string_was}" | ||
| + self.class.remove_from_autocomplete(self.autocomplete_search_string_was, self.autocomplete_prefixes, self.autocomplete_value_was) |
| + Rails.logger.debug "Removing stale from autocomplete: #{autocomplete_search_string_was}" | ||
| + self.class.remove_from_autocomplete(self.autocomplete_search_string_was, self.autocomplete_prefixes, self.autocomplete_value_was) | ||
| + end | ||
| + |
| + before_destroy :before_destroy | ||
| + | ||
| + def before_destroy | ||
| + if self.posted? |
| + | ||
| + def before_destroy | ||
| + if self.posted? | ||
| + users = self.pseuds.collect(&:user).uniq |
| + users = self.pseuds.collect(&:user).uniq | ||
| + orphan_account = User.orphan_account | ||
| + unless users.blank? | ||
| + for user in users |
| + orphan_account = User.orphan_account | ||
| + unless users.blank? | ||
| + for user in users | ||
| + next if user == orphan_account |
| + for user in users | ||
| + next if user == orphan_account | ||
| + # Check to see if this work is being deleted by an Admin | ||
| + if User.current_user.is_a?(Admin) |
| @@ -287,7 +316,7 @@ def clean_up_creatorships | ||
| after_destroy :clean_up_filter_taggings | ||
| def clean_up_filter_taggings | ||
| - FilterTagging.destroy_all("filterable_type = 'Work' AND filterable_id = #{self.id}") | ||
| + FilterTagging.where("filterable_type = 'Work' AND filterable_id = #{self.id}").destroy_all |
| @@ -574,7 +603,7 @@ def update_minor_version | ||
| def set_revised_at(date=nil) | ||
| date ||= self.chapters.where(posted: true).maximum('published_at') || | ||
| - self.revised_at || self.created_at | ||
| + self.revised_at || self.created_at || DateTime.now |
houndci-bot
Aug 2, 2017
Align the operands of an expression in an assignment spanning multiple lines.
Redundant self detected.
| @@ -586,7 +615,7 @@ def set_revised_at_by_chapter(chapter) | ||
| if (self.new_record? || chapter.posted_changed?) && chapter.published_at == Date.today | ||
| self.set_revised_at(Time.now) # a new chapter is being posted, so most recent update is now | ||
| elsif self.revised_at.nil? || | ||
| - chapter.published_at > self.revised_at.to_date || | ||
| + (chapter.published_at && chapter.published_at > self.revised_at.to_date) || |
houndci-bot
Aug 2, 2017
Align the operands of a condition in a elsif statement spanning multiple lines.
Redundant self detected.
| @@ -660,12 +689,12 @@ def adjust_series_restriction | ||
| # Save chapter data when the work is updated | ||
| def save_chapters | ||
| - self.chapters.first.save(validate: false) | ||
| + !self.chapters.first.save(validate: false) |
| end | ||
| # If the work is posted, the first chapter should be posted too | ||
| def post_first_chapter | ||
| - if self.posted_changed? | ||
| + if self.saved_change_to_posted? || (self.chapters.first && self.chapters.first.posted != self.posted) |
| - self.complete = self.chapters.posted.count == expected_number_of_chapters | ||
| - if self.complete_changed? | ||
| - Work.where("id = #{self.id}").update_all("complete = #{self.complete}") | ||
| + if self.chapters.posted.count == expected_number_of_chapters |
| - if self.complete_changed? | ||
| - Work.where("id = #{self.id}").update_all("complete = #{self.complete}") | ||
| + if self.chapters.posted.count == expected_number_of_chapters | ||
| + Work.where("id = #{self.id}").update_all("complete = true") |
| @@ -850,7 +878,7 @@ def download_basename | ||
| def tag_groups | ||
| Rails.cache.fetch(self.tag_groups_key) do | ||
| - if self.placeholder_tags | ||
| + if self.placeholder_tags && !self.placeholder_tags.empty? |
houndci-bot
Aug 2, 2017
Use the return of the conditional for variable assignment and comparison.
Redundant self detected.
| + work = nil | ||
| + if self.respond_to?(:ultimate_parent) | ||
| + work = self.ultimate_parent | ||
| + elsif self.respond_to?(:commentable) |
| + if self.respond_to?(:ultimate_parent) | ||
| + work = self.ultimate_parent | ||
| + elsif self.respond_to?(:commentable) | ||
| + work = self.commentable |
| + work = self.ultimate_parent | ||
| + elsif self.respond_to?(:commentable) | ||
| + work = self.commentable | ||
| + elsif self.respond_to?(:bookmarkable) |
| + elsif self.respond_to?(:commentable) | ||
| + work = self.commentable | ||
| + elsif self.respond_to?(:bookmarkable) | ||
| + work = self.bookmarkable |
| + work.is_a?(Work) ? work : nil | ||
| + end | ||
| +end | ||
| + |
| @@ -132,8 +133,10 @@ def reset_placeholders | ||
| end | ||
| def validate_tags | ||
| - errors.add(:base, "Work must have required tags.") unless self.has_required_tags? | ||
| - self.has_required_tags? | ||
| + unless self.has_required_tags? |
| - errors.add(:base, "Work must have required tags.") unless self.has_required_tags? | ||
| - self.has_required_tags? | ||
| + unless self.has_required_tags? | ||
| + errors.add(:base, "Work must have required tags.") unless self.has_required_tags? |
| @@ -24,5 +24,5 @@ def count | ||
| end | ||
| count | ||
| end | ||
| - | ||
| + |
| @@ -47,15 +47,15 @@ | ||
| describe 'create' do | ||
| it 'sets a notice and redirects' do | ||
| fake_login_known_user(@user) | ||
| - post :create, collection_id: collection.name, challenge_claim: {collection_id: collection.id} | ||
| + post :create, params: { collection_id: collection.name, challenge_claim: {collection_id: collection.id} } |
| it_redirects_to_with_notice(collection_claims_path(collection, for_user: true), \ | ||
| "New claim made.") | ||
| end | ||
| it 'on an exception gives an error and redirects' do | ||
| fake_login_known_user(@user) | ||
| allow_any_instance_of(ChallengeClaim).to receive(:save) { false } | ||
| - post :create, collection_id: collection.name, challenge_claim: {collection_id: collection.id} | ||
| + post :create, params: { collection_id: collection.name, challenge_claim: {collection_id: collection.id} } |
| @@ -113,7 +113,7 @@ | ||
| it "removes things" do | ||
| @approved_work_item = CollectionItem.find_by_item_id(@approved_work.id) | ||
| fake_login_known_user(owner) | ||
| - delete :destroy, id: @approved_work_item.id | ||
| + delete :destroy, params: { id: @approved_work_item.id, work_id: @approved_work.id} |
| @@ -96,7 +96,13 @@ | ||
| context "when the user has permission through an invitation" do | ||
| context "when doing nothing with imported works" do | ||
| it "redirects with a success message" do | ||
| - put :update, invitation_token: invitation.token, id: external_author.id, imported_stories: "nothing" | ||
| + parameters = { |
houndci-bot
Aug 2, 2017
Operator = should be surrounded by a single space.
Unnecessary spacing detected.
| @@ -113,7 +113,7 @@ | ||
| describe "show" do | ||
| context "where tag set is not found" do | ||
| it "redirects and displays an error" do | ||
| - get :show, id: 12345 | ||
| + get :show, params: { id: 12345 } |
| 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 } } |
sarken
added
the
Awaiting review
label
Aug 3, 2017
ariana-paris
changed the title from
Ao3 5034 rails 5 dot 1 upgrade
to
AO3-5034 Rails 5 dot 1 upgrade
Aug 5, 2017
zz9pzza
and others
added some commits
Aug 7, 2017
| @@ -11,7 +11,7 @@ class TagNomination < ActiveRecord::Base | ||
| message: ts("^Tag nominations must be between 1 and #{ArchiveConfig.TAG_MAX} characters.") | ||
| validates_format_of :tagname, | ||
| - if: "!tagname.blank?", | ||
| + if: Proc.new { |tag_nomination| !tag_nomination.tagname.blank? }, |
houndci-bot
Aug 9, 2017
Align the parameters of a method call if they span more than one line.
Use proc instead of Proc.new.
| @@ -1,4 +1,4 @@ | ||
| - class User < ActiveRecord::Base | ||
| + class User < ApplicationRecord |
| - old_pseud = self.pseuds.where(name: login_was).first | ||
| - if login.downcase == login_was.downcase | ||
| + return unless saved_change_to_login? && login_before_last_save.present? | ||
| + old_pseud = self.pseuds.where(name: login_before_last_save).first |
| - if login.downcase == login_was.downcase | ||
| + return unless saved_change_to_login? && login_before_last_save.present? | ||
| + old_pseud = self.pseuds.where(name: login_before_last_save).first | ||
| + if login.downcase == login_before_last_save.downcase |
| @@ -693,7 +694,7 @@ def save_chapters | ||
| # If the work is posted, the first chapter should be posted too | ||
| def post_first_chapter | ||
| - if self.posted_changed? || (self.chapters.first && self.chapters.first.posted != self.posted) | ||
| + if self.saved_change_to_posted? || (self.chapters.first && self.chapters.first.posted != self.posted) |
| else | ||
| - record.remove_stale_from_autocomplete | ||
| + if record.user.saved_changes.any? |
| + | ||
| + patch '/admin/skins/update' => 'admin_skins#update', as: :update_admin_skin | ||
| + | ||
| + get '/admin/admin_users/troubleshoot/:id' =>'admin/admin_users#troubleshoot', as: :troubleshoot_admin_user |
| @@ -113,7 +113,7 @@ | ||
| it "removes things" do | ||
| @approved_work_item = CollectionItem.find_by_item_id(@approved_work.id) | ||
| fake_login_known_user(owner) | ||
| - delete :destroy, params: { id: @approved_work_item.id } | ||
| + delete :destroy, params: { id: @approved_work_item.id, work_id: @approved_work.id} |
| + | ||
| +describe AdminMailer do | ||
| + describe "basic admin emails" do | ||
| + |
| @@ -25,6 +25,10 @@ | ||
| RSpec.configure do |config| | ||
| config.mock_with :rspec | ||
| + config.expect_with :rspec do |c| | ||
| + c.syntax = [:should, :expect] |
sarken
commented on app/models/tag.rb in edc806d
Aug 10, 2017
|
I noticed this removes the |
|
No. I'm pretty sure that's an error caused by the fact that I re-did this a few times in a few different ways before I figured out what worked. I must have missed the last check. Should I fix this? Or is it true that |
sarken
replied
Aug 10, 2017
|
I think we need to add |
| @@ -0,0 +1,26 @@ | ||
| +require 'spec_helper' |
sarken
Aug 10, 2017
Owner
Hmm, I think this spec is for the feature we removed in #2941. I'm a little confused why it's passing.
WendyBeth
Aug 17, 2017
Contributor
There must have been some awkwardness in the merging. It passed because the method its referencing also got merged back in. I've removed both the file and the method.
| update_feedback_in_inbox(parent_comment_owner) | ||
| else | ||
| add_feedback_to_inbox(parent_comment_owner) | ||
| end | ||
| end | ||
| - if parent_comment_owner | ||
| - return parent_comment_owner | ||
| - end |
elzj
Aug 11, 2017
Owner
Taking this out is a problem, because in after_update, we actually do this:
if (parent_comment_owner = notify_parent_comment_owner)
So we do want it to return the value and not just nil all the time. Or we could change the other part of the code.
WendyBeth
Aug 17, 2017
Contributor
I'm a little wary that might break a test - I honestly can't think of why I would have removed these lines otherwise. But all of the tests I could think of to run associated with comments passed locally so perhaps it was just an odd oversight.
sarken
added
Reviewed: Needs Coder Action
and removed
Awaiting review
labels
Aug 13, 2017
| + :remember_me | ||
| + ) | ||
| + end | ||
| + |
| @@ -1,5 +1,5 @@ | ||
| module BookmarksHelper | ||
| - | ||
| + |
| # returns just a url to the new bookmark form | ||
| def get_new_bookmark_path(bookmarkable) | ||
| return case bookmarkable.class.to_s | ||
| when "Chapter" | ||
| new_work_bookmark_path(bookmarkable.work) | ||
| - when "Work" | ||
| + when "Work" |
| @@ -17,7 +17,7 @@ def self.active? | ||
| # we should expire the cache when an active banner is changed or when a banner starts or stops being active | ||
| def should_expire_cache? | ||
| - self.active_changed? || self.active? | ||
| + self.saved_change_to_active? || self.active? |
| + | ||
| + def update_sanitizer_version | ||
| + ArchiveConfig.FIELDS_ALLOWING_HTML.each do |field| | ||
| + if self.respond_to?("#{field}_changed?") && self.send("#{field}_changed?") |
| + def update_sanitizer_version | ||
| + ArchiveConfig.FIELDS_ALLOWING_HTML.each do |field| | ||
| + if self.respond_to?("#{field}_changed?") && self.send("#{field}_changed?") | ||
| + self.send("#{field}_sanitizer_version=", ArchiveConfig.SANITIZER_VERSION) |
| + end | ||
| + end | ||
| + end | ||
| + |
| @@ -282,4 +282,5 @@ def match(other, settings = nil) | ||
| builder.build_potential_match | ||
| end | ||
| + |
| - chapter.position = i+1 | ||
| - if chapter.position_changed? | ||
| - Chapter.where("id = #{chapter.id}").update_all("position = #{chapter.position}") | ||
| + if chapter.position != i+1 |
| - if chapter.position_changed? | ||
| - Chapter.where("id = #{chapter.id}").update_all("position = #{chapter.position}") | ||
| + if chapter.position != i+1 | ||
| + Chapter.where("id = #{chapter.id}").update_all("position = #{i+1}") |
| @@ -79,7 +78,7 @@ def fix_positions | ||
| end | ||
| after_save :invalidate_chapter_count, | ||
| - if: Proc.new { |chapter| chapter.posted_changed? } | ||
| + if: Proc.new { |chapter| chapter.saved_change_to_posted? } |
houndci-bot
Aug 17, 2017
Align the parameters of a method call if they span more than one line.
Use proc instead of Proc.new.
| @@ -100,7 +100,7 @@ def set_anonymous_and_unrevealed | ||
| def update_work | ||
| return unless item_type == 'Work' && work.present? && !work.new_record? | ||
| # Check if this is new - can't use new_record? with after_save | ||
| - if self.id_changed? | ||
| + if self.saved_change_to_id? |
| belongs_to :collection | ||
| after_update :after_update | ||
| def after_update | ||
| if self.collection.valid? && self.valid? | ||
| - if self.unrevealed_changed? && !self.unrevealed? | ||
| + if self.saved_change_to_unrevealed? && !self.unrevealed? |
| self.collection.reveal! | ||
| end | ||
| - if self.anonymous_changed? && !self.anonymous? | ||
| + if self.saved_change_to_anonymous? && !self.anonymous? |
| @@ -64,23 +64,23 @@ def after_update | ||
| users = [] | ||
| admins = [] | ||
| - if self.edited_at_changed? && self.content_changed? && self.moderated_commenting_enabled? && !self.is_creator_comment? | ||
| + if self.saved_change_to_edited_at? && self.saved_change_to_content? && self.moderated_commenting_enabled? && !self.is_creator_comment? |
| # we might need to put it back into moderation | ||
| if content_too_different?(self.content, self.content_was) | ||
| # we use update_column because we don't want to invoke this callback again | ||
| self.update_column(:unreviewed, true) | ||
| end | ||
| end | ||
| - if self.edited_at_changed? || (self.unreviewed_changed? && !self.unreviewed?) | ||
| + if self.saved_change_to_edited_at? || (self.saved_change_to_unreviewed? && !self.unreviewed?) |
| # Reply to owner of parent comment if this is a reply comment | ||
| # Potentially we are notifying the original commenter of a newly-approved reply to their comment | ||
| if (parent_comment_owner = notify_parent_comment_owner) | ||
| users << parent_comment_owner | ||
| end | ||
| end | ||
| - if self.edited_at_changed? | ||
| + if self.saved_change_to_edited_at? |
| @@ -263,25 +263,22 @@ def notify_parent_comment_owner | ||
| # send notification to the owner of the original comment if they're not the same as the commenter | ||
| if (have_different_owner?(parent_comment)) | ||
| if !parent_comment_owner || notify_user_by_email?(parent_comment_owner) || self.ultimate_parent.is_a?(Tag) | ||
| - if self.edited_at_changed? | ||
| + if self.saved_change_to_edited_at? |
| CommentMailer.edited_comment_reply_notification(parent_comment, self).deliver | ||
| else | ||
| CommentMailer.comment_reply_notification(parent_comment, self).deliver | ||
| end | ||
| end | ||
| if parent_comment_owner && notify_user_by_inbox?(parent_comment_owner) | ||
| - if self.edited_at_changed? | ||
| + if self.saved_change_to_edited_at? |
| @@ -63,7 +63,7 @@ def generate_token | ||
| end | ||
| def send_and_set_date | ||
| - if self.invitee_email_changed? && !self.invitee_email.blank? | ||
| + if self.saved_change_to_invitee_email? && !self.invitee_email.blank? |
| @@ -11,12 +11,12 @@ class Kudo < ActiveRecord::Base | ||
| validates_uniqueness_of :pseud_id, | ||
| scope: [:commentable_id, :commentable_type], | ||
| message: ts("^You have already left kudos here. :)"), | ||
| - if: "!pseud.nil?" | ||
| + if: Proc.new { |kudo| !kudo.pseud.nil? } |
| validates_uniqueness_of :ip_address, | ||
| scope: [:commentable_id, :commentable_type], | ||
| message: ts("^You have already left kudos here. :)"), | ||
| - if: "!ip_address.blank?" | ||
| + if: Proc.new { |kudo| !kudo.ip_address.blank? } |
| + # | ||
| + # see psued_sweeper.rb:13 for more context | ||
| + # | ||
| + past_user_name = user.blank? ? "" : (user.login_before_last_save.blank? ? user.login : user.login_before_last_save) |
houndci-bot
Aug 17, 2017
Ternary operators must not be nested. Prefer if or else constructs instead.
| + # see psued_sweeper.rb:13 for more context | ||
| + # | ||
| + past_user_name = user.blank? ? "" : (user.login_before_last_save.blank? ? user.login : user.login_before_last_save) | ||
| + (past_name != past_user_name) ? "#{past_name} (#{past_user_name})" : past_name |
| + # This is a particular case for the Pseud model | ||
| + def remove_stale_from_autocomplete_before_save | ||
| + Rails.logger.debug "Removing stale from autocomplete: #{autocomplete_search_string_was}" | ||
| + self.class.remove_from_autocomplete(self.autocomplete_search_string_was, self.autocomplete_prefixes, self.autocomplete_value_was) |
| + self.class.remove_from_autocomplete(self.autocomplete_search_string_was, self.autocomplete_prefixes, self.autocomplete_value_was) | ||
| + end | ||
| + | ||
| + |
| @@ -1,5 +1,5 @@ | ||
| class SearchResult | ||
| - | ||
| + |
| def current_page | ||
| tire_response.current_page | ||
| end | ||
| - | ||
| + |
| @@ -24,7 +24,7 @@ def reject_children | ||
| true | ||
| end | ||
| - after_save :update_child_parent_tagnames, if: "tagname_changed?" | ||
| + after_save :update_child_parent_tagnames, if: Proc.new { |nom| nom.saved_change_to_tagname? } |
| @@ -24,7 +24,7 @@ def reject_children | ||
| true | ||
| end | ||
| - after_save :update_child_parent_tagnames, if: "tagname_changed?" | ||
| + after_save :update_child_parent_tagnames, if: Proc.new { |nom| nom.saved_change_to_tagname? } |
| @@ -24,6 +24,10 @@ def autocomplete_search_string_was | ||
| "#{name_was}" | ||
| end | ||
| + def autocomplete_search_string_before_last_save | ||
| + "#{name_before_last_save}" |
| @@ -32,6 +36,10 @@ def autocomplete_value_was | ||
| "#{id}#{AUTOCOMPLETE_DELIMITER}#{name_was}" + (self.respond_to?(:title) ? "#{AUTOCOMPLETE_DELIMITER}#{title_was}" : "") | ||
| end | ||
| + def autocomplete_value_before_last_save | ||
| + "#{id}#{AUTOCOMPLETE_DELIMITER}#{name_before_last_save}" + (self.respond_to?(:title) ? "#{AUTOCOMPLETE_DELIMITER}#{title_before_last_save}" : "") |
| - Rails.logger.debug "Removing stale from autocomplete: #{autocomplete_search_string_was}" | ||
| - self.class.remove_from_autocomplete(self.autocomplete_search_string_was, self.autocomplete_prefixes, self.autocomplete_value_was) | ||
| + Rails.logger.debug "Removing stale from autocomplete: #{autocomplete_search_string_before_last_save}" | ||
| + self.class.remove_from_autocomplete(self.autocomplete_search_string_before_last_save, self.autocomplete_prefixes, self.autocomplete_value_before_last_save) |
| - } | ||
| + presence: { method: "Validate.Presence", | ||
| + messages: { | ||
| + failureMessage: "live_validation.presence.failure" |
houndci-bot
Aug 17, 2017
Tab detected.
Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.
| + presence: { method: "Validate.Presence", | ||
| + messages: { | ||
| + failureMessage: "live_validation.presence.failure" | ||
| + } |
houndci-bot
Aug 17, 2017
Tab detected.
Indent the right brace the same as the start of the line where the left brace is.
| }, | ||
| numericality: { method: "Validate.Numericality", | ||
| - messages: { | ||
| - notANumberMessage: "live_validation.numericality.not_a_number", | ||
| + messages: { |
houndci-bot
Aug 17, 2017
Tab detected.
Align the elements of a hash literal if they span more than one line.
| - messages: { | ||
| - notANumberMessage: "live_validation.numericality.not_a_number", | ||
| + messages: { | ||
| + notANumberMessage: "live_validation.numericality.not_a_number", |
houndci-bot
Aug 17, 2017
Tab detected.
Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.
| notAnIntegerMessage: "live_validation.numericality.not_an_integer", | ||
| wrongNumberMessage: "live_validation.numericality.wrong_number", | ||
| tooLowMessage: "live_validation.numericality.too_low", | ||
| tooHighMessage: "live_validation.numericality.too_high" | ||
| - } | ||
| + } |
| }, | ||
| format: { method: "Validate.Format", | ||
| - messages: { | ||
| + messages: { |
houndci-bot
Aug 17, 2017
Tab detected.
Align the elements of a hash literal if they span more than one line.
| failureMessage: "live_validation.format.failure" | ||
| - } | ||
| + } |
| }, | ||
| length: { method: "Validate.Length", | ||
| - messages: { | ||
| - wrongLengthMessage: "live_validation.length.wrong_length", | ||
| + messages: { |
houndci-bot
Aug 17, 2017
Tab detected.
Align the elements of a hash literal if they span more than one line.
| - messages: { | ||
| - wrongLengthMessage: "live_validation.length.wrong_length", | ||
| + messages: { | ||
| + wrongLengthMessage: "live_validation.length.wrong_length", |
houndci-bot
Aug 17, 2017
Tab detected.
Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.
| tooShortMessage: "live_validation.length.too_short", | ||
| - tooLongMessage: "live_validation.length.too_long" | ||
| + tooLongMessage: "live_validation.length.too_long" |
| } | ||
| }, | ||
| acceptance: { method: "Validate.Acceptance", | ||
| - messages: { | ||
| + messages: { |
houndci-bot
Aug 17, 2017
Tab detected.
Align the elements of a hash literal if they span more than one line.
| failureMessage: "live_validation.acceptance.failure" | ||
| } | ||
| }, | ||
| confirmation: { method: "Validate.Confirmation", | ||
| - messages: { | ||
| + messages: { |
houndci-bot
Aug 17, 2017
Tab detected.
Align the elements of a hash literal if they span more than one line.
| failureMessage: "live_validation.confirmation.failure" | ||
| - } | ||
| + } |
| + # | ||
| + alias_method "validates_#{type}_of_without_live_validations".to_sym, :live_validations | ||
| + alias_method :live_validations, "validates_#{type}_of_with_live_validations".to_sym | ||
| + |
WendyBeth
added some commits
Aug 17, 2017
| @@ -280,8 +280,8 @@ def notify_parent_comment_owner | ||
| return parent_comment_owner | ||
| end | ||
| end | ||
| + return nil |
sarken
merged commit f872a75
into
otwcode:master
Aug 18, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
WendyBeth commentedAug 2, 2017
Pull Request Checklist
AO3-1234 Fix thing)Issue
https://otwarchive.atlassian.net/browse/AO3-5034
Purpose
What does this PR do?
This PR upgrades the entire application to Rails 5.1.
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 fact that this is a massive set of changes, extensive testing will be needed to ensure that application behavior has not changed.
Particular attention should be paid to:
Making sure that all expected routes exist. Removing the whitelisted route (
get ':controller(/:action(/:id(.:format)))') from config/routes.rb made a significant number of routes used throughout the application invalid. These should all be whitelisted, but if any have been missed they will need to be added to the routes list (even/especially non-RESTful routes, such as calls to#updatewithout an id that end up being read as '/resources/update' instead of '/resources/:id'.Ensuring that all objects save properly (including sending out expected emails and anything else that might happen in before/after callbacks) when submitting forms since significant changes were made to callbacks with the intention of maintaining existing behavior.
References
Are there any other relevant issues / PRs / mailing lists discussions related to this?
This is rolled in with changes from #2958, so that should be closed.
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)?
(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)
she/her