Merge lp:~intellectronica/launchpad/bug-531963 into lp:launchpad

[Eleanor Berger]

When changing bug status, require confirmation from users who are not in the project team.

This branch adds a confirmation step when a user who has no privileges in a project tries to edit the bug status. It does that by creating a specialised version of the ChoiceEdit widget that displays a javascript confirm dialog before saving if a new parameter is set. The only gotcha here is the use of confirmAnswer in the windmill test. This is how we simulate confirming (or cancelling). Windmill overrides all calls to modal dialogs in order to avoid blocking execution and uses the value in confirmAnswer instead. This value can't be set from python directly, so we use execJS to inject into the JS environment.

[Abel Deuring (adeuring)]

Hi Tom,

the code is fine, but "make lint" has a few complaints. Could you fix them?

review: Approve (code)

[Eleanor Berger (intellectronica)]

Michael, thanks for taking on this UI review. You can see a screenshot at http://launchpadlibrarian.net/41602788/status-confirmation-screenshot.png or if you want to test it yourself on launchpad.dev, go to a bug logged in a no-priv and try to change a status. This is a pretty simple implementation. I would have liked to do this using a LAZR widget, but the work for implementing this is too much, and this should solve the immediate problem and only happen infrequently.

[Michael Nelson (michael.nelson)]

> Michael, thanks for taking on this UI review. You can see a screenshot at
> http://launchpadlibrarian.net/41602788/status-confirmation-screenshot.png or
> if you want to test it yourself on launchpad.dev, go to a bug logged in a no-
> priv and try to change a status. This is a pretty simple implementation. I
> would have liked to do this using a LAZR widget, but the work for implementing
> this is too much, and this should solve the immediate problem and only happen
> infrequently.

Hi Tom,

I agree that it would have been nice to use a LAZR widget, but understand that it's not within scope right now. I do however think it's still worth hiding the choice list before bringing up the confirmation dialog, and don't see why you say you'd need to update the widget to do so. Here's a (crude) diff that gives the behaviour that I mentioned:

http://pastebin.ubuntu.com/399274/

Other than that, I just mentioned in our conversation the addition of "whether" to the sentence (check with mrevell if you want a second opinion).

Thanks!

