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

[Do not merge] Basic registration #149

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

wbamberg
Copy link

@wbamberg wbamberg commented Aug 17, 2024

This is part of a fix for mdn/content#35484.

@hamishwillee suggested the way to address this was first to have updated tutorial code and then to have updated docs, which makes sense. However I think it might make sense to document this in two parts:

  1. the simplest way to add registration
  2. extending this to include an email address (which everyone's going to want to do)

So this PR is to demo the first of these, which uses UserCreationForm directly. So this PR is not intended to be merged, it's just presented for feedback. I've tested it out and it seems to work fine. But I know almost nothing about Django.

The second part would I think subclass UserCreationForm to capture an email address. (One piece missing that I don't know how to do, is that you are I think supposed to send and respond to a confirmation email, to make sure I guess that the new user has got the right email address.)

Copy link

It looks like this is your first pull request. 🎉 Thank you for your contribution! One of the project maintainers will triage and assign the pull request for review. We appreciate your patience. To safeguard the health of the project, please take a moment to read our code of conduct.

@hamishwillee
Copy link
Collaborator

@wbamberg Looks great.

  1. This is is how I would do it too for the basic stuff. For the final tutorial we might include this method and then show how to extend it.
  2. Adding a password to the form is easy, and we've kind of shown it before for Model Forms in tutorial.
    class PasswordUserCreationForm(BaseUserCreationForm):
     """Form for signing on a new user with an email address"""
     email = forms.EmailField(label='email',required=True)
    
     class Meta:
         model = User
         fields = ('username', 'email', 'password1', 'password2')
    Of course getting it to work requires some bits and bobs, so I created a PR that work for you [Do not merge] Basic registration with password #150
  3. In final version of this, might be good to write a tests. They seem very pointless in many cases, but has been very handy for me when upgrading to track breakages.

Your follow on question about how to pipe in email is much harder.

  1. The current tutorial avoided this by not setting up an email server. I just use a console so I can see what emails would be sent EMAIL_BACKEND = 'django.core.mail.backends.console.EmailBackend' in settings.py.
  2. Setting up email is not too hard, but we need to think about this carefully because it is a pain for users, and we want to make it generic. Plenty of tutorials around, such as https://medium.com/django-unleashed/configuring-smtp-server-in-django-a-comprehensive-guide-91810a2bca3f and https://mailtrap.io/blog/django-send-email/
  3. I am not sure exactly how you would do this.
    My thinking is:
    • Extend User model with a new field to indicate whether email has been validated.
    • Create a can_borrow permission that you don't get by default but which can be enabled by email confirmation.
    • Two ways then to do this.
      • Have a separate form to enter code from email and trigger granting of that permission. Not sure how you hook it in yet, but I know I could do this. You would have to store or generate the key associated with user.
      • Have Django regularly fetch the email using a library like celery, parse confirmation, auto-update the database.

Both look a bit tedious.

@wbamberg
Copy link
Author

wbamberg commented Aug 19, 2024

Two ways then to do this.

  • Have a separate form to enter code from email and trigger granting of that permission. Not sure how you hook it in yet, but I know I could do this. You would have to store or generate the key associated with user.
  • Have Django regularly fetch the email using a library like celery, parse confirmation, auto-update the database.

I like the first better than the second. A variant would be to include a link in the email for them to click, like how you do password reset. That would work, wouldn't it?

In fact a lot of email verification seems like password reset, doesn't it? Create a magic token with an expiry, store it alongside the account, check it when the user provides it (e.g. as a URL parameter by clicking the link you sent them). If it checks out, mark the email as confirmed.

(https://www.troyhunt.com/everything-you-ever-wanted-to-know/ is great on this, especially the flow chart)

However, I think in the interests of getting something done I would like to hand-wave email verification for now (as we already handwave password reset).

@wbamberg
Copy link
Author

wbamberg commented Aug 19, 2024

I think it's also time to talk about structure here. The difficulty really is that:

  • ideally this belongs in the authentication & permissions chapter
  • but it depends on the forms chapter, which currently follows the authentication chapter, because
  • the example and challenge used in the forms chapter (renewing books) depend on permissions
  1. The easiest thing to do is:
  • insert "Building a registration page" into the forms chapter, either after ModelForms (which it depends on) or after Generic editing views (which is the last chapter before the challenge).
  • add a note in the authentication & permissions chapter, about why we're not talking about registration yet, and linking forward to "Building a registration page"

I feel though that this gives us a fairly chaotic structure, and forms is already a long and complicated chapter.

  1. The best thing to do (IMO) is
  • have a different example (and challenge) for the forms chapter, that does not depend on permissions
  • move the forms chapter before the authentication & permissions chapter
  • document registration along with the other authentication functions, in the authentication & permissions chapter
  1. But that looks like a lot of work! A compromise might be:
  • move the forms chapter before the authentication & permissions chapter
  • keep the existing example and challenge in forms, but remove the stuff about permissions, so it's just available to anyone
  • in the authentication & permissions chapter, explain how the stuff you just added to renew books &c should not be available to just anyone, so we'll need to learn about authentication & permissions, and lead into it that way.
  • document registration along with the other authentication functions, in the authentication & permissions chapter

I think I'm fine to do (1), and more or less happy to do (3), but (2) sounds like a bigger task than I want to take on for this. WDYT?

@hamishwillee
Copy link
Collaborator

I like the first better than the second. A variant would be to include a link in the email for them to click, like how you do password reset. That would work, wouldn't it?

yes. I like it better because if you use some kind of explicit entry from the email there is no fetching required (easier to setup) and the approach might be reused for, say, an authenticator code.

Yes in general to all, and yes we could handwave the email verification. There is nothing to stop us for demonstration purposes seeing the logged email and copying the provided link into the reset form (or however it is done)

@hamishwillee
Copy link
Collaborator

hamishwillee commented Aug 20, 2024

I think that 2 and 3 will be more or less the same amount of work, and more or less the same end result. In both cases you're trying to move forms before authentication and permissions, and in either case that will mean having form examples without permissions.

The reason I prefer 3 in this case is that I think the examples are sensible for a library - coming up with new ones and a new challenge isn't IMO necessary or better. I also think you can reasonably say in the forms chapter "we'll look at the permissions aspects later" and tidy up the challenge appropriately. This will make the whole authentication topic more complicated though, because you'll have to backfill updates to how things work with forms.

Other concerns are that authentication section currently follows sessions chapter because they build on each other - at least mentally, and otherwise the sessions topic feels like an orphan. So if doing this I would also move session ahead of forms too.

I am not convinced that this organization is better than proposal "1". It would be nice to group all the authentication stuff together, and that is what is driving your thinking here. Others might argue that it is more important to group all the things to do with working with forms together.

I'm not objective here - I see having a small section on signup form in "Forms" as a constrained job which logically groups forms stuff, and actually is quite nice in that it provides another good example of how you can build a model form and use permissions with it.

Where this leaves us is that I won't know for sure until the final form of the example is decided. If the interactions that need to be documented for sign up end up being far more than "just a form"( as I describe above) then your option "3" makes more sense because we don't want to talk too much about authentication workflows in a forms topic.

@wbamberg
Copy link
Author

wbamberg commented Aug 20, 2024

I think that 2 and 3 will be more or less the same amount of work, and more or less the same end result. In both cases you're trying to move forms before authentication and permissions, and in either case that will mean having form examples without permissions.

I think 2 is more work because we need to come up with new examples. As you know thinking of good examples is hard!

The reason I prefer 3 in this case is that I think the examples are sensible for a library - coming up with new ones and a new challenge isn't IMO necessary or better. I also think you can reasonably say in the forms chapter "we'll look at the permissions aspects later" and tidy up the challenge appropriately. This will make the whole authentication topic more complicated though, because you'll have to backfill updates to how things work with forms.

Other concerns are that authentication section currently follows sessions chapter because they build on each other - at least mentally, and otherwise the sessions topic feels like an orphan. So if doing this I would also move session ahead of forms too.

+1 definitely.

I am not convinced that this organization is better than proposal "1". It would be nice to group all the authentication stuff together, and that is what is driving your thinking here.

Others might argue that it is more important to group all the things to do with working with forms together.

I don't think auth is a "thing to do with working with forms" though (in the way that say models, view, and templates are). Forms are more fundamental. Auth builds on and uses forms. Even the current auth chapter uses forms.

I'm not objective here - I see having a small section on signup form in "Forms" as a constrained job which logically groups forms stuff, and actually is quite nice in that it provides another good example of how you can build a model form and use permissions with it.

Where this leaves us is that I won't know for sure until the final form of the example is decided. If the interactions that need to be documented for sign up end up being far more than "just a form"( as I describe above) then your option "3" makes more sense because we don't want to talk too much about authentication workflows in a forms topic.

Then I think I should write up a PR based on (1) and we'll see what it looks like. My feeling is that it will be messy, hence my suggestion of (3). But hopefully we will get a clearer idea when it is in front of us. And this is your baby, so if you are happy with (1) so am I.

@hamishwillee
Copy link
Collaborator

That works for me. Or you can try for 3 if you feel that will definitely be better. I wouldn't block an attempt because I can see the pros and cons. I'd get the code working the way you want it first before doing anything else though.

@github-actions github-actions bot added the idle Issues and pull requests with no activity for three months. label Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idle Issues and pull requests with no activity for three months.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants