Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

taifu
Copy link
Contributor

@taifu taifu commented Jun 15, 2019

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.

  • Add an entry for your name in the docs/authors.rst file if it's not
    already there.

…Medio Campidano, Ogliastra, Olbia-Tempio removed
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           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% <ø> (ø) ⬆️

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.

1 similar comment
@codecov-io
Copy link

codecov-io commented Jun 15, 2019

Codecov Report

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

Impacted file tree graph

@@           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% <ø> (ø) ⬆️

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.

Copy link
Member

@benkonrath benkonrath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@taifu
Copy link
Contributor Author

taifu commented Jun 21, 2019

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
Copy link
Member

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
Copy link
Contributor Author

taifu commented Jun 21, 2019

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
Copy link
Member

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:
#379

@benkonrath benkonrath added this to the 3.0 milestone Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants