Modify the way "Friends" are added #5261
Conversation
config/routes.rb
Outdated
| match "/user/:display_name/make_friend" => "friendships#make_friend", :via => [:get, :post], :as => "make_friend" | ||
| match "/user/:display_name/remove_friend" => "friendships#remove_friend", :via => [:get, :post], :as => "remove_friend" | ||
| match "/user/:display_name/follow_user" => "friendships#follow_user", :via => [:get, :post], :as => "follow_user" | ||
| match "/user/:display_name/unfollow_user" => "friendships#unfollow_user", :via => [:get, :post], :as => "unfollow_user" |
There was a problem hiding this comment.
I think the URL here could just be /follow and /unfollow and we don't really need the _user suffix? Possibly the actions should be create and destroy though? Probably the controller should be renamed to though I'm not sure what to...
There was a problem hiding this comment.
From a quick look at some gems that implement this functionality (1, 2) the convention is to use "follow" as a noun, so table is follows, controller is follows_controller, model is Follow etc. But I think we should rename the controllers/models separately.
What it does suggest though is that we should use resourceful routing with this in mind, so for example "/user/:display_name/follow" with create/destroy actions (and new/edit for the GET pages). See user_mutes for an example in the routes.rb file that's a bit like this.
There was a problem hiding this comment.
When I started implementing same architecture as user_mutes, it caused to change some more files and add some complexities. This PR is already +104 -104 line change. I think, it will be better to create a separate PR about changing routing architecture of the follow functionality after this PR will be merged to avoid making it even bigger and more complex.
There was a problem hiding this comment.
This PR is already +104 -104 line change.
You could have a smaller PR if you only changed the texts but not the routes. Don't we have the existing urls published in notification emails (and maybe in other places)? I don't see redirects from old to new here.
There was a problem hiding this comment.
@AntonKhorev app/views/user_mailer/friendship_notification.html.erb and app/views/user_mailer/friendship_notification.text.erb changes are about redirections from mail notifications. For the consistent visual output this PR is focused on changing every occurrence of "friend" on user side and changing them to "following". Main focus is what will users see (including both texts and URLs). I agree that it will be great if friendship functionality is refactored (currently it is just renamed), but those changes are out of scope of this PR and will be done as a separate PR as soon as this PR will be merged (adding them to this PR will complicate things as there will be not only user side changes, but logical changes in routes.rb, ability.rb and etc. which will need much deeper testing than just text and URL comparisons).
There was a problem hiding this comment.
app/views/user_mailer/friendship_notification.html.erbandapp/views/user_mailer/friendship_notification.text.erbchanges are about redirections from mail notifications.
I don't see any redirects from old GET paths to new GET paths (but I see changes in tests that checked the paths).
For the consistent visual output this PR is focused on changing every occurrence of "friend" on user side and changing them to "following". Main focus is what will users see (including both texts and URLs).
Maybe it shouldn't focus on both?
There was a problem hiding this comment.
PR was updated accordingly to include refactor and renaming of the friendship functionality.
nertc
force-pushed
the
issues_3310_modify_the_way_friends_are_added
branch
from
e1a1b87
to
8393857
Compare
|
Thank you for the review. This PR was updated according to recommendations. |
nertc
force-pushed
the
issues_3310_modify_the_way_friends_are_added
branch
from
8393857
to
02bede7
Compare
Generated by 🚫 Danger |
|
PR was updated. Now friendship controller and model are refactored to be named as "follow" and also implement standard CRUD architecture using |
| scope "/user/:display_name" do | ||
| resource :follow, :only => [:create, :destroy], :path => "follow" | ||
| end | ||
| get "/user/:display_name/follow" => "follows#index" |
There was a problem hiding this comment.
Why index? Can you have multiple follows for one :display_name?
There was a problem hiding this comment.
It's a page that either shows unfollow functionality or follow functionality based on if user already follows :display_name. As it is default GET method for this functionality (while other ones are POST and DELETE), I decided to name it follows#index. If you have an idea of a better name, I am open to it.
This PR addresses "Modify the way 'Friends' are added" issue mentioned in #3310
This PR replaces all occurrences of "friends" to "followers", including texts, tests and URLs.
This PR was not broken down to smaller pieces to maintain consistency and avoid different names of the same functionality.