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.

Sign up for GitHub

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

Jump to bottom

Fixed #28075 -- Prevented ChoiceWidget from localizing option values. #8385

Merged
merged 1 commit into from Apr 21, 2017
Merged

Fixed #28075 -- Prevented ChoiceWidget from localizing option values. #8385

merged 1 commit into from Apr 21, 2017

Conversation

jdufresne
Copy link
Member

@jdufresne jdufresne changed the title Fixed #28075 -- Fixed select_option.html to not localize option values. Fixed #28075 -- Fixed ChoiceWidget to not localize option values. Apr 21, 2017
claudep
claudep reviewed Apr 21, 2017
View reviewed changes
django/forms/widgets.py Outdated
@@ -594,7 +594,7 @@ def create_option(self, name, value, label, selected, index, subindex=None, attr
option_attrs['id'] = self.id_for_label(option_attrs['id'], index)
return {
'name': name,
'value': value,
'value': force_text(value),
Copy link
Member

Choose a reason for hiding this comment

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

On master, I'd rather see a simple str(). Of course, the backport for 1.11 should keep force_text.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. I chose force_text() here to match the earlier use in ChoiceWidget. Do you think it would be best to change this other usage in this PR or in a separate commit & PR?

Copy link
Member

Choose a reason for hiding this comment

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

There's a ticket for auditing existing force_text() usage.

@timgraham timgraham changed the title Fixed #28075 -- Fixed ChoiceWidget to not localize option values. Fixed #28075 -- Prevented ChoiceWidget from localizing option values. Apr 21, 2017
timgraham
timgraham reviewed Apr 21, 2017
View reviewed changes
Copy link
Member

@timgraham timgraham left a comment

Choose a reason for hiding this comment

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

I'm uncertain about the need to test all subclasses of ChoiceWidget but this might be considered later with a mixin so that these don't have to repeated several times.

docs/releases/1.11.1.txt Outdated
@@ -45,3 +45,5 @@ Bugfixes

* Fixed exception reraising in ORM query execution when ``cursor.execute()``
fails and the subsequent ``cursor.close()`` also fails (:ticket:`28091`).

* Fixed ``ChoiceWidget`` to not localize option values (:ticket:`28075`).
Copy link
Member

Choose a reason for hiding this comment

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

ChoiceWidget isn't a public API, so perhaps it would be better to say something like "Fixed a regression where A, B, C, and D localized option values" where the letters are the actual classes.

tests/forms_tests/widget_tests/test_checkboxselectmultiple.py Outdated
@@ -173,3 +176,29 @@ class TestForm(forms.Form):
bound_field = TestForm()['f']
self.assertEqual(bound_field.field.widget.id_for_label('id'), '')
self.assertEqual(bound_field.label_tag(), '<label>F:</label>')

@override_settings(USE_L10N=True, USE_THOUSAND_SEPARATOR=True)
Copy link
Member

Choose a reason for hiding this comment

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

I'd order the test with the other check_html() tests (above test_use_required_attribute).

tests/forms_tests/widget_tests/test_select.py Outdated
@@ -311,3 +313,29 @@ def test_renders_required_when_possible_to_select_empty_field_none(self):
def test_doesnt_render_required_when_no_choices_are_available(self):
widget = self.widget(choices=[])
self.assertIs(widget.use_required_attribute(initial=None), False)

@override_settings(USE_L10N=True, USE_THOUSAND_SEPARATOR=True)
Copy link
Member

Choose a reason for hiding this comment

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

(order as suggested above)

@jdufresne
Copy link
Member Author

I'm uncertain about the need to test all subclasses of ChoiceWidget but this might be considered later with a mixin so that these don't have to repeated several times.

Just to clarify, the reason I wrote the tests for all subclasses is that my first fix only fixed the Select widget. Once I considered all ChoiceWidgets would be affected, I wrote a 2nd test to demonstrate to myself that the first fix was incomplete. For that reason, I found multiple tests useful.

@jdufresne
Copy link
Member Author

Thanks for the reviews. I have made the suggested changes.

@timgraham timgraham merged commit 581879a into django:master Apr 21, 2017
@jdufresne jdufresne mentioned this pull request Apr 21, 2017
Closed
@jdufresne jdufresne deleted the fix-l10n-select branch April 22, 2017 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@claudep claudep claudep left review comments

@timgraham timgraham timgraham left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@jdufresne @claudep @timgraham