Merge lp:~gmb/launchpad/ajax-dupefinder-has-risen-from-the-grave into lp:launchpad

This branch does the final connecting up of JS to make the Asynchronous Dupefinder stuff work.

How to test this branch (UI):

 1. Go to launchpad.dev/firefox/+filebug
 2. Enter a simple title (say, 'a') and hit "next"
 3. You should get a list of possible duplicates for that bug.
 4. You should be able to interact with those dupes in the same way as you always do (subscribe, mark as affecting you, etc).
 5. You should also be able to file a new bug if you need to in the same way as you do currently.

Issues that I'm aware of at this stage:

 1. The animations seem to happen at the wrong time, so slide_ins get played after the element has already been rendered as closed. I'm looking into this.
 2. The way that the dupes appear in the page isn't exactly pretty. I'm open to suggestions about how this should work.

> This branch does the final connecting up of JS to make the Asynchronous
> Dupefinder stuff work.

This is great! I've got three points below that I think are worth addressing - see what you think.

>
> How to test this branch (UI):
>
> 1. Go to launchpad.dev/firefox/+filebug
> 2. Enter a simple title (say, 'a') and hit "next"
> 3. You should get a list of possible duplicates for that bug.
> 4. You should be able to interact with those dupes in the same way as you
> always do (subscribe, mark as affecting you, etc).
> 5. You should also be able to file a new bug if you need to in the same way
> as you do currently.
>
> Issues that I'm aware of at this stage:
>
> 1. The animations seem to happen at the wrong time, so slide_ins get played
> after the element has already been rendered as closed. I'm looking into this.

OK - at first I thought this was a feature :-) (as I try not to read the MP before trying out the feature).

> 2. The way that the dupes appear in the page isn't exactly pretty. I'm open
> to suggestions about how this should work.

It would be nice to see them slide in... once you have the results, add them to the dom, but hidden, and then slide out the div containing them all?

My first thought is that the phrase: 'Is "a" one of these bugs?' seems strange. Even with a proper summary, 'Is "A bunch of soyuz source pages are missing headings" one of these bugs?' seems strange. Do you know the history of that decision? My guess would be that, since the summary entered by the user *wasn't* displayed anywhere else once they had searched, it needed to be included on the page somewhere. But now with your changes here, that the summary and input box remain on the page (at least for the JS version), I think we could just say something like 'Do any of the following bugs describe the bug you are trying to report?'. What do you think? It's great that they can now still see their search and update it if desired!

Secondly, currently in your branch it seems that you cannot have the 'No, I need to report a new bug' section and one of the other potential duplicates open at the same time. For example, as a person I:

1. enter a summary and hit Next,
2. briefly read the titles, but nothing rings a bell, so I click 'No, I need to report a new bug",
3. I start adding the further information, and then realise, hey I want to check one of those bugs above, as it actually could be the same,
4. I click on the expander and zap, my new bug description disappears.

Of course, the info I entered is still there, and I'll see it again if I click again on "No, I need to report a new bug", but I don't see any reason for it disappearing in the first place? (with less justification, the same goes in reverse, closing the expanded dups when I click on 'No, I need to report a new bug.').

Third, just an implementation detail, but hitting Enter while the focus in the summary field doesn't filter - it re-loads the page, which I'm assuming is not what you want.

Thanks for resurrecting this work! It'll be a great improvement :)

On Thu, Dec 03, 2009 at 11:22:46AM -0000, Michael Nelson wrote:
> Review: Needs Information ui
> > This branch does the final connecting up of JS to make the
> > Asynchronous Dupefinder stuff work.
>
> This is great! I've got three points below that I think are worth
> addressing - see what you think.
>
> >
> > How to test this branch (UI):
> >
> > 1. Go to launchpad.dev/firefox/+filebug 2. Enter a simple title
> > (say, 'a') and hit "next" 3. You should get a list of possible
> > duplicates for that bug. 4. You should be able to interact with
> > those dupes in the same way as you always do (subscribe, mark as
> > affecting you, etc). 5. You should also be able to file a new bug
> > if you need to in the same way as you do currently.
> >
> > Issues that I'm aware of at this stage:
> >
> > 1. The animations seem to happen at the wrong time, so slide_ins
> > get played after the element has already been rendered as closed.
> > I'm looking into this.
>
> OK - at first I thought this was a feature :-) (as I try not to read
> the MP before trying out the feature).
>

