launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06221
[Merge] lp:~wallyworld/launchpad/confirm-reviewer-subscription-330290 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/confirm-reviewer-subscription-330290 into lp:launchpad with lp:~wallyworld/launchpad/person-getBranchVisibilityInfo-api as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/confirm-reviewer-subscription-330290/+merge/90070
== Implementation ==
Basically just a bunch of new javascript in module 'lp.code.branchmergeproposal.nominate'. I also moved some javascript from the branch-register-merge.pt TAL to the new module.
The javascript provides a new picker validation callback - when a reviewer is chosen, the validator makes an API call to see if the target branch and source branch are visible to the nominated reviewer. If not, the user is prompted to confirm the selected reviewer.
== Demo and QA ==
The screenshot shows what the confirmation prompt in the picker looks like:
http://people.canonical.com/~ianb/reviewer-confirm.png
== Tests ==
Add some new yui tests for the new javascript module:
test_branchmergeproposal_nominate.js
test_branchmergeproposal_nominate.html
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/code/javascript/branchmergeproposal.nominate.js
lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.html
lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js
lib/lp/code/templates/branch-register-merge.pt
--
https://code.launchpad.net/~wallyworld/launchpad/confirm-reviewer-subscription-330290/+merge/90070
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/confirm-reviewer-subscription-330290 into lp:launchpad.
=== added file 'lib/lp/code/javascript/branchmergeproposal.nominate.js'
--- lib/lp/code/javascript/branchmergeproposal.nominate.js 1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/branchmergeproposal.nominate.js 2012-02-01 13:14:20 +0000
@@ -0,0 +1,121 @@
+/** Copyright (c) 2012, Canonical Ltd. All rights reserved.
+ *
+ * Code for handling the update of the branch merge proposals.
+ *
+ * @module lp.code.branchmergeproposal.nominate
+ */
+
+YUI.add('lp.code.branchmergeproposal.nominate', function(Y) {
+
+var namespace = Y.namespace('lp.code.branchmergeproposal.nominate');
+
+var lp_client;
+
+var confirm_reviewer = function(branches_to_check, branch_info, picker,
+ save_fn, cancel_fn) {
+ var visible_branches = branch_info.visible_branches;
+ if (Y.Lang.isArray(visible_branches)
+ && visible_branches.length !== branches_to_check.length) {
+ var invisible_branches = branches_to_check.filter(function(i) {
+ return visible_branches.indexOf(i) < 0;
+ });
+ var person = Y.Escape.html(branch_info.person_name);
+ var yesno_content_template = [
+ "<p class='large-warning' style='padding:12px 2px 0 36px;'>",
+ "{person_name} does not currently have permission to view ",
+ "branches:",
+ "<ul style='margin-left: 50px'></ul></p>",
+ "<p>If you proceed, {person_name} will be subscribed to the ",
+ "branches.</p>",
+ "<p>Do you really want to nominate them as a reviewer?</p>"
+ ].join('');
+ var yesno_content = Y.Lang.substitute(
+ yesno_content_template, {person_name: person});
+ var yn = Y.Node.create(yesno_content);
+ invisible_branches.forEach(function(branch) {
+ yn.one('ul').appendChild(
+ Y.Node.create('<li></li>').set('text', branch));
+ });
+ Y.lp.app.picker.yesno_save_confirmation(
+ picker, yn, "Nominate", "Choose Again",
+ save_fn, cancel_fn);
+ } else {
+ if (Y.Lang.isFunction(save_fn)) {
+ save_fn();
+ }
+ }
+};
+
+var check_reviewer_can_see_branches = function(picker, value, save_fn,
+ cancel_fn) {
+ if (value === null || !Y.Lang.isValue(value.api_uri)) {
+ if (Y.Lang.isFunction(save_fn)) {
+ save_fn();
+ return;
+ }
+ }
+
+ var reviewer_uri = Y.lp.client.normalize_uri(value.api_uri);
+ reviewer_uri = Y.lp.client.get_absolute_uri(reviewer_uri);
+ var error_handler = new Y.lp.client.ErrorHandler();
+ error_handler.showError = function(error_msg) {
+ picker.set('error', error_msg);
+ };
+
+ var branches_to_check = [LP.cache.context.unique_name];
+ var target_name = Y.DOM.byId('field.target_branch.target_branch').value;
+ if (Y.Lang.trim(target_name) !== '') {
+ branches_to_check.push(target_name);
+ }
+ var confirm = function(branch_info) {
+ namespace.confirm_reviewer(
+ branches_to_check, branch_info, picker, save_fn, cancel_fn);
+ };
+ var y_config = {
+ on: {
+ success: confirm,
+ failure: error_handler.getFailureHandler()
+ },
+ parameters: {
+ person: reviewer_uri,
+ branch_names: branches_to_check
+ }
+ };
+ lp_client.named_get("/branches", "getBranchVisibilityInfo", y_config);
+};
+
+var setup_reviewer_confirmation = function() {
+ var validation_namespace = YUI.namespace('Env.lp.app.picker.validation');
+ var widget_id = 'show-widget-field-reviewer';
+ validation_namespace[widget_id]= check_reviewer_can_see_branches;
+};
+
+// For testing
+namespace.setup_reviewer_confirmation = setup_reviewer_confirmation;
+namespace.check_reviewer_can_see_branches = check_reviewer_can_see_branches;
+namespace.confirm_reviewer = confirm_reviewer;
+
+// We want to disable the review_type field if no reviewer is
+// specified. In such cases, the reviewer will be set by the back end
+// to be the default for the target branch and the review type will be None.
+var reviewer_changed = function(value) {
+ var reviewer = Y.Lang.trim(value);
+ var review_type = Y.DOM.byId('field.review_type');
+ review_type.disabled = (reviewer === '');
+};
+
+namespace.setup = function(conf) {
+ lp_client = new Y.lp.client.Launchpad(conf);
+ Y.on('blur',
+ function(e) {
+ reviewer_changed(e.target.get('value'));
+ },
+ Y.DOM.byId('field.reviewer'));
+ var f = Y.DOM.byId('field.reviewer');
+ reviewer_changed(f.value);
+
+ setup_reviewer_confirmation();
+};
+
+}, "0.1", {"requires": ["io", "substitute", "escape", "dom", "node", "event",
+ "lp.client", "lp.app.picker"]});
=== added file 'lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.html'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.html 1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.html 2012-02-01 13:14:20 +0000
@@ -0,0 +1,58 @@
+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd";>
+<html>
+ <head>
+ <title>Branch Merge Proposal Nominate</title>
+
+ <!-- YUI and test setup -->
+ <script type="text/javascript"
+ src="../../../../canonical/launchpad/icing/yui/yui/yui.js">
+ </script>
+ <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
+ <script type="text/javascript"
+ src="../../../app/javascript/testing/testrunner.js"></script>
+ <script type="text/javascript"
+ src="../../../app/javascript/testing/mockio.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/client.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/lp.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/activator/activator.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/anim/anim.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/confirmationoverlay/confirmationoverlay.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/choiceedit/choiceedit.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/effects/effects.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/expander.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/extras/extras.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/formoverlay/formoverlay.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/inlineedit/editor.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/lazr/lazr.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/overlay/overlay.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/picker/picker.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/picker/picker_patcher.js"></script>
+ <script type="text/javascript" src="../../../app/javascript/picker/person_picker.js"></script>
+
+
+ <!-- expected variable -->
+ <script type="text/javascript">
+ var LP = {
+ cache: {},
+ links: {}
+ };
+ </script>
+
+ <!-- The module under test -->
+ <script type="text/javascript" src="../branchmergeproposal.nominate.js"></script>
+
+ <!-- The test suite -->
+ <script type="text/javascript" src="test_branchmergeproposal.nominate.js"></script>
+
+</head>
+<body class="yui3-skin-sam">
+ <div id="fixture"></div>
+ <script type="text/x-template" id="form-template">
+ <form>
+ <input type="text" name="field.target_branch.target_branch" id="field.target_branch.target_branch" value=""/>
+ <input type="text" name="field.reviewer" id="field.reviewer" value=""/>
+ <input type="text" name="field.review_type" id="field.review_type" value="" disabled="disabled"/>
+ </form>
+ </script>
+</body>
+</html>
=== added file 'lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js 1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.nominate.js 2012-02-01 13:14:20 +0000
@@ -0,0 +1,190 @@
+/* Copyright 2012 Canonical Ltd. This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * Tests for lp.code.branchmergeproposal.nominate.
+ *
+ */
+
+YUI().use('lp.testing.runner', 'test', 'dump', 'console', 'node',
+ 'lp.testing.mockio', 'event', 'node-event-simulate',
+ 'lp.code.branchmergeproposal.nominate', function(Y) {
+
+var suite = new Y.Test.Suite("BranchMergeProposal Nominate Tests");
+var module = Y.lp.code.branchmergeproposal.nominate;
+
+/*
+ * Tests for when a reviewer is nominated for a mp and we check that they
+ * can see the source and target branches.
+ *
+ */
+
+suite.add(new Y.Test.Case({
+
+ name: 'branchmergeproposal-nominate-tests',
+
+ setUp: function() {
+ this.fixture = Y.one('#fixture');
+ var form = Y.Node.create(Y.one('#form-template').getContent());
+ this.fixture.appendChild(form);
+ this.mockio = new Y.lp.testing.mockio.MockIo();
+ window.LP = {
+ links: {me : "/~user"},
+ cache: {
+ context: {
+ unique_name: 'b2'
+ }
+ }
+ };
+ module.setup({io_provider: this.mockio});
+ },
+
+ tearDown: function() {
+ if (this.fixture !== null) {
+ this.fixture.empty();
+ }
+ delete this.fixture;
+ delete this.mockio;
+ delete window.LP;
+ },
+
+ // The module setup function works as expected.
+ test_setup: function() {
+ var validation_namespace =
+ YUI.namespace('Env.lp.app.picker.validation');
+ var widget_id = 'show-widget-field-reviewer';
+ Y.Assert.areEqual(
+ module.check_reviewer_can_see_branches,
+ validation_namespace[widget_id]);
+ var review_type = Y.DOM.byId('field.review_type');
+ Y.Assert.isTrue(review_type.disabled);
+ },
+
+ // The review type field is correctly disabled if there is no reviewer.
+ test_review_type_enable: function() {
+ var reviewer = Y.one("[name='field.reviewer']");
+ var review_type = Y.one("[name='field.review_type']");
+ Y.Assert.isTrue(
+ review_type.get('disabled'),
+ 'review type should be disabled');
+ reviewer.set('value', 'someone');
+ reviewer.simulate('blur');
+ Y.Assert.isFalse(
+ review_type.get('disabled'),
+ 'review type should be enabled');
+ reviewer.set('value', '');
+ reviewer.simulate('blur');
+ Y.Assert.isTrue(
+ review_type.get('disabled'),
+ 'review type should now be disabled');
+ },
+
+ // The check_reviewer function works as expected.
+ test_check_reviewer_can_see_branches: function() {
+ var orig_confirm_reviewer = module.confirm_reviewer;
+ var dummy_picker = {};
+ var confirm_reviewer_called = false;
+ module.confirm_reviewer = function(
+ branches_to_check, branch_info, picker, save_fn, cancel_fn) {
+ Y.Assert.areEqual(dummy_picker, picker);
+ Y.Assert.areEqual('Fred', branch_info.person_name);
+ Y.Assert.areEqual('b2', branch_info.visible_branches[0]);
+ Y.Assert.areEqual('b2', branches_to_check[0]);
+ Y.Assert.areEqual('b1', branches_to_check[1]);
+ confirm_reviewer_called = true;
+ };
+ var selected_value = {
+ api_uri: '~fred'
+ };
+ var target_branch_field =
+ Y.DOM.byId('field.target_branch.target_branch');
+ target_branch_field.value = 'b1';
+ var save_fn = function() {
+
+ };
+ module.check_reviewer_can_see_branches(
+ dummy_picker, selected_value, save_fn);
+ this.mockio.success({
+ responseText:
+ '{"person_name": "Fred", "visible_branches": ["b2"]}',
+ responseHeaders: {'Content-Type': 'application/json'}});
+ module.confirm_reviewer = orig_confirm_reviewer;
+ // Check the parameters passed to the io call.
+ Y.Assert.areEqual(
+ '/api/devel/branches', this.mockio.last_request.url);
+ var reviewer_uri = Y.lp.client.normalize_uri(
+ selected_value.api_uri);
+ reviewer_uri = encodeURIComponent(
+ Y.lp.client.get_absolute_uri(reviewer_uri));
+ Y.Assert.areEqual(
+ 'ws.op=getBranchVisibilityInfo&person=' +
+ reviewer_uri + '&branch_names=b2&branch_names=b1',
+ this.mockio.last_request.config.data);
+ Y.Assert.isTrue(confirm_reviewer_called);
+ },
+
+ // Invoke the validation callback with the specified visible branches.
+ // The branches to check is always ['b1', 'b2'] and the person name is
+ // always 'Fred'. We are checking the correct behaviour depending on what
+ // visible branches are passed in.
+ _invoke_confirm_reviewer: function(visible_branches) {
+ var orig_yesyno = Y.lp.app.picker.yesno_save_confirmation;
+ var dummy_picker = {};
+ var yesno_called = false;
+ Y.lp.app.picker.yesno_save_confirmation = function(
+ picker, content, yes_label, no_label, yes_fn, no_fn) {
+ Y.Assert.areEqual('Nominate', yes_label);
+ Y.Assert.areEqual('Choose Again', no_label);
+ Y.Assert.areEqual(dummy_picker, picker);
+ var message = content.get('text');
+ Y.Assert.isTrue(message.indexOf('Fred') >= 0);
+ var invisible_branches = ['b1', 'b2'].filter(function(i) {
+ return visible_branches.indexOf(i) < 0;
+ });
+ invisible_branches.forEach(function(branch_name) {
+ Y.Assert.isTrue(message.indexOf(branch_name) > 0);
+ });
+ yesno_called = true;
+ };
+ var branch_info = {
+ person_name: 'Fred',
+ visible_branches: visible_branches
+ };
+ var save_fn_called = false;
+ var save_fn = function() {
+ save_fn_called = true;
+ };
+ module.confirm_reviewer(
+ ['b1', 'b2'], branch_info, dummy_picker, save_fn);
+ Y.lp.app.picker.yesno_save_confirmation = orig_yesyno;
+ return {
+ save_called: save_fn_called,
+ yesno_called: yesno_called
+ };
+ },
+
+ // Test the validation callback with all branches being visible.
+ test_confirm_reviewer_all_branches_visible: function() {
+ var result = this._invoke_confirm_reviewer(['b1', 'b2']);
+ Y.Assert.isTrue(result.save_called);
+ Y.Assert.isFalse(result.yesno_called);
+ },
+
+ // Test the validation callback with no branches being visible.
+ test_confirm_reviewer_no_branches_visible: function() {
+ var result = this._invoke_confirm_reviewer([]);
+ Y.Assert.isFalse(result.save_called);
+ Y.Assert.isTrue(result.yesno_called);
+ },
+
+ // Test the validation callback with some branches being visible.
+ test_confirm_reviewer_some_branches_visible: function() {
+ var result = this._invoke_confirm_reviewer(['b1']);
+ Y.Assert.isFalse(result.save_called);
+ Y.Assert.isTrue(result.yesno_called);
+ }
+}));
+
+
+Y.lp.testing.Runner.run(suite);
+
+});
=== modified file 'lib/lp/code/templates/branch-register-merge.pt'
--- lib/lp/code/templates/branch-register-merge.pt 2012-01-19 22:14:01 +0000
+++ lib/lp/code/templates/branch-register-merge.pt 2012-02-01 13:14:20 +0000
@@ -58,32 +58,15 @@
</metal:formbody>
</div>
- <!-- We want to disable the review_type field if no reviewer is
- specified. In such cases, the reviewer will be set by the back end
- to be the default for the target branch and the review type will be
- None.
- -->
<tal:script>
- <script type="text/javascript" tal:content="string:
- YUI().use('node', 'event', function(Y) {
- function reviewer_changed(value) {
- reviewer = Y.Lang.trim(value);
- review_type = document.getElementById('field.review_type');
- review_type.disabled = (reviewer == '');
- };
- Y.on('blur',
- function(e) {
- reviewer_changed(e.target.get('value'))
- },
- document.getElementById('field.reviewer'));
- Y.on('load',
- function(e) {
- f = document.getElementById('field.reviewer')
- reviewer_changed(f.value);
+ <script type="text/javascript">
+ YUI().use('lp.code.branchmergeproposal.nominate', function(Y) {
+ Y.on('domready',
+ function(e) {
+ Y.lp.code.branchmergeproposal.nominate.setup();
},
window);
- });"
- >
+ });
</script>
</tal:script>
