fix(ISSUE-8): contd m2m save values (#11)

* feat(ISSUE-8): ISSUE-8: ManyToManyField causes error on confirmations

* feat(ISSUE-8): Update some readme and remove print statements

* feat(ISSUE-8): Generate new version of package

* feat(ISSUE-3): Adding .travis.yml

* feat(ISSUE-3): Adding coveralls

* feat(ISSUE-3): Trying github actions

* feat(ISSUE-3): remove travis

* feat(ISSUE-3): Change python versions to test

* feat(ISSUE-3): Some refactoring and trying tox

* feat(ISSUE-3): Try action matrix

* feat(ISSUE-3): Some more refactors

* feat(ISSUE-3): Fix tests

* feat(ISSUE-3): Refactor/fix tests

* feat(ISSUE-3): Remove tox

* feat(ISSUE-3): Adding pypi version badge to readme

* feat(ISSUE-3): Update readme again

* feat(ISSUE-3): Remove tox from readme

* feat(ISSUE-8): Adding some tests for m2m and fix save value issue

* feat(ISSUE-8): Updating test todos

* feat(ISSUE-8): Finish up the tests

* feat(ISSUE-8): Rename

* feat(ISSUE-8): Update gitignore

Co-authored-by: Thu Trang Pham <thu@joinmodernhealth.com>

.gitignore

@@ -8,6 +8,9 @@ sdist/
 *.egg-info/
 .DS_Store
 
+# Editor settings
+.vscode/
+
 # Unit test / coverage reports
 htmlcov/
 .tox/

README.md

@@ -157,7 +157,6 @@ Running tests:
 
 ```
 make test
-tox
 ```
 
 Testing new changes on test project:
@@ -171,12 +170,11 @@ make run
 
 Honestly this part is just for my reference. But who knows :) maybe we'll have another maintainer in the future.
 
-Run tests, check coverage, check readme, run tox
+Run tests, check coverage, check readme
 
 ```
 make test
 make check-readme
-tox
 ```
 
 Update version in `setup.py`

admin_confirm/admin.py

@@ -6,7 +6,8 @@ from django.template.response import TemplateResponse
 from django.contrib.admin.options import TO_FIELD_VAR
 from django.utils.translation import gettext as _
 from django.contrib.admin import helpers
-from django.db.models import Model
+from django.db.models import Model, ManyToManyField
+
 from django.forms import ModelForm
 from admin_confirm.utils import snake_to_title_case
 
@@ -119,18 +120,43 @@ class AdminConfirmMixin:
                     default_value = model._meta.get_field(name).get_default()
                     if new_value is not None and new_value != default_value:
                         # Show what the default value is
-                        changed_data[name] = [str(default_value), new_value]
+                        changed_data[name] = [default_value, new_value]
             else:
                 # Parse the changed data - Note that using form.changed_data would not work because initial is not set
                 for name, new_value in form.cleaned_data.items():
                     # Since the form considers initial as the value first shown in the form
                     # It could be incorrect when user hits save, and then hits "No, go back to edit"
                     obj.refresh_from_db()
+                    # Note: getattr does not work on ManyToManyFields
+                    field_object = model._meta.get_field(name)
                     initial_value = getattr(obj, name)
+                    if isinstance(field_object, ManyToManyField):
+                        initial_value = field_object.value_to_string(obj)
+
                     if initial_value != new_value:
                         changed_data[name] = [initial_value, new_value]
+
+        print(changed_data)
         return changed_data
 
+    def _get_form_data(self, request):
+        """
+        Parses the request post params into a format that can be used for the hidden form on the
+        change confirmation page.
+        """
+        form_data = request.POST.copy()
+
+        for key in SAVE_ACTIONS + [
+            "_confirm_change",
+            "_confirm_add",
+            "csrfmiddlewaretoken",
+        ]:
+            if form_data.get(key):
+                form_data.pop(key)
+
+        form_data = [(k, list(v)) for k, v in form_data.lists()]
+        return form_data
+
     def _change_confirmation_view(self, request, object_id, form_url, extra_context):
         # This code is taken from super()._changeform_view
         to_field = request.POST.get(TO_FIELD_VAR, request.GET.get(TO_FIELD_VAR))
@@ -174,18 +200,14 @@ class AdminConfirmMixin:
             return super()._changeform_view(request, object_id, form_url, extra_context)
 
         # Parse raw form data from POST
-        form_data = {}
+        form_data = self._get_form_data(request)
         # Parse the original save action from request
         save_action = None
-        for key, value in request.POST.items():
+        for key in request.POST.keys():
             if key in SAVE_ACTIONS:
                 save_action = key
-                continue
+                break
 
-            if key.startswith("_") or key == "csrfmiddlewaretoken":
-                continue
-
-            form_data[key] = value
 
         title_action = _("adding") if add else _("changing")
 

admin_confirm/templates/admin/change_confirmation.html

@@ -43,8 +43,10 @@
     <form method="post" action="{% url opts|admin_urlname:'change' object_id|admin_urlquote %}">{% csrf_token %}
   {% endif %}
 
