Merge pull request #1271 from django-import-export/check-manytomany-c…

…hanges

Use future value of ManyToManyField to check if value would change

docs/changelog.rst

@@ -10,6 +10,12 @@ Breaking changes
 This release makes the following changes to the API.  You may need to update your implementation to
 accommodate these changes.
 
+- Check value of ManyToManyField in skip_row() (#1271)
+   - This fixes an issue where ManyToMany fields are not checked correctly in `skip_row()`.
+     This means that `skip_row()` now takes `row` as a mandatory arg.
+     If you have overridden `skip_row()` in your own implementation, you will need to add `row`
+     as an arg.
+
 - Use 'create' flag instead of instance.pk (#1362)
    - ``import_export.resources.save_instance()`` now takes an additional mandatory argument: `is_create`.
      If you have over-ridden `save_instance()` in your own code, you will need to add this new argument.

import_export/resources.py

@@ -577,7 +577,7 @@ def for_delete(self, row, instance):
         """
         return False
 
-    def skip_row(self, instance, original):
+    def skip_row(self, instance, original, row):
         """
         Returns ``True`` if ``row`` importing should be skipped.
 
@@ -607,7 +607,13 @@ def skip_row(self, instance, original):
             try:
                 # For fields that are models.fields.related.ManyRelatedManager
                 # we need to compare the results
-                if list(field.get_value(instance).all()) != list(field.get_value(original).all()):
+                if isinstance(field.widget, widgets.ManyToManyWidget):
+                    # compare with the future value to detect changes
+                    instance_value = list(field.clean(row))
+                else:
+                    instance_value = list(field.get_value(instance).all())
+
+                if instance_value != list(field.get_value(original).all()):
                     return False
             except AttributeError:
                 if field.get_value(instance) != field.get_value(original):
@@ -700,7 +706,7 @@ def import_row(self, row, instance_loader, using_transactions=True, dry_run=Fals
                     # validate_instance(), where they can be combined with model
                     # instance validation errors if necessary
                     import_validation_errors = e.update_error_dict(import_validation_errors)
-                if self.skip_row(instance, original):
+                if self.skip_row(instance, original, row):
                     row_result.import_type = RowResult.IMPORT_TYPE_SKIP
                 else:
                     self.validate_instance(instance, import_validation_errors)

tests/core/tests/test_resources.py

@@ -1401,6 +1401,74 @@ def check_value(self, result, export_headers, expected_value):
                          expected_value)
 
 
+class ManyToManyWidgetDiffTest(TestCase):
+    # issue #1270 - ensure ManyToMany fields are correctly checked for
+    # changes when skip_unchanged=True
+    fixtures = ["category", "book"]
+
+    def setUp(self):
+        pass
+
+    def test_many_to_many_widget_create(self):
+        # the book is associated with 0 categories
+        # when we import a book with category 1, the book
+        # should be updated, not skipped
+        book = Book.objects.first()
+        book.categories.clear()
+        dataset_headers = ["id", "name", "categories"]
+        dataset_row = [book.id, book.name, "1"]
+        dataset = tablib.Dataset(headers=dataset_headers)
+        dataset.append(dataset_row)
+
+        book_resource = BookResource()
+        book_resource._meta.skip_unchanged = True
+        self.assertEqual(0, book.categories.count())
+
+        result = book_resource.import_data(dataset, dry_run=False)
+
+        book.refresh_from_db()
+        self.assertEqual(1, book.categories.count())
+        self.assertEqual(result.rows[0].import_type, results.RowResult.IMPORT_TYPE_UPDATE)
+        self.assertEqual(Category.objects.first(), book.categories.first())
+
+    def test_many_to_many_widget_update(self):
+        # the book is associated with 1 category ('Category 2')
+        # when we import a book with category 1, the book
+        # should be updated, not skipped, so that Category 2 is replaced by Category 1
+        book = Book.objects.first()
+        dataset_headers = ["id", "name", "categories"]
+        dataset_row = [book.id, book.name, "1"]
+        dataset = tablib.Dataset(headers=dataset_headers)
+        dataset.append(dataset_row)
+
+        book_resource = BookResource()
+        book_resource._meta.skip_unchanged = True
+        self.assertEqual(1, book.categories.count())
+
+        result = book_resource.import_data(dataset, dry_run=False)
+        self.assertEqual(result.rows[0].import_type, results.RowResult.IMPORT_TYPE_UPDATE)
+        self.assertEqual(1, book.categories.count())
+        self.assertEqual(Category.objects.first(), book.categories.first())
+
+    def test_many_to_many_widget_no_changes(self):
+        # the book is associated with 1 category ('Category 2')
+        # when we import a row with a book with category 1, the book
+        # should be skipped, because there is no change
+        book = Book.objects.first()
+        dataset_headers = ["id", "name", "categories"]
+        dataset_row = [book.id, book.name, book.categories.all()]
+        dataset = tablib.Dataset(headers=dataset_headers)
+        dataset.append(dataset_row)
+
+        book_resource = BookResource()
+        book_resource._meta.skip_unchanged = True
+
+        self.assertEqual(1, book.categories.count())
+        result = book_resource.import_data(dataset, dry_run=False)
+        self.assertEqual(result.rows[0].import_type, results.RowResult.IMPORT_TYPE_SKIP)
+        self.assertEqual(1, book.categories.count())
+
+
 @mock.patch("import_export.resources.Diff", spec=True)
 class SkipDiffTest(TestCase):
     """