launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03324
[Merge] lp:~gary/launchpad/bug728370-direct-subs into lp:launchpad
Gary Poster has proposed merging lp:~gary/launchpad/bug728370-direct-subs into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~gary/launchpad/bug728370-direct-subs/+merge/57703
This branch is another incremental step towards the "unsubscribe in anger story." It has two parts.
The main task is to set up a simple way to include actions. This branch creates placeholder actions corresponding to the initial cut of actions I expect to provide for non-direct non-personal subscriptions and associates them with the proper output. Tests are adjusted to show the new data.
A side task was that I saw that it was possible to have the same team shown multiple times for assignees and direct team subscriptions. I added tests for this case and changed the pertinent code (gosh it would be nice to have a more robust mapping implementation in JS!).
Thank you
Gary
--
https://code.launchpad.net/~gary/launchpad/bug728370-direct-subs/+merge/57703
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug728370-direct-subs into lp:launchpad.
=== modified file 'lib/lp/bugs/javascript/subscription.js'
--- lib/lp/bugs/javascript/subscription.js 2011-04-13 15:47:48 +0000
+++ lib/lp/bugs/javascript/subscription.js 2011-04-14 15:27:20 +0000
@@ -109,6 +109,20 @@
ADMIN_TEAM_OWNER: _ADMIN_BECAUSE_TEAM_IS + _OWNER,
});
+/* These are the actions */
+var actions = {
+ CHANGE_ASSIGNEES: function () {
+ },
+ UNSUBSCRIBE_DUPLICATES: function () {
+ },
+ CHANGE_TEAM_SUBSCRIPTIONS: function () {
+ },
+ SET_BUG_SUPERVISOR: function () {
+ },
+ NONE: function () {
+ }
+};
+namespace._actions = actions;
/**
* Return appropriate object based on the number.
@@ -205,44 +219,43 @@
* than one, variable `teams` is set containing all the teams.
*/
function gather_subscriptions_by_role(
- category, team_singular, team_plural,
- admin_team_singular, admin_team_plural) {
- var subscriptions = [];
- if (category.as_team_member.length > 0) {
- var teams = [];
- for (var index in category.as_team_member) {
- var team_subscription = category.as_team_member[index];
- teams.push(get_link_data(team_subscription.principal));
- }
- var sub = choose_by_number(
- category.as_team_member.length,
- { reason: team_singular,
- vars: {
- team: teams[0] } },
- { reason: team_plural,
- vars: {
- teams: teams } });
- subscriptions.push(sub);
- }
-
- if (category.as_team_admin.length > 0) {
- var teams = [];
- for (var index in category.as_team_admin) {
- var team_subscription = category.as_team_admin[index];
- teams.push(get_link_data(team_subscription.principal));
- }
- var sub = choose_by_number(
- category.as_team_admin.length,
- { reason: admin_team_singular,
- vars: {
- team: teams[0] } },
- { reason: admin_team_plural,
- vars: {
- teams: teams } });
- subscriptions.push(sub);
- }
-
- return subscriptions;
+ category, team_config, admin_team_config) {
+ var results = [],
+ work = [{subscriptions: category.as_team_member,
+ config: team_config},
+ {subscriptions: category.as_team_admin,
+ config: admin_team_config}];
+ for (var work_index in work) {
+ var subscriptions = work[work_index]['subscriptions'];
+ var config = work[work_index]['config'];
+ if (subscriptions.length > 0) {
+ var team_map = {};
+ var teams = [];
+ for (var index in subscriptions) {
+ var team_subscription = subscriptions[index],
+ team = team_subscription.principal,
+ key = team.web_link,
+ key = Y.Lang.isValue(key) ? key : team; // For tests.
+ if (!Y.Lang.isValue(team_map[key])) {
+ var link_data = get_link_data(team);
+ team_map[team.web_link] = link_data;
+ teams.push(link_data);
+ }
+ }
+ var sub = choose_by_number(
+ subscriptions.length,
+ { reason: config.singular,
+ vars: {
+ team: teams[0] } },
+ { reason: config.plural,
+ vars: {
+ teams: teams } });
+ sub['action'] = config.action;
+ results.push(sub);
+ }
+ }
+
+ return results;
}
/**
@@ -255,14 +268,20 @@
if (category.personal.length > 0) {
subscriptions.push(
{ reason: reasons.YOU_ASSIGNED,
- vars: {} });
+ vars: {},
+ action: actions.CHANGE_ASSIGNEES });
}
// We add all the team assignments grouped by roles in the team.
return subscriptions.concat(
gather_subscriptions_by_role(
- category, reasons.TEAM_ASSIGNED, reasons.TEAMS_ASSIGNED,
- reasons.ADMIN_TEAM_ASSIGNED, reasons.ADMIN_TEAMS_ASSIGNED));
+ category,
+ {singular: reasons.TEAM_ASSIGNED,
+ plural: reasons.TEAMS_ASSIGNED,
+ action: actions.NONE},
+ {singular: reasons.ADMIN_TEAM_ASSIGNED,
+ plural: reasons.ADMIN_TEAMS_ASSIGNED,
+ action: actions.CHANGE_ASSIGNEES}));
}
namespace._gather_subscriptions_as_assignee =
gather_subscriptions_as_assignee;
@@ -280,7 +299,8 @@
reason: reasons.YOU_OWNER,
vars: {
pillar: get_link_data(subscription.pillar)
- }
+ },
+ action: actions.SET_BUG_SUPERVISOR
});
}
@@ -291,7 +311,8 @@
vars: {
team: get_link_data(team_subscription.principal),
pillar: get_link_data(team_subscription.pillar)
- }
+ },
+ action: actions.NONE
});
}
@@ -302,7 +323,8 @@
vars: {
team: get_link_data(team_subscription.principal),
pillar: get_link_data(team_subscription.pillar)
- }
+ },
+ action: actions.SET_BUG_SUPERVISOR
});
}
@@ -312,7 +334,7 @@
gather_subscriptions_as_supervisor;
function gather_dupe_subscriptions_by_team(team_subscriptions,
- singular, plural) {
+ singular, plural, action) {
var subscriptions = [];
// Collated list of { team: ..., bugs: []} records.
@@ -346,6 +368,7 @@
{ reason: plural,
vars: { duplicate_bugs: team_dupes.bugs,
team: get_link_data(team_dupes.team) }});
+ sub['action'] = action;
subscriptions.push(sub);
}
return subscriptions;
@@ -371,6 +394,7 @@
vars: { duplicate_bug: dupes[0] }},
{ reason: reasons.YOU_SUBSCRIBED_TO_DUPLICATES,
vars: { duplicate_bugs: dupes }});
+ sub['action'] = actions.UNSUBSCRIBE_DUPLICATES
subscriptions.push(sub);
}
@@ -379,14 +403,16 @@
gather_dupe_subscriptions_by_team(
category.as_team_member,
reasons.TEAM_SUBSCRIBED_TO_DUPLICATE,
- reasons.TEAM_SUBSCRIBED_TO_DUPLICATES));
+ reasons.TEAM_SUBSCRIBED_TO_DUPLICATES,
+ actions.NONE));
// Get subscriptions as team admin, grouped by teams.
subscriptions = subscriptions.concat(
gather_dupe_subscriptions_by_team(
category.as_team_admin,
reasons.ADMIN_TEAM_SUBSCRIBED_TO_DUPLICATE,
- reasons.ADMIN_TEAM_SUBSCRIBED_TO_DUPLICATES));
+ reasons.ADMIN_TEAM_SUBSCRIBED_TO_DUPLICATES,
+ actions.UNSUBSCRIBE_DUPLICATES));
return subscriptions;
}
@@ -399,8 +425,13 @@
function gather_subscriptions_through_team(category) {
var reasons = namespace._reasons;
return gather_subscriptions_by_role(
- category, reasons.TEAM_SUBSCRIBED, reasons.TEAMS_SUBSCRIBED,
- reasons.ADMIN_TEAM_SUBSCRIBED, reasons.ADMIN_TEAMS_SUBSCRIBED);
+ category,
+ {singular: reasons.TEAM_SUBSCRIBED,
+ plural: reasons.TEAMS_SUBSCRIBED,
+ action: actions.NONE},
+ {singular: reasons.ADMIN_TEAM_SUBSCRIBED,
+ plural:reasons.ADMIN_TEAMS_SUBSCRIBED,
+ action: actions.CHANGE_TEAM_SUBSCRIPTIONS});
}
namespace._gather_subscriptions_through_team =
gather_subscriptions_through_team;
=== modified file 'lib/lp/bugs/javascript/tests/test_subscription.js'
--- lib/lp/bugs/javascript/tests/test_subscription.js 2011-04-13 14:19:19 +0000
+++ lib/lp/bugs/javascript/tests/test_subscription.js 2011-04-14 15:27:20 +0000
@@ -123,6 +123,7 @@
var subs = module._gather_subscriptions_as_assignee(mock_category);
Y.Assert.areEqual(1, subs.length);
Y.Assert.areEqual(module._reasons.YOU_ASSIGNED, subs[0].reason);
+ Y.Assert.areEqual(module._actions.CHANGE_ASSIGNEES, subs[0].action);
},
test_team_member: function() {
@@ -139,6 +140,7 @@
Y.Assert.areEqual(module._reasons.TEAM_ASSIGNED, subs[0].reason);
// And there is a 'team' variable containing the team object.
Y.Assert.areEqual('my team', subs[0].vars.team);
+ Y.Assert.areEqual(module._actions.NONE, subs[0].action);
},
test_team_member_multiple: function() {
@@ -158,6 +160,36 @@
// And there is a 'teams' variable containing all the team objects.
Y.ArrayAssert.itemsAreEqual(['team1', 'team2'],
subs[0].vars.teams);
+ Y.Assert.areEqual(module._actions.NONE, subs[0].action);
+ },
+
+ test_team_member_multiple_duplicate: function() {
+ // As with the previous test, but we need to show that each team is
+ // only represented once even if they are responsible for multiple
+ // bug tasks.
+ // We test with full-fledged objects to make sure they work with the
+ // mechanism used to find dupes.
+ var team1 = {display_name: 'team 1',
+ web_link: 'http://launchpad.net/~team1'},
+ team2 = {display_name: 'team 2',
+ web_link: 'http://launchpad.net/~team2'},
+ mock_category = {
+ count: 2,
+ personal: [],
+ as_team_member: [{ principal: team1 },
+ { principal: team2 },
+ { principal: team2 },],
+ as_team_admin: []
+ },
+ subs = module._gather_subscriptions_as_assignee(mock_category);
+ Y.Assert.areEqual(1, subs.length);
+ Y.Assert.areEqual(module._reasons.TEAMS_ASSIGNED, subs[0].reason);
+ // And there is a 'teams' variable containing all the team objects.
+ var teams_found = [];
+ for (var index in subs[0].vars.teams) {
+ teams_found.push(subs[0].vars.teams[index].title);
+ }
+ Y.ArrayAssert.itemsAreEqual(['team 1', 'team 2'], teams_found);
},
test_team_admin: function() {
@@ -175,6 +207,7 @@
module._reasons.ADMIN_TEAM_ASSIGNED, subs[0].reason);
// And there is a 'team' variable containing the team object.
Y.Assert.areEqual('my team', subs[0].vars.team);
+ Y.Assert.areEqual(module._actions.CHANGE_ASSIGNEES, subs[0].action);
},
test_team_admin_multiple: function() {
@@ -195,6 +228,35 @@
// And there is a 'teams' variable containing all the team objects.
Y.ArrayAssert.itemsAreEqual(['team1', 'team2'],
subs[0].vars.teams);
+ Y.Assert.areEqual(module._actions.CHANGE_ASSIGNEES, subs[0].action);
+ },
+
+ test_team_admin_multiple_duplicate: function() {
+ // As with the previous test, but we need to show that each team is
+ // only represented once even if they are responsible for multiple
+ // bug tasks.
+ // We test with full-fledged objects to make sure they work with the
+ // mechanism used to find dupes.
+ var team1 = {display_name: 'team 1',
+ web_link: 'http://launchpad.net/~team1'},
+ team2 = {display_name: 'team 2',
+ web_link: 'http://launchpad.net/~team2'},
+ mock_category = {
+ count: 2,
+ personal: [],
+ as_team_admin: [{ principal: team1 },
+ { principal: team2 },
+ { principal: team2 },],
+ as_team_member: []
+ },
+ subs = module._gather_subscriptions_as_assignee(mock_category);
+ Y.Assert.areEqual(1, subs.length);
+ // And there is a 'teams' variable containing all the team objects.
+ var teams_found = [];
+ for (var index in subs[0].vars.teams) {
+ teams_found.push(subs[0].vars.teams[index].title);
+ }
+ Y.ArrayAssert.itemsAreEqual(['team 1', 'team 2'], teams_found);
},
test_combined: function() {
@@ -259,6 +321,7 @@
Y.Assert.areEqual(1, subs.length);
Y.Assert.areEqual(module._reasons.YOU_OWNER, subs[0].reason);
Y.Assert.areEqual('project', subs[0].vars.pillar);
+ Y.Assert.areEqual(module._actions.SET_BUG_SUPERVISOR, subs[0].action);
},
test_personal_multiple: function() {
@@ -291,6 +354,7 @@
// And there is a 'team' variable containing the team object.
Y.Assert.areEqual('my team', subs[0].vars.team);
Y.Assert.areEqual('project', subs[0].vars.pillar);
+ Y.Assert.areEqual(module._actions.NONE, subs[0].action);
},
test_team_member_multiple: function() {
@@ -327,6 +391,7 @@
// And there is a 'team' variable containing the team object.
Y.Assert.areEqual('my team', subs[0].vars.team);
Y.Assert.areEqual('project', subs[0].vars.pillar);
+ Y.Assert.areEqual(module._actions.SET_BUG_SUPERVISOR, subs[0].action);
},
test_team_admin_multiple: function() {
@@ -415,6 +480,8 @@
Y.Assert.areEqual(
module._reasons.YOU_SUBSCRIBED_TO_DUPLICATE, subs[0].reason);
Y.Assert.areEqual('dupe bug', subs[0].vars.duplicate_bug);
+ Y.Assert.areEqual(module._actions.UNSUBSCRIBE_DUPLICATES,
+ subs[0].action);
},
test_personal_multiple: function() {
@@ -433,6 +500,8 @@
module._reasons.YOU_SUBSCRIBED_TO_DUPLICATES, subs[0].reason);
Y.ArrayAssert.itemsAreEqual(
['dupe1', 'dupe2'], subs[0].vars.duplicate_bugs);
+ Y.Assert.areEqual(module._actions.UNSUBSCRIBE_DUPLICATES,
+ subs[0].action);
},
test_team_member: function() {
@@ -453,6 +522,7 @@
Y.Assert.areEqual('my team', subs[0].vars.team);
// And a 'duplicate_bug' variable pointing to the dupe.
Y.Assert.areEqual('dupe', subs[0].vars.duplicate_bug);
+ Y.Assert.areEqual(module._actions.NONE, subs[0].action);
},
test_team_member_multiple_bugs: function() {
@@ -480,6 +550,7 @@
// And a 'duplicate_bugs' variable with the list of dupes.
Y.ArrayAssert.itemsAreEqual(
['dupe1', 'dupe2'], subs[0].vars.duplicate_bugs);
+ Y.Assert.areEqual(module._actions.NONE, subs[0].action);
},
test_team_member_multiple: function() {
@@ -520,6 +591,8 @@
Y.Assert.areEqual('my team', subs[0].vars.team);
// And a 'duplicate_bug' variable pointing to the dupe.
Y.Assert.areEqual('dupe', subs[0].vars.duplicate_bug);
+ Y.Assert.areEqual(module._actions.UNSUBSCRIBE_DUPLICATES,
+ subs[0].action);
},
test_team_admin_multiple_bugs: function() {
@@ -548,6 +621,8 @@
// And a 'duplicate_bugs' variable with the list of dupes.
Y.ArrayAssert.itemsAreEqual(
['dupe1', 'dupe2'], subs[0].vars.duplicate_bugs);
+ Y.Assert.areEqual(module._actions.UNSUBSCRIBE_DUPLICATES,
+ subs[0].action);
},
test_team_admin_multiple: function() {
@@ -639,6 +714,7 @@
Y.Assert.areEqual(module._reasons.TEAM_SUBSCRIBED, subs[0].reason);
// And there is a 'team' variable containing the team object.
Y.Assert.areEqual('my team', subs[0].vars.team);
+ Y.Assert.areEqual(module._actions.NONE, subs[0].action);
},
test_team_member_multiple: function() {
@@ -656,6 +732,35 @@
// And there is a 'teams' variable containing all the team objects.
Y.ArrayAssert.itemsAreEqual(['team1', 'team2'],
subs[0].vars.teams);
+ Y.Assert.areEqual(module._actions.NONE, subs[0].action);
+ },
+
+ test_team_member_multiple_duplicate: function() {
+ // As with the previous test, but we need to show that each team is
+ // only represented once even if they are responsible for multiple
+ // bug tasks.
+ // We test with full-fledged objects to make sure they work with the
+ // mechanism used to find dupes.
+ var team1 = {display_name: 'team 1',
+ web_link: 'http://launchpad.net/~team1'},
+ team2 = {display_name: 'team 2',
+ web_link: 'http://launchpad.net/~team2'},
+ mock_category = {
+ count: 2,
+ personal: [],
+ as_team_member: [{ principal: team1 },
+ { principal: team2 },
+ { principal: team2 },],
+ as_team_admin: []
+ },
+ subs = module._gather_subscriptions_through_team(mock_category);
+ Y.Assert.areEqual(1, subs.length);
+ // And there is a 'teams' variable containing all the team objects.
+ var teams_found = [];
+ for (var index in subs[0].vars.teams) {
+ teams_found.push(subs[0].vars.teams[index].title);
+ }
+ Y.ArrayAssert.itemsAreEqual(['team 1', 'team 2'], teams_found);
},
test_team_admin: function() {
@@ -672,6 +777,8 @@
module._reasons.ADMIN_TEAM_SUBSCRIBED, subs[0].reason);
// And there is a 'team' variable containing the team object.
Y.Assert.areEqual('my team', subs[0].vars.team);
+ Y.Assert.areEqual(module._actions.CHANGE_TEAM_SUBSCRIPTIONS,
+ subs[0].action);
},
test_team_admin_multiple: function() {
@@ -690,6 +797,36 @@
// And there is a 'teams' variable containing all the team objects.
Y.ArrayAssert.itemsAreEqual(['team1', 'team2'],
subs[0].vars.teams);
+ Y.Assert.areEqual(module._actions.CHANGE_TEAM_SUBSCRIPTIONS,
+ subs[0].action);
+ },
+
+ test_team_admin_multiple_duplicate: function() {
+ // As with the previous test, but we need to show that each team is
+ // only represented once even if they are responsible for multiple
+ // bug tasks.
+ // We test with full-fledged objects to make sure they work with the
+ // mechanism used to find dupes.
+ var team1 = {display_name: 'team 1',
+ web_link: 'http://launchpad.net/~team1'},
+ team2 = {display_name: 'team 2',
+ web_link: 'http://launchpad.net/~team2'},
+ mock_category = {
+ count: 2,
+ personal: [],
+ as_team_admin: [{ principal: team1 },
+ { principal: team2 },
+ { principal: team2 },],
+ as_team_member: []
+ },
+ subs = module._gather_subscriptions_through_team(mock_category);
+ Y.Assert.areEqual(1, subs.length);
+ // And there is a 'teams' variable containing all the team objects.
+ var teams_found = [];
+ for (var index in subs[0].vars.teams) {
+ teams_found.push(subs[0].vars.teams[index].title);
+ }
+ Y.ArrayAssert.itemsAreEqual(['team 1', 'team 2'], teams_found);
},
test_combined: function() {
