Skip to content
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-6469 Upgrade to Rails 6.1. #4439

Merged
merged 5 commits into from
Feb 10, 2023

Conversation

tickinginstant
Copy link
Contributor

@tickinginstant tickinginstant commented Jan 26, 2023

Issue

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

Purpose

  • Upgrade Rails to version 6.1.7.2.
  • Remove case_sensitive: false settings on uniqueness validations, since that's the default now.
  • Add the names of all autocomplete actions to the routes file, instead of using syntax which was deprecated in 6.0 and removed in 6.1.
  • Add the default config/storage.yml file to keep ActiveStorage from complaining.
  • Monkeypatch phraseapp-in-context-editor-ruby to fix a bug that occurs when it's used with Ruby 3.0 + Rails 6.1. The original of this method is defined here: https://github.com/phrase/phraseapp-in-context-editor-ruby/blob/6bf481f672d86c9ad221568f8ec74eea50a13d1a/lib/phraseapp-in-context-editor-ruby/backend_service.rb#L15-L41
        def translate(*args)
          if to_be_translated_without_phraseapp?(args)
            # *Ruby 2.7+ keyword arguments warning*
            #
            # This method uses keyword arguments.
            # There is a breaking change in ruby that produces warning with ruby 2.7 and won't work as expected with ruby 3.0
            # The "hash" parameter must be passed as keyword argument.
            #
            # Good:
            #  I18n.t(:salutation, :gender => 'w', :name => 'Smith')
            #  I18n.t(:salutation, **{ :gender => 'w', :name => 'Smith' })
            #  I18n.t(:salutation, **any_hash)
            #
            # Bad:
            #  I18n.t(:salutation, { :gender => 'w', :name => 'Smith' })
            #  I18n.t(:salutation, any_hash)
            #
            kw_args = args[1]
            if kw_args.present?
              I18n.translate_without_phraseapp(args[0], **kw_args)
            else
              I18n.translate_without_phraseapp(args[0])
            end
          else
            phraseapp_delegate_for(args)
          end
        end
    The method assumes that the first argument in args is always the key to be translated, but calling translate with no key and only keyword arguments is valid. Rails 6.1 actually does this by default in views when it can't find a translation for the first key, and the first key is not a symbol: https://github.com/rails/rails/blob/5c835bd669ae996a30bcc914672c9941539a496e/actionview/lib/action_view/helpers/translation_helper.rb#L96
  • Fix errors[:base] << message syntax, which is newly deprecated in 6.1.

Testing Instructions

See Jira.

if PhraseApp::VERSION == "1.6.0"
PhraseApp::InContextEditor::BackendService.prepend(FixPhraseapp)
else
puts "WARNING: The monkeypatch #{__FILE__} was written for version 1.6.0 of the phraseapp-in-context-editor-ruby gem, but you are running #{PhraseApp::VERSION}. Please update or remove the monkeypatch."
Copy link

Choose a reason for hiding this comment

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

Rails/Output: Do not write to stdout. Use Rails's logger if you want to log.

Gemfile.lock Outdated
@@ -274,7 +278,7 @@ GEM
et-orbi (~> 1.1, >= 1.1.8)
raabro (~> 1.4)
gherkin (5.1.0)
globalid (1.0.0)
globalid (1.0.1)
Copy link
Member

Choose a reason for hiding this comment

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

This can supersede #4437.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually assuming #4437 would be merged as part of the current release -- I figured I would resolve merge conflicts once it was in. But it's up to you.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I've merged that now.

@redsummernight
Copy link
Member

Remove case_sensitive: false settings on uniqueness validations, since that's the default now.

Going to Rails 6.1 technically changes the case sensitivity of

validates :email, presence: true, uniqueness: true, email_veracity: true

but it shouldn't matter because we were down-casing all values for case insensitivity anyway.

app/models/user.rb Outdated Show resolved Hide resolved
config/routes.rb Outdated Show resolved Hide resolved
Copy link
Member

@redsummernight redsummernight left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

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