I've tried to fix this by making the animation duration as short as
possible but that doesn't work terribly well. I think that I can fix it
using some event jiggery-pokery, and I'm still working on that (see next
comment).

>
> > 2. The way that the dupes appear in the page isn't exactly pretty.
> > I'm open to suggestions about how this should work.
>
> It would be nice to see them slide in... once you have the results,
> add them to the dom, but hidden, and then slide out the div containing
> them all?

I'm still working on getting this right. Please feel free to read the
rest of my response and I'll get back to you about this and the above
item.

> My first thought is that the phrase: 'Is "a" one of these bugs?' seems
> strange. Even with a proper summary, 'Is "A bunch of soyuz source
> pages are missing headings" one of these bugs?' seems strange. Do you
> know the history of that decision? My guess would be that, since the
> summary entered by the user *wasn't* displayed anywhere else once they
> had searched, it needed to be included on the page somewhere. But now
> with your changes here, that the summary and input box remain on the
> page (at least for the JS version), I think we could just say
> something like 'Do any of the following bugs describe the bug you are
> trying to report?'. What do you think? It's great that they can now
> still see their search and update it if desired!

Right, I agree. That text is a hold-over, so I've updated the view and
the templates to make it a bit more meaningful in an inline dupe list.
It'll remain the same should someone use the non-ajax version of the
page, though.

> Secondly, currently in your branch it seems that you cannot have the
> 'No, I need to report a new bug' section and one of the other
> potential duplicates open at the same time. For example, as a person
> I:
>
> 1. enter a summary and hit Next, 2. briefly read the titles, but
> nothing rings a bell, so I click 'No, I need to report a new bug", 3.
> I start adding the further information, and then realise, hey I want
> to check one of those bugs ab...

> On Thu, Dec 03, 2009 at 11:22:46AM -0000, Michael Nelson wrote:
> > Review: Needs Information ui
> > > This branch does the final connecting up of JS to make the
> > > Asynchronous Dupefinder stuff work.
> >
> > This is great! I've got three points below that I think are worth
> > addressing - see what you think.
> >
> > >
> > > How to test this branch (UI):
> > >
> > > 1. Go to launchpad.dev/firefox/+filebug 2. Enter a simple title
> > > (say, 'a') and hit "next" 3. You should get a list of possible
> > > duplicates for that bug. 4. You should be able to interact with
> > > those dupes in the same way as you always do (subscribe, mark as
> > > affecting you, etc). 5. You should also be able to file a new bug
> > > if you need to in the same way as you do currently.
> > >
> > > Issues that I'm aware of at this stage:
> > >
> > > 1. The animations seem to happen at the wrong time, so slide_ins
> > > get played after the element has already been rendered as closed.
> > > I'm looking into this.
> >
> > OK - at first I thought this was a feature :-) (as I try not to read
> > the MP before trying out the feature).
> >
>
> I've tried to fix this by making the animation duration as short as
> possible but that doesn't work terribly well. I think that I can fix it
> using some event jiggery-pokery, and I'm still working on that (see next
> comment).
>

I think this is great as it is currently.

My initial comment above was related to when you initially search, and the results appear expanded and then slide in quickly. I wasn't sure why there was any animation there at all (rather than just displaying the closed items) - or whether that was the actual issue you were referring to, and if so, why it is not simple to ensure there is no animation triggered there (as the closed state should just be a css class?)

But as I said, I actually thought it was a feature (bit of bling ;) ).

> >
> > > 2. The way that the dupes appear in the page isn't exactly pretty.
> > > I'm open to suggestions about how this should work.
> >
> > It would be nice to see them slide in... once you have the results,
> > add them to the dom, but hidden, and then slide out the div containing
> > them all?
>
> I'm still working on getting this right. Please feel free to read the
> rest of my response and I'll get back to you about this and the above
> item.

Great, but don't worry if there's no time, it's just a 'would be nice' ;).

>
> > My first thought is that the phrase: 'Is "a" one of these bugs?' seems
> > strange. Even with a proper summary, 'Is "A bunch of soyuz source
> > pages are missing headings" one of these bugs?' seems strange. Do you
> > know the history of that decision? My guess would be that, since the
> > summary entered by the user *wasn't* displayed anywhere else once they
> > had searched, it needed to be included on the page somewhere. But now
> > with your changes here, that the summary and input box remain on the
> > page (at least for the JS version), I think we could just say
> > something like 'Do any of the following bugs describe the bug you are
> > trying to report?'. What do you think? It's great that they can now
> > still see their...

Hi Graham,

Not a lot to say. I have a few suggestions, but that's all they
are. Looking good :)

Gavin.

> === modified file 'lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js'
> --- lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js 2009-12-08 10:57:44 +0000
> +++ lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js 2009-12-09 12:13:43 +0000
> @@ -81,13 +81,6 @@
> * @param e The Event triggering this function.
> */
> function show_bug_reporting_form(e) {
> - if (Y.Lang.isValue(bug_already_reported_expanders)) {
> - // Collapse any duplicate-details divs.
> - Y.each(bug_already_reported_expanders, function(expander) {
> - collapse_bug_details(expander);
> - });
> - }
> -
> // If the bug reporting form is in a hidden container, as it is on
> // the AJAX dupe search, show it.
> var filebug_form_container = Y.get('#filebug-form-container');

Salgado, in his YUI() -> LPS branch, changed a lot of Y.get() calls to
Y.one(). I guess that means things break non-silently when the targets
don't exist. Might be a good idea here too.

> @@ -117,8 +110,37 @@
> * display them in-line.
> */
> function search_for_and_display_dupes() {
> - function show_failure_message() {
> - Y.get('#possible-duplicates').set(INNER_HTML, 'FAIL');
> + function show_failure_message(transaction_id, response, args) {
> + // If the request failed due to a timeout, display a message
> + // explaining how the user may be able to work around it.
> + var error_message = '';
> + if (response.status == 503) {
> + // We treat 503 (service unavailable) as a timeout because
> + // that's what timeouts in LP return.
> + error_message =
> + "<p>Searching for your bug in Launchpad took too long. " +
> + "Try reducing the number of words in the summary " +
> + "field and click \"Check again\" to retry your search. " +
> + "Alternatively, you can enter the details of your bug " +
> + "below.</p>";
> + } else {
> + // Any other error code gets a generic message.
> + error_message =
> + "<p>An error occured whilst trying to find bugs matching " +
> + "the summary you entered. Click \"Check again\" to retry " +
> + "your search. Alternatively, you can enter the " +
> + "details of your bug below.</p>";
> + }
> +
> + Y.get('#possible-duplicates').set(INNER_HTML, error_message);

It works, but I think using innerHTML is bad except for a few
cases. Consider having error_message as just a message, then create a
<p> node and .set('text', error_message). Don't stress too much
though.

> +
> + Y.get('#spinner').setStyle(DISPLAY, NONE);

Consider using the "unseen" CSS class instead, and remove it to make
the spinner visible. Doesn't matter too much though. I'm sure I've
heard arguments that classes are better than manipulating style
directly, but I can't summon them right now.

> + show_bug_reporting_form();
> +
> + Y.get(Y.DOM.byId('field.t...

Hi,

The first part of the code review. Only a suggestion about using a
view argument to canonical_url(). Other than that, it all looks good.

Gavin.

> === modified file 'lib/lp/bugs/browser/bugtarget.py'
> --- lib/lp/bugs/browser/bugtarget.py 2009-11-26 11:32:54 +0000
> +++ lib/lp/bugs/browser/bugtarget.py 2009-12-09 12:13:43 +0000
> @@ -56,6 +56,7 @@
> from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
> from canonical.launchpad.interfaces.temporaryblobstorage import (
> ITemporaryStorageManager)
> +from canonical.launchpad.webapp import urlappend
> from canonical.launchpad.webapp.breadcrumb import Breadcrumb
> from canonical.launchpad.webapp.interfaces import ILaunchBag, NotFoundError
> from lp.bugs.interfaces.bug import (
> @@ -368,7 +369,6 @@
>
> def validate(self, data):
> """Make sure the package name, if provided, exists in the distro."""
> -
> # The comment field is only required if filing a new bug.
> if self.submit_bug_action.submitted():
> comment = data.get('comment')
> @@ -700,6 +700,26 @@
> """Override this method in base classes to show the filebug form."""
> raise NotImplementedError
>
> + @property
> + def inline_filebug_form_url(self):
> + """The URL to the inline filebug form.
> +
> + If a token was passed to this view, it will be be passed through
> + to the inline bug filing form via the returned URL.
> + """
> + url = urlappend(canonical_url(self.context), '+filebug-inline-form')

No need for urlappend here; pass view="+filebug-inline-form" to
canonical_url().

> + if self.extra_data_token is not None:
> + url = urlappend(url, self.extra_data_token)
> + return url
> +
> + @property
> + def duplicate_search_url(self):
> + """The URL to the inline duplicate search view."""
> + url = urlappend(canonical_url(self.context), '+filebug-show-similar')

Again with the view argument to canonical_url().

> + if self.extra_data_token is not None:
> + url = urlappend(url, self.extra_data_token)
> + return url
> +
> def publishTraverse(self, request, name):
> """See IBrowserPublisher."""
> if self.extra_data_token is not None:
> @@ -831,6 +851,11 @@
> return guidelines
>
>
> +class FileBugInlineFormView(FileBugViewBase):
> + """A browser view for displaying the inline filebug form."""
> + schema = IBugAddForm
> +
> +
> class FileBugAdvancedView(FileBugViewBase):
> """Browser view for filing a bug.
>
> @@ -851,7 +876,14 @@
> """
> schema = IBugAddForm
>
> + # XXX: Brad Bollenbach 2006-10-04: This assignment to actions is a
> + # hack to make the action decorator Just Work across inheritance.
> + actions = FileBugViewBase.actions
> + custom_widget('title', TextWidget, displayWidth=40)
> + custom_widget('tags', BugTagsWidget)
> +
> _MATCHING_BUGS_LIMIT = 10
> + show_summary_in_results = False
>
> @property
> def search_context(self):
> @@ -894,22 +926,37 @@
>
> return matching_bugs
>
> + @property
> + def show_duplicate_list(self)...

> > === modified file 'lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js'
> > --- lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js 2009-12-08 10:57:44 +0000
> > +++ lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js 2009-12-09 12:13:43 +0000
> > @@ -81,13 +81,6 @@
> > * @param e The Event triggering this function.
> > */
> > function show_bug_reporting_form(e) {
> > - if (Y.Lang.isValue(bug_already_reported_expanders)) {
> > - // Collapse any duplicate-details divs.
> > - Y.each(bug_already_reported_expanders, function(expander) {
> > - collapse_bug_details(expander);
> > - });
> > - }
> > -
> > // If the bug reporting form is in a hidden container, as it is on
> > // the AJAX dupe search, show it.
> > var filebug_form_container = Y.get('#filebug-form-container');
>
> Salgado, in his YUI() -> LPS branch, changed a lot of Y.get() calls to
> Y.one(). I guess that means things break non-silently when the targets
> don't exist. Might be a good idea here too.
>

Right. Fixed.

> > @@ -117,8 +110,37 @@
> > * display them in-line.
> > */
> > function search_for_and_display_dupes() {
> > - function show_failure_message() {
> > - Y.get('#possible-duplicates').set(INNER_HTML, 'FAIL');
> > + function show_failure_message(transaction_id, response, args) {
> > + // If the request failed due to a timeout, display a message
> > + // explaining how the user may be able to work around it.
> > + var error_message = '';
> > + if (response.status == 503) {
> > + // We treat 503 (service unavailable) as a timeout because
> > + // that's what timeouts in LP return.
> > + error_message =
> > + "<p>Searching for your bug in Launchpad took too long. " +
> > + "Try reducing the number of words in the summary " +
> > + "field and click \"Check again\" to retry your search. " +
> > + "Alternatively, you can enter the details of your bug " +
> > + "below.</p>";
> > + } else {
> > + // Any other error code gets a generic message.
> > + error_message =
> > + "<p>An error occured whilst trying to find bugs matching " +
> > + "the summary you entered. Click \"Check again\" to retry " +
> > + "your search. Alternatively, you can enter the " +
> > + "details of your bug below.</p>";
> > + }
> > +
> > + Y.get('#possible-duplicates').set(INNER_HTML, error_message);
>
> It works, but I think using innerHTML is bad except for a few
> cases. Consider having error_message as just a message, then create a
> <p> node and .set('text', error_message). Don't stress too much
> though.
>

I agree. Fixed.

> > +
> > + Y.get('#spinner').setStyle(DISPLAY, NONE);
>
> Consider using the "unseen" CSS class instead, and remove it to make
> the spinner visible. Doesn't matter too much though. I'm sure I've
> heard arguments that classes are better than manipulating style
> directly, but I can't summon them right now.
> o

I've heard similar ...

> > === modified file 'lib/lp/bugs/browser/bugtarget.py'
> > --- lib/lp/bugs/browser/bugtarget.py 2009-11-26 11:32:54 +0000
> > +++ lib/lp/bugs/browser/bugtarget.py 2009-12-09 12:13:43 +0000
> > @@ -56,6 +56,7 @@
> > from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
> > from canonical.launchpad.interfaces.temporaryblobstorage import (
> > ITemporaryStorageManager)
> > +from canonical.launchpad.webapp import urlappend
> > from canonical.launchpad.webapp.breadcrumb import Breadcrumb
> > from canonical.launchpad.webapp.interfaces import ILaunchBag, NotFoundError
> > from lp.bugs.interfaces.bug import (
> > @@ -368,7 +369,6 @@
> >
> > def validate(self, data):
> > """Make sure the package name, if provided, exists in the distro."""
> > -
> > # The comment field is only required if filing a new bug.
> > if self.submit_bug_action.submitted():
> > comment = data.get('comment')
> > @@ -700,6 +700,26 @@
> > """Override this method in base classes to show the filebug form."""
> > raise NotImplementedError
> >
> > + @property
> > + def inline_filebug_form_url(self):
> > + """The URL to the inline filebug form.
> > +
> > + If a token was passed to this view, it will be be passed through
> > + to the inline bug filing form via the returned URL.
> > + """
> > + url = urlappend(canonical_url(self.context), '+filebug-inline-form')
>
> No need for urlappend here; pass view="+filebug-inline-form" to
> canonical_url().
>

Right. No idea what I was smoking when I wrote that.

> > + if self.extra_data_token is not None:
> > + url = urlappend(url, self.extra_data_token)
> > + return url
> > +
> > + @property
> > + def duplicate_search_url(self):
> > + """The URL to the inline duplicate search view."""
> > + url = urlappend(canonical_url(self.context), '+filebug-show-similar')
>
> Again with the view argument to canonical_url().
>

Yep.

Oops. Incremental diff for the first two parts of the code / js reviews:

=== modified file 'lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js'
--- lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js 2009-12-09 13:05:28 +0000
+++ lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js 2009-12-09 13:20:27 +0000
@@ -16,7 +16,8 @@
     INNER_HTML = 'innerHTML',
     LAZR_CLOSED = 'lazr-closed',
     NONE = 'none',
- SRC = 'src';
+ SRC = 'src',
+ UNSEEN = 'unseen';

 var bugs = Y.namespace('bugs');

@@ -118,34 +119,36 @@
             // We treat 503 (service unavailable) as a timeout because
             // that's what timeouts in LP return.
             error_message =
- "<p>Searching for your bug in Launchpad took too long. " +
+ "Searching for your bug in Launchpad took too long. " +
                 "Try reducing the number of words in the summary " +
                 "field and click \"Check again\" to retry your search. " +
                 "Alternatively, you can enter the details of your bug " +
- "below.</p>";
+ "below.";
         } else {
             // Any other error code gets a generic message.
             error_message =
- "<p>An error occured whilst trying to find bugs matching " +
+ "An error occured whilst trying to find bugs matching " +
                 "the summary you entered. Click \"Check again\" to retry " +
                 "your search. Alternatively, you can enter the " +
- "details of your bug below.</p>";
+ "details of your bug below.";
         }

- Y.one('#possible-duplicates').set(INNER_HTML, error_message);
+ var error_node = Y.Node.create('<p></p>');
+ error_node.set('text', error_message);
+ Y.one('#possible-duplicates').appendChild(error_node);

- Y.one('#spinner').setStyle(DISPLAY, NONE);
+ Y.one('#spinner').addClass(UNSEEN);
         show_bug_reporting_form();

         Y.one(Y.DOM.byId('field.title')).set(
             'value', search_field.get('value'));
         search_button.set('value', 'Check again');
- search_button.setStyle(DISPLAY, INLINE);
+ search_button.removeClass(UNSEEN);
     }

     function on_success(transaction_id, response, args) {
         // Hide the spinner and show the duplicates.
- Y.one('#spinner').setStyle(DISPLAY, NONE);
+ Y.one('#spinner').addClass(UNSEEN);

         var duplicate_div = Y.one('#possible-duplicates');
         duplicate_div.set(INNER_HTML, response.responseText);
@@ -163,13 +166,13 @@

         // Copy the value from the search field into the title field
         // on the filebug form.
- Y.one(Y.DOM.byId('field.title')).set(
+ Y.one('#bug_reporting_form input[name=field.title]').set(
             'value', search_field.get('value'));

         // Finally, change the label on the search button and show it
         // again.
         search_button.set('value', 'Check again');
- search_button.setStyle(DISPLAY, INLINE);
+ search_button.removeClass(UNSEEN);
     }

     var search_term = encodeURI(search_fi...

The rest of the code review. A few minor things, but nothing serious. Rock on :) Get this landed, people want to buy you beer :)

> === modified file 'lib/lp/bugs/browser/configure.zcml'
> --- lib/lp/bugs/browser/configure.zcml 2009-12-09 12:13:11 +0000
> +++ lib/lp/bugs/browser/configure.zcml 2009-12-09 12:13:43 +0000
> @@ -95,6 +95,13 @@
> facet="bugs"
> permission="launchpad.AnyPerson"/>
> <browser:page
> + name="+filebug-inline-form"
> + for="lp.bugs.interfaces.bugtarget.IBugTarget"
> + class="lp.bugs.browser.bugtarget.FilebugShowSimilarBugsView"
> + template="../templates/bugtarget-filebug-inline-form.pt"
> + facet="bugs"
> + permission="launchpad.AnyPerson"/>
> + <browser:page
> name="+manage-official-tags"
> for="lp.bugs.interfaces.bugtarget.IOfficialBugTagTargetRestricted"
> class="lp.bugs.browser.bugtarget.OfficialBugTagsManageView"
>
> === modified file 'lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt'
> --- lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt 2009-11-26 09:18:22 +0000
> +++ lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt 2009-12-09 12:13:43 +0000
> @@ -43,6 +43,25 @@
> u'Test description.'
>
>
> +== URLs to additional FileBug elements ==
> +
> +FileBugViewBase provides properties that return the URLs of further
> +useful parts of the +filebug process.
> +
> +The inline_filebug_form_url property returns the URL of the inline
> +filebug form so that it may be loaded asynchronously.
> +
> + >>> print filebug_view.inline_filebug_form_url
> + http://launchpad.dev/ubuntu/+source/mozilla-firefox/+filebug-inline-form
> +
> +Similarly, the duplicate_search_url property returns the base URL for
> +the duplicate search view, which can be used to load the list of
> +possible duplicates for a bug asynchronously.
> +
> + >>> print filebug_view.duplicate_search_url
> + http://launchpad.dev/ubuntu/+source/mozilla-firefox/+filebug-show-similar
> +
> +
> == Adding extra info to filed bugs ==
>
> It's possible for bug reporting tools to upload a file with debug
>
> === modified file 'lib/lp/bugs/interfaces/bug.py'
> --- lib/lp/bugs/interfaces/bug.py 2009-11-26 09:18:22 +0000
> +++ lib/lp/bugs/interfaces/bug.py 2009-12-09 12:13:43 +0000
> @@ -26,6 +26,7 @@
> from zope.interface import Interface, Attribute
> from zope.schema import (
> Bool, Bytes, Choice, Datetime, Int, List, Object, Text, TextLine)
> +from zope.schema.vocabulary import SimpleVocabulary
> from zope.security.interfaces import Unauthorized
>
> from canonical.launchpad import _
> @@ -789,6 +790,11 @@
> "A sequence of IBugTaskDeltas, one IBugTaskDelta or None.")
>
>
> +# A simple vocabulary for the subscribe_to_existing_bug form field.
> +SUBSCRIBE_TO_BUG_VOCABULARY = SimpleVocabulary.fromItems(
> + [('yes', True), ('no', False)])
> +
> +
> class IBugAddForm(IBug):
> """Information we need to create a bug"""
> id = Int(title=_("Bug #"), required=False)
> @@ -842,8 +848,9 @@
> assignee = PublicPersonChoice(
> title=_('Assign to'), required=False,
> ...

> > > + <td colspan="2">
> > > + <p>
> > > + Please describe the bug in a few words, for example,
> > "weather
> > > + applet crashes on logout":
> > > + </p>
> >
> > I don't think the <p></p> is needed here (i.e. <td>s can have contain
> > inline content)... unless it affects layout.
> >

It does. The paragraphing is deliberate.

> > > + </td>
> > > + </tr>
> > > + <tal:title_widget tal:define="widget nocall:view/widgets/title">
> > > + <tal:comment replace="nothing">
> > > + The desire to have more control over the styling of this
> > widget
> > > + prevents us from using the widget_row macro here.
> > > + </tal:comment>
> > > + <tr>
> > > + <td tal:define="field_name widget/context/__name__;
> > > + error python:view.getFieldError(field_name);
> > > + error_class python:error and 'error' or
> > None;"
> >
> > I think you can say this now:
> >
> > ('error' if error else None)
> >
> > (Woohoo!)

Indeed. Fixed.

> > > + <p>
> > > + <a id="filebug-form-url"
> > > + tal:attributes="href view/inline_filebug_form_url"
> > > + style="display: none;"></a>
> > > + <a id="duplicate-search-url"
> > > + tal:attributes="href view/duplicate_search_url"
> > > + style="display: none;"></a>
> > > + </p>
> >
> > Maybe it's simpler to put the display:none on the <p>? I think empty
> > paragraphs still influence layout.

Fixed.