[openstreetmap/openstreetmap-website] Move max value of issues counter to settings (PR #4201)

[Andy Allan]

@gravitystorm requested changes on this pull request.



> @@ -26,9 +26,9 @@ def reportable_title(reportable)
   end
 
   def open_issues_count
-    count = Issue.visible_to(current_user).open.limit(100).size
-    if count > 99
-      tag.span("99+", :class => "count-number")
+    count = Issue.visible_to(current_user).open.limit(Settings.max_issues_count).size

So I think this will all be more straightforward if the setting is the same as the digits shown. So if you want to show `99+` you set the limit to `99`. It's strange to set the limit to "100" in then a different number to show up.

Additionally, 99+ just means "at least" 99, so it's a valid display for both 99 and 100. The original implementation checks for `99+1` and then shows either `99` or `99+`, but this is unnecessary complexity. Better just to go `97`, `98`, `99+`, and this means no +1 maths is necessary.

> @@ -26,9 +26,9 @@ def reportable_title(reportable)
   end
 
   def open_issues_count
-    count = Issue.visible_to(current_user).open.limit(100).size
-    if count > 99
-      tag.span("99+", :class => "count-number")
+    count = Issue.visible_to(current_user).open.limit(Settings.max_issues_count).size
+    if count >= Settings.max_issues_count
+      tag.span("#{Settings.max_issues_count - 1}+", :class => "count-number")

This is an i18n issue, since not all languages use the `NN+` format. So it should be moved to a translatable string.

I investigated CLDR to get the right terminology here, which is either 'at_least_pattern' or 'atLeast' depending on which be of CLDR you look at. So having that terminology in the key name will be helpful for translators.

https://github.com/search?q=repo%3Aunicode-org%2Fcldr-json%20atleast&type=code

(I have researched if there is something equivalent available in rails-i18n, but with no success. It would be more useful to use CLDR as the source for this, but more hassle than it's worth. But at least we can let our own translators deal with it).

Finally it should be a reusable translation (i.e. not controller-specific), since we'll end up using it for other things like message inbox counts too (which are currently an unlimited `count`, and therefore could be a performance issue).

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

Message ID: <openstreetmap/openstreetmap-website/pull/4201/review/1602546465 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20230830/084e2f05/attachment.htm>