Merge lp:~sinzui/launchpad/unchanged-bug-supervisor into lp:launchpad

[Curtis Hovey]

This is my branch to not raise a bug_supervisor field error for a no-change
event.

    lp:~sinzui/launchpad/unchanged-bug-supervisor
    Diff size: 113
    Launchpad bug:
          https://bugs.launchpad.net/bugs/605491
    Test command: ./bin/test -vv \
          -t test_bug_role_non_admin_can_edit
    Pre-implementation: deryck
    Target release: 10.08

Do not raise a bug_supervisor field error for a no-change event
----------------------------------------------------------------

I'm trying to change the bug guidelines for a project in which the supervisor
is set to a team and I'm a member for that team. Unfortunately, when a click
on 'change' I get an error because I'm trying to set supervisor to a team for
which I don't have administrator rights.

However, the bug supervisor is already set to that value, I just try to change
the bug guidelines. Is there any way to do so? Thanks.

This is a bug 1. there should be no error because there is no change. 2. in
the case of a project, the owner (any member of the owning team) must have
permission to set all fields on the project.

After talking with the bug team, the field permissions will be revised so that
they are the same, and any one who is an owner can make the change, but that
is a separate bug that requires send email. This bug is about the fact the
bug_supervisor field was not changed, there should be no error. This issue
probably affect security_contact too.

Rules
-----

    * Do not raise admin/owner errors for bug_supervisor and security_contact
      if they have not changed. This just affects the +configure-bugs.

QA
--

    * As an member of an owning team, set a team as the project bug
      supervisor using Configure bug tracker.
    * As another member of the owning team, and no an admin of the bug
      supervisor, change the bug reporting guidelines.
    * Verify that the form is saved.

Lint
----

Linting changed files:
  lib/lp/bugs/browser/bugrole.py
  lib/lp/bugs/browser/bugtarget.py
  lib/lp/bugs/browser/tests/test_bugtarget_configure.py

Test
----

    * lib/lp/bugs/browser/tests/test_bugtarget_configure.py
      * Added a test to verify a project owner who is not an admin of the
        bug_supervisor or security_contact teams can change other data in
        the form.

Implementation
--------------

    * lib/lp/bugs/browser/bugrole.py
      * Revised the validators to consider the current state of the field
        and return OK if the state has not changed. Add changeSecurityContact
        since the rule is more complex now.
    * lib/lp/bugs/browser/bugtarget.py
      * Updated the change_action to remove the security_contact from data
        as is done with bug_supervisor to avoid permission constraints.

[Robert Collins (lifeless)]

review: Approve

=== modified file 'lib/lp/bugs/browser/bugrole.py'
--- lib/lp/bugs/browser/bugrole.py	2010-06-11 21:51:48 +0000
+++ lib/lp/bugs/browser/bugrole.py	2010-07-15 14:25:59 +0000
@@ -20,7 +20,7 @@
     OTHER_TEAM = object()
     OK = object()
 
-    def _getFieldState(self, field_name, data):
+    def _getFieldState(self, current_role, field_name, data):
         """Return the enum that summarises the field state."""
         # The field_name will not be in the data if the user did not enter
         # a person in the ValidPersonOrTeam vocabulary.
@@ -28,6 +28,10 @@
             return self.INVALID_PERSON
         role = data[field_name]
         user = self.user
+        # If no data was changed, the field is OK regardless of who the
+        # current user is.
+        if current_role == role:
+            return self.OK
         # The user may assign the role to None, himself, or a team he admins.
         if role is None or self.context.userCanAlterSubscription(role, user):
             return self.OK
@@ -43,7 +47,8 @@
         Verify that the value is None, the user, or a team he administers,
         otherwise, set a field error.
         """
-        field_state = self._getFieldState('bug_supervisor', data)
+        field_state = self._getFieldState(
+            self.context.bug_supervisor, 'bug_supervisor', data)
         if field_state is self.INVALID_PERSON:
             error = (
                 'You must choose a valid person or team to be the '
@@ -78,7 +83,12 @@
         self.setFieldError('bug_supervisor', error)
 
     def changeBugSupervisor(self, bug_supervisor):
-        self.context.setBugSupervisor(bug_supervisor, self.user)
+        if self.context.bug_supervisor != bug_supervisor:
+            self.context.setBugSupervisor(bug_supervisor, self.user)
+
+    def changeSecurityContact(self, security_contact):
+        if self.context.security_contact != security_contact:
+            self.context.security_contact = security_contact
 
     def validateSecurityContact(self, data):
         """Validates the new security contact.
@@ -86,7 +96,8 @@
         Verify that the value is None, the user, or a team he administers,
         otherwise, set a field error.
         """
-        field_state = self._getFieldState('security_contact', data)
+        field_state = self._getFieldState(
+            self.context.security_contact, 'security_contact', data)
         if field_state is self.INVALID_PERSON:
             error = (
                 'You must choose a valid person or team to be the '

=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py	2010-06-18 00:46:17 +0000
+++ lib/lp/bugs/browser/bugtarget.py	2010-07-15 14:25:59 +0000
@@ -167,11 +167,13 @@
 
     @action("Change", name='change')
     def change_action(self, action, data):
-        # bug_supervisor requires a transition method, so it must be
-        # handled separately and removed for the updateContextFromData
-        # to work as expected.
+        # bug_supervisor and security_contactrequires a transition method,
+        # so it must be handled separately and removed for the
+        # updateContextFromData to work as expected.
         self.changeBugSupervisor(data['bug_supervisor'])
         del data['bug_supervisor']
+        self.changeSecurityContact(data['security_contact'])
+        del data['security_contact']
         self.updateContextFromData(data)
 
 

=== modified file 'lib/lp/bugs/browser/tests/test_bugtarget_configure.py'
--- lib/lp/bugs/browser/tests/test_bugtarget_configure.py	2010-06-18 00:46:17 +0000
+++ lib/lp/bugs/browser/tests/test_bugtarget_configure.py	2010-07-15 14:25:59 +0000
@@ -122,6 +122,31 @@
         self.assertEqual([], view.errors)
         self.assertFalse(self.product.enable_bug_expiration)
 
+    def test_bug_role_non_admin_can_edit(self):
+        # Verify that a member of an owning team who is not an admin of
+        # the bug supervisor team or security_contact team can change bug
+        # reporting guidelines.
+        owning_team = self.factory.makeTeam(owner=self.owner)
+        bug_team = self.factory.makeTeam(owner=self.owner)
+        weak_owner = self.factory.makePerson()
+        login_person(self.owner)
+        owning_team.addMember(weak_owner, self.owner)
+        bug_team.addMember(weak_owner, self.owner)
+        self.product.owner = owning_team
+        self.product.setBugSupervisor(bug_team, self.owner)
+        self.product.security_contact = bug_team
+        login_person(weak_owner)
+        form = self._makeForm()
+        # Only the bug_reporting_guidelines are different.
+        form['field.bug_supervisor'] = bug_team.name
+        form['field.security_contact'] = bug_team.name
+        form['field.bug_reporting_guidelines'] = 'new guidelines'
+        view = create_initialized_view(
+            self.product, name='+configure-bugtracker', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(
+            'new guidelines', self.product.bug_reporting_guidelines)
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)