Launchpad itself

Code review comment for lp:~jtv/launchpad/translationimportqueueentry-info

translationimportqueueentry-info
Merge into devel

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2010-02-10:

Hi Henning, thank you for the surprise review! And a thorough job you did of it, too. Glad you like it, because this branch did contribute to a headache. :-)

> Actually, as you know, I have a branch ready that displays the
> error_output in a text field as part of the form, so this part might go
> out again, soon. No offence meant. ;-)

I considered that. But the error_output can be pages and pages of text in some cases. Viewing those in the queue UI isn't very convenient, and neither is scrolling through long and wide text in an input box. So I figured we might sometimes appreciate having it displayed more plainly. It's an extra, so I did put it at the bottom of the page.

> Yes, I agree with that, although a two-column layout wouldn't have been
> bad, either. The form would go on the left, the information on the
> right. But I am not sure if that is easy to do and would fit with our UI
> guidelines.

I'm not sure we can do that with generic forms. I did try it with the yui table layout, but had no luck.

In any case, this is nice and compact, no?

> As I can see, you managed to pull this off without adding any view code.
> Impressive. ;-) I am going to ask you, though, to move some things to
> the view code to keep the TAL clearer. Hope you can go along with that.

Quite right you are. TAL can be nice for prototyping though.

> > === added file 'lib/lp/translations/stories/importqueue/xx-entry-
> details.txt'
> > --- lib/lp/translations/stories/importqueue/xx-entry-details.txt
> 1970-01-01 00:00:00 +0000
> > +++ lib/lp/translations/stories/importqueue/xx-entry-details.txt
> 2010-02-09 23:36:23 +0000
> > @@ -0,0 +1,121 @@
> > += Entry details =
> > +
> > +The translation import queue entry page shows various details about an
> > +entry and its target that may be helpful in queue review.
> > +
> > + >>> from zope.security.proxy import removeSecurityProxy
> > + >>> from lp.translations.model.translationimportqueue import (
> > + ... TranslationImportQueue)
> > +
> > + >>> filename = 'po/foo.pot'
> > +
> > + >>> login(ANONYMOUS)
> > + >>> queue = TranslationImportQueue()
> > + >>> product = factory.makeProduct()
> > + >>> removeSecurityProxy(product).official_rosetta = True
>
> I wonder if removeSecurityProxy would be necessary here if you logged in
> as foo.bar?

Maybe not, but it's yet another dependency on the test data with implicit meaning ("logging in as this user because it's an admin"). Stripping the proxy was easy enough.

> > +In that case, the product is also shown to have translatable series.
> > +
> > + >>> print details_text
> > + Upload attached to
> > + ...
> > + Project has translatable series.
> > + ...
> > +
>
> Would it be too much to list these series here?

Almost, but I did it anyway. :-) It now lists up to three series (with linkified names), and if there's more, it adds an ellipsis.

> > +The string "1 template" neatly adjusts to the actual number of
> > +templates.
> > +
> > + >>> login(ANONYMOUS)
> > + >>> template = factory.makePOTemplate(productseries=trunk)
> > + >>> logout()
> > +
> > + >>> admin_browser.open(entry_url)
> > + >>> details = find_tag_by_id(admin_browser.contents, 'portlet-details')
> > + >>> print extract_text(details.renderContents())
>
> Why is the renderContents necessary here? Just for my own understanding.

It isn't. Force of habit. Removed throughout.

(And by the way, this test stanza is now gone; a unit test on the view takes its place).

> > === added file 'lib/lp/translations/stories/importqueue/xx-entry-error-
> output.txt'
> > --- lib/lp/translations/stories/importqueue/xx-entry-error-output.txt
> 1970-01-01 00:00:00 +0000
> > +++ lib/lp/translations/stories/importqueue/xx-entry-error-output.txt
> 2010-02-09 23:36:23 +0000

> > + >>> entry.error_output = "Things went horribly wrong."
>
> I am surprised you can do that here. I couldn't do it in view code IIRC
> and had to use setErrorOutput. What's different?

I honestly don't know. I tried it and it worked.

> > === modified file 'lib/lp/translations/templates
> /translationimportqueueentry-index.pt'
> > --- lib/lp/translations/templates/translationimportqueueentry-index.pt
> 2009-12-03 18:33:22 +0000
> > +++ lib/lp/translations/templates/translationimportqueueentry-index.pt
> 2010-02-09 23:36:23 +0000
> > @@ -99,6 +99,11 @@
> > </script>
> > </metal:script>
> >
> > + <div metal:fill-slot="extra_info">
> > + <input type="hidden" name="next_url"
> > + tal:attributes="value view/referrer_url" />
> > + </div>
> > +
>
> Didn't we have that already? I know that the form always takes me back
> to the same queue I can from. I think I know at least ... ;-)

Indeed. I accidentally duplicated this bit. Fixed now.

