Update rails to 6.1

[alecslupu]

🎩 What? Why?

This is the rails 6.1 upgrade of the decidim.
Please note, at the moment the controller specs are being disabled due to an error, that i could not yet get my head around, but I am searching for a way to fix those as well.

♥️ Thank you!

[alecslupu]

@leio10 , all the checks have passed. Please note the commit 335f9ea can be removed, as it adds required things to make the generators pass.

I see there is a small drop regarding the coverage metric, and I suspect this is caused by the excluded controller suite, which fails due to some errors in routing. I will try to address that in a separate PR.

[alecslupu]

@leio10 , i have checked the test suite, and the projects_controller.rb which codecov tells us that has a drop in coverage. I have added raise statements in the controller that is being reported as uncovered and then ran the controller specs. The error was triggered. So, i guess that is a false positive.

[ferblape]

@andreslucena we've found an issue in ActiveStorage and Rails 6.0 that gets fixed in Rails 6.1, which prevents creating public urls for attachments. With Rails 6.0.x AS attachments generate only signed urls, which can be a problem if attachments urls are copied and pasted.

It'd be good to include this PR in 0.25 branch once it gets merged, do you think that could be possible?

[alecslupu]

@ferblape , I would push a bit faster a decidim 0.26 that would actually contain the Rails 6.1 upgrade.

[andreslucena]

With Rails 6.0.x AS attachments generate only signed urls, which can be a problem if attachments urls are copied and pasted.

I think I was bitten by this one in Metadecidim, but I didn't have time to review it in deep and wasn't sure that I miss copied the URL. Do you have a step by step on how to reproduce this?

Also, it'd be possible to only fix and backport this bug?

It'd be good to include this PR in 0.25 branch once it gets merged, do you think that could be possible?

We will not backport this to v0.25. We only backport fixes to stable releases. IMHO this is too big.

I would push a bit faster a decidim 0.26 that would actually contain the Rails 6.1 upgrade.

👍🏽

Also for your info, we've talked with @alecslupu that as we're low in maintenance resources we will wait until February to review and merge this one, but I was thinking that there wouldn't be any need to merge this before.

[ferblape]

We've just discovered that the public setting in ActiveStorage was introduced in Rails 6.1 (our fault because we always check https://edgeguides.rubyonrails.org/ instead of https://guides.rubyonrails.org/)

So, in Rails 6.0.x the links to attachments are private, which means that they expire, and they should include a key that signs the parameters. While the links are generated by the application it doesn't matter, because Rails renews the parameters without noticing. The problem is when you copy and paste the link into a document, because you copy the link to the S3 object instead of the Rails URL. That link to the S3 object contains the parameters that expire.

[andreslucena]

Great work!! 👏🏽 👏🏽
There are a couple comments and things to discuss.
We should try to fix all the deprecation warnings that appear when running the spec suite, I'll add it in a comment

[andreslucena]

About the deprecation warnings:

• 1. PaperTrail compatibility

  PaperTrail 10.3.1 is not compatible with ActiveRecord 6.1.4.1. We allow PT
  contributors to install incompatible versions of ActiveRecord, and this
  warning can be silenced with an environment variable, but this is a bad
  idea for normal use. Please install a compatible version of ActiveRecord
  instead (>= 4.2, < 6.1). Please see the discussion in paper_trail/compatibility.rb
  for details.
  

Addressed in core by : #8608

• 2. Rendering actions with '.' in the name is deprecated:

  DEPRECATION WARNING: Rendering actions with '.' in the name is deprecated: decidim/shared/_extended_navigation_bar.html (called from extended_navigation_bar at /home/runner/work/decidim/decidim/decidim-core/app/helpers/decidim/layout_helper.rb:110)
  
  DEPRECATION WARNING: Rendering actions with '.' in the name is deprecated: layouts/decidim/diploma.html.erb (called from add_diploma_attachment at /home/runner/work/decidim/decidim/decidim-conferences/app/mailers/decidim/conferences/admin/send_conference_diploma_mailer.rb:37)
  

