Merge lp:~jtv/launchpad/bug-662552-view-permissions into lp:launchpad

Proposed by Jeroen T. Vermeulen on 2010-10-27
Status: Merged
Approved by: Jeroen T. Vermeulen on 2010-10-28
Approved revision: no longer in the source branch.
Merged at revision: 11818
Proposed branch: lp:~jtv/launchpad/bug-662552-view-permissions
Merge into: lp:launchpad
Diff against target: 112 lines (+19/-19)
3 files modified
lib/lp/translations/browser/pofile.py (+4/-9)
lib/lp/translations/browser/tests/test_translationmessage_view.py (+1/-2)
lib/lp/translations/browser/translationmessage.py (+14/-8)
To merge this branch: bzr merge lp:~jtv/launchpad/bug-662552-view-permissions
Reviewer Review Type Date Requested Status
Tim Penhey (community) code Approve on 2010-10-28
Launchpad code reviewers code 2010-10-27 Pending
Review via email: mp+39481@code.launchpad.net

Commit message

Re-use POFile edit permission info across message sub-views.

Description of the change

= Bug 662552: View and Permissions =

I'm cutting some dead wood out of the POFile:+translate page, which has been timing out.

One thing that can get a bit costly (especially in terms of the number of database queries issued) is checking whether the user has permission to edit the translation that's being browsed. That check is done for each of the messages displayed, normally 10 but up to 50 per page.

This branch hoists the check out of the view constructors (the batched on for the POFile as well as the single-message view) and passes the result in as an argument. That way, the POFile view can do the check once and re-use the outcome for each of the message views it creates. The code for the check is quite complicated and can involve many queries, depending on the user.

While I was at it, I also passed the POFile itself as an argument. A current problem is that TranslationMessage officially doesn't have a reference to a POFile any more, but the view code does still need that reference. We work around that in a few places with a memory-only attribute called browser_pofile. By passing the POFile as an argument, I get to eliminate one use of that malodorous attribute.

There's some legacy lint, most of which I left untouched because we're cleaning it up in other branches. None that I introduced though.

Jeroen

To post a comment you must log in.
Tim Penhey (thumper) wrote :

