Use user single resources instead of /users/me #44

MaicolBen · 2017-03-10T15:13:00Z

Resolves #33

Using /users/me make the next nested resources use the resource_id param instead of id. Also me isn't too much REST, it's recommended to use ids not hardcoded strings.

The downside of this, is that rails shares the same controller for the resources :users, so in the future if developers want to add it, what controller do you recommend? Or maybe rename the current one as user_controller and tell that to the resource route.

matiasmansilla1989 · 2017-03-14T15:31:57Z

apiary.apib

@@ -129,9 +129,49 @@ API BASE is an internal TopTier project created to facilitate and standardize de
                }
            }

+## Get other user's profile [/api/v1/users/{id}]


is this endpoint to get the current user profile? I think that this is to get any user profile and this one is to get the current user profile https://github.com/rootstrap/rails_api_base/pull/44/files#diff-2625016b50d68d922257f74801cac29cR53 right?

I copied/pasted 🙂 , the subtitle below is wrong, it should be Get user.

matiasmansilla1989

Great work!!!

MaicolBen · 2017-04-18T19:43:11Z

spec/requests/api/v1/users/update_spec.rb

@@ -1,23 +1,25 @@
 require 'rails_helper'

 describe 'PUT api/v1/users/me', type: :request do
-  let(:user) { create(:user) }
+  let(:user)             { create(:user) }
+  let(:api_v1_user_path) { '/api/v1/user' }


I don't like this hack, but this helpers returns /api/v1/user/:id otherwise.

That's a Rails bug right?

Seems to be, I couldn't find it in the rails issues

GAKINDUSTRIES · 2017-06-20T18:30:48Z

apiary.apib

+
+ Request (application/json)
+    + Parameters
+        + id (integer, required)


Isn't this implicit in the route? I think that if you specify just the route adding the id should be enough

It just to give more information about the params, this allows to use the apiary live test tool. So I think is fine here, but not in the projects

GAKINDUSTRIES · 2017-06-20T18:33:43Z

apiary.apib

