Launchpad itself

Merge lp://staging/~jtv/launchpad/bug-418430-stuff-to-review-3.0 into lp://staging/launchpad

  1. bug-418430-stuff-to-review-3.0
  2. Merge into devel
Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp://staging/~jtv/launchpad/bug-418430-stuff-to-review-3.0
Merge into: lp://staging/launchpad
Diff against target: None lines
To merge this branch: bzr merge lp://staging/~jtv/launchpad/bug-418430-stuff-to-review-3.0
Reviewer Review Type Date Requested Status
Deryck Hodge (community) code Approve
Review via email: mp+10642@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

= Bug 418430 =

For members of translation groups, the 3.0 personal page shows a list of
translations that await their review. If relevant, it also links to the
full list on a page called +translations-to-review.

When I started creating that page, we did not have the 3.0 macros yet so
I created it in 2.0 style. This branch fixes that. It's an easy change
since there's so little on the page.

Also, I'm adding +translations-to-review to the navigation menu for a
person in Translations. On the +translations-to-review page this shows
up as a list of related pages, since there's nothing else of interest to
show in a sidebar.

One bump in the road was getting the related-pages: I need to get
context/@@+related-pages rather than view/@@+related-pages. I don't
know why that is, but things fail horribly otherwise. I've seen sinzui
add constructors that distinguish between cases where "context" is the
view and cases where it's the underlying model object, but since in this
case there are other pages using the same navigation menu, it seemed
irresponsible to make the same change here without understanding all the
ramifications.

For testing, run the existing Translations pagetests. The only one
affected by the change was the licensing test, which unnecessarily
matches the entire navigation menu.

For Q/A:
 * Visit the home page for someone who is not in any translation groups.
   The navigation menu has no link for translations to review.
 * Visit the home page for someone who is in a translation group. Now
   the link will show up.
 * Follow that link and see the new page. It shows any translations
   awaiting review by this person, and the equivalent of the navigation
   menu at the bottom of the page.
 * Cycle through all the pages in the navigation menu, to make sure that
   I haven't broken any.

Naturally I've done this on the testing branch as well.

No lint.

Jeroen

Revision history for this message
Deryck Hodge (deryck) wrote :

Hi, Jeroen.

This looks nice!

One note about the UI, when I looked at the page itself, the two
headings -- the person name heading in an h2 and the page title in an
h1 -- were in the same purple color used for translations currently.
As I understand it, these should be light gray and green respectively.
I assume there's some current 2.0 LP style rule making all headings
this color for translations. The same is true of the bugs app, I
know. So I just call your attention to it, if you're not already
aware. Nothing to fix now, but at some point it has to be changed.
Maybe you guys have already talked about this, but just in case not, I
mention it.

I see only one minor fix to recommend:

> + <div class="top-portlet">
> + <h1>Translations to review</h1>
> + <tal:work-to-do condition="view/num_projects_and_packages_to_review">
> + <p>
> + These translations are available to be reviewed by
> + <tal:person replace="structure context/fmt:link">R. Viewer</tal:person>:
> + </p>

The h1 of the page should match the page title. In the case of the
page I sampled, the page title was "Translations for review by
Launchpad German translators" so I would expect the main h1 to be that
as well.

Otherwise, this looks good. Marking this as approved, with the
assumption that this one change will be made.

Cheers,
deryck

review: Approve (code)
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

> The h1 of the page should match the page title. In the case of the
> page I sampled, the page title was "Translations for review by
> Launchpad German translators" so I would expect the main h1 to be that
> as well.
>
> Otherwise, this looks good. Marking this as approved, with the
> assumption that this one change will be made.

Consider it done, for verily it is.

Jeroen

Revision history for this message
Deryck Hodge (deryck) wrote :