I don't see anything wrong with this, but I'll have to take your word that it does what it says it does.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/translations/browser/pofile.py'
--- lib/lp/translations/browser/pofile.py 2010-10-27 18:30:31 +0000
+++ lib/lp/translations/browser/pofile.py 2010-10-28 09:58:49 +0000
@@ -824,20 +824,15 @@
824824
825 def _buildTranslationMessageViews(self, for_potmsgsets):825 def _buildTranslationMessageViews(self, for_potmsgsets):
826 """Build translation message views for all potmsgsets given."""826 """Build translation message views for all potmsgsets given."""
827 last = None
828 for potmsgset in for_potmsgsets:827 for potmsgset in for_potmsgsets:
829 assert (last is None or
830 potmsgset.getSequence(
831 self.context.potemplate) >= last.getSequence(
832 self.context.potemplate)), (
833 "POTMsgSets on page not in ascending sequence order")
834 last = potmsgset
835
836 translationmessage = (828 translationmessage = (
837 potmsgset.getCurrentTranslationMessageOrDummy(self.context))829 potmsgset.getCurrentTranslationMessageOrDummy(self.context))
830 error = self.errors.get(potmsgset)
831 can_edit = self.context.canEditTranslations(self.user)
832
838 view = self._prepareView(833 view = self._prepareView(
839 CurrentTranslationMessageView, translationmessage,834 CurrentTranslationMessageView, translationmessage,
840 self.errors.get(potmsgset))835 pofile=self.context, can_edit=can_edit, error=error)
841 view.zoomed_in_view = False836 view.zoomed_in_view = False
842 self.translationmessage_views.append(view)837 self.translationmessage_views.append(view)
843838
844839
=== modified file 'lib/lp/translations/browser/tests/test_translationmessage_view.py'
--- lib/lp/translations/browser/tests/test_translationmessage_view.py 2010-10-04 19:50:45 +0000
+++ lib/lp/translations/browser/tests/test_translationmessage_view.py 2010-10-28 09:58:49 +0000
@@ -44,8 +44,7 @@
44 self.view = CurrentTranslationMessageView(44 self.view = CurrentTranslationMessageView(
45 message, LaunchpadTestRequest(),45 message, LaunchpadTestRequest(),
46 {}, dict(enumerate(message.translations)),46 {}, dict(enumerate(message.translations)),
47 False, False, None, None, True)47 False, False, None, None, True, pofile=self.pofile, can_edit=True)
48 self.view.user_is_official_translator = True
49 self.view.initialize()48 self.view.initialize()
5049
51 def _makeTranslation(self, translation=None,50 def _makeTranslation(self, translation=None,
5251
=== modified file 'lib/lp/translations/browser/translationmessage.py'
--- lib/lp/translations/browser/translationmessage.py 2010-10-27 18:30:31 +0000
+++ lib/lp/translations/browser/translationmessage.py 2010-10-28 09:58:49 +0000
@@ -453,8 +453,9 @@
453 return False453 return False
454 return True454 return True
455455
456 def _prepareView(self, view_class, current_translation_message, error):456 def _prepareView(self, view_class, current_translation_message,
457 """Collect data and build a TranslationMessageView for display."""457 pofile, can_edit, error=None):
458 """Collect data and build a `TranslationMessageView` for display."""
458 # XXX: kiko 2006-09-27:459 # XXX: kiko 2006-09-27:
459 # It would be nice if we could easily check if this is being460 # It would be nice if we could easily check if this is being
460 # called in the right order, after _storeTranslations().461 # called in the right order, after _storeTranslations().
@@ -499,7 +500,7 @@
499 current_translation_message, self.request,500 current_translation_message, self.request,
500 plural_indices_to_store, translations, force_suggestion,501 plural_indices_to_store, translations, force_suggestion,
501 force_diverge, error, self.second_lang_code,502 force_diverge, error, self.second_lang_code,
502 self.form_is_writeable)503 self.form_is_writeable, pofile=pofile, can_edit=can_edit)
503504
504 #505 #
505 # Internals506 # Internals
@@ -824,8 +825,11 @@
824825
825 def _initializeTranslationMessageViews(self):826 def _initializeTranslationMessageViews(self):
826 """See `BaseTranslationView._initializeTranslationMessageViews`."""827 """See `BaseTranslationView._initializeTranslationMessageViews`."""
828 pofile = self.pofile
829 can_edit = pofile.canEditTranslations(self.user)
827 self.translationmessage_view = self._prepareView(830 self.translationmessage_view = self._prepareView(
828 CurrentTranslationMessageZoomedView, self.context, self.error)831 CurrentTranslationMessageZoomedView, self.context, pofile=pofile,
832 can_edit=can_edit, error=self.error)
829833
830 def _submitTranslations(self):834 def _submitTranslations(self):
831 """See `BaseTranslationView._submitTranslations`."""835 """See `BaseTranslationView._submitTranslations`."""
@@ -878,7 +882,8 @@
878882
879 def __init__(self, current_translation_message, request,883 def __init__(self, current_translation_message, request,
880 plural_indices_to_store, translations, force_suggestion,884 plural_indices_to_store, translations, force_suggestion,
881 force_diverge, error, second_lang_code, form_is_writeable):885 force_diverge, error, second_lang_code, form_is_writeable,
886 pofile, can_edit):
882 """Primes the view with information that is gathered by a parent view.887 """Primes the view with information that is gathered by a parent view.
883888
884 :param plural_indices_to_store: A dictionary that indicates whether889 :param plural_indices_to_store: A dictionary that indicates whether
@@ -894,17 +899,18 @@
894 field.alternative_value.899 field.alternative_value.
895 :param form_is_writeable: Whether the form should accept write900 :param form_is_writeable: Whether the form should accept write
896 operations901 operations
902 :param pofile: The `POFile` that's being displayed or edited.
903 :param can_edit: Whether the user has editing privileges on `pofile`.
897 """904 """
898 LaunchpadView.__init__(self, current_translation_message, request)905 LaunchpadView.__init__(self, current_translation_message, request)
899906
900 self.pofile = self.context.browser_pofile907 self.pofile = pofile
901 self.plural_indices_to_store = plural_indices_to_store908 self.plural_indices_to_store = plural_indices_to_store
902 self.translations = translations909 self.translations = translations
903 self.error = error910 self.error = error
904 self.force_suggestion = force_suggestion911 self.force_suggestion = force_suggestion
905 self.force_diverge = force_diverge912 self.force_diverge = force_diverge
906 self.user_is_official_translator = (913 self.user_is_official_translator = can_edit
907 self.pofile.canEditTranslations(self.user))
908 self.form_is_writeable = form_is_writeable914 self.form_is_writeable = form_is_writeable
909 if self.context.is_imported:915 if self.context.is_imported:
910 # The imported translation matches the current one.916 # The imported translation matches the current one.