Skip to content

Commit 7a928fd

Browse files
committed
add notes on improving clarity of intent, and the order of validation and writing to db
1 parent 6a09fb1 commit 7a928fd

File tree

1 file changed

+24
-2
lines changed

1 file changed

+24
-2
lines changed

chapter_13_database_layer_validation.asciidoc

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -198,11 +198,17 @@ Let's hack it in to see it work:
198198
[source,python]
199199
----
200200
with self.assertRaises(ValidationError):
201-
item.save()
202201
item.full_clean()
202+
item.save()
203203
----
204204
====
205205
//19
206+
// CSANAD: full_clean() should be executed before save() is called. It doesn't
207+
// make a difference here since the test DB is destroyed anyway, but
208+
// we are actually saving an invalid item and only then calling full_clean().
209+
// Which results in raising the correct error. But I think we should always
210+
// show the correct order even when it doesn't matter: validation first and
211+
// only then attempting save().
206212

207213
That gets the unit test to pass:
208214

@@ -593,6 +599,19 @@ The current situation is that we have one view and URL for displaying a list,
593599
and one view and URL for processing additions to that list. We're going to
594600
combine them into one. So, in 'list.html', our form will have a different
595601
target:
602+
// CSANAD: Since we are working in a not completely working state, I would
603+
// already mention or move the NOTE here from the end of the subsection:
604+
// "Refactor: Transferring the new_item Functionality into view_list".
605+
// And then just re-reference it from where it is right now.
606+
//
607+
// I think adding a few words on how this is not just a simple refactor but it's
608+
// going to get us to our working state would help the reader recognise these
609+
// situations later, independently.
610+
// Something like pointing out how the FT is partially passing for:
611+
// - adding one empty list item results in an error message
612+
// - adding one valid list item is also passing
613+
// - the error occurs when we try adding an empty list item again
614+
// from which we conclude why we are refactoring the views first
596615

597616
[role="sourcecode"]
598617
.src/lists/templates/list.html (ch11l030)
@@ -634,7 +653,10 @@ NOTE: In this section we're performing a refactor at the application level.
634653
and things are back to working as before.
635654
Have another look at the diagram from the end of <<chapter_04_philosophy_and_refactoring>>
636655
if you need to get your bearings.
637-
656+
// CSANAD: my comment from above to say a few words on why we are refactoring
657+
// despite the failing FT again feels justified since we are even
658+
// mentioning the diagram too. It might feel confusing without telling the
659+
// reader earlier what we are doing here.
638660

639661

640662
==== Refactor: Transferring the new_item Functionality into view_list

0 commit comments

Comments
 (0)