dsc-umass / lmvp

Open source neural network versioning system that separates model management and training operations.
BSD 3-Clause "New" or "Revised" License
6 stars 3 forks source link

Add authentication #24

Open DamiAdesola opened 3 years ago

DamiAdesola commented 3 years ago

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’.

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" }

jbinvnt commented 3 years ago

Can you please rename this request to:

Add authentication and Swagger

This follows the naming convention for commits

jbinvnt commented 3 years ago

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.

jbinvnt commented 3 years ago

@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 commented 3 years ago

@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.

DamiAdesola commented 3 years ago

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

jbinvnt commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

jbinvnt commented 3 years ago

@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 #26 (I have just created a new issue #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 commented 3 years ago

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.

DamiAdesola commented 3 years ago

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.