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

Review custom auth #343

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

seddonym
Copy link
Collaborator

@seddonym seddonym commented Dec 20, 2024

I like the way in this chapter you give a flavour of what it's like to give something a try without TDD - and then return to it.

But - of all the chapters I've reviewed so far, I think this would most benefit with some more work. For me, reading the spike comes down to copy-pasting code that isn't really explained, and I don't think feel carried along with your thought process for that part of the chapter. So I think it would work better to move more of the explanation to the spike phase.

It also might work better to conduct the spike a little differently. For example:

  • You could start with laying out the high level design, with a sequence diagram.
  • Then you could stub out the user interface (without, say, any actual email - it could just print the link out to the terminal). Get us to click around and actually run it.
  • Then you could look at the User model. You could first look at the representation of the User and talk about why you don't want to use Django's built in.
  • Then you could look at the Token, whether it could be a field on the User model versus separate, versus foreign key, versus not stored in the database at all and use a signed token instead.
  • Finally you could figure out the actual sending of the email, maybe entirely separate by doing it from the Django shell.

Once you've done all that design thinking, then it could be time to go back and TDD it...but it will make a lot more sense to the reader about the thought process you've been on.

Use django.contrib.auth less?

Side note: I wonder if it is worth experimenting with using django.contrib.auth even less for this use case, it feels like a slightly awkward fit and a bit difficult to understand what is going on. Then again, maybe not.

Git commits

I've said this in a few comments, I have found it useful to have a predictable history of git commit with messages you provide, so I can find my way back if I go wrong. I won't keep saying this in future reviews unless you want me to!

@seddonym seddonym force-pushed the seddonym-custom-auth branch from a82e4d5 to 7eec016 Compare December 20, 2024 11:26
@seddonym seddonym requested a review from hjwp December 20, 2024 11:44
@seddonym seddonym marked this pull request as ready for review December 20, 2024 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant