launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07011
[Merge] lp:~wallyworld/launchpad/sharing-details-delete3-966641 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/sharing-details-delete3-966641 into lp:launchpad with lp:~wallyworld/launchpad/sharing-details-delete2-966641 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #966641 in Launchpad itself: "Delete button on sharing details page does nothing"
https://bugs.launchpad.net/launchpad/+bug/966641
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/sharing-details-delete3-966641/+merge/100736
== Implementation ==
Complete the work to allow grants on bugs/branches to be revoked for a user. This branch wires up the ui so that when a successful XHR call is made to revoke access, the sharing details table is updated.
A change was made to the original sharing detail implementation. A NotFound error was raised if the Sharing Details page was requested but there were no shared artifacts. This would only have been applicable if the user url hacked since the link to the details page is not rendered on the Sharing Information page if there are no shared items. However, consider the case where a user goes to the Sharing Details page and there are shared artifacts. They delete all of them. This branch includes javascript code to display a suitable message if the XHR call to revoke removes the last shared artifact - "There are no shared bugs or branches.". If the user hits refresh, they would now get an error and this doesn't seem the best thing to do. Instead now, this branch ensures that they simply see the Sharing Details page with the above message.
== Tests ==
Add yui tests for the new sharingdetails widget functionality.
Modify the PillarSharingDetailsMixin to test for the new behaviour of showing a user friendly message if there are no shared items.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/browser/pillar.py
lib/lp/registry/browser/tests/test_pillar_sharing.py
lib/lp/registry/javascript/sharing/sharingdetails.js
lib/lp/registry/javascript/sharing/tests/test_sharingdetails.html
lib/lp/registry/javascript/sharing/tests/test_sharingdetails.js
lib/lp/registry/templates/pillar-sharing-details.pt
--
https://code.launchpad.net/~wallyworld/launchpad/sharing-details-delete3-966641/+merge/100736
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/sharing-details-delete3-966641 into lp:launchpad.
=== modified file 'lib/lp/registry/browser/pillar.py'
--- lib/lp/registry/browser/pillar.py 2012-04-05 03:42:21 +0000
+++ lib/lp/registry/browser/pillar.py 2012-04-05 03:42:22 +0000
@@ -43,10 +43,6 @@
)
from lp.bugs.interfaces.bug import IBug
from lp.code.interfaces.branch import IBranch
-from lp.registry.interfaces.accesspolicy import (
- IAccessPolicyGrantFlatSource,
- IAccessPolicySource,
- )
from lp.registry.interfaces.distributionsourcepackage import (
IDistributionSourcePackage,
)
@@ -85,11 +81,6 @@
person = getUtility(IPersonSet).getByName(name)
if person is None:
return None
- policies = getUtility(IAccessPolicySource).findByPillar([self.context])
- source = getUtility(IAccessPolicyGrantFlatSource)
- artifacts = source.findArtifactsByGrantee(person, policies)
- if artifacts.is_empty():
- return None
return PillarPerson.create(self.context, person)
=== modified file 'lib/lp/registry/browser/tests/test_pillar_sharing.py'
--- lib/lp/registry/browser/tests/test_pillar_sharing.py 2012-04-05 03:42:21 +0000
+++ lib/lp/registry/browser/tests/test_pillar_sharing.py 2012-04-05 03:42:22 +0000
@@ -16,7 +16,6 @@
Raises,
)
from zope.component import getUtility
-from zope.publisher.interfaces import NotFound
from zope.security.interfaces import Unauthorized
from zope.traversing.browser.absoluteurl import absoluteURL
@@ -103,17 +102,19 @@
browser = self.getUserBrowser(user=self.owner, url=url)
self.assertEqual(expected, browser.title)
- def test_not_found_without_sharing(self):
- # If there is no sharing between pillar and person, NotFound is the
- # result.
+ def test_no_sharing_message(self):
+ # If there is no sharing between pillar and person, a suitable message
+ # is displayed.
with FeatureFixture(DETAILS_ENABLED_FLAG):
# We have to do some fun url hacking to force the traversal a user
# encounters.
pillarperson = self.getPillarPerson(with_sharing=False)
url = 'http://launchpad.dev/%s/+sharingdetails/%s' % (
pillarperson.pillar.name, pillarperson.person.name)
- browser = self.getUserBrowser(user=self.owner)
- self.assertRaises(NotFound, browser.open, url)
+ browser = self.getUserBrowser(user=self.owner, url=url)
+ self.assertIn(
+ 'There are no shared bugs or branches.',
+ browser.contents)
def test_init_without_feature_flag(self):
# We need a feature flag to enable the view.
=== modified file 'lib/lp/registry/javascript/sharing/sharingdetails.js'
--- lib/lp/registry/javascript/sharing/sharingdetails.js 2012-04-05 03:42:21 +0000
+++ lib/lp/registry/javascript/sharing/sharingdetails.js 2012-04-05 03:42:22 +0000
@@ -23,7 +23,18 @@
SharingDetailsTable.superclass.constructor.apply(this, arguments);
}
+var clone_data = function(value) {
+ if (!Y.Lang.isArray(value)) {
+ return value;
+ }
+ return Y.JSON.parse(Y.JSON.stringify(value));
+};
+
SharingDetailsTable.ATTRS = {
+ // The duration for various animations eg row deletion.
+ anim_duration: {
+ value: 1
+ },
// The node holding the details table.
details_table_body: {
getter: function() {
@@ -43,11 +54,17 @@
},
bugs: {
- value: []
+ value: [],
+ // We clone the data passed in so external modifications do not
+ // interfere.
+ setter: clone_data
},
branches: {
- value: []
+ value: [],
+ // We clone the data passed in so external modifications do not
+ // interfere.
+ setter: clone_data
},
write_enabled: {
@@ -79,6 +96,9 @@
renderUI: function() {
var branch_data = this.get('branches');
var bug_data = this.get('bugs');
+ if (bug_data.length === 0 && branch_data.length === 0) {
+ return;
+ }
var partials = {
branch: this.get('branch_details_row_template'),
bug: this.get('bug_details_row_template')
@@ -124,6 +144,106 @@
}, 'span[id^=remove-] a');
},
+ /**
+ * The the artifact with id exists in the model, return it.
+ * @param artifact_id
+ * @param id_property
+ * @param model
+ * @return {*}
+ * @private
+ */
+ _get_artifact_from_model: function(artifact_id, id_property, model) {
+ var artifact = null;
+ Y.Array.some(model, function(item) {
+ if (item[id_property] === artifact_id) {
+ artifact = item;
+ return true;
+ }
+ return false;
+ });
+ return artifact;
+ },
+
+ syncUI: function() {
+ // Examine the widget's data model and delete and artifacts which have
+ // been removed.
+ var existing_bugs = this.get('bugs');
+ var existing_branches = this.get('branches');
+ var model_bugs = LP.cache.bugs;
+ var model_branches = LP.cache.branches;
+ var deleted_bugs = [];
+ var deleted_branches = [];
+ var self = this;
+ Y.Array.each(existing_bugs, function(bug) {
+ var model_bug =
+ self._get_artifact_from_model(
+ bug.bug_id, 'bug_id', model_bugs);
+ if (!Y.Lang.isValue(model_bug)) {
+ deleted_bugs.push(bug);
+ }
+ });
+ Y.Array.each(existing_branches, function(branch) {
+ var model_branch =
+ self._get_artifact_from_model(
+ branch.branch_id, 'branch_id', model_branches);
+ if (!Y.Lang.isValue(model_branch)) {
+ deleted_branches.push(branch);
+ }
+ });
+ if (deleted_bugs.length > 0 || deleted_branches.length > 0) {
+ this.delete_artifacts(
+ deleted_bugs, deleted_branches,
+ model_bugs.length === 0 && model_branches.length === 0);
+ }
+ this.set('bugs', model_bugs);
+ this.set('branches', model_branches);
+ },
+
+ // Delete the specified sharees from the table.
+ delete_artifacts: function(bugs, branches, all_rows_deleted) {
+ var deleted_row_selectors = [];
+ var details_table_body = this.get('details_table_body');
+ Y.Array.each(bugs, function(bug) {
+ var selector = 'tr[id=shared-bug-' + bug.bug_id + ']';
+ var table_row = details_table_body.one(selector);
+ if (Y.Lang.isValue(table_row)) {
+ deleted_row_selectors.push(selector);
+ }
+ });
+ Y.Array.each(branches, function(branch) {
+ var selector = 'tr[id=shared-branch-' + branch.branch_id + ']';
+ var table_row = details_table_body.one(selector);
+ if (Y.Lang.isValue(table_row)) {
+ deleted_row_selectors.push(selector);
+ }
+ });
+ if (deleted_row_selectors.length === 0) {
+ return;
+ }
+ var rows_to_delete = details_table_body.all(
+ deleted_row_selectors.join(','));
+ var delete_rows = function() {
+ rows_to_delete.remove(true);
+ if (all_rows_deleted === true) {
+ details_table_body
+ .appendChild('<tr></tr>')
+ .appendChild('<td colspan="4"></td>')
+ .setContent("There are no shared bugs or branches.");
+ }
+ };
+ var anim_duration = this.get('anim_duration');
+ if (anim_duration === 0 ) {
+ delete_rows();
+ return;
+ }
+ var anim = Y.lp.anim.green_flash(
+ {node: rows_to_delete, duration:anim_duration});
+ anim.on('end', function() {
+ delete_rows();
+ });
+ anim.run();
+ },
+
_table_body_template: function() {
return [
'<tbody id="sharing-table-body">',
@@ -188,5 +308,5 @@
namespace.SharingDetailsTable = SharingDetailsTable;
}, "0.1", { "requires": [
- 'node', 'event', 'lp.mustache'
+ 'node', 'event', 'json', 'lp.mustache', 'lp.anim'
] });
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_sharingdetails.html'
--- lib/lp/registry/javascript/sharing/tests/test_sharingdetails.html 2012-04-05 03:42:21 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_sharingdetails.html 2012-04-05 03:42:22 +0000
@@ -29,6 +29,10 @@
src="../../../../../../build/js/lp/app/lp.js"></script>
<script type="text/javascript"
src="../../../../../../build/js/lp/app/mustache.js"></script>
+ <script type="text/javascript"
+ src="../../../../../../build/js/lp/app/anim/anim.js"></script>
+ <script type="text/javascript"
+ src="../../../../../../build/js/lp/app/extras/extras.js"></script>
<!-- The module under test. -->
<script type="text/javascript" src="../sharingdetails.js"></script>
=== modified file 'lib/lp/registry/javascript/sharing/tests/test_sharingdetails.js'
--- lib/lp/registry/javascript/sharing/tests/test_sharingdetails.js 2012-04-05 03:42:21 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_sharingdetails.js 2012-04-05 03:42:22 +0000
@@ -55,6 +55,7 @@
overrides = {};
}
var config = Y.merge({
+ anim_duration: 0,
person_name: 'Fred',
bugs: window.LP.cache.bugs,
branches: window.LP.cache.branches,
@@ -137,6 +138,23 @@
Y.Assert.isTrue(event_fired);
},
+ // Model changes are rendered correctly when syncUI() is called.
+ test_syncUI: function() {
+ this.details_widget = this._create_Widget();
+ this.details_widget.render();
+ // We manipulate the cached model data - delete a bug.
+ var bugs = window.LP.cache.bugs;
+ // Delete the first bug.
+ bugs.splice(0, 1);
+ this.details_widget.syncUI();
+ // Check the results.
+ var bug_row_selector = '#sharing-table-body tr[id=shared-bug-2]';
+ Y.Assert.isNull(Y.one(bug_row_selector));
+ var branch_row_selector =
+ '#sharing-table-body tr[id=shared-branch-2]';
+ Y.Assert.isNotNull(Y.one(branch_row_selector));
+ },
+
// When the branch revoke link is clicked, the correct event is
// published.
test_branch_revoke_click: function() {
@@ -164,6 +182,41 @@
Y.one('#sharing-table-body span[id=remove-branch-2] a');
delete_link_to_click.simulate('click');
Y.Assert.isTrue(event_fired);
+ },
+
+ // The delete_artifacts call removes the specified bugs from the table.
+ test_delete_bugs: function() {
+ this.details_widget = this._create_Widget();
+ this.details_widget.render();
+ var row_selector = '#sharing-table-body tr[id=shared-bug-2]';
+ Y.Assert.isNotNull(Y.one(row_selector));
+ this.details_widget.delete_artifacts(
+ [window.LP.cache.bugs[0]], [], false);
+ Y.Assert.isNull(Y.one(row_selector));
+ },
+
+ // The delete_artifacts call removes the specified branches from the
+ // table.
+ test_delete_branches: function() {
+ this.details_widget = this._create_Widget();
+ this.details_widget.render();
+ var row_selector = '#sharing-table-body tr[id=shared-branch-2]';
+ Y.Assert.isNotNull(Y.one(row_selector));
+ this.details_widget.delete_artifacts(
+ [], [window.LP.cache.branches[0]], false);
+ Y.Assert.isNull(Y.one(row_selector));
+ },
+
+ // When all artifacts are deleted, a suitable message is displayed.
+ test_delete_all_artifacts: function() {
+ this.details_widget = this._create_Widget();
+ this.details_widget.render();
+ this.details_widget.delete_artifacts(
+ [window.LP.cache.bugs[0]],
+ [window.LP.cache.branches[0]], true);
+ Y.Assert.areEqual(
+ 'There are no shared bugs or branches.',
+ Y.one('#sharing-table-body tr').get('text'));
}
}));
=== modified file 'lib/lp/registry/templates/pillar-sharing-details.pt'
--- lib/lp/registry/templates/pillar-sharing-details.pt 2012-04-05 03:42:21 +0000
+++ lib/lp/registry/templates/pillar-sharing-details.pt 2012-04-05 03:42:22 +0000
@@ -50,7 +50,13 @@
</th>
</tr>
</thead>
- <tbody id="sharing-table-body"></tbody>
+ <tbody id="sharing-table-body">
+ <tr>
+ <td colspan="4">
+ There are no shared bugs or branches.
+ </td>
+ </tr>
+ </tbody>
</table>
</div>