-    {% for key, value in form_data.items %}
+    {% for key, values in form_data %}
-        <input type="hidden" name="{{ key }}" value="{{ value }}">
+        {% for v in values %}
+        <input type="hidden" name="{{ key }}" value="{{v}}">
+        {% endfor %}
     {% endfor %}
     {% if is_popup %}<input type="hidden" name="{{ is_popup_var }}" value="1">{% endif %}
     {% if to_field %}<input type="hidden" name="{{ to_field_var }}" value="{{ to_field }}">{% endif %}

admin_confirm/templates/admin/change_data.html

@@ -1,4 +1,4 @@
-{% if changed_data %}
+{% load formatting %} {% if changed_data %}
 <div class="changed-data">
   <p><b>Confirm Values:</b></p>
   <table>
@@ -10,8 +10,8 @@
     {% for field, values in changed_data.items %}
     <tr>
       <td>{{ field }}</td>
-      <td>{{ values.0 }}</td>
+      <td>{{ values.0|format_change_data_field_value }}</td>
-      <td>{{ values.1 }}</td>
+      <td>{{ values.1|format_change_data_field_value }}</td>
     </tr>
     {% endfor %}
   </table>

admin_confirm/templates/admin/submit_line.html

@@ -3,10 +3,10 @@
 
 {% block submit-row %}
     {% if confirm_change %}
-        <input hidden name="_confirm_change" />
+        <input hidden name="_confirm_change" value=True />
     {% endif %}
     {% if confirm_add %}
-        <input hidden name="_confirm_add" />
+        <input hidden name="_confirm_add" value=True />
     {% endif %}
     {{ block.super }}
 {% endblock %}

admin_confirm/templatetags/formatting.py

@@ -0,0 +1,12 @@
+from django import template
+from django.db.models.query import QuerySet
+
+register = template.Library()
+
+
+@register.filter
+def format_change_data_field_value(field_value):
+    if isinstance(field_value, QuerySet):
+        return list(field_value)
+
+    return field_value

admin_confirm/tests/test_confirm_change_and_add.py

@@ -64,7 +64,6 @@ class TestConfirmChangeAndAdd(TestCase):
         ]
         self.assertEqual(response.template_name, expected_templates)
         form_data = {"shop": str(shop.id), "item": str(item.id), "quantity": str(5)}
-        self.assertEqual(response.context_data["form_data"], form_data)
         for k, v in form_data.items():
             self.assertIn(
                 f'<input type="hidden" name="{ k }" value="{ v }">',
@@ -100,7 +99,6 @@ class TestConfirmChangeAndAdd(TestCase):
             "id": str(item.id),
             "currency": Item.VALID_CURRENCIES[0][0],
         }
