Communities - communities #3717

openbrian · 2022-09-24T19:50:10Z

Microcosms was renamed to communities See #3683 for the PR before this one.

openbrian · 2022-10-16T19:07:51Z

@gravitystorm @tomhughes Can either of you enable workflows for this PR?

gravitystorm · 2022-10-27T10:31:08Z

app/models/client_application.rb

@@ -39,6 +39,7 @@ class ClientApplication < ApplicationRecord

  validates :key, :presence => true, :uniqueness => true
  validates :name, :url, :secret, :presence => true
+  # TODO: Consider using UrlValidator from validate_url gem.


Turns out I've considered this before! But I'd forgotten about that. Unfortunately it won't work. validate_url requires a list of permitted schemes, but for oauth callbacks any scheme needs to be allowed. See perfectline/validates_url#99

I think this comment can be removed.

gravitystorm · 2022-11-23T15:08:59Z

For those who are interested in this pull request, @openbrian and I had a conversation about it a couple of weeks ago. The next steps involve working to align this with the new osm-community-index based list of communities, including topics such as how we might like to display both lists of communities, and also how to resolve the (inevitable) situation where a community based entirely on the website (i.e. no external presence, just membership etc through these PRs) ends up listed in the OCI, or existing communities listed in OCI also create a community here.

We also covered the need to avoid merge commits, to rebase this PR onto recent master, and to rework the PR to avoid fixup commits (and generally avoid unnecessary complexity in the history and e.g. git blame). These points are also now covered in the Pull Requests section of CONTRIBUTING.md.

gravitystorm · 2023-01-25T16:52:54Z

Hi @openbrian - just wondering if you've had any chance to work on this recently, or if you have any updates for us. When we discussed what was needed for reviewing this PR a few months ago, the main thing was to work on reworking the branch so that it doesn't have so many fixup commits.

It'll also need rebasing onto master again, since the (other) communities branch has been merged, and we need to resolve the conflicts with that.

If there's anything I can do to help, please let me know!

openbrian · 2023-01-26T01:11:05Z

@gravitystorm Indeed over the past 2 weekends, I've reduced the number of commits from 106 to about 12. I now consider myself an interactive rebase-a-holic. I'll push soon.

openbrian · 2023-01-28T15:44:19Z

I shuffled a LOT of commits. I've already rebased master and resolved conflicts. All were minor.

I'd like to make sure each one build and tests successfully. I find this helps if we ever need to git-bisecting a bug.

Eventually, I intend to git push --force here. Let me know if that's an issue.

gravitystorm · 2023-02-08T16:37:08Z

Eventually, I intend to git push --force here. Let me know if that's an issue.

Force-pushes to PR branches are completely welcome, no issue with that whatsoever.

openbrian · 2023-02-23T02:30:06Z

Failing here diff -uw db/structure.expected db/structure.actual. I'll have to make sure to get the order of the SQL statement right.

openbrian · 2023-03-03T04:50:42Z

Fixed the ordering of statements in db/structure.sql. Will work on other checks next.

gravitystorm

This is only a partial review, since it's a big PR and I've run out of time today.

A few other thoughts here as a reminder for myself for the next review:

Resolve communities vs communities name clash
Consider if communities is already overloaded in the wider osm community e.g. community.osm.org etc
list of commits still has fixup commits
javascript etc unreviewed
can this be slimmed down further e.g. move clever sorting to followup PR etc

gravitystorm · 2023-06-07T15:12:25Z

config/routes.rb

@@ -147,7 +147,7 @@
  get "/help" => "site#help"
  get "/about/:about_locale" => "site#about"
  get "/about" => "site#about"
-  get "/communities" => "site#communities"
+  get "/community_index" => "site#communities"


We need to think carefully about doing this, since it would break all external links to the existing communities page.

Understood. Let's look at the dev site when it's up and running to find the best solution.

The thing is communities is a route resource, so it will get a /communities URL. Maybe that can be customized to avoid this collision.

gravitystorm · 2023-06-07T15:16:53Z

config/routes.rb

+  # user nested resources
+  resources :users, :path => "user", :param => :display_name do
+    resources :communities, :only => [:index]
+  end


This could be combined with the existing resources :users statement, so that resources :users isn't defined twice

