Merge pull request #351 from Xronophobe/csanad-review--chapter-22

Csanád's review of chapter 22

chapter_22_fixtures_and_wait_decorator.asciidoc

@@ -84,18 +84,48 @@ class MyListsTest(FunctionalTest):
 
 <1> We create a session object in the database.  The session key is the
     primary key of the user object (which is actually the user's email address).
+// CSANAD: there is a different suggested way of importing SessionStore, using
+// the SESSION_ENGINE from the `settings`:
+// https://docs.djangoproject.com/en/5.1/topics/http/sessions/#using-sessions-out-of-views
+//
+// Also, I would mention what the Django docs say about what the SessionStore is
+// or why it is useful ("when we need to use Django sessions manually, outside
+// of views..."). We explain what the SESSION_KEY(`_auth_user_id'`) is, but we
+// should also elaborate on BACKEND_SESSION_KEY(`'_auth_user_backend'`) which we
+// set to `accounts.authentication.PasswordlessAuthenticationBackend`.
+//
+// edit: I just noticed we do show this below:
+// ```
+// In [3]: print(session.get_decoded())
+// {'_auth_user_id': '[email protected]', '_auth_user_backend':
+// 'accounts.authentication.PasswordlessAuthenticationBackend'}
+// ```
+// Maybe we could reference it here already, to help with any possible confu-
+// sion about what needs to be set. In fact, I would consider bringing the
+// "Django Sessions: How a User's Cookies Tell the Server She Is Authenticated"
+// up here, before we show how to set up the session cookie in the functional
+// test.
 
 <2> We then add a cookie to the browser that matches the session on the
     server--on our next visit to the site, the server should recognise
     us as a logged-in user.
-
+// CSANAD: we explain how the creation and use of session cookies are done in a
+// real user session, just below ("Django Sessions: How a User's Cookies Tell
+// the Server She Is Authenticated").
+// I think the instructions (how to set up the session cookie manually) may be
+// confusing without that explanation (at least the "why": showing what it is
+// that we are trying to mimick in our test), so once again, I would consider
+// moving it higher up, before we implement `create_pre_authenticated_session`.
 
 
 Note that, as it is, this will only work because we're using `LiveServerTestCase`,
 so the `User` and `Session` objects we create will end up in the same database
 as the test server.
 Later we'll need to modify it so that it works
 against the database on the staging server too.
+// CSANAD: I'm not sure I get it: I thought they would end up in the same DB
+// anyway. Can we add another sentence explaining what would happen on the
+// staging before we reference this modification we will need to do later?
 
 
 
@@ -109,10 +139,26 @@ _Being an attempt to explain sessions, cookies, and authentication in Django._
 Because HTTP is stateless,
 // SEBASTIAN: I remember me, being a junior always puzzled when reading HTTP is stateless. Perhaps an explanation in plain english (periphrasis) would do a better job. It's a second sentence in part that tries to explain something tricky.
 servers need a way of recognising different clients with _every single request_.
-IP addresses can be shared,
-so the usual solution is to give each client a unique session ID,
+We may think of just using IP addresses to associate with clients, but IP
+addresses can be shared among multiple clients, so we cannot use that.
+The usual solution is to give each client a unique session ID,
 which it will store in a cookie and submit with every request.
 // DAVID: what is the 'it' in this sentence?
+
+// CSANAD: it took me several repeated attempts to understand this sentence.
+// The "IP addresses can be shared" was unclear: between whom and for what
+// purpose would the IP address shared, and why do we even mention it - session
+// cookies don't store this information. So, I thought you meant "shared" with
+// the server, i.e. it can read the IP from `requeset`.
+// Only then did I understand that you started the sentence with one possible
+// identifier of a client which would NOT work because it can be the same for
+// multiple clients.
+// I suggest making it a little clearer why we even think of the IP address as
+// a possible identifier for a client.
+// I would also change "it will store in a cookie" to "the browser..." or "the
+// client will store in a cookie" for clarity.
+
+
 The server will store that ID somewhere
 (by default, in the database),
 and then it can recognise each request that comes in
@@ -141,6 +187,10 @@ the server can actually just mark that client's session as being an authenticate
 and associate it with a user ID in its database.
 
 A session is a dictionary-like data structure,
+// CSANAD: I would add that "A Django `session` is a dictionary-like ..." to
+// clarify which interpretation of the word "session" we are talking about here.
+// I think, a "session" without any specifications, should refer to the time
+// period between login and logout.
 and the user ID is stored under the key given by `django.contrib.auth.SESSION_KEY`.
 You can check this out in a [keep-together]#`./manage.py`# `shell` if you like:
 
@@ -166,7 +216,7 @@ as a way of temporarily keeping track of some state.
 This works for non–logged-in users too.
 Just use `request.session` inside any view, and it works as a dict.
 There's more information in the
-https://docs.djangoproject.com/en/5.0/topics/http/sessions/[Django docs on sessions].
+https://docs.djangoproject.com/en/5.1/topics/http/sessions/[Django docs on sessions].
 
 **********************************************************************
 
@@ -305,6 +355,19 @@ and automatically loading them in your test runs
 using the `fixtures` class attribute on `TestCase`.
 
 You'll find people out there saying http://bit.ly/1kSTyrb[don't use JSON fixtures],
