WIP: Cleanup user vs userid

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.

src/collective/workspace/browser.py

@@ -1,21 +1,22 @@
 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
 from z3c.form.interfaces import ActionExecutionError
 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):

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))

src/collective/workspace/templates/team_roster_row.pt

@@ -5,7 +5,7 @@
       i18n:domain="collective.workspace">
 <td tal:condition="view/table_actions">
   <input type="checkbox" name="ids:list"
-         tal:attributes="value options/membership/user" />
+         tal:attributes="value options/membership/userid" />
 </td>
 <td tal:repeat="widget view/widgets/values">
     <div tal:replace="structure widget/render" />
@@ -14,10 +14,10 @@
   <tal:block tal:define="mtool nocall:context/portal_membership"
   	         tal:repeat="action view/row_actions">
   	 <a tal:condition="python:mtool.checkPermission(action.permission, context)"
-  	    tal:attributes="href string:${context/absolute_url}/${action/view_name}/${options/membership/user}"
+  	    tal:attributes="href string:${context/absolute_url}/${action/view_name}/${options/membership/userid}"
   	    tal:content="action/label"
         i18n:translate=""
   	    class="pat-modal" />&nbsp;
   </tal:block>
 </td>
-</html>
+</html>

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,18 +137,18 @@ 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(
             email='user2@example.com',
             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())

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

src/collective/workspace/vocabs.py

@@ -32,6 +32,7 @@ def TeamGroupsVocabulary(context):
         items.append(SimpleTerm(group, group, _(group)))
     return SimpleVocabulary(items)
 
+
 directlyProvides(TeamGroupsVocabulary, IVocabularyFactory)