Launchpad itself

Merge lp:~edwin-grubbs/launchpad/bug-568390-cyclic-membership-error into lp:launchpad

  1. bug-568390-cyclic-membership-error
  2. Merge into devel
Proposed by Edwin Grubbs
Status: Merged
Approved by: Paul Hummer
Approved revision: no longer in the source branch.
Merged at revision: 11110
Proposed branch: lp:~edwin-grubbs/launchpad/bug-568390-cyclic-membership-error
Merge into: lp:launchpad
Diff against target: 157 lines (+115/-7)
3 files modified
lib/lp/registry/browser/objectreassignment.py (+5/-6)
lib/lp/registry/browser/person.py (+27/-1)
lib/lp/registry/browser/tests/team-reassignment-view.txt (+83/-0)
To merge this branch: bzr merge lp:~edwin-grubbs/launchpad/bug-568390-cyclic-membership-error
Reviewer Review Type Date Requested Status
Paul Hummer (community) code Approve
Review via email: mp+29407@code.launchpad.net

Description of the change

Summary
-------

The TeamReassignmentView changes the owner of a team and then adds the
new owner as a member of that team. The view now disipays an error if
this creates a cyclical team membership, so that the model won't raise
an exception.

Implementation details
----------------------

The ObjectReassignmentView.isValidOwner() was not very flexible, since
it shouldn't be used to set a custom error message, so I changed it to
validateOwner(). This gets called at the end of
ObjectReassignmentView.validate(), after a new team has been created, if
the user is not using an existing team. isValidOwner() does not appear
to be used in any of the other subclasses of ObjectReassignmentView.

    lib/lp/registry/browser/objectreassignment.py
    lib/lp/registry/browser/person.py
    lib/lp/registry/browser/tests/team-reassignment-view.txt

Tests
-----

./bin/test -vv -t team-reassignment-view.txt

Demo and Q/A
------------

* Open https://launchpad.dev/people/+newteam
  * Create three new teams.
  * Add teams as members, so A is a member of B is a member of C.
  * Open https://launchpad.dev/~A/+reassign
    * Try to make C the owner of A.
    * It should display an error that this will cause a cyclical team
      membership.

To post a comment you must log in.
Revision history for this message
Paul Hummer (rockstar) wrote :

