launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00113
[Merge] lp:~sinzui/launchpad/unchanged-bug-supervisor into lp:launchpad/devel
Curtis Hovey has proposed merging lp:~sinzui/launchpad/unchanged-bug-supervisor into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#605491 project owner cannot set bug fields because bug_supervisor has different permission
https://bugs.launchpad.net/bugs/605491
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.
--
https://code.launchpad.net/~sinzui/launchpad/unchanged-bug-supervisor/+merge/29998
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/unchanged-bug-supervisor into lp:launchpad/devel.
=== 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__)
