Launchpad itself

Merge lp:~jtv/launchpad/recife-check-conflicts into lp:~launchpad/launchpad/recife

  1. recife-check-conflicts
  2. Merge into recife
Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Graham Binns
Approved revision: no longer in the source branch.
Merged at revision: 9163
Proposed branch: lp:~jtv/launchpad/recife-check-conflicts
Merge into: lp:~launchpad/launchpad/recife
Prerequisite: lp:~jtv/launchpad/recife-retire-traits-helpers
Diff against target: 469 lines (+219/-27)
7 files modified
lib/lp/translations/interfaces/potmsgset.py (+8/-2)
lib/lp/translations/interfaces/translationmessage.py (+2/-1)
lib/lp/translations/model/potmsgset.py (+98/-15)
lib/lp/translations/model/translationmessage.py (+4/-2)
lib/lp/translations/tests/test_clearcurrenttranslation.py (+14/-1)
lib/lp/translations/tests/test_potmsgset.py (+77/-5)
lib/lp/translations/tests/test_translationmessage.py (+16/-1)
To merge this branch: bzr merge lp:~jtv/launchpad/recife-check-conflicts
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Review via email: mp+33494@code.launchpad.net

Commit message

Consistent support for conflict checks.

Description of the change

= Check for Translation Conflicts =

For the Recife feature branch. This implements more consistent support for translation conflict checks.

It's something we're already doing in the data model we have in production: when a translation is changed (whether through the web UI or through an import), a timestamp can be given that basically says, "this is when I looked at the state of this translation and decided to change it." The methods that change the translation can then check whether someone else might have set a new translation in the meantime; if they have, the later change is effectively obsolete.

Here I change the higher-level methods that set translations to accept a lock_timestamp, and check.

You'll notice that _checkForConflict builds on the existing _maybeRaiseTranslationConflict in a slightly awkward way. I hope we'll want to merge the two methods in the future to make this easier, but didn't want to pull in more changes for now. The third alternative would have been to check for identical translations _before_ calling _maybeRaiseTranslationConflict, but since this check is the most expensive step (and this is pretty "hot" code) I also wanted it to be the rarest.

To test:
{{{
./bin/test -vvc -m lp.translations.tests -t TestApprove -t CurrentTranslation
}}}

Jeroen