<rockstar> EdwinGrubbs, maybe validateOwner's abstract method should return False or raise NotImplementedError.
<EdwinGrubbs> rockstar: it's not required that validateOwner() be overridden. The vocabulary normally does a good enough job. This is the only subclass that actually does override it.
<EdwinGrubbs> so raising NotImplementedError isn't necessary, and returning false would be meaningless.
<rockstar> EdwinGrubbs, pass seems like it's very unopinionated
<EdwinGrubbs> rockstar: it's no different than LaunchpadFormView.validate().
<rockstar> EdwinGrubbs, okay.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/browser/objectreassignment.py'
2--- lib/lp/registry/browser/objectreassignment.py 2010-02-16 20:36:48 +0000
3+++ lib/lp/registry/browser/objectreassignment.py 2010-07-09 01:44:44 +0000
4@@ -111,13 +111,13 @@
5 if callable(self.callback):
6 self.callback(self.context, oldOwner, newOwner)
7
8- def isValidOwner(self, newOwner):
9+ def validateOwner(self, new_owner):
10 """Check whether the new owner is acceptable for the context object.
11
12- If it's not acceptable, return False and assign an error message to
13- self.errormessage to inform the user.
14+ If it's not acceptable, display an error by calling:
15+ self.setFieldError(self.ownerOrMaintainerName, 'some error info')
16 """
17- return True
18+ pass
19
20 def _validate(self, action, data):
21 """Override _validate() method."""
22@@ -179,5 +179,4 @@
23 owner = personset.newTeam(
24 self.user, owner_name, owner_name.capitalize())
25
26- if not self.isValidOwner(owner):
27- self.setFieldError(self.ownerOrMaintainerName, 'Invalid owner.')
28+ self.validateOwner(owner)
29
30=== modified file 'lib/lp/registry/browser/person.py'
31--- lib/lp/registry/browser/person.py 2010-07-08 19:29:05 +0000
32+++ lib/lp/registry/browser/person.py 2010-07-09 01:44:44 +0000
33@@ -4780,9 +4780,35 @@
34 schema = ITeamReassignment
35
36 def __init__(self, context, request):
37- ObjectReassignmentView.__init__(self, context, request)
38+ super(TeamReassignmentView, self).__init__(context, request)
39 self.callback = self._addOwnerAsMember
40
41+ def validateOwner(self, new_owner):
42+ """Display error if the owner is not valid.
43+
44+ Called by ObjectReassignmentView.validate().
45+ """
46+ if self.context.inTeam(new_owner):
47+ path = self.context.findPathToTeam(new_owner)
48+ if len(path) == 1:
49+ relationship = 'a direct member'
50+ path_string = ''
51+ else:
52+ relationship = 'an indirect member'
53+ full_path = [self.context] + path
54+ path_string = '(%s)' % '&rArr;'.join(
55+ team.displayname for team in full_path)
56+ error = structured(
57+ 'Circular team memberships are not allowed. '
58+ '%(new)s cannot be the new team owner, since %(context)s '
59+ 'is %(relationship)s of %(new)s. '
60+ '<span style="white-space: nowrap">%(path)s</span>'
61+ % dict(new=new_owner.displayname,
62+ context=self.context.displayname,
63+ relationship=relationship,
64+ path=path_string))
65+ self.setFieldError(self.ownerOrMaintainerName, error)
66+
67 @property
68 def contextName(self):
69 return self.context.displayname
70
71=== added file 'lib/lp/registry/browser/tests/team-reassignment-view.txt'
72--- lib/lp/registry/browser/tests/team-reassignment-view.txt 1970-01-01 00:00:00 +0000
73+++ lib/lp/registry/browser/tests/team-reassignment-view.txt 2010-07-09 01:44:44 +0000
74@@ -0,0 +1,83 @@
75+Team Reassignment
76+=================
77+
78+There are many restrictions for setting the owner of a team.
79+
80+
81+Existing Team Error
82+-------------------
83+
84+The ObjectReassignmentView displays radio buttons that give you the
85+option to create a team as opposed to using an existing team. If the
86+user tries to create a new team with the same name as an existing team,
87+an error is displayed.
88+
89+ >>> from lp.registry.interfaces.person import IPersonSet
90+ >>> person_set = getUtility(IPersonSet)
91+ >>> operator = person_set.getByName('name12')
92+ >>> login_person(operator)
93+ >>> a_team = factory.makeTeam(
94+ ... owner=operator, name='a-team', displayname='A-Team')
95+ >>> b_team = factory.makeTeam(
96+ ... owner=operator, name='b-team', displayname='B-Team')
97+ >>> c_team = factory.makeTeam(
98+ ... owner=operator, name='c-team', displayname='C-Team')
99+ >>> a_team.addMember(b_team, operator)
100+ (True...
101+ >>> b_team.addMember(c_team, operator)
102+ (True...
103+ >>> transaction.commit()
104+
105+ >>> view = create_initialized_view(
106+ ... c_team, '+reassign', principal=operator)
107+ >>> print list(w.name for w in view.widgets)
108+ ['field.owner', 'field.existing']
109+
110+ >>> form = {
111+ ... 'field.owner': 'a-team',
112+ ... 'field.existing': 'new',
113+ ... 'field.actions.change': 'Change',
114+ ... }
115+ >>> view = create_initialized_view(
116+ ... a_team, '+reassign', form=form, principal=operator)
117+ >>> print view.errors
118+ [u"There's already a person/team with the name 'a-team' in Launchpad.
119+ Please choose a different name or select the option to make that
120+ person/team the new owner, if that's what you want."]
121+
122+
123+Cyclical Team Membership Error
124+------------------------------
125+
126+When a person or team becomes the owner of another team, they are also
127+added as a member. Team memberships cannot be cyclical; therefore, the
128+team can't have its owner be a team of which it is a direct or indirect
129+member.
130+
131+ >>> form = {
132+ ... 'field.owner': 'b-team',
133+ ... 'field.existing': 'existing',
134+ ... 'field.actions.change': 'Change',
135+ ... }
136+ >>> view = create_initialized_view(
137+ ... c_team, '+reassign', form=form, principal=operator)
138+ >>> print view.widget_errors
139+ {'owner': u'Circular team memberships are not allowed.
140+ B-Team cannot be the new team owner, since C-Team is a direct member
141+ of B-Team. <...><...>'}
142+
143+If there is an indirect membership between the teams, the path between
144+the teams is displayed so that the user has a better idea how to resolve
145+the issue.
146+
147+ >>> form = {
148+ ... 'field.owner': 'a-team',
149+ ... 'field.existing': 'existing',
150+ ... 'field.actions.change': 'Change',
151+ ... }
152+ >>> view = create_initialized_view(
153+ ... c_team, '+reassign', form=form, principal=operator)
154+ >>> print view.widget_errors
155+ {'owner': u'Circular team memberships are not allowed.
156+ A-Team cannot be the new team owner, since C-Team is an
157+ indirect member of A-Team. <...>(C-Team&rArr;B-Team&rArr;A-Team)<...>'}