• 3. create_after_upload!

  DEPRECATION WARNING: create_after_upload! is deprecated and will be removed from Rails 6.2 (use create_and_upload! instead) (called from block (2 levels) in <module:Assemblies> at /home/runner/work/decidim/decidim/decidim-assemblies/spec/commands/create_assembly_member_spec.rb:14)
  

There are a couple still there:

$ grep -ri create_after_upload decidim*/

decidim-assemblies/spec/commands/update_assembly_member_spec.rb:      ActiveStorage::Blob.create_after_upload!(
decidim-assemblies/spec/commands/update_assembly_member_spec.rb:          ActiveStorage::Blob.create_after_upload!(
decidim-assemblies/spec/commands/create_assembly_member_spec.rb:      ActiveStorage::Blob.create_after_upload!(
decidim-assemblies/spec/commands/create_assembly_member_spec.rb:          ActiveStorage::Blob.create_after_upload!(
decidim-assemblies/spec/presenters/decidim/assembly_member_presenter_spec.rb:          ActiveStorage::Blob.create_after_upload!(
decidim-core/spec/models/decidim/editor_image_spec.rb:          file: ActiveStorage::Blob.create_after_upload!(

• 4. Using return, break or throw to exit a transaction block is deprecated without replacement.

  DEPRECATION WARNING: Using `return`, `break` or `throw` to exit a transaction block is deprecated without replacement. If the `throw` came from `Timeout.timeout(duration)`, pass an exception class as a second argument so it doesn't use `throw` to abort its block. This results in the transaction being committed, but in the next release of Rails it will rollback. (called from call at /home/runner/work/decidim/decidim/decidim-admin/app/commands/decidim/admin/create_import.rb:14)
  

---

These are the main ones that I've found. I'd said that the biggest one for sure is the PaperTrail version compatibility, the rest we could fix them in other PRs after merging this one (although it'd be great to fix them as it doesn't seem difficult to fix)

[alecslupu]

@ahukkanen I have made the test suite Green again.

To make it easier to review, i have reduced the number of files changed (by reverting method calls that are yielding deprecation warnings).
Those deprecations will be handled in a separate MR (#8610) after this is getting merged.

[ahukkanen]

This one small thing I still noticed.

Otherwise LGTM, nice work again!

[ahukkanen]

LGTM, great work!

@andreslucena Do you want to merge this first and then change the branch names in the generators in another PR after this? Or should those be changed first before merging?

I believe that if we change them before merging the generator tests will fail in this PR. but they would be green after merging.

I don't know what has been the convention regarding this before, so which way you prefer?

[alecslupu]

LGTM, great work!

@andreslucena Do you want to merge this first and then change the branch names in the generators in another PR after this? Or should those be changed first before merging?

I believe that if we change them before merging the generator tests will fail in this PR. but they would be green after merging.

I don't know what has been the convention regarding this before, so which way you prefer?

@ahukkanen, in the previous MRs where I had this operation, the changes related to branch name were done before merging.
also check's Andres' comment: #8411 (comment)

Once @andreslucena says that we are okay with it, I will commit those reverts.

[andreslucena]

I don't know what has been the convention regarding this before, so which way you prefer?

At the moment we've done what @alecslupu said, doing it in the same branch, so it's always pointing to this same repository. This means that we'll have the CI in red when merging this 🤷🏽‍♂️

(If you guys have any proposal on how to improve this flow, let me know!!)

Also, great news that we'll be merging this 🚀!!

[ahukkanen]

@andreslucena I think this is fine. I think it's better than a separate PR to fix that.

Can you also approve this PR @andreslucena to give signal for @alecslupu to revert those generator changes, after which we can finally merge? 🥳

[alecslupu]

I have just pushed d283ec4, which reverts the generator changes.

[andreslucena]

LGTM, thanks @alecslupu for your time and effort with this issue!