django / django-localflavor

Country-specific Django helpers, formerly of contrib fame

https://django-localflavor.readthedocs.io/

BSD 3-Clause "New" or "Revised" License

813 stars 289 forks source link

Italian provinces updated: Sud Sardegna added and Carbonia-Iglesias, … #378

Closed taifu closed 4 years ago

taifu commented 5 years ago

Italian provinces updated:

Sud Sardegna added
Carbonia-Iglesias, Medio Campidano, Ogliastra, Olbia-Tempio removed

2016 provinces changes

All Changes

[ ] Add an entry to the docs/changelog.rst describing the change.
[X] Add an entry for your name in the docs/authors.rst file if it's not already there.

codecov-io commented 5 years ago

Codecov Report

Merging #378 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #378   +/-   ##
=======================================
  Coverage   95.81%   95.81%           
=======================================
  Files         162      162           
  Lines        3995     3995           
  Branches      533      533           
=======================================
  Hits         3828     3828           
  Misses        100      100           
  Partials       67       67

Impacted Files	Coverage Δ
localflavor/it/it_province.py	`100% <ø> (ø)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c012384...d41f91f. Read the comment docs.

codecov-io commented 5 years ago

Codecov Report

Merging #378 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #378   +/-   ##
=======================================
  Coverage   95.81%   95.81%           
=======================================
  Files         162      162           
  Lines        3995     3995           
  Branches      533      533           
=======================================
  Hits         3828     3828           
  Misses        100      100           
  Partials       67       67

Impacted Files	Coverage Δ
localflavor/it/it_province.py	`100% <ø> (ø)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c012384...d41f91f. Read the comment docs.

taifu commented 5 years ago

Thanks for the PR - it looks good. The only missing part is the change log entry. You should specifically mention in the change log entry that a data migration is required for users of the ITRegionProvinceSelect: CI, VS, OG, and OT will need to be converted to SU.

I don't think this is mandatory. For example an old customer should stay with his old province, without any data migration, whilst an active one should be migrated. What do you think? Am I wrong?

benkonrath commented 5 years ago

Here's how I understand things: Django won't be able to generate a selected value for the choice field when one of the removed choices is stored in the database. In this case it will just show the default value, not the database value.

I'm guessing a bit here because it's been a while since I've encountered this behaviour. Are you able to do a little test in your local dev environment? You can just store one of the deleted values and see how it shows up in a form with this new version of the code.

taifu commented 5 years ago

Here's how I understand things: Django won't be able to generate a selected value for the choice field when one of the removed choices is stored in the database. In this case it will just show the default value, not the database value.

Yep. This is how it works. So what do you think is it right to write in the changelog?

benkonrath commented 5 years ago

After rebasing (to get the latest update to the change log), you can make a normal change log entry about the change in the correct place and also add some information about the data migration above 'New flavors'.

For an example, you can see how I just updated the change log for removing the deprecated code: https://github.com/django/django-localflavor/pull/379