Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add authentication #24

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Add authentication #24

wants to merge 10 commits into from

Conversation

DamiAdesola
Copy link

My Changes
-Added Swagger, to allow better visualisation of API(added in requirements.txt file).
-Added Authentication app, which allows users to register.
-Added Email Verification, which sends users verification tokens, which can then be successfully verified on the API (Email used is a test Gmail account. Details are in the setting.py file in ‘webapp’.

  • Added all Serializations and Models required to, register verify and login Users
    -Added Jwt for token generation (added in requirements.txt file)
    -Two tokens are generated, refresh and access
  • Added login through Swagger interface, which requires users to input generated token, to access information from the API.
    -Added Renderers in order to ensure consistent responses in the API, especially when an error is generated.

email:lmvptest@gmail.com
password:lmvptest_user

From there, i have a sample user created for testing purposes , on functions like 'auth/login/'
Example:{
"username": "testuser1",
"email": "user@example.com",
"password": "test1234"
}

Oluwadamilola Adesola added 2 commits August 2, 2020 10:12
-Added Swagger, to allow better visualisation of API(added in requirements.txt file).
-Added Authentication app, which allows users to register.
-Added Email Verification, which sends users verification tokens, which can then be successfully verified on the API (Email used is a test Gmail account. Details are in the setting.py file in ‘webapp’.
- Added all Serializations and Models required to, register verify and login Users
-Added Jwt for token generation (added in requirements.txt file)
-Two tokens are generated, refresh and access
- Added login through Swagger interface, which requires users to input generated token, to access information from the API.
-Added Renderers in order to ensure consistent responses in the API, especially when an error is generated.
@DamiAdesola DamiAdesola added the enhancement New feature or request label Aug 3, 2020
@jbinvnt
Copy link
Member

jbinvnt commented Aug 3, 2020

Can you please rename this request to:

Add authentication and Swagger

This follows the naming convention for commits

@jbinvnt
Copy link
Member

jbinvnt commented Aug 3, 2020

Also, Git should not be tracking .DS_Store files. Can you please add the following line to the end of the .gitignore file:

.DS_Store

Then run git rm --cached .DS_Store, then git add . then git commit -m "Remove DS Store files" then git push.

Once this is done we will not need to worry about these files again.

django/webapp/urls.py Outdated Show resolved Hide resolved
@jbinvnt jbinvnt linked an issue Aug 3, 2020 that may be closed by this pull request
@jbinvnt
Copy link
Member

jbinvnt commented Aug 3, 2020

@DamiAdesola Impressive work! It will take me some more time to review everything, and it's looking good so far. As a general rule, two separate tasks like adding authentication and visualization should be handled in separate issues/branches/pull requests. But this time it's okay to keep them together because you've already done the work on this branch.

@DamiAdesola DamiAdesola changed the title Lmvp 6 Add authentication and Swagger Aug 4, 2020
@DamiAdesola
Copy link
Author

DamiAdesola commented Aug 4, 2020

@DamiAdesola Impressive work! It will take me some more time to review everything, and it's looking good so far. As a general rule, two separate tasks like adding authentication and visualization should be handled in separate issues/branches/pull requests. But this time it's okay to keep them together because you've already done the work on this branch.

Thanks, @jbinvnt, I can separate them if you want. Into two different commits and two different branches.

Oluwadamilola Adesola added 3 commits August 4, 2020 11:43
-Added Swagger, to allow better visualisation of API(added in requirements.txt file).
-Added Authentication app, which allows users to register.
-Added Email Verification, which sends users verification tokens, which can then be successfully verified on the API (Email used is a test Gmail account. Details are in the setting.py file in ‘webapp’.
- Added all Serializations and Models required to, register verify and login Users
-Added Jwt for token generation (added in requirements.txt file)
-Two tokens are generated, refresh and access
- Added login through Swagger interface, which requires users to input generated token, to access information from the API.
-Added Renderers in order to ensure consistent responses in the API, especially when an error is generated.
@DamiAdesola
Copy link
Author

Hi @jbinvnt , I have taken your comment into consideration and separated, Authentication and (Authentication with Swagger) into two separate branches. This branch LMVP-6 does not have the swagger visualisation installed, but the branck LMVP-Swagger has swagger

@DamiAdesola DamiAdesola changed the title Add authentication and Swagger Add Authentication Aug 4, 2020
@jbinvnt
Copy link
Member

jbinvnt commented Aug 4, 2020

I think it would be most efficient to focus on getting this branch ready to merge first, because it directly impacts the frontend tasks. Then we can look at visualization separately.

@jbinvnt
Copy link
Member

jbinvnt commented Aug 4, 2020

Can you please add to the README on this branch, giving instructions for any steps needed with manage.py to create users/superusers for authentication? Additionally, I think it would make sense to have information about how to create a JWT and use it in API requests. This will help with both writing tests and with frontend development.

@DamiAdesola
Copy link
Author

This will help with both writing tests and with frontend development.

Can you please add to the README on this branch, giving instructions for any steps needed with manage.py to create users/superusers for authentication? Additionally, I think it would make sense to have information about how to create a JWT and use it in API requests. This will help with both writing tests and frontend development.

Hey, @jbinvnt thanks for the feedback. I will start working on getting that done, with all the information included.

@DamiAdesola
Copy link
Author

Hey @jbinvnt, i think in order to make it easier and smoother to collaborate, with the front end team, we use swagger visualisation. This is because while running some test and updating the read I noticed some things would clash with each other. I would be able to also add to the read me how to effectively use the swagger interface.

@DamiAdesola DamiAdesola changed the title Add Authentication Add authentication and Swagger Aug 5, 2020
@jbinvnt
Copy link
Member

jbinvnt commented Aug 5, 2020

@DamiAdesola to clarify, I agree that we should keep your changes that allow Swagger visualization. Just for organization, it would be helpful to have two different pull requests and branches. Let me know what you think of this idea for how to accomplish that without losing any of your work:

  1. Rename this pull request to "Add authentication" and keep it associated with LMVP-6. From now on this branch and pull request will be only for authentication features. This will be the next pull request that we merge into master
  2. Create a new pull request called "Add Swagger visualization" and link it to the LMVP-26 branch and issue Add API visualization #26 (I have just created a new issue Add API visualization #26 for visualization and a branch called LMVP-26. The branch is identical to the old LMVP-Swagger branch, but it has a number corresponding to an issue number--following the convention)
  3. From then on, all changes related to visualization will happen in the visualization pull request for the LMVP-26 branch. The visualization will get merged after we merge this pull request, to avoid conflicts. The LMVP-26 branch still has some authentication changes, but we will intentionally ignore those during the rebase process because your most up-to-date work on authentication will have previously been added to the master branch from this pull request.

@DamiAdesola
Copy link
Author

Hey @jbinvnt ,Thanks for the message.
I get what you mean, but if the front-end developers are fine using postman to test the API, then it's fine, but my current update read me before I saw your message has instructions in relation to the swagger interface, so if you don't mind can you take a look test it out a bit and then if you don't approve of it, I have the authentication file without any swagger updates.

Oluwadamilola Adesola added 2 commits August 6, 2020 00:45
@DamiAdesola
Copy link
Author

Hey @jbinvnt ,Thanks for the message.
I get what you mean, but if the front-end developers are fine using postman to test the API, then it's fine, but my current update read me before I saw your message has instructions in relation to the swagger interface, so if you don't mind can you take a look test it out a bit and then if you don't approve of it, I have the authentication file without any swagger updates.

So before I do any more work and change anything please have a look at the pull request for LMVP-26, and if you think you will prefer to have it differently then I understand.

@DamiAdesola DamiAdesola changed the title Add authentication and Swagger Add authentication Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User authentication
2 participants