[openstreetmap/openstreetmap-website] Upgrade to rails 7.1.0 (PR #4278)

[Andy Allan]

@gravitystorm commented on this pull request.

Looks good! I've added a few comments on specific things. 

Two of which are bugs that are already fixed in the `7_1_stable` branch but I don't have any visibility into when there might be a 7.1.1 release.

> @@ -124,7 +124,7 @@ def friendship_notification(friendship)
   def note_comment_notification(comment, recipient)
     with_recipient_locale recipient do
       @noteurl = note_url(comment.note)
-      @place = Nominatim.describe_location(comment.note.lat, comment.note.lon, 14, I18n.locale)
+      @place = Nominatim.describe_location(comment.note.lat.to_f, comment.note.lon.to_f, 14, I18n.locale)

This change defeats the purpose of using `DelegateClass`, because e.g. note.lat is supposed to be indistinguishable from it being a float, apart from how we format it as a string. So I'd really like to avoid having to call `.to_f` on something that's supposed to act like a float. And I also think this might break the formatting too, since to_s will be called on the float, not on the `Coord`

I spent several hours figuring out what is going on, and narrowed it down to https://github.com/rails/rails/pull/49466 and there is a fix already at https://github.com/rails/rails/pull/49576 which was merged earlier today. We should either wait for that fix to be released, or revert this commit when it happens.

> @@ -56,10 +56,8 @@
   # Highlight code that triggered database queries in logs.
   config.active_record.verbose_query_logs = true unless Settings.status == "database_offline"
 
-  # Debug mode disables concatenation and preprocessing of assets.
-  # This option may cause significant delays in view rendering with a large
-  # number of complex assets.
-  config.assets.debug = true

This change breaks my local development environment. Without the setting, I get html generated that refers to incorrect stylesheet paths. But if I add this setting back in, everything works.

Is there a reason for it being removed? If not, I would keep it for now.

I don't know why removing the setting breaks my environment, but until I figure it out I'm concerned that it might break things for other developers too.

>    # config.require_master_key = true
 
-  # Disable serving static files from the `/public` folder by default since
-  # Apache or NGINX already handles this.
-  config.public_file_server.enabled = ENV["RAILS_SERVE_STATIC_FILES"].present?

It looks to me like this is switching from 'false' to 'true'? I thought we used apache to serve static files in production.

>  
-  # Compress JavaScripts and CSS.
-  config.assets.js_compressor = Terser.new

Is this change deliberate? If so, can we remove the Terser gem?


> @@ -138,7 +138,7 @@ def test_create_invalid_xml
       xml = "<osm><node lat='#{lat}' lon='#{lon}' changeset='#{changeset.id}'><tag k='foo' v='#{'x' * 256}'/></node></osm>"
       put node_create_path, :params => xml, :headers => auth_header
       assert_response :bad_request, "node upload did not return bad_request status"
-      assert_equal ["NodeTag ", " v: is too long (maximum is 255 characters) (\"#{'x' * 256}\")"], @response.body.split(/[0-9]+,foo:/)

I couldn't figure out why this test has changed - is the output from the API different than before?

> @@ -15,19 +15,7 @@ def test_showing_create_diary_entry
     # We can now login
     post "/login", :params => { "username" => user.email, "password" => "test", :referer => "/diary/new" }
     assert_response :redirect
-
-    # Check that there is some payload alerting the user to the redirect

If you want it for the commit message, I found the change is https://github.com/rails/rails/pull/44554

>  
-  # Eager loading loads your whole application. When running a single test locally,
-  # this probably isn't necessary. It's a good idea to do in a continuous integration
-  # system, or in some way before deploying your code.
-  config.eager_load = ENV["CI"].present?

The commit message links to a PR that has been declined, but the issue has been fixed on the 7.1 branch in https://github.com/rails/rails/commit/174bd99ac74f20387da3bb59c19090da4b9a8f52

We can either wait for the fix to be released, or we need to remember to undo this when the next release is made. 

I'll leave it to you to decide whether you're happy with this being disabled! It makes a reasonably large change to bring CI closer to production, but I guess it's a while since we've had any eager-loading problems.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/4278#pullrequestreview-1671509679
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/4278/review/1671509679 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20231011/8cf313b8/attachment-0001.htm>