[#309] Add separate fields for dutch addresses #329
Conversation
danielmursa-dev
force-pushed
the
feature/add-separate-fields-for-dutch-addresses
branch
from
2f31a2e to
add1d20
Compare
danielmursa-dev
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #329 +/- ##
==========================================
+ Coverage 97.10% 97.36% +0.26%
==========================================
Files 179 191 +12
Lines 8671 9543 +872
==========================================
+ Hits 8420 9292 +872
Misses 251 251 ☔ View full report in Codecov by Sentry. |
| @@ -17,6 +25,44 @@ class AdresMixin(models.Model): | |||
| validators=[validate_bag_id], | |||
| blank=True, | |||
| ) | |||
| adres_straatnaam = models.CharField( | |||
| _("straatnaam"), | |||
| help_text=_("Straatnaam in het Basisregistratie Adressen en Gebouwen."), | |||
There was a problem hiding this comment.
Might be good to double check this with @alextreme, but from what I understand from the issue, these fields are only used if we don't have a BAG-id for the adres
| help_text=_("Straatnaam in het Basisregistratie Adressen en Gebouwen."), | |
| help_text=_("Straatnaam van het adres (indien het een Nederlands adres betreft zonder BAG-id)."), |
| help_text=_("Straatnaam in het Basisregistratie Adressen en Gebouwen."), | ||
| max_length=255, | ||
| blank=True, | ||
| null=True, |
There was a problem hiding this comment.
null=True on string based fields is generally not desirable, because it means that there are two values at the database level for "no value": null and "" (see https://docs.djangoproject.com/en/4.2/ref/models/fields/#django.db.models.Field.null)
Could you remove it from the other charfields as well?
There was a problem hiding this comment.
Yes right, I'm totally agree with you, but in this way I have to add default=True, because it gives me an error during migration because I have to set values for these fields also for the previous records, should I proceed in this way?
| postcode: | ||
| type: string | ||
| description: Postcode in het Basisregistratie Adressen en Gebouwen. | ||
| pattern: ^[1-9][0-9]{3} ?[a-zA-Z]{2}$ |
There was a problem hiding this comment.
I'm not sure about allowing both lower and uppercase and space vs no space. Having this means that you can have different value for the same postcode (1234 AB vs 1234AB vs 1234 ab). Additionally the maxLength is incorrect if we allow spaces, because then it should be 7.
I'm fairly sure that only capital letters are allowed but I'm not 100% sure about having a space vs no space
@alextreme what do you think about this, should we make validation stricter to only allow one specific format, or should we normalize the values when saving them?
Fixes #309