Merge pull request #189 from seddonym/incrementally

Review working incrementally

chapter_07_working_incrementally.asciidoc

@@ -43,6 +43,8 @@ Instead of a long up-front design phase,
 we try to put a _minimum viable application_ out there early,
 and let the design evolve gradually based on feedback from real-world usage.
 
+// DAVID: I would say the more common term is Minimum Viable Product, I haven't heard people say
+// 'minimum viable application.'
 
 But that doesn't mean that thinking about design is outright banned!
 In the last big chapter we saw how just blundering ahead without thinking can _eventually_ get us to the right answer,
@@ -125,6 +127,7 @@ To create a brand new list, we'll have a special URL that accepts POST requests:
     /lists/new
 ----
 
+// DAVID: for consistency, personally I would add trailing slashes to all the URLs.
 // SEBASTIAN: Why not just POST /lists/ ?
 //      Unless it's a URL for a view with a form 😅
 
@@ -136,6 +139,7 @@ we'll have a separate URL, to which we can send POST requests:
     /lists/<list identifier>/add_item
 ----
 
+// DAVID: I would use kebab case for URLs -> /add-item/
 // SEBASTIAN: Why not just POST /lists/<list identifier>/item ?
 //      Unless it's a URL for a view with a form 😅
 
@@ -253,8 +257,8 @@ and that they get their own unique URL for their list:
     ## as a way of simulating a brand new user session  # <1>
     self.browser.delete_all_cookies()
 
-    # Francis visits the home page.  There is no sign of Edith's
-    # list
+    # Francis visits the home page.  There is no sign of
+    # Edith's list
     self.browser.get(self.live_server_url)
     page_text = self.browser.find_element(By.TAG_NAME, "body").text
     self.assertNotIn("Buy peacock feathers", page_text)
@@ -460,13 +464,15 @@ urlpatterns = [
 ====
 
 <1> We'll just point our new URL at the existing home page view.
-    This is the minimimal change.
+    This is the minimal change.
 
 TIP: Watch out for trailing slashes in URLs,
     both here in _urls.py_ and in the tests.
     They're a common source of bugs.
     ((("troubleshooting", "URL mappings")))
 
+// DAVID: Why bother including a URL name if we're not going to use it in the call to redirect?
+
 //TODO: add or link to an explanation about leading and trailing slashes in
 //urlpatterns, redirects, etc.
 
@@ -510,6 +516,8 @@ a brand new list based on its first item.
 * the list view page needs to be able to display existing list items
 and add new items to the list
 
+// DAVID: don't know if this matters, but these bullets don't display properly in Github.
+
 Let's split out some tests for our new URL.
 
 Open up 'lists/tests.py', and add a new test class called `ListViewTest`.
@@ -707,6 +715,11 @@ And our unit tests are all passing,
 so we're pretty sure the URLs and views that we _do_ have are doing what they should.
 Let's have a quick look at those unit tests to see what they tell us:
 
+// DAVID: at this point, rather than trying to reason about what might be happening, I'm
+// much more likely to just spin up the server and try to do manually what the functional
+// test is doing. That will give me a better sense of how the bug is manifesting. Then I'd
+// drop down to this level.
+
 [subs="specialcharacters,quotes"]
 ----
 $ *grep -E "class|def" lists/tests.py*
@@ -733,6 +746,9 @@ it's often pointing at a problem that's not
 covered by the unit tests,
 and in our case, that's often a template problem.
 
+// DAVID: It might be fun at this point to encourage the reader
+// to stop and see if they can work out what's wrong.
+
 The answer is that our _home.html_ input form
 currently doesn't specify an explicit URL to POST to:
 
@@ -845,6 +861,9 @@ class ListViewTest(TestCase):
 ----
 ====
 
+// DAVID: FWIW I don't think this test adds value, it's an internal detail. We're refactoring anyway
+// so we would expect not to have to change tests - because we don't want tests to be overly coupled
+// to the way our code is factored anyway.
 
 Let's see what it says:
 
@@ -998,16 +1017,28 @@ Let's commit our progress so far:
 
 [subs="specialcharacters,quotes"]
 ----
-$ *git status* # should show 4 changed files and 1 new file, list.html
+$ *git status* # should show 5 changed files and 1 new file, list.html
+// DAVID: I get five changed files:
+// 	modified:   functional_tests/tests.py
+//	modified:   lists/templates/home.html
+//	modified:   lists/tests.py
+//	modified:   lists/views.py
+//	modified:   superlists/urls.py
 $ *git add lists/templates/list.html*
 $ *git diff* # should show we've simplified home.html,
+           # moved one test to a new class in lists/tests.py added a new view
+           # in views.py, and simplified home_page and added a line to urls.py
            # moved one test to a new class in lists/tests.py,
            # added a new view and simplified home_page in views.py,
            # and added a line to urls.py.
 $ *git commit -a* # add a message summarising the above, maybe something like
                 # "new URL, view and template to display lists"
 ----
 
+// DAVID: the FT is also changed. Also it wasn't just adding a line to urls as
+// it also switched `from list.views import home_page` to `from lists import views`.
+// TBH I'm not sure we really need this para, most readers will skip it.
+
 
 === A Third Small Step: A New URL for Adding List Items
 
@@ -1062,6 +1093,11 @@ TIP: This is another place to pay attention to trailing slashes, incidentally.
     The convention I'm using is that
     URLs without a trailing slash are "action" URLs which modify the database.
 
+// DAVID: Is that a common convention? We've already got HTTP methods to denote that,
+// as we can have different methods on the same URL.
+// Also, the way our Django app is configured, it will automatically redirect to a URL
+// with the slash appended, so I'm not sure this makes much sense.
+// https://docs.djangoproject.com/en/4.2/ref/settings/#append-slash
 
 Try running that:
 
@@ -1135,6 +1171,11 @@ def new_list(request):
 ----
 ====
 
+// DAVID: I do wonder about this approach, given that the application is now
+// vulnerable to a CSRF attack (it changes the database from a GET request).
+// We should be thinking about security even as we incrementally change
+// things, IMO. This is the kind of thing that could be missed later.
+
 That gives:
 
 ----
@@ -1251,6 +1292,10 @@ FAILED (errors=2)
 Once again, the FTs pick up a tricky little bug,
 something that our unit tests alone would find it hard to catch.
 
+// DAVID: again, at this point I'd spin up a browser and
+// actually see what's wrong. Maybe a good point to get the user
+// to try debugging before moving on?
+
 It's because our forms are still pointing to the old URL.
 In _both_ _home.html_ and _lists.html_, let's change them to:
 
@@ -1278,7 +1323,7 @@ FAILED (failures=1)
 That's another nicely self-contained commit,
 in that we've made a bunch of changes to our URLs,
 our _views.py_ is looking much neater and tidier,
-and we're sure the application is still working as well as it did before.
+and we're sure the application is still working as well as it was before.
 We're getting good at this working-state-to-working-state malarkey!
 
 [subs="specialcharacters,quotes"]
@@ -1356,6 +1401,10 @@ Again, a diff is a good way to see the changes:
 ----
 ====
 
+// DAVID: it's a smell to have a lot of assertions in a single unit test.
+// But then maybe the problem with this test is it's testing the framework
+// anyway...IMO it doesn't add value.
+
 We create a new `List` object
 and then we assign each item to it by setting it as its `.list` property.
 We check that the list is properly saved,
@@ -1390,6 +1439,9 @@ Fix that, and then you should see:
 AttributeError: 'List' object has no attribute 'save'
 ----
 
+// DAVID: FWIW I didn't see this as I had subclassed Model as the first step.
+// I wonder if on balance many readers would do that too.
+
 Next you should see:
 
 [role="dofirst-ch07l031"]
@@ -1482,6 +1534,11 @@ class Item(models.Model):
 ----
 ====
 
+// DAVID: this provides None as a default, but the field is non-nullable. Consider adding
+// null=True too? Or else (and I would actually prefer this), don't provide a default
+// and get them to delete their database and remigrate. We don't really want Items
+// in the database that have no list.
+
 That'll need a migration too.  Since the last one was a red herring, let's
 delete it and replace it with a new one:
 
@@ -1506,7 +1563,7 @@ WARNING: Deleting migrations is dangerous.
     A good rule of thumb is that you should never delete or modify
     a migration that's already been committed to your VCS.
 
-
+// DAVID: I would say 'can be' dangerous. It's not dangerous in this case.
 
 ==== Adjusting the Rest of the World to Our New Models
 
@@ -1603,6 +1660,10 @@ Are you wondering about the strange spelling of the "nulist" variable?
 Other options are "list", which would shadow the built-in `list()` function,
 and `new_list`, which would shadow the name of the function that contains it.
 Or `list1` or `listey` or `mylist`, but none are particularly satisfactory.]