+                {
+                  "email": "test@test.com",
+                  "username": "test",
+                  "first_name": "Juanito",


I will avoid all these things. I was going to create a PR to modify this. There's no need to specify the name of someone here. I would use common example names in programming such as John Doe or Jane Doe

@matiasmansilla1989 fault, I just copied/pasted haha

GAKINDUSTRIES · 2017-06-20T18:35:40Z

app/controllers/api/v1/users_controller.rb

@@ -4,10 +4,17 @@ module Api
  module V1
    class UsersController < Api::V1::ApiController
      def show
+        @user = User.find(params[:id])


I would prefer to use a helper method for user in this case since it's being used in the view rendered.

But there is one with current_user, which one do you recommend adding to the helper method?

I mean something like this:

helper_method :user def user @user ||= User.find(params[:id]) || current_user end

make sense?

Maybe User.find_by(id: params[:id]) , because find blows up when it can't find it

Exactly. Using find_by will be the correct way to go here 👍

santiagovidal

Nice! I don't like the me either.

If you have time, tests for profile would be nice.

santiagovidal · 2017-06-21T17:59:10Z

app/controllers/api/v1/users_controller.rb

@@ -16,6 +22,10 @@ def update
      def user_params
        params.require(:user).permit(:username, :first_name, :last_name, :email)
      end
+
+      def user
+        @user ||= User.find_by(id: params[:id]) || current_user


Another way to write this would be: params[:id].present? ? User.find(id: params[:id]) : current_user

It's longer but you would be avoiding the extra query if params[:id] is nil.

Also, if you receive an invalid id you will return an error instead of current_user

santiagovidal · 2017-06-21T18:03:34Z

spec/requests/api/v1/users/show_spec.rb

@@ -2,16 +2,17 @@

 describe 'GET api/v1/users/me', type: :request do


We need to change this too.

santiagovidal · 2017-06-21T18:09:58Z

spec/requests/api/v1/users/update_spec.rb

@@ -1,23 +1,25 @@
 require 'rails_helper'

 describe 'PUT api/v1/users/me', type: :request do
-  let(:user) { create(:user) }
+  let(:user)             { create(:user) }
+  let(:api_v1_user_path) { '/api/v1/user' }


That's a Rails bug right?

MaicolBen added the On hold label Mar 14, 2017

MaicolBen force-pushed the refactor/single-resource branch from 27cb458 to c69b8a8 Compare March 14, 2017 15:08

MaicolBen removed the On hold label Mar 14, 2017

matiasmansilla1989 requested changes Mar 14, 2017

View reviewed changes

MaicolBen force-pushed the refactor/single-resource branch from c69b8a8 to 124726d Compare March 14, 2017 15:39

matiasmansilla1989 approved these changes Mar 14, 2017

View reviewed changes

MaicolBen force-pushed the refactor/single-resource branch from 124726d to 324a1db Compare April 18, 2017 19:40

MaicolBen commented Apr 18, 2017

View reviewed changes

MaicolBen force-pushed the refactor/single-resource branch from 324a1db to 6dc5d61 Compare April 18, 2017 19:46

MaicolBen force-pushed the refactor/single-resource branch from 6dc5d61 to bf34219 Compare April 28, 2017 14:52

MaicolBen force-pushed the refactor/single-resource branch 2 times, most recently from 8e37790 to faffb4c Compare June 20, 2017 17:39

MaicolBen requested a review from GAKINDUSTRIES June 20, 2017 17:42

GAKINDUSTRIES reviewed Jun 20, 2017

View reviewed changes

GAKINDUSTRIES approved these changes Jun 20, 2017

View reviewed changes

MaicolBen force-pushed the refactor/single-resource branch 2 times, most recently from d1289a6 to b10e568 Compare June 20, 2017 19:08

MaicolBen requested a review from santiagovidal June 20, 2017 20:43

santiagovidal reviewed Jun 21, 2017

View reviewed changes

MaicolBen added the needs frontend change label Jul 4, 2017

This was referenced Jul 10, 2017

Change "me" routes rootstrap/ios-base#49

Closed

Change "me" routes rootstrap/react-native-base#3

Closed

glm4 mentioned this pull request Jul 12, 2017

Issue/fix me routes rootstrap/ios-base#50

Merged

MaicolBen force-pushed the refactor/single-resource branch from b10e568 to 61d61b8 Compare July 13, 2017 17:46

Use user single resources instead of /users/me

3d66914

MaicolBen force-pushed the refactor/single-resource branch from 61d61b8 to 3d66914 Compare July 13, 2017 17:54

santiagovidal approved these changes Jul 14, 2017

View reviewed changes

MaicolBen removed the needs frontend change label Jul 14, 2017

MaicolBen merged commit 5b007d6 into master Jul 14, 2017

MaicolBen deleted the refactor/single-resource branch July 14, 2017 18:10

JuannFerrari mentioned this pull request Oct 2, 2020

Remove profile endpoint and specs #267

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use user single resources instead of /users/me #44

Use user single resources instead of /users/me #44

MaicolBen commented Mar 10, 2017 •

edited

Loading

matiasmansilla1989 Mar 14, 2017 •

edited by MaicolBen

Loading

MaicolBen Mar 14, 2017 •

edited

Loading

matiasmansilla1989 left a comment

MaicolBen Apr 18, 2017 •

edited

Loading

GAKINDUSTRIES Jun 20, 2017

santiagovidal Jun 21, 2017

MaicolBen Jul 13, 2017

MaicolBen Jul 13, 2017

GAKINDUSTRIES Jun 20, 2017

MaicolBen Jun 20, 2017

GAKINDUSTRIES Jun 20, 2017

GAKINDUSTRIES Jun 20, 2017 •

edited

Loading

MaicolBen Jun 20, 2017

GAKINDUSTRIES Jun 20, 2017

MaicolBen Jun 20, 2017

GAKINDUSTRIES Jun 20, 2017 •

edited

Loading

MaicolBen Jun 20, 2017

GAKINDUSTRIES Jun 20, 2017

santiagovidal left a comment

santiagovidal Jun 21, 2017 •

edited

Loading

santiagovidal Jun 21, 2017

santiagovidal Jun 21, 2017

		@@ -2,16 +2,17 @@

		describe 'GET api/v1/users/me', type: :request do

Use user single resources instead of /users/me #44

Use user single resources instead of /users/me #44

Conversation

MaicolBen commented Mar 10, 2017 • edited Loading

matiasmansilla1989 Mar 14, 2017 • edited by MaicolBen Loading

Choose a reason for hiding this comment

MaicolBen Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

matiasmansilla1989 left a comment

Choose a reason for hiding this comment

MaicolBen Apr 18, 2017 • edited Loading

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

GAKINDUSTRIES Jun 20, 2017 • edited Loading

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

GAKINDUSTRIES Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santiagovidal left a comment

Choose a reason for hiding this comment

santiagovidal Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaicolBen commented Mar 10, 2017 •

edited

Loading

matiasmansilla1989 Mar 14, 2017 •

edited by MaicolBen

Loading

MaicolBen Mar 14, 2017 •

edited

Loading

MaicolBen Apr 18, 2017 •

edited

Loading

GAKINDUSTRIES Jun 20, 2017 •

edited

Loading

GAKINDUSTRIES Jun 20, 2017 •

edited

Loading

santiagovidal Jun 21, 2017 •

edited

Loading