To post a comment you must log in.
Revision history for this message
Graham Binns (gmb) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/translations/interfaces/potmsgset.py'
2--- lib/lp/translations/interfaces/potmsgset.py 2010-08-10 17:28:08 +0000
3+++ lib/lp/translations/interfaces/potmsgset.py 2010-08-24 07:55:55 +0000
4@@ -267,7 +267,8 @@
5 """
6
7 def setCurrentTranslation(pofile, submitter, translations, origin,
8- share_with_other_side=False):
9+ share_with_other_side=False,
10+ lock_timestamp=None):
11 """Set the message's translation in Ubuntu, or upstream, or both.
12
13 :param pofile: `POFile` you're setting translations in. Other
14@@ -279,6 +280,8 @@
15 :param origin: A `RosettaTranslationOrigin`.
16 :param share_with_other_side: When sharing this translation,
17 share it with the other `TranslationSide` as well.
18+ :param lock_timestamp: Timestamp of the original translation state
19+ that this change is based on.
20 """
21
22 def resetCurrentTranslation(pofile, lock_timestamp):
23@@ -294,7 +297,8 @@
24 """
25
26 def clearCurrentTranslation(pofile, submitter, origin,
27- share_with_other_side=False):
28+ share_with_other_side=False,
29+ lock_timestamp=None):
30 """Set the current message in `pofile` to be untranslated.
31
32 If the current message is shared, this will also clear it in
33@@ -310,6 +314,8 @@
34 current on the other side (i.e. the Ubuntu side if working
35 upstream, or vice versa) then should it be cleared there as
36 well?
37+ :param lock_timestamp: Timestamp of the original translation state
38+ that this change is based on.
39 """
40
41 def applySanityFixes(unicode_text):
42
43=== modified file 'lib/lp/translations/interfaces/translationmessage.py'
44--- lib/lp/translations/interfaces/translationmessage.py 2010-08-20 05:56:43 +0000
45+++ lib/lp/translations/interfaces/translationmessage.py 2010-08-24 07:55:55 +0000
46@@ -240,7 +240,8 @@
47 It must not be referenced by any other object.
48 """
49
50- def approve(pofile, reviewer, share_with_other_side=False):
51+ def approve(pofile, reviewer, share_with_other_side=False,
52+ lock_timestamp=None):
53 """Approve this suggestion, making it a current translation."""
54
55 # XXX CarlosPerelloMarin 20071022: We should move this into browser code.
56
57=== modified file 'lib/lp/translations/model/potmsgset.py'
58--- lib/lp/translations/model/potmsgset.py 2010-08-24 07:55:54 +0000
59+++ lib/lp/translations/model/potmsgset.py 2010-08-24 07:55:55 +0000
60@@ -37,8 +37,7 @@
61 IPOTMsgSet,
62 POTMsgSetInIncompatibleTemplatesError,
63 TranslationCreditsType)
64-from lp.translations.interfaces.side import (
65- ITranslationSideTraitsSet, TranslationSide)
66+from lp.translations.interfaces.side import ITranslationSideTraitsSet
67 from lp.translations.interfaces.translationfileformat import (
68 TranslationFileFormat)
69 from lp.translations.interfaces.translationimporter import (
70@@ -89,6 +88,22 @@
71 incumbent_unknown = object()
72
73
74+def dictify_msgstrs(potranslations):
75+ """Represent `potranslations` as a normalized dict.
76+
77+ :param potranslations: a dict or sequence of `POTranslation`s per
78+ plural form.
79+ :return: a dict mapping each translated plural form to a
80+ `POTranslation`. Untranslated forms are omitted.
81+ """
82+ if not isinstance(potranslations, dict):
83+ potranslations = dict(enumerate(potranslations))
84+ return dict(
85+ (form, msgstr)
86+ for form, msgstr in potranslations.iteritems()
87+ if msgstr is not None)
88+
89+
90 class POTMsgSet(SQLBase):
91 implements(IPOTMsgSet)
92
93@@ -932,6 +947,60 @@
94 origin=RosettaTranslationOrigin.ROSETTAWEB, submitter=submitter,
95 **forms)
96
97+ def _checkForConflict(self, current_message, lock_timestamp,
98+ potranslations=None):
99+ """Check `message` for conflicting changes since `lock_timestamp`.
100+
101+ Call this before changing this message's translations, to ensure
102+ that a read-modify-write operation on a message does not
103+ accidentally overwrite newer changes based on older information.
104+
105+ One example of a read-modify-write operation is: user downloads
106+ translation file, translates a message, then re-uploads.
107+ Another is: user looks at a message in the web UI, decides that
108+ neither the current translation nor any of the suggestions are
109+ right, and clears the message.
110+
111+ In these scenarios, it's possible for someone else to come along
112+ and change the message's translation between the time we provide
113+ the user with a view of the current state and the time we
114+ receive a change from the user. We call this a conflict.
115+
116+ Raises `TranslationConflict` if a conflict exists.
117+
118+ :param currentmessage: The `TranslationMessage` that is current
119+ now. This is where we'll see any conflicting changes
120+ reflected in the date_reviewed timestamp.
121+ :param lock_timestamp: The timestamp of the translation state
122+ that the change is based on.
123+ :param potranslations: `POTranslation`s dict for the new
124+ translation. If these are given, and identical to those of
125+ `current_message`, there is no conflict.
126+ """
127+ if lock_timestamp is None:
128+ # We're not really being asked to check for conflicts.
129+ return
130+ if current_message is None:
131+ # There is no current message to conflict with.
132+ return
133+ try:
134+ self._maybeRaiseTranslationConflict(
135+ current_message, lock_timestamp)
136+ except TranslationConflict:
137+ if potranslations is None:
138+ # We don't know what translations are going to be set;
139+ # based on the timestamps this is a conflict.
140+ raise
141+ old_msgstrs = dictify_msgstrs(current_message.all_msgstrs)
142+ new_msgstrs = dictify_msgstrs(potranslations)
143+ if new_msgstrs != old_msgstrs:
144+ # Yup, there really is a difference. This is a proper
145+ # conflict.
146+ raise
147+ else:
148+ # Two identical translations crossed. Not a conflict.
149+ pass
150+
151 def _maybeRaiseTranslationConflict(self, message, lock_timestamp):
152 """Checks if there is a translation conflict for the message.
153
154@@ -1008,7 +1077,7 @@
155 **translation_args)
156
157 def approveSuggestion(self, pofile, suggestion, reviewer,
158- share_with_other_side=False):
159+ share_with_other_side=False, lock_timestamp=None):
160 """Approve a suggestion.
161
162 :param pofile: The `POFile` that the suggestion is being approved for.
163@@ -1017,6 +1086,8 @@
164 suggestion.
165 :param share_with_other_side: Policy selector: share this change with
166 the other translation side if possible?
167+ :param lock_timestamp: Timestamp of the original translation state
168+ that this change is based on.
169 """
170 template = pofile.potemplate
171 traits = getUtility(ITranslationSideTraitsSet).getTraits(
172@@ -1030,7 +1101,7 @@
173 activated_message = self._setTranslation(
174 pofile, translator, suggestion.origin, potranslations,
175 share_with_other_side=share_with_other_side,
176- identical_message=suggestion)
177+ identical_message=suggestion, lock_timestamp=lock_timestamp)
178
179 activated_message.markReviewed(reviewer)
180 if reviewer != translator:
181@@ -1038,7 +1109,8 @@
182 template.awardKarma(reviewer, 'translationreview')
183
184 def setCurrentTranslation(self, pofile, submitter, translations, origin,
185- share_with_other_side=False):
186+ share_with_other_side=False,
187+ lock_timestamp=None):
188 """See `IPOTMsgSet`."""
189 potranslations = self._findPOTranslations(translations)
190 identical_message = self._findTranslationMessage(
191@@ -1046,10 +1118,12 @@
192 return self._setTranslation(
193 pofile, submitter, origin, potranslations,
194 share_with_other_side=share_with_other_side,
195- identical_message=identical_message)
196+ identical_message=identical_message,
197+ lock_timestamp=lock_timestamp)
198
199 def _setTranslation(self, pofile, submitter, origin, potranslations,
200- identical_message=None, share_with_other_side=False):
201+ identical_message=None, share_with_other_side=False,
202+ lock_timestamp=None):
203 """Set the current translation.
204
205 :param pofile: The `POFile` to set the translation in.
206@@ -1060,11 +1134,13 @@
207 :param identical_message: The already existing message, if any,
208 that's either shared or diverged for `pofile.potemplate`,
209 whose translations are identical to the ones we're setting.
210- :param share_with_other_side:
211- :return:
212+ :param share_with_other_side: Propagate this change to the other
213+ translation side if appropriate.
214+ :param lock_timestamp: The timestamp of the translation state
215+ that the change is based on.
216+ :return: The `TranslationMessage` that is current after
217+ completion.
218 """
219- # XXX JeroenVermeulen 2010-08-16: Update pofile.date_changed.
220-
221 twin = identical_message
222
223 traits = getUtility(ITranslationSideTraitsSet).getTraits(
224@@ -1074,6 +1150,9 @@
225 incumbent_message = traits.getCurrentMessage(
226 self, pofile.potemplate, pofile.language)
227
228+ self._checkForConflict(
229+ incumbent_message, lock_timestamp, potranslations=potranslations)
230+
231 # Summary of the matrix:
232 # * If the incumbent message is diverged and we're setting a
233 # translation that's already shared: converge.
234@@ -1178,7 +1257,7 @@
235 traits.other_side_traits.getCurrentMessage(
236 self, pofile.potemplate, pofile.language))
237 if other_incumbent is None:
238- traits.other_side_side.setFlag(message, True)
239+ traits.other_side_traits.setFlag(message, True)
240 elif character == '+':
241 if share_with_other_side:
242 traits.other_side_traits.setFlag(message, True)
243@@ -1189,7 +1268,9 @@
244 if decisions == '':
245 message = twin
246
247- traits.setFlag(message, True)
248+ if not traits.getFlag(message):
249+ traits.setFlag(message, True)
250+ pofile.markChanged(translator=submitter)
251
252 return message
253
254@@ -1214,11 +1295,13 @@
255 pofile.date_changed = UTC_NOW
256
257 def clearCurrentTranslation(self, pofile, submitter, origin,
258- share_with_other_side=False):
259+ share_with_other_side=False,
260+ lock_timestamp=None):
261 """See `IPOTMsgSet`."""
262 message = self.setCurrentTranslation(
263 pofile, submitter, [], origin,
264- share_with_other_side=share_with_other_side)
265+ share_with_other_side=share_with_other_side,
266+ lock_timestamp=lock_timestamp)
267 message.markReviewed(submitter)
268
269 def applySanityFixes(self, text):
270
271=== modified file 'lib/lp/translations/model/translationmessage.py'
272--- lib/lp/translations/model/translationmessage.py 2010-08-20 05:56:43 +0000
273+++ lib/lp/translations/model/translationmessage.py 2010-08-24 07:55:55 +0000
274@@ -326,11 +326,13 @@
275 date_reviewed = current.date_created
276 return date_reviewed > self.date_created
277
278- def approve(self, pofile, reviewer, share_with_other_side=False):
279+ def approve(self, pofile, reviewer, share_with_other_side=False,
280+ lock_timestamp=None):
281 """See `ITranslationMessage`."""
282 self.potmsgset.approveSuggestion(
283 pofile, self, reviewer,
284- share_with_other_side=share_with_other_side)
285+ share_with_other_side=share_with_other_side,
286+ lock_timestamp=lock_timestamp)
287
288 def getOnePOFile(self):
289 """See `ITranslationMessage`."""
290
291=== modified file 'lib/lp/translations/tests/test_clearcurrenttranslation.py'
292--- lib/lp/translations/tests/test_clearcurrenttranslation.py 2010-08-20 05:43:19 +0000
293+++ lib/lp/translations/tests/test_clearcurrenttranslation.py 2010-08-24 07:55:55 +0000
294@@ -21,7 +21,9 @@
295 from lp.testing import TestCaseWithFactory
296 from lp.translations.interfaces.side import ITranslationSideTraitsSet
297 from lp.translations.interfaces.translationmessage import (
298- RosettaTranslationOrigin)
299+ RosettaTranslationOrigin,
300+ TranslationConflict,
301+ )
302
303 ORIGIN = RosettaTranslationOrigin.SCM
304
305@@ -249,6 +251,17 @@
306 self.assertEqual(new_reviewer, current.reviewer)
307 self.assertSqlAttributeEqualsDate(current, 'date_reviewed', UTC_NOW)
308
309+ def test_detects_conflict(self):
310+ pofile = self._makePOFile()
311+ current_message = self.factory.makeCurrentTranslationMessage(
312+ pofile=pofile)
313+ old = datetime.now(UTC) - timedelta(days=7)
314+
315+ self.assertRaises(
316+ TranslationConflict,
317+ current_message.potmsgset.clearCurrentTranslation,
318+ pofile, self.factory.makePerson(), ORIGIN, lock_timestamp=old)
319+
320
321 class TestClearCurrentTranslationUpstream(TestCaseWithFactory,
322 ScenarioMixin):
323
324=== modified file 'lib/lp/translations/tests/test_potmsgset.py'
325--- lib/lp/translations/tests/test_potmsgset.py 2010-08-24 07:55:54 +0000
326+++ lib/lp/translations/tests/test_potmsgset.py 2010-08-24 07:55:55 +0000
327@@ -14,6 +14,8 @@
328 from zope.security.proxy import isinstance as zope_isinstance
329 from zope.security.proxy import removeSecurityProxy
330
331+from storm.locals import Store
332+
333 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
334 from lp.registry.interfaces.person import IPersonSet
335 from lp.registry.interfaces.product import IProductSet
336@@ -24,7 +26,6 @@
337 TranslationFileFormat)
338 from lp.translations.interfaces.translationmessage import (
339 RosettaTranslationOrigin, TranslationConflict)
340-from lp.translations.interfaces.side import TranslationSide
341 from lp.translations.model.translationmessage import (
342 DummyTranslationMessage)
343
344@@ -945,11 +946,10 @@
345 name='main', product=self.foo)
346 removeSecurityProxy(self.foo).official_rosetta = True
347
348- self.potemplate = self.factory.makePOTemplate(
349+ template = self.potemplate = self.factory.makePOTemplate(
350 productseries=self.foo_main, name="messages")
351- self.potmsgset = self.factory.makePOTMsgSet(self.potemplate,
352- sequence=1)
353- self.pofile = self.factory.makePOFile('eo', self.potemplate)
354+ self.potmsgset = self.factory.makePOTMsgSet(template, sequence=1)
355+ self.pofile = self.factory.makePOFile('eo', template)
356
357 def test_resetCurrentTranslation_shared(self):
358 # Resetting a shared current translation will change iscurrent=False
359@@ -1625,3 +1625,75 @@
360
361 self.assertEqual(message, potmsgset.getImportedTranslationMessage(
362 pofile.potemplate, pofile.language))
363+
364+ def test_detects_conflict(self):
365+ pofile, potmsgset = self._makePOFileAndPOTMsgSet()
366+ translations = self._makeTranslations(potmsgset)
367+ origin = RosettaTranslationOrigin.ROSETTAWEB
368+
369+ # A translator bases a change on a page view from 5 minutes ago.
370+ lock_timestamp = datetime.now(pytz.UTC) - timedelta(minutes=5)
371+
372+ # Meanwhile someone else changes the same message's translation.
373+ newer_translation = self.factory.makeCurrentTranslationMessage(
374+ pofile=pofile, potmsgset=potmsgset)
375+
376+ # This raises a translation conflict.
377+ self.assertRaises(
378+ TranslationConflict,
379+ potmsgset.setCurrentTranslation,
380+ pofile, pofile.potemplate.owner, translations, origin,
381+ lock_timestamp=lock_timestamp)
382+
383+
384+class TestCheckForConflict(TestCaseWithFactory):
385+ """Test POTMsgSet._checkForConflict."""
386+
387+ layer = ZopelessDatabaseLayer
388+
389+ def test_passes_nonconflict(self):
390+ # If there is no conflict, _checkForConflict completes normally.
391+ current_tm = self.factory.makeCurrentTranslationMessage()
392+ potmsgset = removeSecurityProxy(current_tm.potmsgset)
393+ newer = current_tm.date_reviewed + timedelta(days=1)
394+
395+ potmsgset._checkForConflict(current_tm, newer)
396+
397+ def test_detects_conflict(self):
398+ # If there's been another translation since lock_timestamp,
399+ # _checkForConflict raises TranslationConflict.
400+ current_tm = self.factory.makeCurrentTranslationMessage()
401+ potmsgset = removeSecurityProxy(current_tm.potmsgset)
402+ older = current_tm.date_reviewed - timedelta(days=1)
403+
404+ self.assertRaises(
405+ TranslationConflict,
406+ potmsgset._checkForConflict,
407+ current_tm, older)
408+
409+ def test_passes_identical_change(self):
410+ # When concurrent translations are identical, there is no
411+ # conflict.
412+ current_tm = self.factory.makeCurrentTranslationMessage()
413+ potmsgset = removeSecurityProxy(current_tm.potmsgset)
414+ older = current_tm.date_reviewed - timedelta(days=1)
415+
416+ potmsgset._checkForConflict(
417+ current_tm, older, potranslations=current_tm.all_msgstrs)
418+
419+ def test_quiet_if_no_current_message(self):
420+ # If there is no current translation, _checkForConflict accepts
421+ # that as conflict-free.
422+ potemplate = self.factory.makePOTemplate()
423+ potmsgset = self.factory.makePOTMsgSet(potemplate, sequence=1)
424+ old = datetime.now(pytz.UTC) - timedelta(days=366)
425+
426+ removeSecurityProxy(potmsgset)._checkForConflict(None, old)
427+
428+ def test_quiet_if_no_timestamp(self):
429+ # If there is no lock_timestamp, _checkForConflict does not
430+ # check for conflicts.
431+ current_tm = self.factory.makeCurrentTranslationMessage()
432+ potmsgset = removeSecurityProxy(current_tm.potmsgset)
433+
434+ removeSecurityProxy(potmsgset)._checkForConflict(current_tm, None)
435
436=== modified file 'lib/lp/translations/tests/test_translationmessage.py'
437--- lib/lp/translations/tests/test_translationmessage.py 2010-08-17 10:13:25 +0000
438+++ lib/lp/translations/tests/test_translationmessage.py 2010-08-24 07:55:55 +0000
439@@ -18,7 +18,9 @@
440 from lp.translations.model.translationmessage import DummyTranslationMessage
441 from lp.translations.interfaces.side import ITranslationSideTraitsSet
442 from lp.translations.interfaces.translationmessage import (
443- ITranslationMessage)
444+ ITranslationMessage,
445+ TranslationConflict,
446+ )
447 from lp.translations.interfaces.translations import TranslationConstants
448 from canonical.testing import ZopelessDatabaseLayer
449
450@@ -250,6 +252,19 @@
451
452 self.assertEqual([], karmarecorder.karma_events)
453
454+ def test_approve_detects_conflict(self):
455+ pofile = self.factory.makePOFile('bo')
456+ current = self.factory.makeCurrentTranslationMessage(pofile=pofile)
457+ potmsgset = current.potmsgset
458+ suggestion = self.factory.makeSuggestion(
459+ pofile=pofile, potmsgset=potmsgset)
460+ old = datetime.now(UTC) - timedelta(days=1)
461+
462+ self.assertRaises(
463+ TranslationConflict,
464+ suggestion.approve,
465+ pofile, self.factory.makePerson(), lock_timestamp=old)
466+
467
468 class TestTranslationMessageFindIdenticalMessage(TestCaseWithFactory):
469 """Tests for `TranslationMessage.findIdenticalMessage`."""

Subscribers

People subscribed via source and target branches