IRC conversation:
13:01 < noodles775> intellectronica: why is it that if I'm logged in as cprov (<email address hidden>:cprov) I'm unable to modify the bug status (the mouse-over tells me "Changeable only by a project maintainer or a bug supervisor"), yet I can as no-privs? Is no-privs a bug supervisor without privs in the project?
[snip]
13:07 < intellectronica> noodles775: that's because that bug is assigned to a bugwatch
13:07 < noodles775> Ah ok. Thanks. Two more things:
13:08 < noodles775> First, I think the wording might be missing "whether"? eg. "If you're not certain whether changing the status of this bug is correct,..." What you you think?
13:09 < noodles775> And second, how difficult would it be to first close the overlay before popping up the confirmation?
13:09 < intellectronica> noodles775: as a non-native speaker i'll have to trust you on that. both some equally good to me
13:09 < noodles775> I think that's quite important (a popup within a popup is really bad).
13:09 < intellectronica> noodles775: very difficult
13:09 < noodles775> :(
13:09 < noodles775> intellectronica: couldn't in just be hidden with CSS when you bring up the confirmation?
13:10 < noodles775> (ie. you shouldn't need to change the behaviour of the widget?)
13:10 < intellectronica> that would be a great solution, and even better one would be to handle it within the widget itself. unfortunately both require a complete restructuring of that widget which we can't afford right now
13:11 < intellectronica> noodles775: the problem is the way the widget interacts with the web service. it saves before closing, and changing that would require changing the widget, and quite possibly changing other widgets that use the api patch plugin

review: Approve (ui)

=== modified file 'lib/canonical/launchpad/javascript/bugs/bugtask-index.js'
--- lib/canonical/launchpad/javascript/bugs/bugtask-index.js	2010-03-03 00:38:38 +0000
+++ lib/canonical/launchpad/javascript/bugs/bugtask-index.js	2010-03-23 10:47:49 +0000
@@ -1325,13 +1325,14 @@
         }
 
         if (conf.user_can_edit_status) {
-            var status_choice_edit = new Y.ChoiceSource({
+            var status_choice_edit = new BugStatusEdit({
                 contentBox: status_content,
                 value: conf.status_value,
                 title: 'Change status to',
                 items: conf.status_widget_items,
                 elementToFlash: status_content.get('parentNode'),
-                backgroundColor: tr.hasClass('highlight') ? '#FFFF99' : '#FFFFFF'
+                backgroundColor: tr.hasClass('highlight') ? '#FFFF99' : '#FFFFFF',
+                requires_confirmation: conf.status_requires_confirmation
             });
             status_choice_edit.showError = function(err) {
               Y.lp.display_error(null, err);
@@ -1634,6 +1635,80 @@
     }
 });
 
+var BugStatusEdit = function() {
+    BugStatusEdit.superclass.constructor.apply(this, arguments);
+}
+
+BugStatusEdit.NAME = 'bugstatusedit';
+BugStatusEdit.NS = 'bugstatusedit';
+
+BugStatusEdit.ATTRS = {
+    requires_confirmation: {
+        value: false
+    }
+};
+
+Y.extend(BugStatusEdit, Y.ChoiceSource, {
+    initializer: function(cfg) {
+        BugStatusEdit.superclass.initializer.apply(this, arguments);
+    },
+
+    onClick: function(e) {
+
+        // Only continue if the down button is the left one.
+        if (e.button != 1) {
+            return;
+        }
+
+        this._choice_list = new Y.ChoiceList({
+            value:          this.get("value"),
+            title:          this.get("title"),
+            items:          this.get("items"),
+            value_location: this.get("value_location"),
+            progressbar:    false
+        });
+
+        var that = this;
+        this._choice_list.on("valueChosen", function(e) {
+            if (that.get("requires_confirmation")) {
+                // Ensure that the choicelist is hidden before the
+                // confirmation box appears. Note: there must be
+                // a better way to get the choice list?
+                cl = Y.get("table.yui-ichoicelist-focused");
+                cl.replaceClass(
+                    'yui-ichoicelist-focused', 'yui-ichoicelist-hidden');
+                var confirmed = confirm([
+                    'Change bug status?',
+                    '\n\n',
+                    'Bug status is normally set by the project ',
+                    'team, based on the current state of the work ',
+                    'involved in diagnosing and fixing the bug.',
+                    '\n\n',
+                    'If you\'re not certain whether changing the status of ',
+                    'this bug is correct, you can cancel and instead ',
+                    'contact the project team.'].join(''));
+            } else {
+                var confirmed = true;
+            }
+            if (confirmed) {
+                that._chosen_value = e.details[0];
+                that._saveData(e.details[0]);
+            } else {
+                return false;
+            }
+        });
+
+        // Stuff the mouse coordinates into the list object,
+        // by the time we'll need them, they won't be available.
+        this._choice_list._mouseX = e.clientX + window.pageXOffset;
+        this._choice_list._mouseY = e.clientY + window.pageYOffset;
+
+        this._choice_list.render();
+
+        e.halt();
+    }
+});
+
 /*
  * Check if the current user can unsubscribe the person
  * being subscribed.
@@ -1861,4 +1936,4 @@
                       "substitute", "widget-position-ext", "lazr.formoverlay",
                       "lazr.anim", "lazr.base", "lazr.overlay",
                       "lazr.choiceedit", "lp.picker", "lp.client.plugins",
-                      "lp.subscriber", "lp.errors"]});
+                      "lp.subscriber", "lp.errors", "bugs.status_edit"]});

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2010-03-15 17:59:47 +0000
+++ lib/lp/bugs/browser/bugtask.py	2010-03-23 10:47:49 +0000
@@ -3437,6 +3437,7 @@
                                 None),
             'user_can_edit_milestone': self.user_can_edit_milestone,
             'user_can_edit_status': not self.context.bugwatch,
+            'status_requires_confirmation': not self.user_can_edit_importance,
             'user_can_edit_importance': (
                 self.user_can_edit_importance and
                 not self.context.bugwatch)})

=== added file 'lib/lp/bugs/windmill/tests/test_bug_status.py'
--- lib/lp/bugs/windmill/tests/test_bug_status.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/windmill/tests/test_bug_status.py	2010-03-23 10:47:49 +0000
@@ -0,0 +1,69 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test for the bug status entry."""
+
+__metaclass__ = type
+__all__ = []
+
+import unittest
+
+from canonical.launchpad.windmill.testing import constants, lpuser
+from lp.bugs.windmill.testing import BugsWindmillLayer
+from lp.testing import WindmillTestCase
+
+class TestBugStatusConfirmation(WindmillTestCase):
+
+    layer = BugsWindmillLayer
+    suite_name = "Bug status confirmation step test"
+
+    def test_bug_status_confirmation(self):
+        """Test the confirmation step of the bug status entry."""
+        client = self.client
+
+        # Open a bug page and wait for it to finish loading
+        client.open(url=u'http://bugs.launchpad.dev:8085/bugs/4')
+        client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
+        # NO_PRIV must confirm editting the bug status in this project.
+        lpuser.NO_PRIV.ensure_login(client)
+        client.waits.forElement(
+            xpath=u"//div[@class='status-content yui-bugstatusedit-content']//img[@class='editicon']",
+            timeout=constants.FOR_ELEMENT)
+
+        # Initially, the bug status is New.
+        client.asserts.assertText(
+            xpath=u"//div[contains(@class, 'status-content')]//a[1]",
+            validator=u"New")
+        client.click(
+            xpath=u"//div[contains(@class, 'status-content')]//img[@class='editicon']")
+        client.waits.forElement(
+            id=u'yui-pretty-overlay-modal', timeout=constants.FOR_ELEMENT)
+        # We open the status editor and change the status to In Progress.
+        # When a confirmation dialog is shown, we will choose to cancel.
+        client.commands.execJS(code=u"windmill.confirmAnswer = false")
+        client.click(
+            xpath=u"//div[@id='yui-pretty-overlay-modal']"
+                  u"//a[contains(@href, '#In Progress')]")
+        # The status didn't change.
+        client.asserts.assertText(
+            xpath=u"//div[contains(@class, 'status-content')]//a[1]",
+            validator=u"New")
+
+        # We repeat the same sequence.
+        client.click(
+            xpath=u"//div[contains(@class, 'status-content')]//img[@class='editicon']")
+        client.waits.forElement(
+            id=u'yui-pretty-overlay-modal', timeout=constants.FOR_ELEMENT)
+        # This time, when the confirmation dialog appears, we will confirm.
+        client.commands.execJS(code=u"windmill.confirmAnswer = true")
+        client.click(
+            xpath=u"//div[@id='yui-pretty-overlay-modal']"
+                  u"//a[contains(@href, '#In Progress')]")
+        # The status changed to In Progress.
+        client.asserts.assertText(
+            xpath=u"//div[contains(@class, 'status-content')]//a[1]",
+            validator=u"In Progress")
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)