+
+// DAVID: A more conventional approach would be to append an underscore,
+// e.g. list_.
+
 gets our tests passing again:
 
 ----
@@ -1711,11 +1772,13 @@ NOTE: Are you wondering about the line spacing in the test?
     This isn't obligatory, but it does help see the structure of the test.
     Some people refer to this structure as _Arrange-Act-Assert_,
     or _Given-When-Then_: _Given_ the database contains our list with two items,
-    and another list, _When_ I do a GET request for our list,
+    and another list, _When_ I make a GET request for our list,
     _Then_ I see the items in our list, but not the items in the other list.
     ((("Arrange, Act, Assert")))
     ((("Given / When / Then")))
 
+// DAVID: I think it would be better to do that spacing earlier, in test_displays_all_list_items.
+// It also makes it easier to see the difference between the two tests.
 // SEBASTIAN: That's exactly my approach as well, +1 for that
 //      One could add that difficulty in grouping visually may indicate
 //      that there's an issue with test design or scope
@@ -1758,6 +1821,9 @@ We adjust the regular expression for our URL to include a 'capture group',
 `<int:list_id>`, which will match any numerical characters, up to the following `/`,
 The captured id will get passed to the view as an argument.
 
+// DAVID: this is written in terms of regular expressions, but these are now using
+// Django URL patterns instead. See https://docs.djangoproject.com/en/4.2/ref/urls/#path.
+
 In other words, if we go to the URL '/lists/1/', `view_list` will get a second
 argument after the normal `request` argument, namely the integer `1`.
 
@@ -1984,6 +2050,9 @@ class NewItemTest(TestCase):
 ----
 ====
 
+// DAVID: In the second test, the variable other_list isn't used. Might want to just call `List.objects.create()`
+// without assigning it to a variable.
+
 // SEBASTIAN: maybe rename correct_list to current_list? Personally, I have an allergy
 //      to words like 'correct' in tests because what is correct today may not correct tomorrow
 //      I feel 'current' is more accurate as it simply reflects that we'll be adding items to one of the lists
@@ -2262,6 +2331,9 @@ which is what we were trying to do anyway:
     object's related items from a different table...
     ((("reverse lookups")))
 
+// DAVID: instead of using item_set, might want to consider defining a related name 'items' when we first
+// define the foreign key. It's more explicit and I think people new to Django might understand it better.
+
 So that gets the unit tests to pass:
 
 ----