Launchpad itself

lib/lp/bugs/javascript/bug_notification_level.js (+200/-49)
lib/lp/bugs/javascript/bugtask_index_portlets.js (+51/-58)
lib/lp/bugs/javascript/tests/test_bug_notification_level.html (+41/-0)
lib/lp/bugs/javascript/tests/test_bug_notification_level.js (+460/-0)
lib/lp/bugs/templates/bug-subscription.pt (+7/-6)

To merge this branch:

bzr merge lp://staging/~danilo/launchpad/advanced-bug-subscription-cleanup

Related bugs:

Bug #740627: Fold "Tell me when" section in bug subscription overlay when unsubscribe options are selected	High	Fix Released
Bug #770460: Subscription level picker animation sometimes leaves picker broken.	High	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Brad Crittenden (community)	code	2011-04-27	Approve on 2011-04-27
Review via email: mp+59235@code.staging.launchpad.net

Commit message

[r=bac][bug=740627,770460] Fix all issues with showing/hiding of bug notification level options in advanced subscription overlay.

Description of the change

= Bug 740627 and bug 770460 =

The logic that shows/hides the options to set different levels of subscriptions on bug subscription overlay is very unreliable at the moment. Animation is also very jerky and unstable.

Some of the problems are that reloading the overlay sometimes doesn't load the animation handlers at all, or that wrong options are shown depending on the conditions.

The root cause is that some code expected at most one subscription overlay, and yet more than one was created because of the mute functionality.

Sorry for the over-sized branch, but over half is unit tests.

== Proposed fix ==

* Rename bug_subscription.js to bug_notification_level.js and refactor it so it works correctly and tries to make at least a bit of sense of the Python-generated web form we are using; extensive test coverage is provided

* In bugtask_index_portlets.js, ensure that we never try to create multiple overlays because that adds identical elements into the page DOM which then causes problems in existing code which can't handle it; I still do not provide any tests for this, but I am working on that in a separate branch

== Implementation details ==

Other notes:

* I am not changing this to allow more than one overlay because it's too ingrained in the code using Y.one/Y.all where we'd need the parent/container node instead
* In bugtask_index_portlets.js I do away with a hack where we were not using loadFormContentAndRender() and instead copy-pasted the code; we use Y.on('contentready',...) to wait for elements to load and ensure no empty form is shown.
* clean_up_level_div() is not necessary anymore since it was a work-around for the problem when slide-out was still happening (and continuing) when slide-in was requested; this is now fixed properly

== Tests ==

lib/lp/bugs/javascript/tests/test_bug_notification_level.html

== Demo and Q/A ==

Set feature flag:
'malone.advanced-subscriptions.enabled default 1 on'

https://bugs.launchpad.dev/firefox/+bug/1/+subscribe
https://bugs.launchpad.dev/firefox/+bug/1/ (and play with Subscribe/Mute links)

For mute link to show, eg. set assignee to yourself.

Demo:

https://devpad.canonical.com/~danilo/screencasts/bug-notification-level-anim.ogv

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/javascript/bug_notification_level.js
  lib/lp/bugs/javascript/tests/test_bug_notification_level.html
  lib/lp/bugs/javascript/bugtask_index_portlets.js
  lib/lp/bugs/templates/bug-subscription.pt
  lib/lp/bugs/javascript/tests/test_bug_notification_level.js

Revision history for this message

Brad Crittenden (bac) wrote on 2011-04-27:

Hi Danilo,

* I notice when I click on 'Unmute' there is some jumpiness as the overlay seems to be drawn one way at first and then redrawn. I can see it in your screencast too. Do you know the reason for this?

* Also when we the unmute overlay is show, the first time you select 'unmute and subscribe' the overlay has to resize to accommodate the extra width of the recently revealed text. Can that be taken into account at the beginning so we don't see the jump?

* You have written "when either the selected radio box" but there is no 'or' clause. I think 'either' should be deleted or you need to complete your thought.

* Many of your comments are of the form "Is there something." Those should be punctuated as questions with '?'

* Typo: s/A list of radio in/A list of radio buttons in/

* s/radio box/radio button/ I've always see "checkbox" and "radio button" but never (before now!) "radio box".

* I'm glad you removed the hack for loadFormContentAndRender but I don't understand why it was there in the first place.

* In all but one test case you have repeated:
+ // Restore the default value.
+ module._bug_notification_level_visible = true;

This setting should be done in the setUp (or tearDown) function. I think the one place you didn't do it was a mistake, no?

*** Thanks for all of the great tests! They are clear and (seem to be) comprehensive.

review: Approve (code)

Revision history for this message

Данило Шеган (danilo) wrote on 2011-04-28:

Download full text (3.2 KiB)

Hi Brad,

У сре, 27. 04 2011. у 18:42 +0000, Brad Crittenden пише:

> * I notice when I click on 'Unmute' there is some jumpiness as the
> overlay seems to be drawn one way at first and then redrawn. I can see
> it in your screencast too. Do you know the reason for this?

Sort of: the reason is that the level options are initially shown in the
form (we can't do much about that since it's a static form), and then we
try to quickly hide them (the quick_close parameter in
toggle_field_visibility). However, I haven't solved the problem before
proposing for merge of how to delay rendering until initialize() has
completed. Going through this review, I came to a realization that this
is quite easy: I'll fire a special event at the end of setup() and show
the form when that happens (instead of on 'contentready'). I've also
added tests to confirm event is fired in all cases.

> * Also when we the unmute overlay is show, the first time you select
> 'unmute and subscribe' the overlay has to resize to accommodate the
> extra width of the recently revealed text. Can that be taken into
> account at the beginning so we don't see the jump?

For me, the overlay doesn't resize but the text does "jump". I changed
the quick_close to set height:0 and overflow:hidden and now it works
much better (instead of setting display:none).

I was already aware of these problems, so I've got to thank you for
nudging me to look further into them and to solve them. :)

> * You have written "when either the selected radio box" but there is no
> 'or' clause. I think 'either' should be deleted or you need to
> complete your thought.

Fixed.

> * Many of your comments are of the form "Is there something." Those
> should be punctuated as questions with '?'

Fixed (hopefully) all of these as well.

> * Typo: s/A list of radio in/A list of radio buttons in/

Fixed.

> * s/radio box/radio button/ I've always see "checkbox" and "radio button" but never (before now!) "radio box".

You can see I've been confused myself. :) I started with "radio boxes"
and then realized that this doesn't make sense, and switched to "radio
buttons". However, I forgot to fix all the occurrences of the former,
which I did now.

> * I'm glad you removed the hack for loadFormContentAndRender but I don't understand why it was there in the first place.

Well, I assume it's because of the 'contentready' usage missing: it used
to call bug_notification_level.setup() only in on_success handler to
ensure the form was loaded before the overlay was shown. Now we show
the overlay only after the newly introduced event fires (or on
io:failure).

> * In all but one test case you have repeated:
> + // Restore the default value.
> + module._bug_notification_level_visible = true;
>
> This setting should be done in the setUp (or tearDown) function. I think the one place you didn't do it was a mistake, no?

Oh, in that one case I believe there's an assertTrue(module._bug_n_l_v)
as one of the very last statements. Though sure, it'd be better to move
it to tearDown(), so I did that.

> *** Thanks for all of the great tests! They are clear and (seem to be) comprehensive.

Thank you for a very nice ...

Hi Brad,

У сре, 27. 04 2011. у 18:42 +0000, Brad Crittenden пише:

> * I notice when I click on 'Unmute' there is some jumpiness as the
> overlay seems to be drawn one way at first and then redrawn.  I can see
> it in your screencast too.  Do you know the reason for this?

Sort of: the reason is that the level options are initially shown in the
form (we can't do much about that since it's a static form), and then we
try to quickly hide them (the quick_close parameter in
toggle_field_visibility).  However, I haven't solved the problem before
proposing for merge of how to delay rendering until initialize() has
completed.  Going through this review, I came to a realization that this
is quite easy: I'll fire a special event at the end of setup() and show
the form when that happens (instead of on 'contentready').  I've also
added tests to confirm event is fired in all cases.

> * Also when we the unmute overlay is show, the first time you select
> 'unmute and subscribe' the overlay has to resize to accommodate the
> extra width of the recently revealed text.  Can that be taken into
> account at the beginning so we don't see the jump?

For me, the overlay doesn't resize but the text does "jump".  I changed
the quick_close to set height:0 and overflow:hidden and now it works
much better (instead of setting display:none).

I was already aware of these problems, so I've got to thank you for
nudging me to look further into them and to solve them. :)

> * You have written "when either the selected radio box" but there is no
> 'or' clause.  I think 'either' should be deleted or you need to
> complete your thought.

Fixed.

> * Many of your comments are of the form "Is there something."  Those
> should be punctuated as questions with '?'

Fixed (hopefully) all of these as well.

> * Typo: s/A list of radio in/A list of radio buttons in/

Fixed.

> * s/radio box/radio button/  I've always see "checkbox" and "radio button" but never (before now!) "radio box".

You can see I've been confused myself.  :) I started with "radio boxes"
and then realized that this doesn't make sense, and switched to "radio
buttons".  However, I forgot to fix all the occurrences of the former,
which I did now.

> * I'm glad you removed the hack for loadFormContentAndRender but I don't understand why it was there in the first place.

Well, I assume it's because of the 'contentready' usage missing: it used
to call bug_notification_level.setup() only in on_success handler to
ensure the form was loaded before the overlay was shown.  Now we show
the overlay only after the newly introduced event fires (or on
io:failure).

> * In all but one test case you have repeated:
> +        // Restore the default value.
> +        module._bug_notification_level_visible = true;
> 
> This setting should be done in the setUp (or tearDown) function.  I think the one place you didn't do it was a mistake, no?

Oh, in that one case I believe there's an assertTrue(module._bug_n_l_v)
as one of the very last statements.  Though sure, it'd be better to move
it to tearDown(), so I did that.

> *** Thanks for all of the great tests!  They are clear and (seem to be) comprehensive.

Thank you for a very nice review: the branch is so much better for it!

Cheers,
Danilo