+// CSANAD: I think it's better to use unshortened URL-s
+// http://blog.muhuk.com/2012/04/09/carl-meyers-testing-talk-at-pycon-2012.html
+//
+// Also, we aren't really showing what JSON fixtures are (well, the reader can
+// check by running the aforementioned `manage.py dumpdata` but it isn't shown
+// explicitly and the `loaddata` is only mentioned at the very end of this
+// chapter). So I think it might be confusing for some readers why we are
+// warning against the use of something we didn't use. Maybe showing a wrong,
+// JSON-based example would be nice, and then a good, ORM based example -
+// but the ORM-based one is what we did earlier in the chapter, in
+// `create_pre_authenticated_session`, so in this case, this warning against
+// JSON fixtures feels like it's placed too late and should be moved higher up,
+// right where we use the ORM `(...).objects.create`.
 and I tend to agree.
 They're a nightmare to maintain when your model changes.
 Plus it's difficult for the reader
@@ -463,14 +526,16 @@ that have [keep-together]#arguments#:
 
 `wait_to_be_logged_in` takes `self` and `email` as positional arguments.
 But when it's decorated, it's replaced with `modified_fn`,
-which current takes no arguments.
+which currently takes no arguments.
 How do we magically make it so our `modified_fn` can handle the same arguments
 as whatever `fn` the decorator gets given has?
 
 The answer is a bit of Python magic,
 `*args` and `**kwargs`, more formally known as
 https://docs.python.org/3/tutorial/controlflow.html#keyword-arguments["variadic arguments"],
 apparently (I only just learned that):
+// CSANAD: the way I read it, I think they only call `*args` "variadic arguments":
+// https://docs.python.org/3/tutorial/controlflow.html#arbitrary-argument-lists
 
 
 
@@ -546,6 +611,28 @@ Ran 8 tests in 19.379s
 OK
 ----
 
+// CSANAD: we can further simplify by removing the `wait_for`, which is just a
+// wrapper for `wait` -ed functions at this point:
+// Instead of calling e.g:
+// ```
+//         self.wait_for(
+//            lambda: self.assertIn(
+//                "Check your email",
+//                self.browser.find_element(By.CSS_SELECTOR, "body").text,
+//            )
+//        )
+// ```
+//
+// we could simply import `wait` from `base` and use it as a normal function:
+// ```
+//         wait(
+//             lambda: self.assertIn(
+//                 "Check your email",
+//                 self.browser.find_element(By.CSS_SELECTOR, "body").text,
+//             )
+//         )
+// ```
+
 And we're good to cross of that scratchpad item:
 
 [role="scratchpad"]