diff --git a/chapter_20_mocking_1.asciidoc b/chapter_20_mocking_1.asciidoc index 3714835e..d3182af7 100644 --- a/chapter_20_mocking_1.asciidoc +++ b/chapter_20_mocking_1.asciidoc @@ -147,6 +147,7 @@ or what's sometimes called https://en.wikipedia.org/wiki/Monkey_patch[monkeypatc Let's suppose that, as a first step, we want to get to some code that invokes `send_mail` with the right subject line, from address, and to address. +// CSANAD: wouldn't "sender address" and "recipient address" sound better? That would look something like this: @@ -430,6 +431,7 @@ from django.test import TestCase ---- ==== +// CSANAD: using `self.assertTrue(mock_send_mail.called)` would be nicer. // SEBASTIAN: I'd give some hint (maybe visual?) that one should look at the decorator. // At first, I got lost as I was expecting something to change in the test itself, @@ -464,6 +466,9 @@ def send_login_email(request): [...] ---- ==== +// CSANAD: I suggest adding a bit of a text for the print: +// `print(f'DEBUG send_mail type:{type(send_mail)}')` +// it's arguably better practice and certainly easier for the eyes to find Let's run the tests again: @@ -559,6 +564,15 @@ to log in\nStart a new To-Do list' Submitting the email address currently has no effect, because the form isn't sending the data anywhere. +// CSANAD: It is sending the data! +// In the previous chapter at ch18l020 we set the form to +// `
` +// I suggest moving this changing of the hard-coded url higher up, maybe after +// ch19l004 along something like: +// "...and while we are at it, we can change the hard-coded url in the form to +// the named-path we just re-introduced from the spike into `src/accounts/urls.py`: +// `` + Let's wire it up in _base.html_: [role="sourcecode small-code"] @@ -582,7 +596,7 @@ We'll use Django's "messages framework", which is often used to display ephemeral "success" or "warning" messages to show the results of an action. Have a look at the -https://docs.djangoproject.com/en/1.11/ref/contrib/messages/[django messages docs] +https://docs.djangoproject.com/en/5.1/ref/contrib/messages/[django messages docs] if you haven't come across it already. Testing Django messages is a bit contorted--we have to pass `follow=True` @@ -650,6 +664,8 @@ def send_login_email(request): ******************************************************************************* TIP: This sidebar is an intermediate-level testing tip. +// CSANAD: not sure what sidebar we are talking about here. This TIP renders +// on the center for me. If it goes over your head the first time around, come back and take another look when you've finished this chapter. Consider also going through <> @@ -765,6 +781,8 @@ $ pass:quotes[*python src/manage.py test functional_tests.test_login*] [...] AssertionError: 'Use this link to log in' not found in 'body text tbc' ---- +// CSANAD: we haven't changed the views or the models, and we just had the unit +// tests passing before the TIP, so running it again is redundant here. We need to fill out the body text of the email, @@ -904,6 +922,7 @@ from accounts.models import Token self.assertIn(expected_url, body) ---- ==== +// CSANAD: will that be clear these go under `SendLoginEmailViewTest`? The first test is fairly straightforward; it checks that the token we create in the database @@ -1046,6 +1065,7 @@ Decoding this: * We return `None` if it doesn't. * If it does exist, we extract an email address, and either find an existing user with that address, or create a new one. +// CSANAD: shouldn't we use the numbered annotations instead? @@ -1172,7 +1192,12 @@ class PasswordlessAuthenticationBackend: ==== -That gets one test passing but breaks another one: +That changes the `FAILED` test to also result in a "matching query does not +exist" `ERROR`. +// CSANAD: the first test was already passing even with the placeholder +// `authenticate`, the second was failing and the third resulted in an error. +// With this first cut version the passing test remains passing and the other +// two result in DoesNotExist errors. [subs="specialcharacters,macros"] @@ -1192,7 +1217,12 @@ accounts.models.User.DoesNotExist: User matching query does not exist. ---- Let's fix each of those in turn: - +// CSANAD: I think it would be nice to explain what this error means and why we +// can fix it by returning None when it occurs. +// E.g. "The token with the given `uid` does not exist - this means we couldn't +// find the uuid in the database, so we should not authorize this login request. +// In the password-based world this would be the equivalent of an incorrect or +// missing password." [role="sourcecode"] .src/accounts/authentication.py (ch19l027) @@ -1223,6 +1253,9 @@ FAILED (errors=1) And we can handle the final case like this: +// CSANAD: again, I would add a bit more explanation: +// "If we can't find a user with the given email, then we will treat it as a +// sign-up request for a new user instead." [role="sourcecode"] .src/accounts/authentication.py (ch19l028) @@ -1433,6 +1466,11 @@ The mock.patch decorator:: Mocks can leave you tightly coupled to the implementation:: As we saw in <>, mocks can leave you tightly coupled to your implementation. +// CSANAD: this link reads as +// "As we saw in Mocks Can Leave You Tightly Coupled to the Implementation, +// mocks can leave you tightly coupled to your implementation" so it just +// repeats itself. I would re-word it e.g. "As we saw in (...), mocks can make +// your tests tied to implementation details too much." For that reason, you shouldn't use them unless you have a good reason. *******************************************************************************