gravitystorm · 2023-06-07T15:29:59Z

test/controllers/communities_controller_test.rb

+  def test_edit_get_no_session
+    # arrange
+    c = create(:community)
+    # act
+    get edit_community_path(c)
+    # assert
+    assert_response :redirect
+    assert_redirected_to login_path(:referer => edit_community_path(c))
+  end


I don't think the hundreds of repeated # arrange, # act, # assert comments are helpful - they are just more text to read and maintain (e.g. when refactoring tests).

I suggest removing the # arrange comments entirely, and changing the other two to blank lines. For example:

def test_edit_get_no_session c = create(:community) get edit_community_path(c) assert_response :redirect assert_redirected_to login_path(:referer => edit_community_path(c)) end

This keeps the readability of having the three sections being distinct from one another, but with less clutter.

I can do that. Just to be clear, it is a unit test design pattern that goes back 20 years. http://wiki.c2.com/?ArrangeActAssert

Sure, the pattern is good, full agreement there. But as the link you gave also suggests, please use blank lines rather than comments:

"Each method should group these functional sections, separated by blank lines: "

gravitystorm · 2023-06-07T15:41:25Z

app/models/community_link.rb

+
+class CommunityLink < ApplicationRecord
+  belongs_to :community
+  validates :site, :presence => true, :length => 1..255, :characters => true


From previous review, "site" here is not clear. Perhaps one of: name, label, text.

