launchpad-reviewers team mailing list archive

[Данило Шеган <danilo@xxxxxxxxxxxxx>]

Данило Шеган has proposed merging lp:~danilo/launchpad/duplicate-pillars-subscriptions into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~danilo/launchpad/duplicate-pillars-subscriptions/+merge/58135

= Group pillar subscriptions =

In testing and QA for bug 728370, we've noticed that sometimes multiple identical subscription descriptions show up.  This turns out to be when there are several bug tasks for the same pillar (eg. one for Ubuntu Hoary, and another for Ubuntu Warty).  Thus, we need to group them together.

== Proposed fix ==

Treat a list of subscriptions for supervisor as a set.

I am not fixing any of the lint issues yet (I've got a bigger branch up for review that I'd rather not have hit any conflicts if I fix them in there as well).

== Tests ==

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

== Demo and Q/A ==

https://bugs.launchpad.dev/tomcat/+bug/2/+subscriptions

To facilitate the demo:

  - set 'malone.advanced-structural-subscriptions.enabled default 1 on'
    on https://launchpad.dev/+feature-rules
  - log in as cprov@xxxxxxxxxxxxx:cprov

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/javascript/subscription.js
  lib/lp/bugs/javascript/tests/test_subscription.js

./lib/lp/bugs/javascript/subscription.js
      99: Unexpected ','.
     109: Unexpected ','.
     136: Expected '===' and instead saw '=='.
     156: Move 'var' declarations to the top of the function.
     156: Stopping.  (20% scanned).
       0: JSLINT had a fatal error.
./lib/lp/bugs/javascript/tests/test_subscription.js
      49: Expected ';' and instead saw 'Y'.
     182: Unexpected ','.
     190: Move 'var' declarations to the top of the function.
     190: Stopping.  (12% scanned).
       0: JSLINT had a fatal error.
-- 
https://code.launchpad.net/~danilo/launchpad/duplicate-pillars-subscriptions/+merge/58135
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/duplicate-pillars-subscriptions into lp:launchpad.

=== modified file 'lib/lp/bugs/javascript/subscription.js'
--- lib/lp/bugs/javascript/subscription.js	2011-04-14 20:03:32 +0000
+++ lib/lp/bugs/javascript/subscription.js	2011-04-18 14:39:26 +0000
@@ -286,6 +286,35 @@
         gather_subscriptions_as_assignee;
 
 /**
+ * Adds a `subscription` to `subscriptions` if it's not in the list already.
+ * Compares reason, action and all the `vars` from existing subscription.
+ */
+function add_subscription_to_set(subscriptions, subscription) {
+    for (var index in subscriptions) {
+        var sub = subscriptions[index];
+        if (sub.reason === subscription.reason &&
+            sub.action === subscription.action) {
+            var are_vars_same = true;
+            for (var param in sub.vars) {
+                // We only check vars from the existing subscription.
+                // Theoretically, there could be a var on `subscription`
+                // not present on `sub`, but we're guarding against that
+                // with reason/action checks.
+                if (sub.vars[param].self !== subscription.vars[param].self) {
+                    are_vars_same = false;
+                    break;
+                }
+            }
+            if (are_vars_same) {
+                return;
+            }
+        }
+    }
+    // We haven't found matching subscriptions, add it.
+    subscriptions.push(subscription);
+}
+
+/**
  * Gather subscription information for implicit bug supervisor.
  */
 function gather_subscriptions_as_supervisor(category) {
@@ -294,7 +323,7 @@
 
     for (var index in category.personal) {
         var subscription = category.personal[index];
-        subscriptions.push({
+        add_subscription_to_set(subscriptions, {
             reason: reasons.YOU_OWNER,
             vars: {
                 pillar: get_link_data(subscription.pillar)
@@ -305,7 +334,7 @@
 
     for (var index in category.as_team_member) {
         var team_subscription = category.as_team_member[index];
-        subscriptions.push({
+        add_subscription_to_set(subscriptions, {
             reason: reasons.TEAM_OWNER,
             vars: {
                 team: get_link_data(team_subscription.principal),
@@ -317,7 +346,7 @@
 
     for (var index in category.as_team_admin) {
         var team_subscription = category.as_team_admin[index];
-        subscriptions.push({
+        add_subscription_to_set(subscriptions, {
             reason: reasons.ADMIN_TEAM_OWNER,
             vars: {
                 team: get_link_data(team_subscription.principal),

=== modified file 'lib/lp/bugs/javascript/tests/test_subscription.js'
--- lib/lp/bugs/javascript/tests/test_subscription.js	2011-04-14 20:03:32 +0000
+++ lib/lp/bugs/javascript/tests/test_subscription.js	2011-04-18 14:39:26 +0000
@@ -330,7 +330,8 @@
         // supervisor.
         var mock_category = {
             count: 2,
-            personal: [{pillar: 'project'}, {pillar: 'distro'}],
+            personal: [ {pillar: {title: 'project'} },
+                        {pillar: {title:'distro'} }],
             as_team_member: [],
             as_team_admin: []
         };
@@ -365,9 +366,9 @@
             count: 2,
             personal: [],
             as_team_member: [{ principal: 'team1',
-                               pillar: 'project' },
+                               pillar: {display_name: 'project'} },
                              { principal: 'team2',
-                               pillar: 'distro' }],
+                               pillar: {display_name: 'distro'} }],
             as_team_admin: []
         };
         var subs = module._gather_subscriptions_as_supervisor(mock_category);
@@ -403,14 +404,33 @@
             personal: [],
             as_team_member: [],
             as_team_admin: [{ principal: 'team1',
-                               pillar: 'project' },
-                             { principal: 'team2',
-                               pillar: 'distro' }]
+                              pillar: {display_name: 'project'} },
+                            { principal: 'team2',
+                              pillar: {display_name: 'distro'} }]
         };
         var subs = module._gather_subscriptions_as_supervisor(mock_category);
         Y.Assert.areEqual(2, subs.length);
     },
 
+    test_repeated_pillars: function() {
+        // Different bug tasks might still be on the same pillar,
+        // and we should only get one action.
+        var mock_pillar = { display_name: 'project',
+                            web_link: 'http://project/' };
+        var mock_category = {
+            count: 1,
+            personal: [{pillar: mock_pillar},
+                       {pillar: mock_pillar}],
+            as_team_member: [],
+            as_team_admin: []
+        };
+        var subs = module._gather_subscriptions_as_supervisor(mock_category);
+        Y.Assert.areEqual(1, subs.length);
+        Y.Assert.areEqual(module._reasons.YOU_OWNER, subs[0].reason);
+        Y.Assert.areEqual(mock_pillar, subs[0].vars.pillar.self);
+        Y.Assert.areEqual(module._actions.SET_BUG_SUPERVISOR, subs[0].action);
+    },
+
     test_combined: function() {
         // Test that multiple implicit bug supervisor roles
         // are all returned.