Compare foreign keys using pk, not id (#5681)

Trying to compare revisions of a page that includes changes to a foreign key
field of a related model that declared a custom primary key failed with an
uncaught exception.

The root cause was ForeignObjectComparison filtering by the id field, which is
not present in models that declare a custom primary key.

The solution is simply to filter by pk instead of id, which always maps to the
primary key of the corresponding model.

Include a regression unit test.

CHANGELOG.txt

@@ -11,6 +11,7 @@ Changelog
  * Fix: Rename documents listing column 'uploaded' to 'created' (LB (Ben Johnston))
  * Fix: Submenu items longer then the page height are no longer broken by the submenu footer (Igor van Spengen)
  * Fix: Unbundle the l18n library as it was bundled to avoid installation errors which have been resolved (Matt Westcott)
+ * Fix: Prevent error when comparing pages that reference a model with a custom primary key (Fidel Ramos)
 
 2.7 LTS (06.11.2019)
 ~~~~~~~~~~~~~~~~~~~~

docs/releases/2.8.rst

@@ -26,6 +26,7 @@ Bug fixes
  * Rename documents listing column 'uploaded' to 'created' (LB (Ben Johnston))
  * Submenu items longer then the page height are no longer broken by the submenu footer (Igor van Spengen)
  * Unbundle the l18n library as it was bundled to avoid installation errors which have been resolved (Matt Westcott)
+ * Prevent error when comparing pages that reference a model with a custom primary key (Fidel Ramos)
 
 
 Upgrade considerations

wagtail/admin/compare.py

@@ -288,8 +288,8 @@ class TagsFieldComparison(M2MFieldComparison):
 class ForeignObjectComparison(FieldComparison):
     def get_objects(self):
         model = self.field.related_model
-        obj_a = model.objects.filter(id=self.val_a).first()
-        obj_b = model.objects.filter(id=self.val_b).first()
+        obj_a = model.objects.filter(pk=self.val_a).first()
+        obj_b = model.objects.filter(pk=self.val_b).first()
         return obj_a, obj_b
 
     def htmldiff(self):

wagtail/admin/tests/test_compare.py

@@ -8,7 +8,8 @@ from wagtail.core.blocks import StreamValue
 from wagtail.images import get_image_model
 from wagtail.images.tests.utils import get_test_image_file
 from wagtail.tests.testapp.models import (
-    EventCategory, EventPage, EventPageSpeaker, HeadCountRelatedModelUsingPK, SimplePage,
+    AdvertWithCustomPrimaryKey, EventCategory, EventPage, EventPageSpeaker,
+    HeadCountRelatedModelUsingPK, SimplePage, SnippetChooserModelWithCustomPrimaryKey,
     StreamPage, TaggedPage)
 
 
@@ -508,6 +509,54 @@ class TestForeignObjectComparison(TestCase):
         self.assertTrue(comparison.has_changed())
 
 
+class TestForeignObjectComparisonWithCustomPK(TestCase):
+    """ForeignObjectComparison works with models declaring a custom primary key field"""
+
+    comparison_class = compare.ForeignObjectComparison
+
+    @classmethod
+    def setUpTestData(cls):
+        ad1 = AdvertWithCustomPrimaryKey.objects.create(
+            advert_id='ad1',
+            text='Advert 1'
+        )
+        ad2 = AdvertWithCustomPrimaryKey.objects.create(
+            advert_id='ad2',
+            text='Advert 2'
+        )
+        cls.test_obj_1 = SnippetChooserModelWithCustomPrimaryKey.objects.create(
+            advertwithcustomprimarykey=ad1
+        )
+        cls.test_obj_2 = SnippetChooserModelWithCustomPrimaryKey.objects.create(
+            advertwithcustomprimarykey=ad2
+        )
+
+    def test_hasnt_changed(self):
+        comparison = self.comparison_class(
+            SnippetChooserModelWithCustomPrimaryKey._meta.get_field('advertwithcustomprimarykey'),
+            self.test_obj_1,
+            self.test_obj_1,
+        )
+
+        self.assertTrue(comparison.is_field)
+        self.assertFalse(comparison.is_child_relation)
+        self.assertEqual(comparison.field_label(), 'Advertwithcustomprimarykey')
+        self.assertEqual(comparison.htmldiff(), 'Advert 1')
+        self.assertIsInstance(comparison.htmldiff(), SafeString)
+        self.assertFalse(comparison.has_changed())
+
+    def test_has_changed(self):
+        comparison = self.comparison_class(
+            SnippetChooserModelWithCustomPrimaryKey._meta.get_field('advertwithcustomprimarykey'),
+            self.test_obj_1,
+            self.test_obj_2,
+        )
+
+        self.assertEqual(comparison.htmldiff(), '<span class="deletion">Advert 1</span><span class="addition">Advert 2</span>')
+        self.assertIsInstance(comparison.htmldiff(), SafeString)
+        self.assertTrue(comparison.has_changed())
+
+
 class TestChildRelationComparison(TestCase):
     field_comparison_class = compare.FieldComparison
     comparison_class = compare.ChildRelationComparison