-        self.assertEqual(response.context_data["form_data"], form_data)
         for k, v in form_data.items():
             self.assertIn(
                 f'<input type="hidden" name="{ k }" value="{ v }">',

admin_confirm/tests/test_confirm_change_and_add_m2m_field.py

@@ -0,0 +1,183 @@
+from django.test import TestCase, RequestFactory
+from django.contrib.auth.models import User
+from django.urls import reverse
+
+
+from tests.market.admin import InventoryAdmin, ShoppingMallAdmin
+from tests.market.models import Item, Inventory, ShoppingMall
+from tests.factories import ItemFactory, ShopFactory, InventoryFactory
+
+
+class TestConfirmChangeAndAddM2MField(TestCase):
+    @classmethod
+    def setUpTestData(cls):
+        cls.superuser = User.objects.create_superuser(
+            username="super", email="super@email.org", password="pass"
+        )
+
+    def setUp(self):
+        self.client.force_login(self.superuser)
+        self.factory = RequestFactory()
+
+    def test_post_add_without_confirm_add_m2m(self):
+        shops = [ShopFactory() for i in range(3)]
+
+        data = {"name": "name", "shops": [s.id for s in shops]}
+        response = self.client.post(reverse("admin:market_shoppingmall_add"), data)
+        # Redirects to changelist and is added
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response.url, "/admin/market/shoppingmall/")
+        self.assertEqual(ShoppingMall.objects.count(), 1)
+
+    def test_post_add_with_confirm_add_m2m(self):
+        ShoppingMallAdmin.confirmation_fields = ["shops"]
+        shops = [ShopFactory() for i in range(3)]
+
+        data = {"name": "name", "shops": [s.id for s in shops], "_confirm_add": True}
+        response = self.client.post(reverse("admin:market_shoppingmall_add"), data)
+
+        # Ensure not redirected (confirmation page does not redirect)
+        self.assertEqual(response.status_code, 200)
+        expected_templates = [
+            "admin/market/shoppingmall/change_confirmation.html",
+            "admin/market/change_confirmation.html",
+            "admin/change_confirmation.html",
+        ]
+        self.assertEqual(response.template_name, expected_templates)
+        form_data = [("name", "name")] + [("shops", s.id) for s in shops]
+        for k, v in form_data:
+            self.assertIn(
+                f'<input type="hidden" name="{ k }" value="{ v }">',
+                response.rendered_content,
+            )
+
+        # Should not have been added yet
+        self.assertEqual(ShoppingMall.objects.count(), 0)
+
+    def test_m2m_field_post_change_with_confirm_change(self):
+        shops = [ShopFactory() for i in range(10)]
+        shopping_mall = ShoppingMall.objects.create(name="My Mall")
+        shopping_mall.shops.set(shops)
+        # Currently ShoppingMall configured with confirmation_fields = ['name']
+        data = {
+            "name": "Not My Mall",
+            "shops": "1",
+            "id": shopping_mall.id,
+            "_confirm_change": True,
+            "csrfmiddlewaretoken": "fake token",
+            "_save": True,
+        }
+        response = self.client.post(
+            f"/admin/market/shoppingmall/{shopping_mall.id}/change/", data
+        )
+        # Ensure not redirected (confirmation page does not redirect)
+        self.assertEqual(response.status_code, 200)
+        expected_templates = [
+            "admin/market/shoppingmall/change_confirmation.html",
+            "admin/market/change_confirmation.html",
+            "admin/change_confirmation.html",
+        ]
+        self.assertEqual(response.template_name, expected_templates)
+        form_data = {
+            "name": "Not My Mall",
+            "shops": "1",
+            "id": str(shopping_mall.id),
+        }
+
+        for k, v in form_data.items():
+            self.assertIn(
+                f'<input type="hidden" name="{ k }" value="{ v }">',
+                response.rendered_content,
+            )
+
+        # Hasn't changed item yet
+        shopping_mall.refresh_from_db()
+        self.assertEqual(shopping_mall.name, "My Mall")
+
+    def test_m2m_field_post_change_with_confirm_change_multiple_selected(self):
+        shops = [ShopFactory() for i in range(10)]
+        shopping_mall = ShoppingMall.objects.create(name="My Mall")
+        shopping_mall.shops.set(shops)
+        # Currently ShoppingMall configured with confirmation_fields = ['name']
+        data = {
+            "name": "Not My Mall",
+            "shops": ["1", "2", "3"],
+            "id": shopping_mall.id,
+            "_confirm_change": True,
+            "csrfmiddlewaretoken": "fake token",
+            "_save": True,
+        }
+        response = self.client.post(
+            f"/admin/market/shoppingmall/{shopping_mall.id}/change/", data
+        )
+        # Ensure not redirected (confirmation page does not redirect)
+        self.assertEqual(response.status_code, 200)
+        expected_templates = [
+            "admin/market/shoppingmall/change_confirmation.html",
+            "admin/market/change_confirmation.html",
+            "admin/change_confirmation.html",
+        ]
+        self.assertEqual(response.template_name, expected_templates)
+        form_data = [
+            ("name", "Not My Mall"),
+            ("shops", "1"),
+            ("shops", "2"),
+            ("shops", "3"),
+            ("id", str(shopping_mall.id)),
+        ]
+
+        for k, v in form_data:
+            self.assertIn(
+                f'<input type="hidden" name="{ k }" value="{ v }">',
+                response.rendered_content,
+            )
+
+        # Hasn't changed item yet
+        shopping_mall.refresh_from_db()
+        self.assertEqual(shopping_mall.name, "My Mall")
+
+    def test_post_change_without_confirm_change_m2m_value(self):
+        # make the m2m the confirmation_field
+        ShoppingMallAdmin.confirmation_fields = ["shops"]
+        shops = [ShopFactory() for i in range(3)]
+        shopping_mall = ShoppingMall.objects.create(name="name")
+        shopping_mall.shops.set(shops)
+        assert shopping_mall.shops.count() == 3
+
+        data = {"name": "name", "id": str(shopping_mall.id), "shops": ["1"]}
+        response = self.client.post(
+            f"/admin/market/shoppingmall/{shopping_mall.id}/change/", data
+        )
+        # Redirects to changelist
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response.url, "/admin/market/shoppingmall/")
+        # Shop has changed
+        shopping_mall.refresh_from_db()
+        self.assertEqual(shopping_mall.shops.count(), 1)
+
+    def test_form_invalid_m2m_value(self):
+        ShoppingMallAdmin.confirmation_fields = ["shops"]
+        shopping_mall = ShoppingMall.objects.create(name="name")
+
+        data = {
+            "id": shopping_mall.id,
+            "name": "name",
+            "shops": ["1", "2", "3"],  # These shops don't exist
+            "_confirm_change": True,
+            "csrfmiddlewaretoken": "fake token",
+        }
+        response = self.client.post(
+            f"/admin/market/shoppingmall/{shopping_mall.id}/change/", data
+        )
+
+        # Form invalid should show errors on form
+        self.assertEqual(response.status_code, 200)
+        self.assertIsNotNone(response.context_data.get("errors"))
+        self.assertTrue(
+            str(response.context_data["errors"][0][0]).startswith(
+                "Select a valid choice."
+            )
+        )
+        # Should not have updated inventory
+        shopping_mall.refresh_from_db()
+        self.assertEqual(shopping_mall.shops.count(), 0)