launchpad-reviewers team mailing list archive

[Colin Watson <mp+451670@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:fix-stormify-person into launchpad:master.

Commit message:
Fix test failures due to "Convert Person to Storm"

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/451670

I had to reintroduce the old `Person._storm_sortingColumns`, but I took the opportunity to give it a less horribly confusing name and explain the circumstances where it's still needed.  The other changes are just little details I got slightly wrong.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-stormify-person into launchpad:master.

diff --git a/lib/lp/registry/model/distroseries.py b/lib/lp/registry/model/distroseries.py
index 4119824..2e19dfb 100644
--- a/lib/lp/registry/model/distroseries.py
+++ b/lib/lp/registry/model/distroseries.py
@@ -1580,7 +1580,7 @@ class DistroSeries(
             POTemplate.distroseries == self,
             POTemplate.iscurrent == True,
         )
-        contributors = contributors.order_by(*Person._sortingColumns)
+        contributors = contributors.order_by(Person._separated_sortingColumns)
         contributors = contributors.config(distinct=True)
         return contributors
 
diff --git a/lib/lp/registry/model/person.py b/lib/lp/registry/model/person.py
index dcf118e..778f1dd 100644
--- a/lib/lp/registry/model/person.py
+++ b/lib/lp/registry/model/person.py
@@ -498,6 +498,11 @@ class Person(
         return self.id
 
     sortingColumns = SQL("person_sort_key(Person.displayname, Person.name)")
+    # If we're using SELECT DISTINCT, then we can't use sortingColumns
+    # unless `person_sort_key(Person.displayname, Person.name)` is also in
+    # the select list, which usually isn't convenient.  Provide a separated
+    # version instead.
+    _separated_sortingColumns = ("Person.displayname", "Person.name")
     # When doing any sort of set operations (union, intersect, except_) with
     # Storm we can't use sortingColumns because the table name Person is not
     # available in that context, so we use this one.
diff --git a/lib/lp/registry/vocabularies.py b/lib/lp/registry/vocabularies.py
index 0104a33..b267fec 100644
--- a/lib/lp/registry/vocabularies.py
+++ b/lib/lp/registry/vocabularies.py
@@ -401,7 +401,7 @@ class UserTeamsParticipationVocabulary(StormVocabularyBase):
             teams = list(
                 IStore(Person)
                 .find(Person, *clauses)
-                .order_by(Person._sortingColumns)
+                .order_by(Person.sortingColumns)
             )
             # Users can view all the teams they belong to.
             precache_permission_for_objects(
diff --git a/lib/lp/translations/model/pofile.py b/lib/lp/translations/model/pofile.py
index 623fb69..96196ca 100644
--- a/lib/lp/translations/model/pofile.py
+++ b/lib/lp/translations/model/pofile.py
@@ -456,7 +456,7 @@ class POFile(StormBase, POFileMixIn):
             )
             .config(distinct=True)
         )
-        contributors = contributors.order_by(*Person._sortingColumns)
+        contributors = contributors.order_by(Person._separated_sortingColumns)
         contributors = contributors.config(distinct=True)
         return contributors
 
diff --git a/lib/lp/translations/scripts/remove_translations.py b/lib/lp/translations/scripts/remove_translations.py
index e854640..c042b8a 100644
--- a/lib/lp/translations/scripts/remove_translations.py
+++ b/lib/lp/translations/scripts/remove_translations.py
@@ -444,11 +444,11 @@ def remove_translations(
     conditions = set()
     if submitter is not None:
         conditions.add(
-            "TranslationMessage.submitter = %s" % sqlvalues(submitter.id)
+            "TranslationMessage.submitter = %s" % sqlvalues(submitter)
         )
     if reviewer is not None:
         conditions.add(
-            "TranslationMessage.reviewer = %s" % sqlvalues(reviewer.id)
+            "TranslationMessage.reviewer = %s" % sqlvalues(reviewer)
         )
     if date_created is not None:
         conditions.add(
diff --git a/lib/lp/translations/scripts/tests/test_remove_translations.py b/lib/lp/translations/scripts/tests/test_remove_translations.py
index 15c2638..dbea556 100644
--- a/lib/lp/translations/scripts/tests/test_remove_translations.py
+++ b/lib/lp/translations/scripts/tests/test_remove_translations.py
@@ -420,7 +420,7 @@ class TestRemoveTranslations(TestCase):
         # on reviewer instead.
         new_nl_message.reviewer = self.potemplate.owner
 
-        self._removeMessages(submitter=carlos)
+        self._removeMessages(submitter=carlos.id)
         self._checkInvariant()
 
     def test_RemoveByReviewer(self):
@@ -434,7 +434,7 @@ class TestRemoveTranslations(TestCase):
         new_nl_message.reviewer = carlos
         new_de_message.reviewer = carlos
 
-        self._removeMessages(reviewer=carlos)
+        self._removeMessages(reviewer=carlos.id)
         self._checkInvariant()
 
     def test_RemoveByDateCreated(self):
@@ -473,7 +473,7 @@ class TestRemoveTranslations(TestCase):
         )
         removeSecurityProxy(new_de_message).submitter = mark
 
-        self._removeMessages(submitter=carlos, date_created="2015-05-12")
+        self._removeMessages(submitter=carlos.id, date_created="2015-05-12")
 
         # First make sure we're not reading out of cache.
         Store.of(self.nl_pofile).flush()
@@ -502,7 +502,7 @@ class TestRemoveTranslations(TestCase):
         )
 
         rowcount = self._removeMessages(
-            submitter=carlos, date_created="2015-05-12"
+            submitter=carlos.id, date_created="2015-05-12"
         )
 
         self.assertEqual(rowcount, 1)