From 5e50c1a7e3df568ee09f902d4b1536f08059779e Mon Sep 17 00:00:00 2001 From: ale-rt Date: Mon, 26 Feb 2018 12:58:29 +0100 Subject: [PATCH] Cleanup --- CHANGES.rst | 3 + src/collective/workspace/browser.py | 7 +- src/collective/workspace/membership.py | 23 +++--- .../workspace/templates/team_roster_row.pt | 6 +- .../workspace/tests/test_workspace.py | 43 +++++------ .../workspace/tests/workspace.robot | 4 +- src/collective/workspace/vocabs.py | 1 + src/collective/workspace/workspace.py | 74 +++++++++---------- 8 files changed, 81 insertions(+), 80 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index c59518a..58901da 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -6,6 +6,9 @@ Changelog Changed functionality: +- Before version 2.0, in many parts of the code, a variable called user + was actually storing the userid string. + This is not happening anymore. - The `workspace_groups` PAS plugin now stores groups in the same way as normal Plone groups, rather than doing catalog queries to find workspaces. This performs much better even without enabling caching for the plugin. diff --git a/src/collective/workspace/browser.py b/src/collective/workspace/browser.py index f7e6746..eb3f25d 100644 --- a/src/collective/workspace/browser.py +++ b/src/collective/workspace/browser.py @@ -1,14 +1,14 @@ from AccessControl import getSecurityManager -from Products.statusmessages.interfaces import IStatusMessage +from collections import namedtuple from collective.workspace.interfaces import _ from collective.workspace.interfaces import IRosterView from collective.workspace.interfaces import IWorkspace -from collections import namedtuple from plone import api from plone.autoform.base import AutoFields from plone.autoform.form import AutoExtensibleForm from plone.z3cform import z2 from Products.Five.browser.pagetemplatefile import ViewPageTemplateFile +from Products.statusmessages.interfaces import IStatusMessage from z3c.form import button from z3c.form.form import DisplayForm from z3c.form.form import EditForm @@ -16,6 +16,7 @@ from zope.cachedescriptors.property import Lazy as lazy_property from zope.interface import implementer from zope.publisher.interfaces.browser import IPublishTraverse + import transaction @@ -100,7 +101,7 @@ def updateFields(self): super(TeamMemberEditForm, self).updateFields() # don't show the user field if we are editing if self.user_id: - del self.fields['user'] + del self.fields['userid'] @lazy_property def ignoreContext(self): diff --git a/src/collective/workspace/membership.py b/src/collective/workspace/membership.py index ea0da16..8374cc3 100644 --- a/src/collective/workspace/membership.py +++ b/src/collective/workspace/membership.py @@ -3,8 +3,8 @@ from collective.workspace.events import TeamMemberRemovedEvent from collective.workspace.interfaces import _ from collective.workspace.interfaces import IWorkspace -from collective.workspace.pas import get_workspace_groups_plugin from collective.workspace.pas import add_group +from collective.workspace.pas import get_workspace_groups_plugin from collective.workspace.vocabs import UsersSource from copy import deepcopy from plone.autoform import directives as form @@ -22,8 +22,8 @@ class ITeamMembership(model.Schema): """Schema for one person's membership in a team.""" - form.widget(user=AutocompleteFieldWidget) - user = schema.Choice( + form.widget(userid=AutocompleteFieldWidget) + userid = schema.Choice( title=_(u'User'), source=UsersSource, ) @@ -85,16 +85,19 @@ def _update_groups(self, old_groups, new_groups, add_members=True): for group_name in (new_groups - old_groups): group_id = '{}:{}'.format(group_name.encode('utf8'), uid) try: - workspace_groups.addPrincipalToGroup(self.user, group_id) + workspace_groups.addPrincipalToGroup(self.userid, group_id) except KeyError: # group doesn't exist title = '{}: {}'.format( group_name.encode('utf8'), context.Title()) add_group(group_id, title) - workspace_groups.addPrincipalToGroup(self.user, group_id) + workspace_groups.addPrincipalToGroup(self.userid, group_id) for group_name in (old_groups - new_groups): group_id = '{}:{}'.format(group_name.encode('utf8'), uid) try: - workspace_groups.removePrincipalFromGroup(self.user, group_id) + workspace_groups.removePrincipalFromGroup( + self.userid, + group_id, + ) except KeyError: # group doesn't exist pass @@ -107,7 +110,7 @@ def groups(self): def update(self, data): old = self.__dict__.copy() user_changed = False - if 'user' in data and old['user'] != data['user']: + if 'userid' in data and old['userid'] != data['userid']: # User is changing, so remove the old user from groups. user_changed = True self._update_groups(old['groups'], set()) @@ -117,10 +120,10 @@ def update(self, data): workspace = self.workspace if user_changed: # User changed; remove old entry in _team - del workspace.context._team[old['user']] + del workspace.context._team[old['userid']] # Add new user to groups self._update_groups(set(), self.groups) - workspace.context._team[self.user] = self.__dict__ + workspace.context._team[self.userid] = self.__dict__ # update counters for name, func in workspace.counters: @@ -159,7 +162,7 @@ def remove_from_team(self): for name, func in workspace.counters: if func(self.__dict__): workspace.context._counters[name].change(-1) - del self.workspace.members[self.user] + del self.workspace.members[self.userid] self._update_groups(self.groups, set(), add_members=False) self.handle_removed() notify(TeamMemberRemovedEvent(self.workspace.context, self)) diff --git a/src/collective/workspace/templates/team_roster_row.pt b/src/collective/workspace/templates/team_roster_row.pt index 21d38f1..4731548 100644 --- a/src/collective/workspace/templates/team_roster_row.pt +++ b/src/collective/workspace/templates/team_roster_row.pt @@ -5,7 +5,7 @@ i18n:domain="collective.workspace"> + tal:attributes="value options/membership/userid" />
@@ -14,10 +14,10 @@   - \ No newline at end of file + diff --git a/src/collective/workspace/tests/test_workspace.py b/src/collective/workspace/tests/test_workspace.py index cbc67f3..8d22a31 100644 --- a/src/collective/workspace/tests/test_workspace.py +++ b/src/collective/workspace/tests/test_workspace.py @@ -1,12 +1,11 @@ -import unittest - +# coding=utf-8 +from collective.workspace.interfaces import IWorkspace +from collective.workspace.testing import COLLECTIVE_WORKSPACE_INTEGRATION_TESTING # noqa: E501 from plone import api -from plone.testing import z2 from plone.app.testing import SITE_OWNER_NAME +from plone.testing import z2 -from collective.workspace.testing import \ - COLLECTIVE_WORKSPACE_INTEGRATION_TESTING -from collective.workspace.interfaces import IWorkspace +import unittest class TestWorkspace(unittest.TestCase): @@ -49,14 +48,12 @@ def test_removing_workspace_removes_groups(self): self.assertIsNone(group) def test_add_to_team(self): - self.ws.add_to_team( - user=self.user1.getId() - ) + self.ws.add_to_team(self.user1.getId()) self.assertIn(self.user1.getId(), list(self.ws.members)) def test_adding_team_member_updates_groups(self): self.ws.add_to_team( - user=self.user1.getId(), + self.user1.getId(), groups=(u'Admins',), ) self.assertIn( @@ -72,7 +69,7 @@ def test_adding_team_member_updates_groups(self): def test_updating_team_member_updates_groups(self): self.ws.add_to_team( - user=self.user1.getId() + self.user1.getId() ) self.ws[self.user1.getId()].update({'groups': set([u'Admins'])}) self.assertIn( @@ -88,7 +85,7 @@ def test_updating_team_member_updates_groups(self): def test_direct_set_of_membership_property_is_blocked(self): self.ws.add_to_team( - user=self.user1.getId() + self.user1.getId() ) try: self.ws[self.user1.getId()].position = u'Tester' @@ -103,7 +100,7 @@ def test_direct_set_of_membership_property_is_blocked(self): def test_local_role_team_member(self): self.ws.add_to_team( - user=self.user1.getId() + self.user1.getId() ) pmt = api.portal.get_tool('portal_membership') member = pmt.getMemberById(self.user1.getId()) @@ -112,20 +109,20 @@ def test_local_role_team_member(self): def test_remove_from_team(self): self.ws.add_to_team( - user=self.user1.getId() + self.user1.getId() ) self.ws.remove_from_team( - user=self.user1.getId() + self.user1.getId() ) self.assertNotIn(self.user1.getId(), list(self.ws.members)) def test_removing_team_member_updates_groups(self): self.ws.add_to_team( - user=self.user1.getId(), + self.user1.getId(), groups=(u'Admins',), ) self.ws.remove_from_team( - user=self.user1.getId() + self.user1.getId() ) self.assertNotIn( self.user1.getId(), @@ -140,7 +137,7 @@ def test_removing_team_member_updates_groups(self): def test_reparent_team_member(self): self.ws.add_to_team( - user=self.user1.getId(), + self.user1.getId(), groups=(u'Admins',), ) user2 = api.user.create( @@ -148,10 +145,10 @@ def test_reparent_team_member(self): username='user2', password='123' ) - self.ws[self.user1.getId()].update({'user': user2.getId()}) + self.ws[self.user1.getId()].update({'userid': user2.getId()}) self.assertNotIn(self.user1.getId(), self.workspace._team) self.assertIn(user2.getId(), self.workspace._team) - self.assertEqual(self.ws[user2.getId()].user, user2.getId()) + self.assertEqual(self.ws[user2.getId()].userid, user2.getId()) self.assertNotIn( self.user1.getId(), self.portal.portal_groups.getGroupMembers( @@ -165,7 +162,7 @@ def test_reparent_team_member(self): def test_removing_user_removes_workspace_memberships(self): userid = self.user1.getId() - self.ws.add_to_team(user=userid) + self.ws.add_to_team(userid) self.assertIn(userid, self.ws.members) api.user.delete(userid) self.assertNotIn(userid, self.ws.members) @@ -186,13 +183,13 @@ def test_copying_workspace_clears_roster(self): def test_add_guest_to_team(self): self.ws.add_to_team( - user=self.user1.getId(), groups=['Guests'] + self.user1.getId(), groups=['Guests'] ) self.assertIn(self.user1.getId(), list(self.ws.members)) def test_guest_has_no_team_member_role(self): self.ws.add_to_team( - user=self.user1.getId(), groups=['Guests'] + self.user1.getId(), groups=['Guests'] ) pmt = api.portal.get_tool('portal_membership') member = pmt.getMemberById(self.user1.getId()) diff --git a/src/collective/workspace/tests/workspace.robot b/src/collective/workspace/tests/workspace.robot index 7fa82cf..28f1686 100644 --- a/src/collective/workspace/tests/workspace.robot +++ b/src/collective/workspace/tests/workspace.robot @@ -40,13 +40,13 @@ a test workspace the test user is added to the roster Click link Roster Click Overlay Link workspace-add-user - Input text form-widgets-user-widgets-query test + Input text form-widgets-userid-widgets-query test Wait until page contains test_user_1_ Click element jquery=li:contains('test_user_1_') Click button Save the test user appears in the roster - Page should contain element css=a[href$="edit-roster/test_user_1_"] + Page should contain element css=a[href$="edit-roster/test_user_1_"] the test user can view the workspace Log in as test user diff --git a/src/collective/workspace/vocabs.py b/src/collective/workspace/vocabs.py index 2972fda..745ef7c 100644 --- a/src/collective/workspace/vocabs.py +++ b/src/collective/workspace/vocabs.py @@ -32,6 +32,7 @@ def TeamGroupsVocabulary(context): items.append(SimpleTerm(group, group, _(group))) return SimpleVocabulary(items) + directlyProvides(TeamGroupsVocabulary, IVocabularyFactory) diff --git a/src/collective/workspace/workspace.py b/src/collective/workspace/workspace.py index 47b216c..f6c8a92 100644 --- a/src/collective/workspace/workspace.py +++ b/src/collective/workspace/workspace.py @@ -1,21 +1,20 @@ -from BTrees.Length import Length -from BTrees.OOBTree import OOBTree -from Products.PluggableAuthService.interfaces.events import \ - IPrincipalDeletedEvent from .events import TeamMemberAddedEvent from .interfaces import IHasWorkspace from .interfaces import IWorkspace from .membership import ITeamMembership from .membership import TeamMembership -from .pas import get_workspace_groups_plugin from .pas import add_group +from .pas import get_workspace_groups_plugin +from BTrees.Length import Length +from BTrees.OOBTree import OOBTree from plone import api +from Products.PluggableAuthService.interfaces.events import IPrincipalDeletedEvent # noqa: E501 from zope.component import adapter from zope.container.interfaces import IObjectAddedEvent from zope.container.interfaces import IObjectRemovedEvent -from zope.lifecycleevent.interfaces import IObjectModifiedEvent -from zope.lifecycleevent.interfaces import IObjectCopiedEvent from zope.event import notify +from zope.lifecycleevent.interfaces import IObjectCopiedEvent +from zope.lifecycleevent.interfaces import IObjectModifiedEvent class Workspace(object): @@ -84,56 +83,53 @@ def __iter__(self): for userid in self.context._team.iterkeys(): yield self[userid] - def add_to_team(self, user, groups=None, **kw): + def add_to_team(self, userid, groups=None, **kw): """ Makes sure a user is in this workspace's team. - :param user: The id of the user to add to this workspace - :type user: str + :param userid: The id of the user to add to this workspace + :type userid: str :param groups: The set of workspace groups to add this user to :type groups: set :param kw: Pass user and any other attributes that should be set on the team member. :type kw: dict """ - # TODO: user argument should be renamed to userid for clarity - # however doing so now would break backwards compatibility data = kw.copy() - data['user'] = user - if groups is not None: - data['groups'] = groups = set(groups) + data['userid'] = userid + data['groups'] = groups = set(groups or []) members = self.members - if user not in self.members: - if groups is None: - data['groups'] = groups = set() - members[user] = data - for name, func in self.counters: - if func(data): - if name not in self.context._counters: - self.context._counters[name] = Length() - self.context._counters[name].change(1) - membership = self.membership_factory(self, data) - membership.handle_added() - membership._update_groups(set(), groups) - notify(TeamMemberAddedEvent(self.context, membership)) - self.context.reindexObject( - idxs=['workspace_members', 'workspace_leaders'] - ) - else: - membership = self.membership_factory(self, self.members[user]) + + if userid in members: + membership = self.membership_factory(self, members[userid]) membership.update(data) + return membership + + members[userid] = data + + for name, func in self.counters: + if func(data): + if name not in self.context._counters: + self.context._counters[name] = Length() + self.context._counters[name].change(1) + + membership = self.membership_factory(self, data) + membership.handle_added() + membership._update_groups(set(), groups) + notify(TeamMemberAddedEvent(self.context, membership)) + self.context.reindexObject( + idxs=['workspace_members', 'workspace_leaders'] + ) return membership - def remove_from_team(self, user): + def remove_from_team(self, userid): """ Remove a user from the workspace - :param user: The id of the user to remove from this workspace - :type user: str + :param userid: The id of the user to remove from this workspace + :type userid: str """ - # TODO: user argument should be renamed to userid for clarity - # however doing so now would break backwards compatibility - membership = self.get(user) + membership = self.get(userid) if membership is not None: membership.remove_from_team() return membership