astrosat / django-astrosat-users

Common backend library for Astrosat projects' user management
GNU General Public License v3.0
2 stars 0 forks source link

ongoing code review #6

Closed allynt closed 4 years ago

allynt commented 5 years ago

This is a bit of a brain dump, I'm not all that keen on sending it too you for that reason, neither have I looked at everything, but here goes:

django-astrosat-users

Readme

1. Should we have to do anything to use astrosat templates, I would expect not, that these would be the default. 2. You mention a typical error, but not how to resolve

Code

views_api

1. The code handles Json Web Tokens, not sure we should, they are okay in development, but everyone states they should not be used in production. They appear to be one of those tools that people use in demos or howtos on the web, but then go on to state that you should never use them in production. 2. Not sure anyone should be able to do a GET, does that mean I could do a get request to get your details? 3. Should IsAdminOrOwner be in this module, I thought there would be a separate permissions.py module, so we can extend it later on, if necessary. For instance, if we are to share some custom data, I would expect there to be some permission checking, so another class could be expected. 4. rest_disabled to me implies there is no API, not that signup/registration is closed, better renaming to be clearer. The file views_api.py suggests restfulness, don't think it adds anything by re-stating rest as function name prefix. 5. You state rest_auth isn't always generic enough to cope with the config you want, can you maybe explain further.

  1. Email confirmation, does this allow for multiple email addresses attached to the one account? 7.urls.py isn't quite restful, rather it uses different endpoints. take user-detail and user-update as an example, the same url could be used, but limit the method, GET, PUT etc. 8. Why allow post on rest-disabled, I took that to be a query endpoint. 9. In perform_create, I don't see where the token is used, can you explain.
  2. Sending an email verification on login if not verified sounds like it could cause confusion. I can see a number of users registering and forgetting an email has been sent, then logs in and another email is sent. I few of those will also try to click the link in the old email, is that still valid? It shouldn't be. Instead, just sending the message an email had been sent, should be enough, that message could include a link whereby if the user clicks it, a new verification email is sent, in case they accidentally deleted the email, never received it etc. 11. Raising an exception with status 200, doesn't feel right, maybe a 400 Bad Request would be more appropriate. 12. I think we will want a user viewset for all users and just active users. I can see an admin wanting to see all users, active or not and a user wanting to get a list of active users to be able to give that user permission to view some custom data.

    views

    1. What does a slug represent

    tokens

    1. Why import PasswordesetTokenGenerator, it never seems to be used.