(You previously suggested "link" but I think that's even more confusing with "link" vs "url"!)

"text" - done

gravitystorm · 2023-06-07T15:43:49Z

config/locales/en.yml

+    label:
+      community:
+        description: "Description"
+        lat: "Latitude"


I thought the model had attributes of "latitude", not "lat"? Same for lon.

gravitystorm · 2023-06-07T15:45:42Z

config/locales/en.yml

    submit:
+      community:
+        create: "Create Community"


Isn't "Create %{model_name}" the default here?

gravitystorm · 2023-06-07T15:48:47Z

lib/osm.rb

@@ -527,4 +527,9 @@ def self.http_client
  def self.maxmind_database
    @maxmind_database ||= MaxMindDB.new(Settings.maxmind_database) if Settings.key?(:maxmind_database)
  end
+
+  # Convert any longitude input to the range -180..180.
+  def self.normalize_longitude(longitude_in)


just longitude since there's no other longitude in this function

gravitystorm · 2023-06-07T15:49:24Z

test/abilities/abilities_test.rb

@@ -59,6 +71,15 @@ class UserAbilityTest < AbilityTest
      assert ability.cannot?(action, Issue), "should not be able to #{action} Issues"
    end
  end
+
+  # This does not take into account object level access control.


note to self: should it?

Yes. Anyone can create a community. But a community can only be edited by its owner. I've added test cases.

gravitystorm · 2023-06-07T16:01:34Z

test/factories/communities.rb

+    lat1 = Random.rand(-90.0..90.0)
+    lat2 = Random.rand(-90.0..90.0)
+    lat1, lat2 = lat2, lat1 if lat2 < lat1


A more compact version, using sort:

lat1, lat2 = [Random.rand(-90.0..90.0), Random.rand(-90.0..90.0)].sort

To avoid duplicating the random number bit, you can use an Array construction with a block, but it's perhaps less readable? Probably a personal preference between these two.

lat1, lat2 = Array.new(2) { Random.rand(-90.0..90.0) }.sort

Nice, I went with the Array construction with a block.

openbrian · 2023-06-07T18:33:04Z

Thanks for the feedback so far.

hallahan · 2023-06-10T22:14:41Z

LGTM!!!

openbrian · 2024-05-31T01:39:15Z

I've removed all references to "microcosm". I've consolidated all commits into 2 commits:

create communities
create community links

Later PRs will have members, events, and rsvps.

While a community will eventually have multiple organizers, we need at least one person to be responsible if an issue is raised. The community has one owner. In later PRs, there will be multiple members and multiple organizers.

Links must be https. Add validate_url ruby gem to validate the urls. Add not null on link site and url. Add relevant FK.

gravitystorm

I'm at a bit of a loss how to approach this review, since the diff is huge (1700+ additions).

So we could do this in two ways - I can go through the details (e.g. <label> should only be used for forms, it shouldn't have custom CSS, the <h1> in communities#index uses lego translations etc) or we can tackle the big picture (e.g. renaming the existing Communities model/controller, whether we want Yet Another OSM Thing Called Communities, etc).

What would be more helpful? Or should I just send you a stream of feedback as I go?

AntonKhorev · 2024-09-06T04:25:01Z

config/routes.rb

+  resources :communities do
+    resources :community_links, :only => [:create, :index, :new]
+  end
+  resources :community_links, :only => [:destroy, :edit, :update]


Doesn't :shallow option of resources do the same thing?

openbrian mentioned this pull request Sep 24, 2022

Microcosms microcosms #3683

Closed

gravitystorm reviewed Oct 27, 2022

View reviewed changes

This was referenced Nov 23, 2022

Communities membership #3742

Draft

Add microcosms to the website #2390

Closed

LanesGood mentioned this pull request Jan 10, 2023

Question: osm-teams and microcosm developmentseed/osm-teams#125

Closed

openbrian force-pushed the communities-communities branch from ee2085c to 8a6ecb2 Compare January 14, 2023 20:13

openbrian force-pushed the communities-communities branch 5 times, most recently from 8afa8e6 to 93763d0 Compare February 23, 2023 02:11

openbrian force-pushed the communities-communities branch from 93763d0 to 6097680 Compare March 3, 2023 04:30

openbrian force-pushed the communities-communities branch 6 times, most recently from a71d408 to 22c4492 Compare March 4, 2023 19:14

openbrian force-pushed the communities-communities branch 4 times, most recently from 38106a3 to 33a56ab Compare April 15, 2023 17:02

openbrian force-pushed the communities-communities branch from 5d60113 to 430899f Compare April 19, 2023 03:41

openbrian force-pushed the communities-communities branch from 430899f to 2921f5d Compare May 10, 2023 00:45

openbrian force-pushed the communities-communities branch from 2921f5d to 2eb53c1 Compare May 25, 2023 18:57

gravitystorm requested changes Jun 7, 2023

View reviewed changes

openbrian force-pushed the communities-communities branch 6 times, most recently from c7b51f1 to 65caec0 Compare May 31, 2024 01:37

openbrian force-pushed the communities-communities branch 4 times, most recently from 7c41be5 to 468d58c Compare July 31, 2024 12:42

openbrian force-pushed the communities-communities branch 3 times, most recently from a002e2b to c5f7dca Compare August 8, 2024 12:39

openbrian requested a review from gravitystorm August 8, 2024 13:10

openbrian force-pushed the communities-communities branch 2 times, most recently from 1f84b71 to 9dd48e9 Compare August 9, 2024 02:40

openbrian added 2 commits August 9, 2024 09:10

Rename Community model to CommunityIndex.

9e53ac8

Add Community.

8895cfc

While a community will eventually have multiple organizers, we need at least one person to be responsible if an issue is raised. The community has one owner. In later PRs, there will be multiple members and multiple organizers.

openbrian force-pushed the communities-communities branch from 9dd48e9 to 1f59d7c Compare August 9, 2024 14:10

Create CommunityLinks.

706667e

Links must be https. Add validate_url ruby gem to validate the urls. Add not null on link site and url. Add relevant FK.

openbrian force-pushed the communities-communities branch from 1f59d7c to 706667e Compare August 9, 2024 15:40

gravitystorm reviewed Aug 21, 2024

View reviewed changes

AntonKhorev reviewed Sep 6, 2024

View reviewed changes

Communities - communities #3717

Are you sure you want to change the base?

Communities - communities #3717

Conversation

openbrian commented Sep 24, 2022

openbrian commented Oct 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gravitystorm commented Nov 23, 2022

gravitystorm commented Jan 25, 2023

openbrian commented Jan 26, 2023

openbrian commented Jan 28, 2023

gravitystorm commented Feb 8, 2023

openbrian commented Feb 23, 2023

openbrian commented Mar 3, 2023

gravitystorm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openbrian commented Jun 7, 2023

hallahan commented Jun 10, 2023

openbrian commented May 31, 2024

gravitystorm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment