# HG changeset patch
# User Olly Cope <olly@ollycope.com>
# Date 1636715458 0
#      Fri Nov 12 11:10:58 2021 +0000
# Node ID be8cbf23a3b17b2b2995588770aac5178f85b580
# Parent  b87bcd67b9a285c27e97e324c9fc44b56a2984a0
choices: add widgets.StrChoiceMapper and make this the default

diff --git a/doc/fields.rst b/doc/fields.rst
--- a/doc/fields.rst
+++ b/doc/fields.rst
@@ -131,22 +131,29 @@
                                     validate_choices=False)
 
 
-Choice fields are keyed on the json encoded representation of the value.
-The ``value_mapper`` argument can override this.
+Mapping choice values to strings
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 When rendered in HTML forms choice values must be converted to strings, and
 then back again when the form is submitted and processed.
 
-The default strategy is to convert your choice keys to a JSON representation.
-This handles the common cases of strings and integer keys well (and also
-``None``).
+Starting from version 2.0, :class:`morf.fields.Choice` fields with non-string
+values will have their values converted to strings when rendered in HTML.
+
+Two other options exist.
 
-An alternative implementation is offered, which adds a key based on a simple indexing of the items based on their position in the list. For example, the choices::
+:class:`~morf.widgets.JSONChoiceMapper` converts choice values to JSON when rendering as HTML. This was the default behaviour prior to v2.0.
+
+:class:`~morf.widgets.IndexChoiceMapper` uses the position of each value in the choice list as the key. For example, the choices:
+
+..code-block::python
 
     [('11:00', 'Elevenses'), ('13:00', 'Lunch time'), ('16:00', 'Tea time')]
 
 Would result in options indexed as follows::
 
+..code-block::html
+
     <select>
         <option value="0">Elevenses</option>
         <option value="1">Lunch time</option>
@@ -157,34 +164,36 @@
 morf looks up the index and populates the field with the
 original object, in this case ``16:00``
 
-The advantage with this strategy is that object identity is preserved.
-You can use any object as a value
-(perhaps a `:class:datetime.time` object in this case)
-and your form field will be populated with the very same object,
-thus elimating all the tedious mucking about converting values to
-strings and back again.
-
-However while this sounds neat, in practice there are major disadvantages:
-
-- It's fragile in the case that the list of values changes.
+To enable this behaviour use the ``mapper`` argument when creating the
+widget:
 
-- It's hard to test — to create test values for a choice field
-  you have to know the index of the value you want.
-
-- It's hard to generate values for the field
-  from other parts of your application
-  (eg imagine a search form has a 'category' choice.
-  Now you can't easily generate search URLs with
-  ``category=...`` in the querystring)
-
-To enable this behaviour use the ``mapper`` argument when creating the
-widget::
+..code-block::python
 
     from morf import fields, widgets
 
+    # JSON-encoded choices
     choicefield = fields.Choice(
-                    choices=['a', 'b', 'c'],
-                    widget=widgets.Select(mapper=widgets.IndexChoiceMapper())
+        choices=['a', 'b', 'c'],
+        widget=widgets.Select(mapper=widgets.JSONChoiceMapper())
+    )
+
+    # Index choices
+    choicefield = fields.Choice(
+        choices=['a', 'b', 'c'],
+        widget=widgets.Select(mapper=widgets.IndexChoiceMapper())
+    )
+
+To revert to the pre-2.0 default of using the
+:class:`~morf.widgets.JSONChoiceMapper` globally, add this code to your
+application:
+
+..code-block::python
+
+    from morf import widgets
+
+    widgets.SingleChoiceWidget.choice_mapper = widgets.JSONChoiceMapper
+
+
 
 fields.MultipleChoice
 ~~~~~~~~~~~~~~~~~~~~~
diff --git a/morf/tests/test_widgets.py b/morf/tests/test_widgets.py
--- a/morf/tests/test_widgets.py
+++ b/morf/tests/test_widgets.py
@@ -221,10 +221,10 @@
     field = fields.Str(choices=[("Y", "yes"), ("N", "no")])
 
     def test_it_renders(self):
-        expect(self.rendered({"s": '"N"'})) == (
+        assert self.rendered({"s": "N"}) == (
             '<select name="s" required="">'
-            '<option value="&#34;Y&#34;">yes</option>'
-            '<option selected="selected" value="&#34;N&#34;">no</option>'
+            '<option value="Y">yes</option>'
+            '<option selected="selected" value="N">no</option>'
             "</select>"
         )
 
@@ -235,10 +235,10 @@
         widget = widgets.Select()
         field = copy(self.field)
         field.choices = ["a", "a"]
-        expect(self.rendered({"s": '"a"'}, widget=widget, field=field)) == (
+        expect(self.rendered({"s": "a"}, widget=widget, field=field)) == (
             '<select name="s" required="">'
-            '<option selected="selected" value="&#34;a&#34;">a</option>'
-            '<option value="&#34;a&#34;">a</option>'
+            '<option selected="selected" value="a">a</option>'
+            '<option value="a">a</option>'
             "</select>"
         )
 
@@ -281,7 +281,7 @@
         r = f.as_p().render()
         expect(r).contains('<optgroup label="upper"><option value="65">A')
         expect(r).contains(
-            '<optgroup label="lower">' '<option value="&#34;a&#34;">a'
+            '<optgroup label="lower">' '<option value="a">a'
         )
 
 
@@ -292,9 +292,9 @@
 
     def test_it_renders(self):
         expect(self.rendered()) == (
-            '<input name="s" required="" type="radio" value="&#34;Y&#34;" /> '
+            '<input name="s" required="" type="radio" value="Y" /> '
             "<label>yes</label> "
-            '<input name="s" required="" type="radio" value="&#34;N&#34;" /> '
+            '<input name="s" required="" type="radio" value="N" /> '
             "<label>no</label>"
         )
 
@@ -307,27 +307,27 @@
     def test_it_adds_container(self):
         expect(self.rendered(widget=widgets.RadioGroup(class_="foo"))) == (
             '<div class="foo">'
-            '<input name="s" required="" type="radio" value="&#34;Y&#34;" /> '
+            '<input name="s" required="" type="radio" value="Y" /> '
             "<label>yes</label> "
-            '<input name="s" required="" type="radio" value="&#34;N&#34;" /> '
+            '<input name="s" required="" type="radio" value="N" /> '
             "<label>no</label>"
             "</div>"
         )
 
     def test_it_renders_text_first(self):
         expect(self.rendered(widget=widgets.RadioGroup.label_first())) == (
-            '<label>yes <input name="s" required="" type="radio" value="&#34;Y&#34;" />'
+            '<label>yes <input name="s" required="" type="radio" value="Y" />'
             "</label> "
-            '<label>no <input name="s" required="" type="radio" value="&#34;N&#34;" />'
+            '<label>no <input name="s" required="" type="radio" value="N" />'
             "</label>"
         )
 
     def test_it_renders_as_ul(self):
         expect(self.rendered(widget=widgets.RadioGroup.as_ul())) == (
             "<ul>"
-            '<li><input name="s" required="" type="radio" value="&#34;Y&#34;" /> '
+            '<li><input name="s" required="" type="radio" value="Y" /> '
             "<label>yes</label></li>"
-            '<li><input name="s" required="" type="radio" value="&#34;N&#34;" /> '
+            '<li><input name="s" required="" type="radio" value="N" /> '
             "<label>no</label></li>"
             "</ul>"
         )
@@ -336,10 +336,10 @@
         expect(self.rendered(widget=widgets.RadioGroup.as_table())) == (
             "<table><tbody>"
             "<tr><td>"
-            '<input name="s" required="" type="radio" value="&#34;Y&#34;" /></td>'
+            '<input name="s" required="" type="radio" value="Y" /></td>'
             "<td><label>yes</label></td></tr>"
             "<tr><td>"
-            '<input name="s" required="" type="radio" value="&#34;N&#34;" /></td>'
+            '<input name="s" required="" type="radio" value="N" /></td>'
             "<td><label>no</label></td></tr>"
             "</tbody></table>"
         )
@@ -349,7 +349,7 @@
         r = self.rendered(widget=widgets.RadioGroup(), field=field)
         expect(r) == (
             "<fieldset><legend>foo</legend>"
-            '<input name="s" required="" type="radio" value="&#34;Y&#34;" /> '
+            '<input name="s" required="" type="radio" value="Y" /> '
             "<label>yes</label>"
             "</fieldset>"
         )
@@ -372,14 +372,14 @@
 
     def test_it_renders(self):
         expect(self.rendered()) == (
-            '<input name="s" type="checkbox" value="&#34;Y&#34;" /> '
+            '<input name="s" type="checkbox" value="Y" /> '
             "<label>yes</label>"
         )
 
     def test_it_renders_as_ul(self):
         expect(self.rendered(widget=widgets.CheckboxGroup.as_ul())) == (
             "<ul>"
-            '<li><input name="s" type="checkbox" value="&#34;Y&#34;" /> '
+            '<li><input name="s" type="checkbox" value="Y" /> '
             "<label>yes</label></li>"
             "</ul>"
         )
@@ -388,7 +388,7 @@
         expect(self.rendered(widget=widgets.CheckboxGroup.as_table())) == (
             "<table><tbody>"
             "<tr><td>"
-            '<input name="s" type="checkbox" value="&#34;Y&#34;" /></td>'
+            '<input name="s" type="checkbox" value="Y" /></td>'
             "<td><label>yes</label></td></tr>"
             "</tbody></table>"
         )
@@ -409,9 +409,7 @@
             initial=["a", "c"],
         )
         checked = r.findall(".//input")
-        expect(
-            [c.attrib["value"] for c in checked if "checked" in c.attrib]
-        ) == ['"a"', '"c"']
+        assert [c.attrib["value"] for c in checked if "checked" in c.attrib] == ["a", "c"]
 
     def test_it_places_fieldname_in_inputs_not_container(self):
         el = self.rendered_dom(
@@ -421,10 +419,10 @@
 
         # Has it assigned the fieldname as a input element atrribute?
         for inp in el.findall(".//input"):
-            expect(inp.attrib["name"]) == "foo"
+            assert inp.attrib["name"] == "foo"
 
         # Has it not assigned the fieldname as a container element atrribute?
-        expect(el.attrib) == {}
+        assert el.attrib == {}
 
 
 class TestJSONChoiceMapper(object):
@@ -467,6 +465,41 @@
             assert self.mapper.choice_map(self.choice_items)[key] is Undefined
 
 
+class TestStrChoiceMapper(object):
+
+    mapper = widgets.StrChoiceMapper()
+    choice_items = [
+        (1, "one"),
+        ("two", "two"),
+        (None, "nothing"),
+        ("optgroup_label", choices.OptGroup(["a", "b"])),
+    ]
+
+    def test_it_indexes_choices(self):
+        indexed_choices = self.mapper.indexed_choices(self.choice_items)
+        indexed_choices = listify(indexed_choices)
+        assert indexed_choices == [
+            ("1", 1, "one"),
+            ("two", "two", "two"),
+            ("None", None, "nothing"),
+            (
+                widgets.ChoiceMapper.OPTGROUP,
+                [("a", "a", "a"), ("b", "b", "b")],
+                "optgroup_label",
+            ),
+        ]
+
+    def test_it_maps_keys_to_values(self):
+        expected_mapped_values = {
+            "1": 1,
+            "two": "two",
+            "a": "a",
+            "None": None,
+        }
+        for key, expected in expected_mapped_values.items():
+            assert self.mapper.choice_map(self.choice_items)[key] == expected
+
+
 class TestIndexChoiceMapper(object):
 
     mapper = widgets.IndexChoiceMapper()
diff --git a/morf/widgets.py b/morf/widgets.py
--- a/morf/widgets.py
+++ b/morf/widgets.py
@@ -310,6 +310,26 @@
             return Undefined
 
 
+class StrChoiceMapper(ChoiceMapper):
+    def indexed_choices(self, choices):
+        for value, label in choices:
+            if isinstance(label, OptGroup):
+                yield (self.OPTGROUP, self.indexed_choices(label), str(value))
+            else:
+                yield str(value), value, label
+
+    def choice_map(self, choices):
+        def build_map(choices):
+            result = {}
+            for value, label in choices:
+                if isinstance(label, OptGroup):
+                    result.update(build_map(label))
+                else:
+                    result[str(value)] = value
+            return result
+        return build_map(choices)
+
+
 class IndexChoiceMapper(ChoiceMapper):
     """
     A :class:`ChoiceMapper` implementation that adds a integer index based
@@ -359,7 +379,7 @@
     """
 
     choices = None
-    choice_mapper = JSONChoiceMapper()
+    choice_mapper = StrChoiceMapper()
 
     def __init__(self, *args, **kwargs):
         self.choice_mapper = kwargs.pop("mapper", self.choice_mapper)