<p></p>
<p><b>@gravitystorm</b> commented on this pull request.</p>
<p dir="auto">Looks good! I've added a few comments on specific things.</p>
<p dir="auto">Two of which are bugs that are already fixed in the <code class="notranslate">7_1_stable</code> branch but I don't have any visibility into when there might be a 7.1.1 release.</p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4278#discussion_r1355180786">app/mailers/user_mailer.rb</a>:</p>
<pre style='color:#555'>> @@ -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)
</pre>
<p dir="auto">This change defeats the purpose of using <code class="notranslate">DelegateClass</code>, 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 <code class="notranslate">.to_f</code> 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 <code class="notranslate">Coord</code></p>
<p dir="auto">I spent several hours figuring out what is going on, and narrowed it down to <a class="issue-link js-issue-link" data-error-text="Failed to load title" data-id="1923295267" data-permission-text="Title is private" data-url="https://github.com/rails/rails/issues/49466" data-hovercard-type="pull_request" data-hovercard-url="/rails/rails/pull/49466/hovercard" href="https://github.com/rails/rails/pull/49466">rails/rails#49466</a> and there is a fix already at <a class="issue-link js-issue-link" data-error-text="Failed to load title" data-id="1936403355" data-permission-text="Title is private" data-url="https://github.com/rails/rails/issues/49576" data-hovercard-type="pull_request" data-hovercard-url="/rails/rails/pull/49576/hovercard" href="https://github.com/rails/rails/pull/49576">rails/rails#49576</a> which was merged earlier today. We should either wait for that fix to be released, or revert this commit when it happens.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4278#discussion_r1355184171">config/environments/development.rb</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p dir="auto">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.</p>
<p dir="auto">Is there a reason for it being removed? If not, I would keep it for now.</p>
<p dir="auto">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.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4278#discussion_r1355187166">config/environments/production.rb</a>:</p>
<pre style='color:#555'>> # 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?
</pre>
<p dir="auto">It looks to me like this is switching from 'false' to 'true'? I thought we used apache to serve static files in production.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4278#discussion_r1355191312">config/environments/production.rb</a>:</p>
<pre style='color:#555'>>
- # Compress JavaScripts and CSS.
- config.assets.js_compressor = Terser.new
</pre>
<p dir="auto">Is this change deliberate? If so, can we remove the Terser gem?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4278#discussion_r1355205217">test/controllers/api/nodes_controller_test.rb</a>:</p>
<pre style='color:#555'>> @@ -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:/)
</pre>
<p dir="auto">I couldn't figure out why this test has changed - is the output from the API different than before?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4278#discussion_r1355208736">test/integration/user_diaries_test.rb</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p dir="auto">If you want it for the commit message, I found the change is <a class="issue-link js-issue-link" data-error-text="Failed to load title" data-id="1150693228" data-permission-text="Title is private" data-url="https://github.com/rails/rails/issues/44554" data-hovercard-type="pull_request" data-hovercard-url="/rails/rails/pull/44554/hovercard" href="https://github.com/rails/rails/pull/44554">rails/rails#44554</a></p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4278#discussion_r1355229649">config/environments/test.rb</a>:</p>
<pre style='color:#555'>>
- # 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?
</pre>
<p dir="auto">The commit message links to a PR that has been declined, but the issue has been fixed on the 7.1 branch in <a class="commit-link" data-hovercard-type="commit" data-hovercard-url="https://github.com/rails/rails/commit/174bd99ac74f20387da3bb59c19090da4b9a8f52/hovercard" href="https://github.com/rails/rails/commit/174bd99ac74f20387da3bb59c19090da4b9a8f52">rails/rails@<tt>174bd99</tt></a></p>
<p dir="auto">We can either wait for the fix to be released, or we need to remember to undo this when the next release is made.</p>
<p dir="auto">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.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4278#pullrequestreview-1671509679">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLIAREH6HXKDLW2YNKDX624P3ANCNFSM6AAAAAA5XRWRQY">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLLPG7FRB327OJTJTJ3X624P3A5CNFSM6AAAAAA5XRWRQ2WGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTTDUE3K6.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><openstreetmap/openstreetmap-website/pull/4278/review/1671509679</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/4278#pullrequestreview-1671509679",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/4278#pullrequestreview-1671509679",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>