Code review comment for lp://staging/~jtv/launchpad/bug-418430-stuff-to-review-3.0

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

« Back to merge proposal