> One note about the UI, when I looked at the page itself, the two
> headings -- the person name heading in an h2 and the page title in an
> h1 -- were in the same purple color used for translations currently.
> As I understand it, these should be light gray and green respectively.
> I assume there's some current 2.0 LP style rule making all headings
> this color for translations. The same is true of the bugs app, I
> know. So I just call your attention to it, if you're not already
> aware. Nothing to fix now, but at some point it has to be changed.
> Maybe you guys have already talked about this, but just in case not, I
> mention it.
>

So I made a bit of mistake here. I was thinking about the rule that the h2 should be gray, and blanked out for a minute that green is the registry color. So the h2 should be gray, and currently the h1 the app-specific color. I'm just updating the MP to admit my mistake if someone comes along later.

I did talk to beuno about this. And he does have some desire to have a single h1 color across apps. But that is a separate issue.

I filed bug 418666 about needing to have the context.title headings be gray across all app-specific pages.

Cheers,
deryck

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/translations/browser/person.py'
--- lib/lp/translations/browser/person.py 2009-08-19 14:25:32 +0000
+++ lib/lp/translations/browser/person.py 2009-08-25 06:11:12 +0000
@@ -36,11 +36,18 @@
36from lp.translations.model.productserieslanguage import ProductSeriesLanguage36from lp.translations.model.productserieslanguage import ProductSeriesLanguage
3737
3838
39def person_is_reviewer(person):
40 """Is `person` a translations reviewer?"""
41 groups = ITranslationsPerson(person).translation_groups
42 return groups.any() is not None
43
44
39class PersonTranslationsMenu(NavigationMenu):45class PersonTranslationsMenu(NavigationMenu):
4046
41 usedfor = IPerson47 usedfor = IPerson
42 facet = 'translations'48 facet = 'translations'
43 links = ('overview', 'licensing', 'imports')49 links = ('overview', 'licensing', 'imports', 'translations_to_review')
50 title = "Related pages"
4451
45 def overview(self):52 def overview(self):
46 text = 'Overview'53 text = 'Overview'
@@ -55,6 +62,11 @@
55 enabled = (self.context == self.user)62 enabled = (self.context == self.user)
56 return Link('+licensing', text, enabled=enabled)63 return Link('+licensing', text, enabled=enabled)
5764
65 def translations_to_review(self):
66 text = 'Translations to review'
67 enabled = person_is_reviewer(self.context)
68 return Link('+translations-to-review', text, enabled=enabled)
69
5870
59class PersonTranslationView(LaunchpadView):71class PersonTranslationView(LaunchpadView):
60 """View for translation-related Person pages."""72 """View for translation-related Person pages."""
@@ -96,7 +108,7 @@
96 @property108 @property
97 def person_is_reviewer(self):109 def person_is_reviewer(self):
98 """Is this person in a translation group?"""110 """Is this person in a translation group?"""
99 return len(self.translation_groups) != 0111 return person_is_reviewer(self.context)
100112
101 def should_display_message(self, translationmessage):113 def should_display_message(self, translationmessage):
102 """Should a certain `TranslationMessage` be displayed.114 """Should a certain `TranslationMessage` be displayed.
103115
=== modified file 'lib/lp/translations/stories/standalone/xx-licensing.txt'
--- lib/lp/translations/stories/standalone/xx-licensing.txt 2009-07-01 20:45:39 +0000
+++ lib/lp/translations/stories/standalone/xx-licensing.txt 2009-08-25 06:39:38 +0000
@@ -16,7 +16,7 @@
16 >>> print browser.title16 >>> print browser.title
17 Translations licensing17 Translations licensing
18 >>> print extract_text(find_main_content(browser.contents))18 >>> print extract_text(find_main_content(browser.contents))
19 Overview / Translations licensing / Import queue19 Overview / Translations licensing / ...
20 Sorry to interrupt...20 Sorry to interrupt...
2121
22Karl is asked whether he wants to license his translations.22Karl is asked whether he wants to license his translations.
2323
=== modified file 'lib/lp/translations/templates/person-translations-to-review.pt'
--- lib/lp/translations/templates/person-translations-to-review.pt 2009-08-19 16:48:17 +0000
+++ lib/lp/translations/templates/person-translations-to-review.pt 2009-08-25 06:11:12 +0000
@@ -3,57 +3,58 @@
3 xmlns:tal="http://xml.zope.org/namespaces/tal"3 xmlns:tal="http://xml.zope.org/namespaces/tal"
4 xmlns:metal="http://xml.zope.org/namespaces/metal"4 xmlns:metal="http://xml.zope.org/namespaces/metal"
5 xmlns:i18n="http://xml.zope.org/namespaces/i18n"5 xmlns:i18n="http://xml.zope.org/namespaces/i18n"
6 xml:lang="en"6 metal:use-macro="view/macro:page/main_only"
7 lang="en"
8 dir="ltr"
9 metal:use-macro="view/macro:page/onecolumn"
10>7>
11<body>8<body>
12 <div metal:fill-slot="main">9 <div metal:fill-slot="main">
13 <h1>Translations to review</h1>10 <div class="top-portlet">
14 <tal:work-to-do condition="view/num_projects_and_packages_to_review">11 <h1>Translations to review</h1>
15 <p>12 <tal:work-to-do condition="view/num_projects_and_packages_to_review">
16 These translations are available to be reviewed by13 <p>
17 <tal:person replace="structure context/fmt:link">R. Viewer</tal:person>:14 These translations are available to be reviewed by
18 </p>15 <tal:person replace="structure context/fmt:link">R. Viewer</tal:person>:
16 </p>
1917
20 <table class="listing"18 <table class="listing"
21 id="translations-to-review-table"19 id="translations-to-review-table"
22 style="max-width:800px">20 style="max-width:800px">
23 <tr tal:repeat="target_info view/all_projects_and_packages_to_review">21 <tr tal:repeat="target_info view/all_projects_and_packages_to_review">
24 <td>22 <td>
25 <tal:product condition="target_info/is_product"23 <tal:product condition="target_info/is_product"
26 replace="structure target_info/target/fmt:link">24 replace="structure target_info/target/fmt:link">
27 alsa-utils25 alsa-utils
28 </tal:product>26 </tal:product>
29 <tal:package condition="not: target_info/is_product">27 <tal:package condition="not: target_info/is_product">
30 <a tal:attributes="href target_info/target/fmt:url">28 <a tal:attributes="href target_info/target/fmt:url">
31 <img src="/@@/distribution" />29 <img src="/@@/distribution" />
32 <tal:packagename replace="target_info/target/name">30 <tal:packagename replace="target_info/target/name">
33 alsa-utils31 alsa-utils
34 </tal:packagename>32 </tal:packagename>
33 </a>
34 </tal:package>
35 </td>
36 <td>
37 needs
38 <a tal:attributes="href target_info/link">
39 <tal:stringcount replace="target_info/count_wording">
40 1 string
41 </tal:stringcount>
42 reviewed
35 </a>43 </a>
36 </tal:package>44 </td>
37 </td>45 </tr>
38 <td>46 </table>
39 needs47
40 <a tal:attributes="href target_info/link">48 </tal:work-to-do>
41 <tal:stringcount replace="target_info/count_wording">49 <tal:no-work condition="not: view/num_projects_and_packages_to_review">
42 1 string50 <p>
43 </tal:stringcount>51 No translations waiting for review by
44 reviewed52 <tal:person replace="structure context/fmt:link">R. Viewer</tal:person>.
45 </a>53 </p>
46 </td>54 </tal:no-work>
47 </tr>55 </div>
48 </table>56
4957 <tal:menu replace="structure context/@@+related-pages" />
50 </tal:work-to-do>
51 <tal:no-work condition="not: view/num_projects_and_packages_to_review">
52 <p>
53 No translations waiting for review by
54 <tal:person replace="structure context/fmt:link">R. Viewer</tal:person>.
55 </p>
56 </tal:no-work>
57 </div>58 </div>
58</body>59</body>
59</html>60</html>