> > --- lib/lp/translations/templates/translationimportqueueentry-portlet-
> details.pt 2009-07-17 17:59:07 +0000
> > +++ lib/lp/translations/templates/translationimportqueueentry-portlet-
> details.pt 2010-02-09 23:36:23 +0000
> > @@ -4,41 +4,68 @@
> > xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> > omit-tag="">
> >
> > -<div class="portlet" id="portlet-details">
> > - <h2>Import entry details</h2>
> > +<div class="columns portlet" id="portlet-details">
> > + <div class="two column left">
> > + Upload attached to
> > + <tal:package condition="context/sourcepackage">
> > + <a tal:replace="structure context/sourcepackage/fmt:link">
> > + Evolution in Ubuntu Hoary</a>.
> > + </tal:package>
> > + <tal:productseries condition="context/productseries">
> > + <a tal:replace="structure context/productseries/fmt:link">
> > + Evolution trunk series</a>.
> > + <ul class="bulleted" tal:define="product
> context/productseries/product">
>
> I am not the UI reviewer but this list looks kind of funny. the first
> entry is closer to the line above it than the second entry. What's wrong
> here?

Maybe the <ul> CSS assumes a paragraph boundary. We can play with that during UI review; thanks for pointing it out.

> > + <tal:many condition="python: series_templates > 1">
> > + <tal:count replace="series_templates">2</tal:count>
> templates.
> > + </tal:many>
> > + </a>
> > + </li>
>
> Ouch, too many tal:conditions for my taste. And using Python expressions
> in TAL is not very well looked upon, either. Please relieve my pain and
> consider making this into a view property that returns the full <a> tag
> (or just "no templates").

Sure. Once it's working in TAL, it's easier to move over to browser code. One reason to do it this way is the faster feedback during experimentation: just save the template and reload the page.

> > + <div class="two column right">
> > + File
> > + <a tal:attributes="href context/content/http_url"
> > + tal:content="context/path">po/messages.pot</a>
> > + uploaded by
> > + <a tal:replace="structure context/importer/fmt:link">
> > + Arne Goetje
> > + </a>
> > + <tal:upload_date replace="context/dateimported/fmt:displaydate">
> > + 2010-02-10
> > + </tal:upload_date>.
>
> <br> missing. Or some divs.

I'm not so sure... I deliberately went for a running-text style here. It's easy enough to pick out the elements on an incidental basis. The left-hand list is a bullet list because it ticks off items on the review checklist. But it's a style that I don't like to overdo.

> > + <tal:status_change
> > + define="upload_date context/dateimported; change_date
> context/date_status_changed"
> > + condition="python: change_date != upload_date">
> > + Entry last changed
> > + <tal:status_date replace="change_date/fmt:displaydate">
> > + 2010-02-12</tal:status_date>.
> > + </tal:status_change>
>
> Oh yes, another great thing to do in view code... ;-) At least the
> python condition, please?

Done. The change string is now generated in the view. If there are no status changes to report (apart from upload), the string is simply empty.

Jeroen

Hi Henning, thank you for the surprise review!  And a thorough job you did of it, too.  Glad you like it, because this branch did contribute to a headache.  :-)

> Actually, as you know, I have a branch ready that displays the
> error_output in a text field as part of the form, so this part might go
> out again, soon. No offence meant. ;-)

I considered that.  But the error_output can be pages and pages of text in some cases.  Viewing those in the queue UI isn't very convenient, and neither is scrolling through long and wide text in an input box.  So I figured we might sometimes appreciate having it displayed more plainly.  It's an extra, so I did put it at the bottom of the page.

I'm not sure we can do that with generic forms.  I did try it with the yui table layout, but had no luck.

In any case, this is nice and compact, no?

Quite right you are.  TAL can be nice for prototyping though.

> > === added file 'lib/lp/translations/stories/importqueue/xx-entry-
> details.txt'
> > --- lib/lp/translations/stories/importqueue/xx-entry-details.txt
> 1970-01-01 00:00:00 +0000
> > +++ lib/lp/translations/stories/importqueue/xx-entry-details.txt
> 2010-02-09 23:36:23 +0000
> > @@ -0,0 +1,121 @@
> > += Entry details =
> > +
> > +The translation import queue entry page shows various details about an
> > +entry and its target that may be helpful in queue review.
> > +
> > +    >>> from zope.security.proxy import removeSecurityProxy
> > +    >>> from lp.translations.model.translationimportqueue import (
> > +    ...     TranslationImportQueue)
> > +
> > +    >>> filename = 'po/foo.pot'
> > +
> > +    >>> login(ANONYMOUS)
> > +    >>> queue = TranslationImportQueue()
> > +    >>> product = factory.makeProduct()
> > +    >>> removeSecurityProxy(product).official_rosetta = True
> 
> I wonder if removeSecurityProxy would be necessary here if you logged in
> as foo.bar?

Maybe not, but it's yet another dependency on the test data with implicit meaning ("logging in as this user because it's an admin").  Stripping the proxy was easy enough.

> > +In that case, the product is also shown to have translatable series.
> > +
> > +    >>> print details_text
> > +    Upload attached to
> > +    ...
> > +    Project has translatable series.
> > +    ...
> > +
> 
> Would it be too much to list these series here?

