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

Fix deprecation warnings from rails 6.1 update #8610

Merged

Conversation

alecslupu
Copy link
Contributor

@alecslupu alecslupu commented Dec 13, 2021

🎩 What? Why?

This PR aims to fix all the deprecations resulted in the Rails 6.1 upgrade (#8411)

📌 Related Issues

Link your PR to an issue

Testing

Browse the Action logs and see there are no Deprecation warnings.

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

📷 Screenshots

Please add screenshots of the changes you're proposing
Description

♥️ Thank you!

@alecslupu alecslupu force-pushed the upgrade/rails-6.1-deprecations branch 4 times, most recently from 5535aa3 to 91b3c4c Compare December 14, 2021 08:02
@andreslucena
Copy link
Member

@alecslupu I was going to say that we should change the base branch from develop to the one from #8411 (tremend-cofe:upgrade/rails-6.1) but as it's from the tremend-cofe organization I think that we can't do that. So, how about if we review and merge this after we merge #8411? (So it's easier to review, if not my head can explode with that many changes at the same time 😅)

As I said in that PR, the only one that I fell is a stopper for merging was the PaperTrail warnings, and that's already fixed in #8608

@alecslupu alecslupu force-pushed the upgrade/rails-6.1-deprecations branch from 91b3c4c to b25b8ae Compare December 15, 2021 05:39
@alecslupu
Copy link
Contributor Author

@alecslupu I was going to say that we should change the base branch from develop to the one from #8411 (tremend-cofe:upgrade/rails-6.1) but as it's from the tremend-cofe organization I think that we can't do that. So, how about if we review and merge this after we merge #8411? (So it's easier to review, if not my head can explode with that many changes at the same time sweat_smile)

As I said in that PR, the only one that I fell is a stopper for merging was the PaperTrail warnings, and that's already fixed in #8608

If you manage to review and merge the #8411, then this will be a very simple one (10 - 20 files changed) to address the deprecations.

I think you may be able to see this link : tremend-cofe/decidim@upgrade/rails-6.1...tremend-cofe:upgrade/rails-6.1-deprecations

@andreslucena andreslucena changed the title Upgrade/rails 6.1 deprecations Fix deprecation warnings from rails 6.1 update Dec 15, 2021
@alecslupu alecslupu mentioned this pull request Mar 10, 2022
@alecslupu alecslupu marked this pull request as ready for review March 15, 2022 13:09
@ahukkanen
Copy link
Contributor

@alecslupu Can you rebase with develop and resolve the conflicts now that #8411 is merged? 🙏

@alecslupu alecslupu force-pushed the upgrade/rails-6.1-deprecations branch 8 times, most recently from 24efce1 to df57357 Compare March 16, 2022 20:45
@alecslupu alecslupu force-pushed the upgrade/rails-6.1-deprecations branch from df57357 to 3ec1694 Compare March 17, 2022 05:14
@alecslupu alecslupu force-pushed the upgrade/rails-6.1-deprecations branch from 0c983ee to c602c84 Compare March 17, 2022 08:26
@ahukkanen
Copy link
Contributor

ahukkanen commented Mar 17, 2022

Few things I've noticed:

ActiveModel::Errors#keys is deprecated

https://github.com/tremend-cofe/decidim/blob/c602c84ea15e721d2927fc18eb67d8676bb43dc4/decidim-elections/spec/forms/decidim/elections/admin/answer_form_spec.rb#L62

Enumerating ActiveModel::Errors as a hash has been deprecated.

https://github.com/tremend-cofe/decidim/blob/c602c84ea15e721d2927fc18eb67d8676bb43dc4/decidim-initiatives/app/commands/decidim/initiatives/admin/create_initiative_type.rb#L29

Initialization autoloaded the constant Decidim::Budgets::OrderReminderGenerator.

  1. Go to the development app
  2. rm -f log/development.log
  3. bundle exec rails s
  4. cat log/development.log

To fix this, we need to move this:
https://github.com/tremend-cofe/decidim/blob/c602c84ea15e721d2927fc18eb67d8676bb43dc4/decidim-budgets/app/services/decidim/budgets/order_reminder_generator.rb

To the lib folder and add an autoload statement for it within the module.

Rails console giving warnings with Spring

NOTE: This is fixed in another PR: #9032

As I mentioned in the previous review comments, the rails console is giving lots of warnings with spring:
#8411 (comment)

Judging by this, this is also the reason why it hangs for you (as you mentioned in that discussion):
rails/spring#396

There is quite a clean workaround for this reported at:
rails/spring-watcher-listen#15 (comment)

So, if we modify the application's config/spring.rb to apply this in Decidim, it would look something like this:

# add these to config/spring.rb

module SpringWatcherListenIgnorer
  def start
    super
    listener.ignore(/^(node_modules|storage|tmp)/)
  end
end

Spring::Watcher::Listen.prepend SpringWatcherListenIgnorer

This config file is coming from here:
https://github.com/rails/rails/blob/d38ff07a39c98f5db2b15ca3a51acee48c972205/railties/lib/rails/generators/rails/app/app_generator.rb#L123

And it is defined here:
https://github.com/rails/rails/blob/d38ff07a39c98f5db2b15ca3a51acee48c972205/railties/lib/rails/generators/rails/app/templates/config/spring.rb.tt

@ahukkanen ahukkanen mentioned this pull request Mar 17, 2022
12 tasks
@ahukkanen
Copy link
Contributor

@alecslupu I fixed the errors with Spring in another PR: #9032

@alecslupu
Copy link
Contributor Author

@ahukkanen I will handle the autoload in a separate PR.

Copy link
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

LGTM

I checked through all the job run logs and did not find any Rails related deprecation errors.

I also tried to do some grepping in the code base to try to identify possible deprecations and did not find any places.

I am quite sure we may find these more as we go forward but I think it's best to merge this now that all the noticed deprecation issues are fixed and then handle those other ones as we bump into them.

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.

3 participants