[openstreetmap/openstreetmap-website] Add Communities page (#3301)

[Andy Allan]

@gravitystorm requested changes on this pull request.

I've left a bunch of inline notes, happy to discuss any aspect of them. 

One further thing - please run rubocop! My code editor lights up with all warnings that it highlights, and it makes my life a little bit easier if they are taken care of before review.

> @@ -147,6 +147,8 @@
   get "/help" => "site#help"
   get "/about/:about_locale" => "site#about"
   get "/about" => "site#about"
+  get "/communities" => "site#communities"
+  get "/communities/:communities_locale" => "site#communities"

As previously mentioned, best to drop this locale override. It's not something we only provide in exceptional circumstances. Moreover, it doesn't work for this PR anyway, while massively increasing the code complexity!

> @@ -2197,6 +2198,30 @@ en:
           Just go to <a href='%{map_url}'>the map</a> and click the note icon:
           <span class='icon note'></span>. This will add a marker to the map, which you can move
           by dragging. Add your message, then click save, and other mappers will investigate.
+    communities:
+      title: Communities
+      lede_text: |
+        People from all over the world contribute to or use OpenStreetMap.
+        Whilst some are content to participate as individuals, others have formed communities.

"Whilst" is a britishism and should be saved for the en_GB translation :smile:

> @@ -0,0 +1,19 @@
+<% content_for :heading do %>
+  <%= tag.h1 :lang => @locale, :dir => t("html.dir", :locale => @locale) do %>

There's no need for all the `:lang` and `:dir` attributes, since we're not providing an override and the page layout will take care of all of this.

> @@ -0,0 +1,19 @@
+<% content_for :heading do %>
+  <%= tag.h1 :lang => @locale, :dir => t("html.dir", :locale => @locale) do %>
+    <%= t ".title", :locale => @locale %>
+  <% end %>
+<% end %>
+
+<%= tag.div :lang => @locale, :dir => t("html.dir", :locale => @locale) do %>
+  <p><%= t ".lede_text", :locale => @locale %></p>

the `:locale` option can be dropped here too.

> @@ -0,0 +1,39 @@
+module OsmCommunityIndex
+  class LocalChapter

I don't understand why your most recent commit was to move away from having an `app/models/local_chapter` file and instead putting everything in `lib`. I much prefer the idea of having a LocalChapter model that has some class methods for getting all the local chapters and each local chapter being an instance of the model class. What drove the change in a57bc15 ?

> @@ -0,0 +1,52 @@
+module OsmCommunityIndex
+  class OsmCommunityIndex

I think we should consider moving the json+yaml parsing to an initializer, but I'm interested to hear what @tomhughes thinks. 

* Firstly, these files only needs parsing once, and will be saved until the app restarts, and so an initializer seems like the right place to do it. There is an edge case where you might want to move any complex code into another file (e.g. in `lib/`) so that you can write some testcases for it, but logically I would expect to find this stuff in `config/initializers`
* Secondly, class-variables aren't thread safe, so there's a small chance that something goes wrong in a multi-threaded server. Since we're only mutating the variable once (i.e. to set it) we should be using (frozen) constants for the loaded data structures, and that way we don't need to care about thread safety! Doubly so if it's something that's done in an initializer.

https://bearmetal.eu/theden/how-do-i-know-whether-my-rails-app-is-thread-safe-or-not/ for much more explanation on this.


> +    def self.local_chapters_with_locale(locale)
+      @localised_chapters[locale] ||= load_local_chapters(locale)
+    end
+
+    protected
+
+    def self.load_local_chapters(locale)
+      community_index = OsmCommunityIndex.community_index
+      localised_strings = OsmCommunityIndex.localised_strings(locale)
+      local_chapters = []
+      community_index["resources"].each do |id, resource|
+        resource.each do |key, value|
+          next unless key == "type" && value == "osm-lc" && id != "OSMF"
+
+          strings = resource["strings"]
+          name = localised_strings.dig(id, "name") || strings["name"] || strings["community"]

Is this a workaround for something weird in the osm-community-index data? It feels like every LC should have a name attribute, right? But I don't know what the osm-community-index policy is on these things.

> +      end
+
+      # now try it without it's country part (eg 'en' instead of 'en_GB')
+      shortened_locale = locale.split("_").first
+      unless shortened_locale === locale
+        json = load_locale_json(shortened_locale)
+        unless json.nil?
+          return json
+        end
+      end
+
+      # if nothing else works, then return "en"
+      load_locale_json("en")
+    end
+
+    def self.load_locale_json(locale)

I agree that it's fine to use the osm-community-index list of translations, but I feel like there must be a better way of doing so using Rails internal translations system (e.g. telling rails "for each of your locales, here's some more data to add" and feeding this info in there). We're already duplicating the concept of fallback locales here.

But perhaps this is too much for this PR, the current solution is not a blocker if you can't find a better way we can file an issue and tackle it later.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/3301#pullrequestreview-778658131
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20211013/5b47280e/attachment.htm>