Almost, but I did it anyway.  :-)  It now lists up to three series (with linkified names), and if there's more, it adds an ellipsis.

> > +The string "1 template" neatly adjusts to the actual number of
> > +templates.
> > +
> > +    >>> login(ANONYMOUS)
> > +    >>> template = factory.makePOTemplate(productseries=trunk)
> > +    >>> logout()
> > +
> > +    >>> admin_browser.open(entry_url)
> > +    >>> details = find_tag_by_id(admin_browser.contents, 'portlet-details')
> > +    >>> print extract_text(details.renderContents())
> 
> Why is the renderContents necessary here? Just for my own understanding.

It isn't.  Force of habit.  Removed throughout.

(And by the way, this test stanza is now gone; a unit test on the view takes its place).

> > +    >>> entry.error_output = "Things went horribly wrong."
> 
> I am surprised you can do that here. I couldn't do it in view code IIRC
> and had to use setErrorOutput. What's different?

I honestly don't know.  I tried it and it worked.

> > === modified file 'lib/lp/translations/templates
> /translationimportqueueentry-index.pt'
> > --- lib/lp/translations/templates/translationimportqueueentry-index.pt
> 2009-12-03 18:33:22 +0000
> > +++ lib/lp/translations/templates/translationimportqueueentry-index.pt
> 2010-02-09 23:36:23 +0000
> > @@ -99,6 +99,11 @@
> >      </script>
> >    </metal:script>
> >
> > +  <div metal:fill-slot="extra_info">
> > +    <input type="hidden" name="next_url"
> > +           tal:attributes="value view/referrer_url" />
> > +  </div>
> > +
> 
> Didn't we have that already? I know that the form always takes me back
> to the same queue I can from. I think I know at least ... ;-)

Indeed.  I accidentally duplicated this bit.  Fixed now.

> > --- lib/lp/translations/templates/translationimportqueueentry-portlet-
> details.pt      2009-07-17 17:59:07 +0000
> > +++ lib/lp/translations/templates/translationimportqueueentry-portlet-
> details.pt      2010-02-09 23:36:23 +0000
> > @@ -4,41 +4,68 @@
> >    xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> >    omit-tag="">
> >
> > -<div class="portlet" id="portlet-details">
> > -  <h2>Import entry details</h2>
> > +<div class="columns portlet" id="portlet-details">
> > +  <div class="two column left">
> > +    Upload attached to
> > +    <tal:package condition="context/sourcepackage">
> > +      <a tal:replace="structure context/sourcepackage/fmt:link">
> > +        Evolution in Ubuntu Hoary</a>.
> > +    </tal:package>
> > +    <tal:productseries condition="context/productseries">
> > +      <a tal:replace="structure context/productseries/fmt:link">
> > +        Evolution trunk series</a>.
> > +      <ul class="bulleted" tal:define="product
> context/productseries/product">
> 
> I am not the UI reviewer but this list looks kind of funny. the first
> entry is closer to the line above it than the second entry. What's wrong
> here?

Maybe the <ul> CSS assumes a paragraph boundary.  We can play with that during UI review; thanks for pointing it out.

> > +            <tal:many condition="python: series_templates > 1">
> > +              <tal:count replace="series_templates">2</tal:count>
> templates.
> > +            </tal:many>
> > +          </a>
> > +        </li>
> 
> Ouch, too many tal:conditions for my taste. And using Python expressions
> in TAL is not very well looked upon, either. Please relieve my pain and
> consider making this into a view property that returns the full <a> tag
> (or just "no templates").

Sure.  Once it's working in TAL, it's easier to move over to browser code.  One reason to do it this way is the faster feedback during experimentation: just save the template and reload the page.

> > +  <div class="two column right">
> > +    File
> > +    <a tal:attributes="href context/content/http_url"
> > +       tal:content="context/path">po/messages.pot</a>
> > +    uploaded by
> > +    <a tal:replace="structure context/importer/fmt:link">
> > +      Arne Goetje
> > +    </a>
> > +    <tal:upload_date replace="context/dateimported/fmt:displaydate">
> > +      2010-02-10
> > +    </tal:upload_date>.
> 
> <br> missing. Or some divs.

I'm not so sure...  I deliberately went for a running-text style here.  It's easy enough to pick out the elements on an incidental basis.  The left-hand list is a bullet list because it ticks off items on the review checklist.  But it's a style that I don't like to overdo.

> > +    <tal:status_change
> > +      define="upload_date context/dateimported; change_date
> context/date_status_changed"
> > +      condition="python: change_date != upload_date">
> > +      Entry last changed
> > +      <tal:status_date replace="change_date/fmt:displaydate">
> > +        2010-02-12</tal:status_date>.
> > +    </tal:status_change>
> 
> Oh yes, another great thing to do in view code... ;-) At least the
> python condition, please?

Done.  The change string is now generated in the view.  If there are no status changes to report (apart from upload), the string is simply empty.

Jeroen

« Back to merge proposal