launchpad-reviewers team mailing list archive

[Colin Watson <mp+427833@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:factory-proxy-translations into launchpad:master.

Commit message:
Return proxied object from various Translations factory methods

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

`LaunchpadObjectFactory` issues `UnproxiedFactoryMethodWarning` when its methods return objects not wrapped in a security proxy, since that tends to result in tests that are less accurate simulations of production.

Some tests failed in a way that revealed that `findIdenticalMessage` wasn't on the `ITranslationMessage` interface, which I think is a bug since it was called by production code outside the model (`lp.translations.utilities.translationmerger`), so I added that and also implemented it on `PlaceholderTranslationMessage`.  This allowed removing a `removeSecurityProxy` call from `lp.translations.utilities.translationmerger`, which seems like a good thing for hygiene.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:factory-proxy-translations into launchpad:master.

diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index ac56caf..2b2e111 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -4075,8 +4075,8 @@ class LaunchpadObjectFactory(ObjectFactory):
             potemplate = self.makePOTemplate(owner=owner, side=side)
         else:
             assert side is None, "Cannot specify both side and potemplate."
-        return potemplate.newPOFile(
-            language_code, create_sharing=create_sharing
+        return ProxyFactory(
+            potemplate.newPOFile(language_code, create_sharing=create_sharing)
         )
 
     def makePOTMsgSet(
@@ -4110,7 +4110,7 @@ class LaunchpadObjectFactory(ObjectFactory):
         if flagscomment is not None:
             potmsgset.flagscomment = flagscomment
         removeSecurityProxy(potmsgset).sync()
-        return potmsgset
+        return ProxyFactory(potmsgset)
 
     def makePOFileAndPOTMsgSet(
         self, language_code=None, msgid=None, with_plural=False, side=None
@@ -4176,7 +4176,7 @@ class LaunchpadObjectFactory(ObjectFactory):
             )
             naked_translation_message.date_created = date_created
             naked_translation_message.sync()
-        return translation_message
+        return ProxyFactory(translation_message)
 
     def makeCurrentTranslationMessage(
         self,
@@ -4290,7 +4290,7 @@ class LaunchpadObjectFactory(ObjectFactory):
 
         message.markReviewed(reviewer, date_reviewed)
 
-        return message
+        return ProxyFactory(message)
 
     def makeDivergedTranslationMessage(
         self,
diff --git a/lib/lp/translations/interfaces/translationmessage.py b/lib/lp/translations/interfaces/translationmessage.py
index f3fc23e..2301a9a 100644
--- a/lib/lp/translations/interfaces/translationmessage.py
+++ b/lib/lp/translations/interfaces/translationmessage.py
@@ -320,6 +320,16 @@ class ITranslationMessage(Interface):
         and self is deleted.
         """
 
+    def findIdenticalMessage(target_potmsgset, target_potemplate):
+        """Find a twin message to this one in a different template.
+
+        This returns the message distinct from this one with the lowest ID
+        that has the given template message (`target_potmsgset`) and
+        template (`target_potemplate`), the same language as this one, and
+        all the same translations as this one.  If there is no such message,
+        it returns None.
+        """
+
     def isHidden(pofile):
         """Whether this is an unused, hidden suggestion in `pofile`.
 
diff --git a/lib/lp/translations/model/translationmessage.py b/lib/lp/translations/model/translationmessage.py
index 2f7f237..a310b31 100644
--- a/lib/lp/translations/model/translationmessage.py
+++ b/lib/lp/translations/model/translationmessage.py
@@ -218,6 +218,10 @@ class PlaceholderTranslationMessage(TranslationMessageMixIn):
     def shareIfPossible(self):
         """See `ITranslationMessage`."""
 
+    def findIdenticalMessage(self, target_potmsgset, target_potemplate):
+        """See `ITranslationMessage`."""
+        return None
+
 
 @implementer(ITranslationMessage)
 class TranslationMessage(SQLBase, TranslationMessageMixIn):
diff --git a/lib/lp/translations/tests/test_translationmerger.py b/lib/lp/translations/tests/test_translationmerger.py
index 2ea8931..f1c6e54 100644
--- a/lib/lp/translations/tests/test_translationmerger.py
+++ b/lib/lp/translations/tests/test_translationmerger.py
@@ -5,6 +5,7 @@ import gc
 from logging import ERROR
 
 import transaction
+from storm.store import Store
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -580,14 +581,17 @@ class TestRemoveDuplicates(TestCaseWithFactory, TranslatedProductMixin):
             "snaggle", "snaggle"
         )
         trunk_message.is_current_upstream = False
-        trunk_message.sync()
+        store = Store.of(trunk_message)
+        store.flush()
+        store.autoreload(trunk_message)
 
         potmsgset = trunk_message.potmsgset
 
         stable_message.is_current_ubuntu = True
         stable_message.potemplate = trunk_message.potemplate
         stable_message.potmsgset = potmsgset
-        stable_message.sync()
+        store.flush()
+        store.autoreload(stable_message)
 
         # We've set up a situation where trunk has two identical
         # messages (one of which is current, the other imported) and
diff --git a/lib/lp/translations/utilities/translationmerger.py b/lib/lp/translations/utilities/translationmerger.py
index 01b849c..297f54a 100644
--- a/lib/lp/translations/utilities/translationmerger.py
+++ b/lib/lp/translations/utilities/translationmerger.py
@@ -560,8 +560,6 @@ class TranslationMerger:
                 seen_potmsgsets.add(subordinate.id)
 
                 for message in subordinate.getAllTranslationMessages():
-                    message = removeSecurityProxy(message)
-
                     (
                         clashing_current,
                         clashing_imported,