Merge pull request #344 from Xronophobe/csanad-review--chapter-20

Csanád - review of chapter 20

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
+// `<form method="POST" action="/accounts/send_login_email">`
+// 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`:
+// `<form method="POST" action="{% url 'send_login_email' %}">`
+
 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 <<appendix_purist_unit_tests>>
@@ -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-tightly-coupled-sidebar>>,
   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.
 
 *******************************************************************************