launchpad-reviewers team mailing list archive

[Henning Eggers <henning.eggers@xxxxxxxxxxxxx>]

Henning Eggers has proposed merging lp:~henninge/launchpad/bug-753387-ajax-bug-subscription into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #753387 in Launchpad itself: "mute_subscription is null in update_mute_after_subscription_change"
  https://bugs.launchpad.net/launchpad/+bug/753387

For more details, see:
https://code.launchpad.net/~henninge/launchpad/bug-753387-ajax-bug-subscription/+merge/62256

= Summary =

Even without the new "mute" feature enabled (part of structural bug
subscriptions), subscribing and unscribing tried to update the "mute
subscription" node. This failed because that node does not exist if
the feature is not enabled.

== Proposed fix ==

Check if the node exists before doing stuff with it.

== Pre-implementation notes ==

None, all my own thinking.

== Implementation details ==

The check had only to be added in two places although the function that
eventually fails (update_mute_after_subscription_change) is called more
often. Those other call sites are only reached when the feature is
enabled and therefore when the node is actually on the page.
The two affected call sites can easily be spotted because they are the
only ones that call get_mute_subscription directly before.

== Tests ==

None, this is transitional anyway.

== Demo and Q/A ==

Open any bug when the feature is not enabled, open a javascript
console. Click on "Subscribe" and watch that you *don't* see an
error message on the console.
The second callsite is in "Subscribe someone else" but is only
triggered when the user has permissions to also unsubscribe that
other person. I could not (easily) figure out how to get into that
position.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/javascript/bugtask_index_portlets.js
-- 
https://code.launchpad.net/~henninge/launchpad/bug-753387-ajax-bug-subscription/+merge/62256
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/bug-753387-ajax-bug-subscription into lp:launchpad.

=== modified file 'lib/lp/bugs/javascript/bugtask_index_portlets.js'
--- lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-05-18 18:34:19 +0000
+++ lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-05-25 08:31:36 +0000
@@ -769,7 +769,9 @@
 
                 add_user_name_link(subscription);
                 var mute_subscription = get_mute_subscription();
-                update_mute_after_subscription_change(mute_subscription);
+                if (mute_subscription !== null) {
+                    update_mute_after_subscription_change(mute_subscription);
+                }
 
                 // Only when the link is added to the page, indicate success.
                 Y.on('contentready', function() {
@@ -1394,8 +1396,10 @@
                                 add_temp_user_name(subscription);
                                 var mute_subscription;
                                 mute_subscription = get_mute_subscription();
-                                update_mute_after_subscription_change(
-                                    mute_subscription);
+                                if (mute_subscription !== null) {
+                                    update_mute_after_subscription_change(
+                                        mute_subscription);
+                                }
                             } else {
                                 subscription.set(
                                    'can_be_unsubscribed', false);