Merge lp:~jtv/launchpad/recife-pre-sharing-permissions into lp:~launchpad/launchpad/recife
- recife-pre-sharing-permissions
- Merge into recife
Proposed by
Jeroen T. Vermeulen
| Status: | Merged |
|---|---|
| Approved by: | Gavin Panella |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | 9173 |
| Proposed branch: | lp:~jtv/launchpad/recife-pre-sharing-permissions |
| Merge into: | lp:~launchpad/launchpad/recife |
| Diff against target: |
801 lines (+239/-168) 15 files modified
lib/lp/testing/factory.py (+8/-2) lib/lp/testing/tests/test_factory.py (+8/-0) lib/lp/translations/browser/pofile.py (+0/-9) lib/lp/translations/browser/tests/pofile-base-views.txt (+0/-2) lib/lp/translations/browser/tests/test_translationmessage_view.py (+63/-4) lib/lp/translations/browser/tests/translationmessage-views.txt (+2/-28) lib/lp/translations/browser/translationmessage.py (+74/-56) lib/lp/translations/interfaces/pofile.py (+3/-0) lib/lp/translations/model/pofile.py (+35/-11) lib/lp/translations/stories/standalone/xx-licensing.txt (+0/-11) lib/lp/translations/stories/standalone/xx-pofile-translate.txt (+0/-12) lib/lp/translations/stories/standalone/xx-translationmessage-translate.txt (+0/-12) lib/lp/translations/tests/test_pofile.py (+43/-18) lib/lp/translations/utilities/tests/test_file_importer.py (+2/-2) lib/lp/translations/utilities/translation_import.py (+1/-1) |
| To merge this branch: | bzr merge lp:~jtv/launchpad/recife-pre-sharing-permissions |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Gavin Panella (community) | code | Approve | |
| Launchpad code reviewers | code | Pending | |
|
Review via email:
|
|||
Commit message
Cleanups in preparation for Recife sharing permissions check.
Description of the change
= Cleanups prior to Recife sharing permissions checks =
This is a cleanup to be merged into the Recife feature branch.
I'm working on the part of the Recife model change where we check if a particular translation submission for Ubuntu should be shared with upstream, or vice versa. In my preparations I came across some things that needed cleaning up:
* The basic translation view checked far too many separate things in one big method.
* View failures generated some truly horrible error messages.
* Existing permissions checks needed breaking down into smaller chunks to be comprehensible.
* Some inefficient "do we know how plural forms work for this POFile" logic was also redundant.
* Reading of "lock_timestamp" was tested in a doctest instead of a view unit test.
* Lots and lots of trailing whitespace.
I sped up the inefficient logic by doing the cheapest (and often sufficient) test first, centralized it into a single method, and replaced the view properties that duplicated it with a single attribute.
To test, best run all Translations tests really:
{{{
./bin/test -vvc lp.translations
}}}
There's a bit of lint left, most related to comment blocks that aren't directly attached to their respective next lines. I think that style is valid where the comment applies to a section of a class, as these do, so I didn't touch these cases. The other source of lint is Moin-style headings in doctests, which I didn't touch in order to stay under the review limit.
Jeroen
To post a comment you must log in.
Revision history for this message
| Gavin Panella (allenap) : | # |
review:
Approve
(code)
Revision history for this message
| Jeroen T. Vermeulen (jtv) wrote : | # |
> Nice branch, r=me, with a few very minor comments.
Thanks! Good stuff. My replies:
> [1]
>
> +from __future__ import with_statement
>
> I don't think we need to do this anymore.
So did I, but there have been nasty surprises lately where we _thought_ we were all the way there (just a few days ago there was another "we're on 2.6!—er, almost" thread) so I decided to be conservative. I suppose at some point we'll clean these up collectively.
> [2]
>
> + if blank_timestamp:
> + view.lock_timestamp = None
> + else:
> + view.lock_timestamp = datetime.
>
> The blank_timestamp argument is only used once. If you did the
> following in that one place, could this argument be removed and the
> code above simplified?
>
> view = self._makeView()
> view.lock_timestamp = None
Yup. Much better!
> [3]
>
> +All the tests will be submitted as comming from Kurem, an editor for the
>
> s/comming/coming/
Thanks for spotting that. Normally I'm the big pedant.
> [4]
>
> + if method == 'POST' and self.request.
>
> What should submit_translations look like? The style guide would say
> that this needs to be explicit.
You're right. AFAICS the ideal spelling is "if 'submit_
> [5]
>
> + if is_read_only():
> + raise UnexpectedFormData(
> + "Launchpad is currently in read-only mode for maintenance. "
> + "Please try again later.")
>
> Is this not taken care of earlier in the request by, say, the
> publication machinery? I haven't done anything to make views cope with
> read-only mode, so I'd like to know if I can continue to be oblivious
> to this or if I need to do something about it.
In theory, yes, this is taken care of. But read-only mode ignores transaction boundaries and isolation levels: it is signaled by the presence of a particular file. That file can appear while processing a request. This check is here because we had one or two oopses while switching to read-only mode.
> [6]
>
> + if self.user is None:
> + raise UnexpectedFormD
>
> Is this needed? Similar question to [5] really.
It's needed for, arguably, a stupid reason. (Don't think I knew this; I checked up just now). Yes, the permissions machinery takes care of it but at a later stage. We're checking it here because the rest of the initialize code makes use of self.user, and bailing out here is easier than guarding all those cases just so we can arrive at another similar error at the end.
Jeroen
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
| 1 | === modified file 'lib/lp/testing/factory.py' |
| 2 | --- lib/lp/testing/factory.py 2010-10-05 17:06:23 +0000 |
| 3 | +++ lib/lp/testing/factory.py 2010-10-15 04:57:46 +0000 |
| 4 | @@ -1794,16 +1794,22 @@ |
| 5 | MessageChunk(message=message, sequence=1, content=content) |
| 6 | return message |
| 7 | |
| 8 | - def makeLanguage(self, language_code=None, name=None, pluralforms=None): |
| 9 | + def makeLanguage(self, language_code=None, name=None, pluralforms=None, |
| 10 | + plural_expression=None): |
| 11 | """Makes a language given the language_code and name.""" |
| 12 | if language_code is None: |
| 13 | language_code = self.getUniqueString('lang') |
| 14 | if name is None: |
| 15 | name = "Language %s" % language_code |
| 16 | + if plural_expression is None and pluralforms is not None: |
| 17 | + # If the number of plural forms is known, the language |
| 18 | + # should also have a plural expression and vice versa. |
| 19 | + plural_expression = 'n %% %d' % pluralforms |
| 20 | |
| 21 | language_set = getUtility(ILanguageSet) |
| 22 | return language_set.createLanguage( |
| 23 | - language_code, name, pluralforms=pluralforms) |
| 24 | + language_code, name, pluralforms=pluralforms, |
| 25 | + pluralexpression=plural_expression) |
| 26 | |
| 27 | def makeLibraryFileAlias(self, filename=None, content=None, |
| 28 | content_type='text/plain', restricted=False, |
| 29 | |
| 30 | === modified file 'lib/lp/testing/tests/test_factory.py' |
| 31 | --- lib/lp/testing/tests/test_factory.py 2010-09-30 17:57:01 +0000 |
| 32 | +++ lib/lp/testing/tests/test_factory.py 2010-10-15 04:57:46 +0000 |
| 33 | @@ -431,6 +431,14 @@ |
| 34 | for number_of_forms in [None, 1, 3]: |
| 35 | language = self.factory.makeLanguage(pluralforms=number_of_forms) |
| 36 | self.assertEqual(number_of_forms, language.pluralforms) |
| 37 | + self.assertEqual( |
| 38 | + number_of_forms is None, language.pluralexpression is None) |
| 39 | + |
| 40 | + def test_makeLanguage_with_plural_expression(self): |
| 41 | + expression = '(n+1) % 5' |
| 42 | + language = self.factory.makeLanguage( |
| 43 | + pluralforms=5, plural_expression=expression) |
| 44 | + self.assertEqual(expression, language.pluralexpression) |
| 45 | |
| 46 | # makeSourcePackagePublishingHistory |
| 47 | def test_makeSourcePackagePublishingHistory_returns_ISPPH(self): |
| 48 | |
| 49 | === modified file 'lib/lp/translations/browser/pofile.py' |
| 50 | --- lib/lp/translations/browser/pofile.py 2010-09-06 10:40:54 +0000 |
| 51 | +++ lib/lp/translations/browser/pofile.py 2010-10-15 04:57:46 +0000 |
| 52 | @@ -364,15 +364,6 @@ |
| 53 | return statement |
| 54 | |
| 55 | @property |
| 56 | - def has_plural_form_information(self): |
| 57 | - """Return whether we know the plural forms for this language.""" |
| 58 | - if self.context.potemplate.hasPluralMessage(): |
| 59 | - return self.context.language.pluralforms is not None |
| 60 | - # If there are no plural forms, we assume that we have the |
| 61 | - # plural form information for this language. |
| 62 | - return True |
| 63 | - |
| 64 | - @property |
| 65 | def number_of_plural_forms(self): |
| 66 | """The number of plural forms for the language or 1 if not known.""" |
| 67 | if self.context.language.pluralforms is not None: |
| 68 | |
| 69 | === modified file 'lib/lp/translations/browser/tests/pofile-base-views.txt' |
| 70 | --- lib/lp/translations/browser/tests/pofile-base-views.txt 2009-09-09 18:03:44 +0000 |
| 71 | +++ lib/lp/translations/browser/tests/pofile-base-views.txt 2010-10-15 04:57:46 +0000 |
| 72 | @@ -37,8 +37,6 @@ |
| 73 | |
| 74 | The view has information about the languages plural forms: |
| 75 | |
| 76 | - >>> print view.has_plural_form_information |
| 77 | - True |
| 78 | >>> print view.number_of_plural_forms |
| 79 | 2 |
| 80 | >>> print view.plural_expression |
| 81 | |
| 82 | === modified file 'lib/lp/translations/browser/tests/test_translationmessage_view.py' |
| 83 | --- lib/lp/translations/browser/tests/test_translationmessage_view.py 2010-08-23 08:35:29 +0000 |
| 84 | +++ lib/lp/translations/browser/tests/test_translationmessage_view.py 2010-10-15 04:57:46 +0000 |
| 85 | @@ -1,22 +1,30 @@ |
| 86 | # Copyright 2009 Canonical Ltd. This software is licensed under the |
| 87 | # GNU Affero General Public License version 3 (see the file LICENSE). |
| 88 | |
| 89 | +from __future__ import with_statement |
| 90 | + |
| 91 | __metaclass__ = type |
| 92 | |
| 93 | from datetime import ( |
| 94 | datetime, |
| 95 | timedelta, |
| 96 | ) |
| 97 | -from unittest import TestLoader |
| 98 | |
| 99 | import pytz |
| 100 | |
| 101 | from canonical.launchpad.webapp.servers import LaunchpadTestRequest |
| 102 | from canonical.testing import ZopelessDatabaseLayer |
| 103 | -from lp.testing import TestCaseWithFactory |
| 104 | +from lp.app.errors import UnexpectedFormData |
| 105 | +from lp.testing import ( |
| 106 | + anonymous_logged_in, |
| 107 | + person_logged_in, |
| 108 | + TestCaseWithFactory, |
| 109 | + ) |
| 110 | from lp.translations.browser.translationmessage import ( |
| 111 | + CurrentTranslationMessagePageView, |
| 112 | CurrentTranslationMessageView, |
| 113 | ) |
| 114 | +from lp.translations.interfaces.translationsperson import ITranslationsPerson |
| 115 | |
| 116 | |
| 117 | class TestCurrentTranslationMessage_can_dismiss(TestCaseWithFactory): |
| 118 | @@ -168,5 +176,56 @@ |
| 119 | self._assertConfirmEmptyPluralPackaged(True, False, False, False) |
| 120 | |
| 121 | |
| 122 | -def test_suite(): |
| 123 | - return TestLoader().loadTestsFromName(__name__) |
| 124 | +class TestCurrentTranslationMessagePageView(TestCaseWithFactory): |
| 125 | + """Test `CurrentTranslationMessagePageView` and its base class.""" |
| 126 | + |
| 127 | + layer = ZopelessDatabaseLayer |
| 128 | + |
| 129 | + def _makeView(self): |
| 130 | + message = self.factory.makeTranslationMessage() |
| 131 | + request = LaunchpadTestRequest() |
| 132 | + view = CurrentTranslationMessagePageView(message, request) |
| 133 | + view.lock_timestamp = datetime.now(pytz.utc) |
| 134 | + return view |
| 135 | + |
| 136 | + def test_extractLockTimestamp(self): |
| 137 | + view = self._makeView() |
| 138 | + view.request.form['lock_timestamp'] = u'2010-01-01 00:00:00 UTC' |
| 139 | + self.assertEqual( |
| 140 | + datetime(2010, 01, 01, tzinfo=pytz.utc), |
| 141 | + view._extractLockTimestamp()) |
| 142 | + |
| 143 | + def test_extractLockTimestamp_returns_None_by_default(self): |
| 144 | + view = self._makeView() |
| 145 | + self.assertIs(None, view._extractLockTimestamp()) |
| 146 | + |
| 147 | + def test_extractLockTimestamp_returns_None_for_bogus_timestamp(self): |
| 148 | + view = self._makeView() |
| 149 | + view.request.form['lock_timestamp'] = u'Hi mom!' |
| 150 | + self.assertIs(None, view._extractLockTimestamp()) |
| 151 | + |
| 152 | + def test_checkSubmitConditions_passes(self): |
| 153 | + with person_logged_in(self.factory.makePerson()): |
| 154 | + view = self._makeView() |
| 155 | + view._checkSubmitConditions() |
| 156 | + |
| 157 | + def test_checkSubmitConditions_requires_lock_timestamp(self): |
| 158 | + with person_logged_in(self.factory.makePerson()): |
| 159 | + view = self._makeView() |
| 160 | + view.lock_timestamp = None |
| 161 | + self.assertRaises(UnexpectedFormData, view._checkSubmitConditions) |
| 162 | + |
| 163 | + def test_checkSubmitConditions_rejects_anonymous_request(self): |
| 164 | + with anonymous_logged_in(): |
| 165 | + view = self._makeView() |
| 166 | + self.assertRaises(UnexpectedFormData, view._checkSubmitConditions) |
| 167 | + |
| 168 | + def test_checkSubmitConditions_rejects_license_decliners(self): |
| 169 | + # Users who have declined the relicensing agreement can't post |
| 170 | + # translations. |
| 171 | + decliner = self.factory.makePerson() |
| 172 | + ITranslationsPerson(decliner).translations_relicensing_agreement = ( |
| 173 | + False) |
| 174 | + with person_logged_in(decliner): |
| 175 | + view = self._makeView() |
| 176 | + self.assertRaises(UnexpectedFormData, view._checkSubmitConditions) |
| 177 | |
| 178 | === modified file 'lib/lp/translations/browser/tests/translationmessage-views.txt' |
| 179 | --- lib/lp/translations/browser/tests/translationmessage-views.txt 2010-09-06 10:40:54 +0000 |
| 180 | +++ lib/lp/translations/browser/tests/translationmessage-views.txt 2010-10-15 04:57:46 +0000 |
| 181 | @@ -11,8 +11,8 @@ |
| 182 | >>> from lp.services.worlddata.interfaces.language import ILanguageSet |
| 183 | >>> from canonical.launchpad.webapp import canonical_url |
| 184 | |
| 185 | -All the tests will be submitted as comming from Kurem, an editor for the POFile |
| 186 | -that we are going to edit. |
| 187 | +All the tests will be submitted as coming from Kurem, an editor for the |
| 188 | +POFile that we are going to edit. |
| 189 | |
| 190 | >>> login('kurem@debian.cz') |
| 191 | |
| 192 | @@ -230,30 +230,6 @@ |
| 193 | |
| 194 | A new translation is submitted through the view. |
| 195 | |
| 196 | - >>> server_url = '/'.join( |
| 197 | - ... [canonical_url(translationmessage), '+translate']) |
| 198 | - >>> form = { |
| 199 | - ... 'alt': None, |
| 200 | - ... 'msgset_1': None, |
| 201 | - ... 'msgset_1_es_translation_0_new_checkbox': True, |
| 202 | - ... 'msgset_1_es_translation_0_new': 'Foo', |
| 203 | - ... 'submit_translations': 'Save & Continue'} |
| 204 | - >>> translationmessage_page_view = create_view( |
| 205 | - ... translationmessage, "+translate", form=form, |
| 206 | - ... layer=TranslationsLayer, server_url=server_url) |
| 207 | - >>> translationmessage_page_view.request.method = 'POST' |
| 208 | - |
| 209 | -And when we initialise the view class, we detect that we missed the timestamp |
| 210 | -that allow us to know whether we are working with latest submitted value or |
| 211 | -someone updated the database while we were working on those translations. |
| 212 | - |
| 213 | - >>> translationmessage_page_view.initialize() |
| 214 | - Traceback (most recent call last): |
| 215 | - ... |
| 216 | - UnexpectedFormData: We didn't find the timestamp... |
| 217 | - |
| 218 | -We do a new submit, but this time including a lock_timestamp field. |
| 219 | - |
| 220 | >>> form = { |
| 221 | ... 'lock_timestamp': '2006-11-28T13:00:00+00:00', |
| 222 | ... 'alt': None, |
| 223 | @@ -269,8 +245,6 @@ |
| 224 | >>> translationmessage_page_view.initialize() |
| 225 | >>> transaction.commit() |
| 226 | |
| 227 | -This time we didn't get any problem with the submission. |
| 228 | - |
| 229 | Now, let's see how the system prevents a submission that has a timestamp older |
| 230 | than when last current translation was submitted. |
| 231 | |
| 232 | |
| 233 | === modified file 'lib/lp/translations/browser/translationmessage.py' |
| 234 | --- lib/lp/translations/browser/translationmessage.py 2010-09-06 10:40:54 +0000 |
| 235 | +++ lib/lp/translations/browser/translationmessage.py 2010-10-15 04:57:46 +0000 |
| 236 | @@ -36,6 +36,7 @@ |
| 237 | from zope.interface import implements |
| 238 | from zope.schema.vocabulary import getVocabularyRegistry |
| 239 | |
| 240 | +from canonical.launchpad.readonly import is_read_only |
| 241 | from canonical.launchpad.webapp import ( |
| 242 | ApplicationMenu, |
| 243 | canonical_url, |
| 244 | @@ -64,6 +65,7 @@ |
| 245 | RosettaTranslationOrigin, |
| 246 | TranslationConflict, |
| 247 | ) |
| 248 | +from lp.translations.interfaces.translations import TranslationConstants |
| 249 | from lp.translations.interfaces.translationsperson import ITranslationsPerson |
| 250 | from lp.translations.utilities.validate import GettextValidationError |
| 251 | |
| 252 | @@ -230,9 +232,6 @@ |
| 253 | """ |
| 254 | |
| 255 | pofile = None |
| 256 | - # There will never be 100 plural forms. Usually, we'll be iterating |
| 257 | - # over just two or three. |
| 258 | - MAX_PLURAL_FORMS = 100 |
| 259 | |
| 260 | @property |
| 261 | def label(self): |
| 262 | @@ -246,6 +245,9 @@ |
| 263 | def initialize(self): |
| 264 | assert self.pofile, "Child class must define self.pofile" |
| 265 | |
| 266 | + self.has_plural_form_information = ( |
| 267 | + self.pofile.hasPluralFormInformation()) |
| 268 | + |
| 269 | # These two dictionaries hold translation data parsed from the |
| 270 | # form submission. They exist mainly because of the need to |
| 271 | # redisplay posted translations when they contain errors; if not |
| 272 | @@ -286,6 +288,16 @@ |
| 273 | |
| 274 | self._initializeAltLanguage() |
| 275 | |
| 276 | + method = self.request.method |
| 277 | + if method == 'POST': |
| 278 | + self.lock_timestamp = self._extractLockTimestamp() |
| 279 | + self._checkSubmitConditions() |
| 280 | + else: |
| 281 | + # It's not a POST, so we should generate lock_timestamp. |
| 282 | + UTC = pytz.timezone('UTC') |
| 283 | + self.lock_timestamp = datetime.datetime.now(UTC) |
| 284 | + |
| 285 | + |
| 286 | # The batch navigator needs to be initialized early, before |
| 287 | # _submitTranslations is called; the reason for this is that |
| 288 | # _submitTranslations, in the case of no errors, redirects to |
| 289 | @@ -297,50 +309,65 @@ |
| 290 | self.start = self.batchnav.start |
| 291 | self.size = self.batchnav.currentBatch().size |
| 292 | |
| 293 | - if self.request.method == 'POST': |
| 294 | - if self.user is None: |
| 295 | - raise UnexpectedFormData( |
| 296 | - "Anonymous users or users who are not accepting our " |
| 297 | - "licensing terms cannot do POST submissions.") |
| 298 | - translations_person = ITranslationsPerson(self.user) |
| 299 | - if (translations_person.translations_relicensing_agreement |
| 300 | - is not None and |
| 301 | - not translations_person.translations_relicensing_agreement): |
| 302 | - raise UnexpectedFormData( |
| 303 | - "Users who do not agree to licensing terms " |
| 304 | - "cannot do POST submissions.") |
| 305 | - try: |
| 306 | - # Try to get the timestamp when the submitted form was |
| 307 | - # created. We use it to detect whether someone else updated |
| 308 | - # the translation we are working on in the elapsed time |
| 309 | - # between the form loading and its later submission. |
| 310 | - self.lock_timestamp = zope_datetime.parseDatetimetz( |
| 311 | - self.request.form.get('lock_timestamp', u'')) |
| 312 | - except zope_datetime.DateTimeError: |
| 313 | - # invalid format. Either we don't have the timestamp in the |
| 314 | - # submitted form or it has the wrong format. |
| 315 | - raise UnexpectedFormData( |
| 316 | - "We didn't find the timestamp that tells us when was" |
| 317 | - " generated the submitted form.") |
| 318 | - |
| 319 | - # Check if this is really the form we are listening for.. |
| 320 | - if self.request.form.get("submit_translations"): |
| 321 | - # Check if this is really the form we are listening for.. |
| 322 | - if self._submitTranslations(): |
| 323 | - # .. and if no errors occurred, adios. Otherwise, we |
| 324 | - # need to set up the subviews for error display and |
| 325 | - # correction. |
| 326 | - return |
| 327 | - else: |
| 328 | - # It's not a POST, so we should generate lock_timestamp. |
| 329 | - UTC = pytz.timezone('UTC') |
| 330 | - self.lock_timestamp = datetime.datetime.now(UTC) |
| 331 | + if method == 'POST' and 'submit_translations' in self.request.form: |
| 332 | + if self._submitTranslations(): |
| 333 | + # If no errors occurred, adios. Otherwise, we need to set up |
| 334 | + # the subviews for error display and correction. |
| 335 | + return |
| 336 | |
| 337 | # Slave view initialization depends on _submitTranslations being |
| 338 | # called, because the form data needs to be passed in to it -- |
| 339 | # again, because of error handling. |
| 340 | self._initializeTranslationMessageViews() |
| 341 | |
| 342 | + def _extractLockTimestamp(self): |
| 343 | + """Extract the lock timestamp from the request. |
| 344 | + |
| 345 | + The lock_timestamp is used to detect conflicting concurrent |
| 346 | + translation updates: if the translation that is being changed |
| 347 | + has been set after the current form was generated, the user |
| 348 | + chose a translation based on outdated information. In that |
| 349 | + case there is a conflict. |
| 350 | + """ |
| 351 | + try: |
| 352 | + return zope_datetime.parseDatetimetz( |
| 353 | + self.request.form.get('lock_timestamp', u'')) |
| 354 | + except zope_datetime.DateTimeError: |
| 355 | + # invalid format. Either we don't have the timestamp in the |
| 356 | + # submitted form or it has the wrong format. |
| 357 | + return None |
| 358 | + |
| 359 | + def _checkSubmitConditions(self): |
| 360 | + """Verify that this submission is possible and valid. |
| 361 | + |
| 362 | + :raises: `UnexpectedFormData` if conditions are not met. In |
| 363 | + principle the user should not have been given the option to |
| 364 | + submit the current request. |
| 365 | + """ |
| 366 | + if is_read_only(): |
| 367 | + raise UnexpectedFormData( |
| 368 | + "Launchpad is currently in read-only mode for maintenance. " |
| 369 | + "Please try again later.") |
| 370 | + |
| 371 | + if self.user is None: |
| 372 | + raise UnexpectedFormData("You are not logged in.") |
| 373 | + |
| 374 | + # Users who have declined the licensing agreement can't post |
| 375 | + # translations. We don't stop users who haven't made a decision |
| 376 | + # yet at this point; they may be project owners making |
| 377 | + # corrections. |
| 378 | + translations_person = ITranslationsPerson(self.user) |
| 379 | + relicensing = translations_person.translations_relicensing_agreement |
| 380 | + if relicensing is not None and not relicensing: |
| 381 | + raise UnexpectedFormData( |
| 382 | + "You can't post translations since you have not agreed to " |
| 383 | + "our translations licensing terms.") |
| 384 | + |
| 385 | + if self.lock_timestamp is None: |
| 386 | + raise UnexpectedFormData( |
| 387 | + "Your form submission did not contain the lock_timestamp " |
| 388 | + "that tells Launchpad when the submitted form was generated.") |
| 389 | + |
| 390 | # |
| 391 | # API Hooks |
| 392 | # |
| 393 | @@ -587,15 +614,6 @@ |
| 394 | self.second_lang_code = second_lang_code |
| 395 | |
| 396 | @property |
| 397 | - def has_plural_form_information(self): |
| 398 | - """Return whether we know the plural forms for this language.""" |
| 399 | - if self.pofile.potemplate.hasPluralMessage(): |
| 400 | - return self.pofile.language.pluralforms is not None |
| 401 | - # If there are no plural forms, we assume that we have the |
| 402 | - # plural form information for this language. |
| 403 | - return True |
| 404 | - |
| 405 | - @property |
| 406 | def user_is_official_translator(self): |
| 407 | """Determine whether the current user is an official translator.""" |
| 408 | return self.pofile.canEditTranslations(self.user) |
| 409 | @@ -658,7 +676,7 @@ |
| 410 | # Extract the translations from the form, and store them in |
| 411 | # self.form_posted_translations. We try plural forms in turn, |
| 412 | # starting at 0. |
| 413 | - for pluralform in xrange(self.MAX_PLURAL_FORMS): |
| 414 | + for pluralform in xrange(TranslationConstants.MAX_PLURAL_FORMS): |
| 415 | msgset_ID_LANGCODE_translation_PLURALFORM_new = '%s%d_new' % ( |
| 416 | msgset_ID_LANGCODE_translation_, pluralform) |
| 417 | if msgset_ID_LANGCODE_translation_PLURALFORM_new not in form: |
| 418 | @@ -737,7 +755,7 @@ |
| 419 | potmsgset].append(pluralform) |
| 420 | else: |
| 421 | raise AssertionError('More than %d plural forms were submitted!' |
| 422 | - % self.MAX_PLURAL_FORMS) |
| 423 | + % TranslationConstants.MAX_PLURAL_FORMS) |
| 424 | |
| 425 | def _observeTranslationUpdate(self, potmsgset): |
| 426 | """Observe that a translation was updated for the potmsgset. |
| 427 | @@ -1056,7 +1074,7 @@ |
| 428 | |
| 429 | diverged_and_have_shared = ( |
| 430 | self.context.potemplate is not None and |
| 431 | - self.shared_translationmessage is not None) |
| 432 | + self.shared_translationmessage is not None) |
| 433 | if diverged_and_have_shared: |
| 434 | pofile = self.shared_translationmessage.ensureBrowserPOFile() |
| 435 | if pofile is None: |
| 436 | @@ -1155,10 +1173,10 @@ |
| 437 | |
| 438 | def _setOnePOFile(self, messages): |
| 439 | """Return a list of messages that all have a browser_pofile set. |
| 440 | - |
| 441 | + |
| 442 | If a pofile cannot be found for a message, it is not included in |
| 443 | the resulting list. |
| 444 | - """ |
| 445 | + """ |
| 446 | result = [] |
| 447 | for message in messages: |
| 448 | if message.browser_pofile is None: |
| 449 | @@ -1170,7 +1188,7 @@ |
| 450 | message.setPOFile(pofile) |
| 451 | result.append(message) |
| 452 | return result |
| 453 | - |
| 454 | + |
| 455 | def _buildAllSuggestions(self): |
| 456 | """Builds all suggestions and puts them into suggestions_block. |
| 457 | |
| 458 | |
| 459 | === modified file 'lib/lp/translations/interfaces/pofile.py' |
| 460 | --- lib/lp/translations/interfaces/pofile.py 2010-09-06 10:40:54 +0000 |
| 461 | +++ lib/lp/translations/interfaces/pofile.py 2010-10-15 04:57:46 +0000 |
| 462 | @@ -168,6 +168,9 @@ |
| 463 | for. |
| 464 | """ |
| 465 | |
| 466 | + def hasPluralFormInformation(): |
| 467 | + """Do we know the plural-forms information for this `POFile`?""" |
| 468 | + |
| 469 | def getHeader(): |
| 470 | """Return an `ITranslationHeaderData` representing its header.""" |
| 471 | |
| 472 | |
| 473 | === modified file 'lib/lp/translations/model/pofile.py' |
| 474 | --- lib/lp/translations/model/pofile.py 2010-09-30 08:51:21 +0000 |
| 475 | +++ lib/lp/translations/model/pofile.py 2010-10-15 04:57:46 +0000 |
| 476 | @@ -177,6 +177,24 @@ |
| 477 | return False |
| 478 | |
| 479 | |
| 480 | +def is_admin(user): |
| 481 | + """Is `user` an admin or Translations admin?""" |
| 482 | + celebs = getUtility(ILaunchpadCelebrities) |
| 483 | + return user.inTeam(celebs.admin) or user.inTeam(celebs.rosetta_experts) |
| 484 | + |
| 485 | + |
| 486 | +def is_product_owner(user, potemplate): |
| 487 | + """Is `user` the owner of a `Product` that `potemplate` belongs to? |
| 488 | + |
| 489 | + A product's owners have edit rights on the product's translations. |
| 490 | + """ |
| 491 | + productseries = potemplate.productseries |
| 492 | + if productseries is None: |
| 493 | + return False |
| 494 | + |
| 495 | + return user.inTeam(productseries.product.owner) |
| 496 | + |
| 497 | + |
| 498 | def _can_edit_translations(pofile, person): |
| 499 | """Say if a person is able to edit existing translations. |
| 500 | |
| 501 | @@ -190,29 +208,25 @@ |
| 502 | translation team for the given `IPOFile`.translationpermission and the |
| 503 | language associated with this `IPOFile`. |
| 504 | """ |
| 505 | + if person is None: |
| 506 | + # Anonymous users can't edit anything. |
| 507 | + return False |
| 508 | + |
| 509 | if is_read_only(): |
| 510 | # Nothing can be edited in read-only mode. |
| 511 | return False |
| 512 | |
| 513 | - # If the person is None, then they cannot edit |
| 514 | - if person is None: |
| 515 | - return False |
| 516 | - |
| 517 | # XXX Carlos Perello Marin 2006-02-07 bug=4814: |
| 518 | # We should not check the permissions here but use the standard |
| 519 | # security system. |
| 520 | |
| 521 | # Rosetta experts and admins can always edit translations. |
| 522 | - admins = getUtility(ILaunchpadCelebrities).admin |
| 523 | - rosetta_experts = getUtility(ILaunchpadCelebrities).rosetta_experts |
| 524 | - if (person.inTeam(admins) or person.inTeam(rosetta_experts)): |
| 525 | + if is_admin(person): |
| 526 | return True |
| 527 | |
| 528 | # The owner of the product is also able to edit translations. |
| 529 | - if pofile.potemplate.productseries is not None: |
| 530 | - product = pofile.potemplate.productseries.product |
| 531 | - if person.inTeam(product.owner): |
| 532 | - return True |
| 533 | + if is_product_owner(person, pofile.potemplate): |
| 534 | + return True |
| 535 | |
| 536 | # If a person has decided not to license their translations under BSD |
| 537 | # license they can't edit translations. |
| 538 | @@ -284,6 +298,16 @@ |
| 539 | """See `IPOFile`.""" |
| 540 | return self.language.guessed_pluralforms |
| 541 | |
| 542 | + def hasPluralFormInformation(self): |
| 543 | + """See `IPOFile`.""" |
| 544 | + if self.language.pluralforms is None: |
| 545 | + # We have no plural information for this language. It |
| 546 | + # doesn't actually matter unless the template contains |
| 547 | + # messages with plural forms. |
| 548 | + return not self.potemplate.hasPluralMessage() |
| 549 | + else: |
| 550 | + return True |
| 551 | + |
| 552 | def canEditTranslations(self, person): |
| 553 | """See `IPOFile`.""" |
| 554 | return _can_edit_translations(self, person) |
| 555 | |
| 556 | === modified file 'lib/lp/translations/stories/standalone/xx-licensing.txt' |
| 557 | --- lib/lp/translations/stories/standalone/xx-licensing.txt 2009-09-18 15:42:19 +0000 |
| 558 | +++ lib/lp/translations/stories/standalone/xx-licensing.txt 2010-10-15 04:57:46 +0000 |
| 559 | @@ -53,17 +53,6 @@ |
| 560 | >>> for input in main_content.findAll('input'): |
| 561 | ... print 'Found input:\n%s' % input |
| 562 | |
| 563 | -Karl is also denied submitting a POST request. |
| 564 | - |
| 565 | - >>> print http(r""" |
| 566 | - ... POST /alsa-utils/trunk/+pots/alsa-utils/es/+translate HTTP/1.1 |
| 567 | - ... Host: translations.launchpad.dev |
| 568 | - ... """) |
| 569 | - HTTP/1.1 500 Internal Server Error |
| 570 | - ... |
| 571 | - ...UnexpectedFormData: Anonymous users or users who are not accepting |
| 572 | - our licensing terms cannot do POST submissions... |
| 573 | - |
| 574 | Karl changes his mind. He returns to the licensing page. |
| 575 | |
| 576 | >>> browser.open('http://translations.launchpad.dev/~karl/') |
| 577 | |
| 578 | === modified file 'lib/lp/translations/stories/standalone/xx-pofile-translate.txt' |
| 579 | --- lib/lp/translations/stories/standalone/xx-pofile-translate.txt 2010-06-12 05:44:36 +0000 |
| 580 | +++ lib/lp/translations/stories/standalone/xx-pofile-translate.txt 2010-10-15 04:57:46 +0000 |
| 581 | @@ -62,18 +62,6 @@ |
| 582 | data. That part is more convenience than security. The server does not |
| 583 | remember what form it provided to which requester. |
| 584 | |
| 585 | -Separately from that, however, the form also rejects anonymous POST |
| 586 | -submissions. |
| 587 | - |
| 588 | - >>> print http(r""" |
| 589 | - ... POST /ubuntu/hoary/+source/evolution/+pots/evolution-2.2/es/+translate HTTP/1.1 |
| 590 | - ... Host: translations.launchpad.dev |
| 591 | - ... """) |
| 592 | - HTTP/1.1 500 Internal Server Error |
| 593 | - ... |
| 594 | - ...UnexpectedFormData: Anonymous users or users who are not accepting |
| 595 | - our licensing terms cannot do POST submissions... |
| 596 | - |
| 597 | |
| 598 | Translation Admin Access |
| 599 | ------------------------ |
| 600 | |
| 601 | === modified file 'lib/lp/translations/stories/standalone/xx-translationmessage-translate.txt' |
| 602 | --- lib/lp/translations/stories/standalone/xx-translationmessage-translate.txt 2010-05-25 16:45:00 +0000 |
| 603 | +++ lib/lp/translations/stories/standalone/xx-translationmessage-translate.txt 2010-10-15 04:57:46 +0000 |
| 604 | @@ -44,18 +44,6 @@ |
| 605 | >> print nav.getLink("Translation details").url |
| 606 | https://translations.launchpad.dev/ubuntu/hoary/+source/evolution/+pots/evolution-2.2/es/+details |
| 607 | |
| 608 | -Also, any anonymous POST submission should fail: |
| 609 | - |
| 610 | - >>> print http(r""" |
| 611 | - ... POST /ubuntu/hoary/+source/evolution/+pots/evolution-2.2/es/5/+translate HTTP/1.1 |
| 612 | - ... Host: translations.launchpad.dev |
| 613 | - ... """) |
| 614 | - HTTP/1.1 500 Internal Server Error |
| 615 | - ... |
| 616 | - ...UnexpectedFormData: Anonymous users or users who are not accepting |
| 617 | - our licensing terms cannot do POST submissions... |
| 618 | - |
| 619 | - |
| 620 | Let's log in. |
| 621 | |
| 622 | >>> browser = setupBrowser(auth='Basic carlos@canonical.com:test') |
| 623 | |
| 624 | === modified file 'lib/lp/translations/tests/test_pofile.py' |
| 625 | --- lib/lp/translations/tests/test_pofile.py 2010-09-30 17:57:01 +0000 |
| 626 | +++ lib/lp/translations/tests/test_pofile.py 2010-10-15 04:57:46 +0000 |
| 627 | @@ -1888,6 +1888,31 @@ |
| 628 | self.pofile.markChanged(translator=translator) |
| 629 | self.assertEqual(translator, self.pofile.lasttranslator) |
| 630 | |
| 631 | + def test_hasPluralFormInformation_bluffs_if_irrelevant(self): |
| 632 | + # If the template has no messages that use plural forms, the |
| 633 | + # POFile has all the relevant plural-form information regardless |
| 634 | + # of whether we know the plural forms for the language. |
| 635 | + language = self.factory.makeLanguage() |
| 636 | + pofile, potmsgset = self.factory.makePOFileAndPOTMsgSet( |
| 637 | + language.code, with_plural=False) |
| 638 | + self.assertTrue(pofile.hasPluralFormInformation()) |
| 639 | + |
| 640 | + def test_hasPluralFormInformation_admits_defeat(self): |
| 641 | + # If there are messages with plurals, hasPluralFormInformation |
| 642 | + # needs the plural-form information for the language. |
| 643 | + language = self.factory.makeLanguage() |
| 644 | + pofile, potmsgset = self.factory.makePOFileAndPOTMsgSet( |
| 645 | + language.code, with_plural=True) |
| 646 | + self.assertFalse(pofile.hasPluralFormInformation()) |
| 647 | + |
| 648 | + def test_hasPluralFormInformation_uses_language_info(self): |
| 649 | + # hasPluralFormInformation returns True if plural forms |
| 650 | + # information is available for the language. |
| 651 | + language = self.factory.makeLanguage(pluralforms=5) |
| 652 | + pofile, potmsgset = self.factory.makePOFileAndPOTMsgSet( |
| 653 | + language.code, with_plural=True) |
| 654 | + self.assertTrue(pofile.hasPluralFormInformation()) |
| 655 | + |
| 656 | |
| 657 | class TestPOFileTranslationMessages(TestCaseWithFactory): |
| 658 | """Test PO file getTranslationMessages method.""" |
| 659 | @@ -1905,66 +1930,66 @@ |
| 660 | # A shared message is included in this POFile's messages. |
| 661 | message = self.factory.makeTranslationMessage( |
| 662 | potmsgset=self.potmsgset, pofile=self.pofile, force_shared=True) |
| 663 | - |
| 664 | + |
| 665 | self.assertEqual( |
| 666 | [message], list(self.pofile.getTranslationMessages())) |
| 667 | - |
| 668 | + |
| 669 | def test_getTranslationMessages_current_diverged(self): |
| 670 | # A diverged message is included in this POFile's messages. |
| 671 | message = self.factory.makeTranslationMessage( |
| 672 | potmsgset=self.potmsgset, pofile=self.pofile, force_diverged=True) |
| 673 | - |
| 674 | + |
| 675 | self.assertEqual( |
| 676 | [message], list(self.pofile.getTranslationMessages())) |
| 677 | - |
| 678 | + |
| 679 | def test_getTranslationMessages_suggestion(self): |
| 680 | # A suggestion is included in this POFile's messages. |
| 681 | message = self.factory.makeTranslationMessage( |
| 682 | potmsgset=self.potmsgset, pofile=self.pofile) |
| 683 | - |
| 684 | + |
| 685 | self.assertEqual( |
| 686 | [message], list(self.pofile.getTranslationMessages())) |
| 687 | - |
| 688 | + |
| 689 | def test_getTranslationMessages_obsolete(self): |
| 690 | # A message on an obsolete POTMsgSEt is included in this |
| 691 | # POFile's messages. |
| 692 | potmsgset = self.factory.makePOTMsgSet(self.potemplate, sequence=0) |
| 693 | message = self.factory.makeTranslationMessage( |
| 694 | potmsgset=potmsgset, pofile=self.pofile, force_shared=True) |
| 695 | - |
| 696 | + |
| 697 | self.assertEqual( |
| 698 | [message], list(self.pofile.getTranslationMessages())) |
| 699 | - |
| 700 | + |
| 701 | def test_getTranslationMessages_other_pofile(self): |
| 702 | # A message from another POFiles is not included. |
| 703 | other_pofile = self.factory.makePOFile('de') |
| 704 | self.factory.makeTranslationMessage( |
| 705 | potmsgset=self.potmsgset, pofile=other_pofile) |
| 706 | - |
| 707 | + |
| 708 | self.assertEqual([], list(self.pofile.getTranslationMessages())) |
| 709 | - |
| 710 | + |
| 711 | def test_getTranslationMessages_condition_matches(self): |
| 712 | # A message matching the given condition is included. |
| 713 | # Diverged messages are linked to a specific POTemplate. |
| 714 | message = self.factory.makeTranslationMessage( |
| 715 | potmsgset=self.potmsgset, pofile=self.pofile, force_diverged=True) |
| 716 | - |
| 717 | + |
| 718 | self.assertContentEqual( |
| 719 | [message], |
| 720 | self.pofile.getTranslationMessages( |
| 721 | "TranslationMessage.potemplate IS NOT NULL")) |
| 722 | - |
| 723 | + |
| 724 | def test_getTranslationMessages_condition_matches_not(self): |
| 725 | # A message not matching the given condition is excluded. |
| 726 | # Shared messages are not linked to a POTemplate. |
| 727 | self.factory.makeTranslationMessage( |
| 728 | potmsgset=self.potmsgset, pofile=self.pofile, force_shared=True) |
| 729 | - |
| 730 | + |
| 731 | self.assertContentEqual( |
| 732 | [], |
| 733 | self.pofile.getTranslationMessages( |
| 734 | "TranslationMessage.potemplate IS NOT NULL")) |
| 735 | - |
| 736 | + |
| 737 | def test_getTranslationMessages_condition_matches_in_other_pofile(self): |
| 738 | # A message matching given condition but located in another POFile |
| 739 | # is not included. |
| 740 | @@ -1972,12 +1997,12 @@ |
| 741 | self.factory.makeTranslationMessage( |
| 742 | potmsgset=self.potmsgset, pofile=other_pofile, |
| 743 | force_diverged=True) |
| 744 | - |
| 745 | + |
| 746 | self.assertContentEqual( |
| 747 | [], |
| 748 | self.pofile.getTranslationMessages( |
| 749 | "TranslationMessage.potemplate IS NOT NULL")) |
| 750 | - |
| 751 | + |
| 752 | def test_getTranslationMessages_diverged_elsewhere(self): |
| 753 | # Diverged messages from sharing POTemplates are not included. |
| 754 | # Create a sharing potemplate in another product series and share |
| 755 | @@ -1992,9 +2017,9 @@ |
| 756 | self.factory.makeTranslationMessage( |
| 757 | potmsgset=self.potmsgset, pofile=other_pofile, |
| 758 | force_diverged=True) |
| 759 | - |
| 760 | + |
| 761 | self.assertEqual([], list(self.pofile.getTranslationMessages())) |
| 762 | - |
| 763 | + |
| 764 | |
| 765 | class TestPOFileToTranslationFileDataAdapter(TestCaseWithFactory): |
| 766 | """Test POFile being adapted to IPOFileToTranslationFileData.""" |
| 767 | |
| 768 | === modified file 'lib/lp/translations/utilities/tests/test_file_importer.py' |
| 769 | --- lib/lp/translations/utilities/tests/test_file_importer.py 2010-10-06 11:05:13 +0000 |
| 770 | +++ lib/lp/translations/utilities/tests/test_file_importer.py 2010-10-15 04:57:46 +0000 |
| 771 | @@ -613,7 +613,7 @@ |
| 772 | if side == self.UPSTREAM: |
| 773 | potemplate = self.upstream_template |
| 774 | else: |
| 775 | - # Create a template in a source package. |
| 776 | + # Create a template in a source package. |
| 777 | ubuntu = getUtility(ILaunchpadCelebrities).ubuntu |
| 778 | distroseries = self.factory.makeDistroSeries(distribution=ubuntu) |
| 779 | ubuntu.translation_focus = distroseries |
| 780 | @@ -639,7 +639,7 @@ |
| 781 | uploader=uploader, content=self.POFILE) |
| 782 | entry.potemplate = potemplate |
| 783 | entry.pofile = pofile |
| 784 | - # The uploaded file is only created in the librarian by a commit. |
| 785 | + # The uploaded file is only created in the librarian by a commit. |
| 786 | transaction.commit() |
| 787 | return entry |
| 788 | |
| 789 | |
| 790 | === modified file 'lib/lp/translations/utilities/translation_import.py' |
| 791 | --- lib/lp/translations/utilities/translation_import.py 2010-10-01 14:37:53 +0000 |
| 792 | +++ lib/lp/translations/utilities/translation_import.py 2010-10-15 04:57:46 +0000 |
| 793 | @@ -493,7 +493,7 @@ |
| 794 | """Are these English strings instead of translations? |
| 795 | |
| 796 | If this template uses symbolic message ids, the English POFile |
| 797 | - will contain the English original texts that correspond to the |
| 798 | + will contain the English original texts that correspond to the |
| 799 | symbols.""" |
| 800 | return ( |
| 801 | self.importer.uses_source_string_msgids and |
Nice branch, r=me, with a few very minor comments.
[1]
+from __future__ import with_statement
+
I don't think we need to do this anymore.
[2]
+ if blank_timestamp:
+ view.lock_timestamp = None
+ else:
+ view.lock_timestamp = datetime.
The blank_timestamp argument is only used once. If you did the
following in that one place, could this argument be removed and the
code above simplified?
view = self._makeView()
view.
[3]
+All the tests will be submitted as comming from Kurem, an editor for the
s/comming/coming/
[4]
+ if method == 'POST' and self.request.
What should submit_translations look like? The style guide would say
that this needs to be explicit.
[5]
+ if is_read_only():
+ raise UnexpectedFormData(
+ "Launchpad is currently in read-only mode for maintenance. "
+ "Please try again later.")
Is this not taken care of earlier in the request by, say, the
publication machinery? I haven't done anything to make views cope with
read-only mode, so I'd like to know if I can continue to be oblivious
to this or if I need to do something about it.
[6]
+ if self.user is None:
+ raise UnexpectedFormD
Is this needed? Similar question to [5] really.