Skip to content

Initial API Structure + Auth Route#4976

Merged
xihai01 merged 15 commits into
rubyforgood:mainfrom
xihai01:api
Oct 16, 2023
Merged

Initial API Structure + Auth Route#4976
xihai01 merged 15 commits into
rubyforgood:mainfrom
xihai01:api

Conversation

@xihai01
Copy link
Copy Markdown
Collaborator

@xihai01 xihai01 commented Jul 11, 2023

As per discussions in #4934

What github issue is this PR for, if any?

This PR sets up the initial structure of the API for the app we are building. An auth route for sign in is also added.
Unlike before, the api routes will start with the /api/v1/* pattern.
It isn't linked to any issue in this repo - we forked a copy of this repo and are working on that forked copy.

What changed, and why?

No longer using JWT with devise-jwt gem - instead, opted for a token based authentication approach.
In a nutshell, a token col is created for every row in the user table. The server sends this token in its response body to the client when they sign in. For future requests to server, the client sends this token in its request authorization header.
The reason is because configuring devise to also authenticate (in addition with the current existing web app auth) using devise-jwt was a major headache. I guess it is doable, but would require too much altering existing controllers and files.

I also added active model serializers to help structuring the json data.

Please take a look at the code and give me some feedback! Thank you.

How is this tested? (please write tests!) 💖💪

I created api specs for good/bad login to verify correct headers to ensure correct responses were sent such as token and error msg for bad credentials.
rswag gem used to create api documentations.
Postman was used to verify by hitting the login route for the api.

Screenshots please :)

Postman: Successful login

Screenshot from 2023-07-11 12-21-25

Postman: Unsuccessful login

Screenshot from 2023-07-11 12-21-52

Feedback please? (optional)

We are very interested in your feedback! Please give us some :) https://forms.gle/1D5ACNgTs2u9gSdh9

@xihai01 xihai01 added the codethechange code.the.change developers label Jul 11, 2023
@xihai01 xihai01 self-assigned this Jul 11, 2023
@github-actions github-actions Bot added dependencies Touches dependency files ruby Touches Ruby code zzz_archived: Tests no-skin-tone labels Jul 11, 2023
@xihai01
Copy link
Copy Markdown
Collaborator Author

xihai01 commented Jul 12, 2023

@seanmarcia can I get some feedback please? Thank you!

Copy link
Copy Markdown
Collaborator

@michaelruhl michaelruhl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, I'd be curious to know if @seanmarcia has any tips though :)

@seanmarcia
Copy link
Copy Markdown
Member

Hey, sorry for the slow reply. Our large in-person Ruby for Good event is happening this weekend so things have been pretty hectic.

Yes, this is a much better direction and is a great start!

@xihai01 xihai01 requested a review from michaelruhl August 7, 2023 20:06
@xihai01 xihai01 marked this pull request as ready for review August 7, 2023 20:07
@xihai01 xihai01 changed the title Initial API Structure + Auth Route (WIP) Initial API Structure + Auth Route Aug 7, 2023
@xihai01
Copy link
Copy Markdown
Collaborator Author

xihai01 commented Aug 7, 2023

not to sure why rspec and docker is failing..

Copy link
Copy Markdown
Collaborator

@littleforest littleforest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting a start on this. We will also need API documentation. I personally like to use the rswag gem which has you create request specs, and from there it will generate Swagger API documentation.

Comment thread Gemfile Outdated
Comment thread app/controllers/api/v1/base_controller.rb Outdated
Comment thread app/controllers/api/v1/base_controller.rb Outdated
Comment thread app/controllers/api/v1/users/sessions_controller.rb Outdated
Comment thread app/controllers/api/v1/users/sessions_controller.rb Outdated
Comment thread spec/controllers/api/v1/users/sessions_controller_spec.rb Outdated
Comment thread spec/controllers/api/v1/base_controller_spec.rb Outdated
Comment thread spec/controllers/api/v1/base_controller_spec.rb Outdated
Comment thread spec/controllers/api/v1/base_controller_spec.rb Outdated
Comment thread spec/controllers/api/v1/base_controller_spec.rb Outdated
@xihai01 xihai01 requested a review from littleforest August 16, 2023 21:40
@littleforest
Copy link
Copy Markdown
Collaborator

littleforest commented Aug 18, 2023

@xihai01 thanks for your changes. There are a lot of unaddressed comments and requests in the PR from the previous review. Also, could you please merge or rebase and fix the merge conflict?

@xihai01
Copy link
Copy Markdown
Collaborator Author

xihai01 commented Aug 18, 2023

@xihai01 thanks for your changes. There are a lot of unaddressed comments and requests in the PR from the previous review. Also, could you please merge or rebase and fix the merge conflict?

ah, my bad. It seems I missed them. I'll work on it right away!

@xihai01
Copy link
Copy Markdown
Collaborator Author

xihai01 commented Aug 31, 2023

@littleforest I've addressed your suggestions and changes!

@xihai01
Copy link
Copy Markdown
Collaborator Author

xihai01 commented Sep 28, 2023

@littleforest any thoughts? Is it good to merge?

Copy link
Copy Markdown
Collaborator

@littleforest littleforest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xihai01 thank you for addressing all of those changes. Sorry for the delay in review.

Rails.application.routes.draw do
get "/index", to: "base#index"
end
end
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is okay for now, but let's remove once we have a route we can actually test authentication against.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright.

@littleforest
Copy link
Copy Markdown
Collaborator

@xihai01 looks like there are a number of merge conflicts now (sorry again for the delay). Can you merge in main and address the conflicts?

@xihai01 xihai01 merged commit 7cae9aa into rubyforgood:main Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codethechange code.the.change developers dependencies Touches dependency files ruby Touches Ruby code 🧪 Tests Tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants