From 425d1b3536d34fe234c97950fabd007cf6026b1d Mon Sep 17 00:00:00 2001 From: David Seddon Date: Tue, 22 Aug 2023 12:56:16 +0100 Subject: [PATCH] Review working incrementally --- chapter_07_working_incrementally.asciidoc | 89 ++++++++++++++++++++--- 1 file changed, 80 insertions(+), 9 deletions(-) diff --git a/chapter_07_working_incrementally.asciidoc b/chapter_07_working_incrementally.asciidoc index dbfeba93c..84c5a18fd 100644 --- a/chapter_07_working_incrementally.asciidoc +++ b/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, @@ -118,6 +120,8 @@ 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. + To add a new item to an existing list, we'll have a separate URL, to which we can send POST requests: @@ -126,6 +130,8 @@ we'll have a separate URL, to which we can send POST requests: /lists//add_item ---- +// DAVID: I would use kebab case for URLs -> /add-item/ + (Again, we're not trying to perfectly follow the rules of REST, which would use a PUT request here--we're just using REST for inspiration. Apart from anything else, you can't use PUT in a standard HTML form.) @@ -241,8 +247,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) @@ -441,13 +447,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. @@ -490,6 +498,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`. @@ -687,6 +697,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* @@ -713,6 +728,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: @@ -816,6 +834,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: @@ -969,16 +990,24 @@ 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 +// 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. $ *git commit -a* # add a message summarising the above, maybe something like # "new URL, view and template to display lists" ---- - === A Third Small Step: A New URL for Adding List Items ((("multiple lists testing", "list item URLs", id="MLTlist07"))) @@ -1032,6 +1061,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: @@ -1105,6 +1139,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: ---- @@ -1220,6 +1259,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: @@ -1247,7 +1290,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"] @@ -1325,6 +1368,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 assigning it as its `.list` property. We check that the list is properly saved, @@ -1359,6 +1406,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"] @@ -1451,6 +1501,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: @@ -1475,7 +1530,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 @@ -1569,6 +1624,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: ---- @@ -1637,7 +1696,7 @@ Probably the simplest thing, for now, is just to use the auto-generated `id` field from the database. Let's change `ListViewTest` so that the two tests point at new URLs. -We'll also change the old `test_displays_all_items` test +We'll also change the old `test_displays_all_list_items` test and call it `test_displays_only_items_for_that_list` instead, making it check that only the items for a specific list are displayed: @@ -1675,11 +1734,14 @@ 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. + // TODO: promote the above note to a sidebar? // and/or move it back to chapter_05_post_and_database given how long this chapter already is. @@ -1718,6 +1780,9 @@ We adjust the regular expression for our URL to include a 'capture group', ``, 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`. @@ -1944,6 +2009,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. + NOTE: Are you wondering about `other_list`? A bit like in the tests for viewing a specific list, it's important that we add items to a specific list. @@ -2209,6 +2277,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: ----