Store workspace groups in a PAS plugin, take 2

[davisagli]

Okay, I dusted off the native-groups branch and I think this could become a 2.0 release of collective.workspace which avoids the performance problems that required the use of caching. This time the workspace groups are still kept in a separate plugin from source_users so that you can easily disable group enumeration for workspace groups if you don't want them showing up in the Groups control panel.

Since I've been using the native-groups approach for a while for my client, it has also grown some other changes and fixes, which are included here. Now would be a good time if you want to push back on any of them, so that we can make sure 2.0 will work for all of us. The full changelog is as follows.

Changed functionality

• 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. These workspace-based groups are updated automatically as workspace rosters are edited. (A separate plugin is used so that listing of workspace groups in the Plone control panel can easily be disabled.) There is an upgrade step which needs to be run to create the groups for existing workspaces.
• The Members group is no longer displayed as an option in the UI, since it is assigned automatically.
• Copying and pasting a workspace now empties the roster of the copy.

  Bugs fixed

• A change to the user of a workspace membership is now handled properly (which might need to happen, for example, when merging users).
• Membership fields can now have a defaultFactory that is context-aware (the field is now bound to the membershp before fetching the default).
• When a user is deleted from the site, their memberships in any workspaces are now also deleted.
• New workspace counters can now be added without breaking existing workspaces.

  Cleanup

• Removed code related to caching (purge_workspace_pas_cache) that is no longer used.

---

Possibly of interest to: @mattss @adamcheasley @pgrunewald @malthe @pysailor @gyst

[pysailor]

@davisagli Wow, looks like a solid piece of work. I will test this with ploneintranet soon to see if there are any problems, especially with our behavior for workspaces that turns a workspace itself into a PAS group.

Are you OK with merging #18 first and getting a release 1.3 out, before this larger change happens?

[davisagli]

@pysailor thanks for taking a look. I'm okay with work continuing on the 1.x branch, including merging #18...would you be willing to take care of that, since I'm not actively using the 1.x code myself?

[pysailor]

@davisagli 1) Sure, I'll gladly handle #18 and do a new 1.x release. This time I might even remember to push everything :) 2) d95a5b2 works nicely! 3) I found a new problem: Because of the way Products.PlonePAS handles group removal, removing a (regular) group can fail. PlonePAS iterates over all group managers in acl_users and tries to remove a group with the given id from all group managers. So in the workspace_groups plugin this naturally fails with a KeyError. I added a failing test for this.

[davisagli]

Good catch, and thanks for the test. I think we need to make sure the workspace_groups plugin doesn't get registered as a group management plugin, since we only want it to add/remove groups and group members when we call it directly from collective.workspace code. I can take care of that tomorrow.

[pysailor]

Cool, sounds good. PlonePAS seems to consider all plugins that implement IGroupManagement as group management plugins, which PlonePAS.plugins.group.GroupManager (the base class for the WorkspaceGroupManager) does.

[pgrunewald]

I have extended the catalog-based PAS plugin to provide IGroupManagement to utilize it in the group control panel and gathered some information about this topic.

If you reconsider to allow the integration in the control panel, it should suffice to implement addPrincipalToGroup and removePrincipalFromGroup with the existing user management code from collective.workspace and additionally return false for all other remaining methods from IGroupManagement (addGroup, updateGroup, setRolesForGroup and removeGroup).

Especially removeGroup() is the culprit for @pysailor's mentioned exception. It should also be made sure, that "workspace_groups" is inserted before "source_groups" within the group management plugins.

Having these things in mind, it should be working happily.

[davisagli]

@pgrunewald thanks, that's pretty helpful

[davisagli]

@pysailor I fixed the issue with removing standard Plone groups. @pgrunewald I haven't made the plugin handle managing the workspace groups from the Plone control panel, but am fine with that feature getting added if you want to make a pull request.

[pgrunewald]

@davisagli Alright! My pull request will follow, when this one is finished.

[davisagli]

@pysailor does this work for you now?

[pysailor]

@davisagli Sorry for the delay. I found 3 issues:

In some of our tests, we run into a problem in get_workspace_groups_plugin (https://github.com/collective/collective.workspace/blob/native-groups-2/src/collective/workspace/pas.py#L72) when the context is not properly acquisiton-wrapped. If plone.api is used instead, this works, since it pulls the context out of thin air if need be:

acl_users = api.portal.get_tool('acl_users')

(provided you're OK with making plone.api a dependency)

Then we have a weird failure in a test where we delete a membrane user profile, using api.user.delete(username=username). We land in this event handler: https://github.com/collective/collective.workspace/blob/native-groups-2/src/collective/workspace/workspace.py#L186 Three items are found, none of them are workspaces, so there's a 'could not adapt' TypeError. From pdb:

 /home/thomas/dev/ploneintranet/src/collective.workspace/src/collective/workspace/workspace.py(191)handle_principal_deleted()
-> for b in catalog.unrestrictedSearchResults(workspace_members=principal):
(Pdb) ws = [b.getObject() for b in catalog.unrestrictedSearchResults(workspace_members=principal)]
(Pdb) ws
[<UserProfileContainer at /plone/profiles>, <UserProfile at /plone/profiles/sync-user-0>, <UserProfile at /plone/profiles/test_user_1_>]
(Pdb) from .interfaces import IHasWorkspace
(Pdb) [IHasWorkspace.providedBy(x) for x in ws]
[False, False, False]

So I wonder why they get found by the indexer for workspace_members. Maybe just be more defensive and do something like?

workspace = IWorkspace(b._unrestrictedGetObject(), None)
if workspace:
    ...

Although I really would like to find the cause for this weird behavior. Might be some problem in our own stack.

And lastly, we have code that does this:

                if not is_admin:
                    ws.membership_factory(ws, member).groups -= {'Admins'}
                else:
                    ws.membership_factory(ws, member).groups |= {'Admins'}

Since groups is now a @property and no longer a simple attribute, this does not work any more. Does it make sense to add a simple setter for this use-case?

[davisagli]

@pysailor how are you ending up with a non-acquisition-wrapped workspace context? That sounds to me like a bug in your code, though I'm okay with adding the plone.api dependency so I'll probably go ahead and do that.

That's quite odd that the workspace_members query is returning items that don't have a workspace. The workspace_members indexer in catalog.py is registered for IHasWorkspace and returns set(IWorkspace(obj).members) so I would expect it to only put a value in the index for items with a workspace. But maybe your UserProfileContainer happens to have a workspace_members attribute, and since it doesn't provide IHasWorkspace Plone uses the default indexer (i.e. attribute lookup) and finds it. I will adjust the indexer to apply to all items and just raise AttributeError if the context doesn't provide IHasWorkspace.

And adding a setter for the groups property sounds good. I'll look into that.

Thanks again for the careful testing.

[davisagli]

@pysailor actually I don't know why that query is returning non-workspaces, because we do have a null indexer that should make sure items without IHasWorkspace do not get indexed in that index. Is it possible you've removed the index and ZCatalog is returning all items?

[davisagli]

@pysailor on second thought, I'm disinclined to add the groups setter because setting workspace membership properties directly is not recommended...all updates to membership properties should be made via the update method to make sure everything is accounted for correctly (updating groups, updating counters, calling handle_modified, and firing the TeamMemberModifiedEvent).

[davisagli]

I added a more explicit check to prevent other membership attributes from being set directly.

[pysailor]

@davisagli It's all looking quite good now. All tests are passing (when using ws.membership_factory(ws, member).update(data={'groups': modified_groups}) instead of directly trying to set the groups property in our code.)

I didn't yet check why we sometimes have non aq-wrapped context, but this might simply be due to test setup. Same for the weird effect that non-workspaces are returned for workspace_members - that also only seems to happen in tests.

[davisagli]

@pysailor btw, is there a reason you're using ws.membership_factory(ws, member) instead of ws[userid]? membership_factory was meant to be an implementation detail.

[pysailor]

Good question :) I'm just using the code that others have created before me. But I'll check out if membership_factory is indeed still necessary. Thanks for the hint.

[pgrunewald]

@davisagli: I have found an issue, that causes to find no workspace groups when searching in the Group Control Panel UI.

I found out, that the group tool, that is responsible for searching, does not have the information at hand. So whenever group.setProperties() in handle_workspace_modified() is being called, the following code block is needed:

plugin = context.acl_users.plugins.get('workspace_groups')
plugin.updateGroup(group_id, title=group_title)

Otherwise the plugin's attribute _groups won't be updated, that is necessary for search.

Update: group.updateGroup(group_id, title) is also needed in add_group().

[pgrunewald]

I also found a bug, that occurs, when you have used add_to_team in the old base and passed for the groups parameter a list and not a set. The new code base takes care of that however.

That causes later problem, when trying to access groups of the TeamMembership class:

@implementer(ITeamMembership)
class TeamMembership(object):

    @property
    def groups(self):
        groups = self.__dict__.get('groups', set()).copy()  # <--- does not work for lists
        groups -= set([u'Members'])
        return groups

In that case groups would create a AttributeError (for 'copy') and subsequently returns None.

[ale-rt]

@davisagli this seems a very nice improvement to me. Do you mind releasing a 2.0.0a1?

[davisagli]

@ale-rt just saw this; thanks for merging! I will try to review this (I might have a few more fixes from a client's project) and make a pre-release in the next few days. Feel free to ping me if I forget.

[pysailor]

@davisagli About that pre-release... ;) Since I have the permissions on pypi, I could also do it (2.0.0a1), if you're OK with it. We'd really like to test the latest improvements in the wild now.

[davisagli]

Sorry for being slow. I have one more PR which I'd like to get in (to make it possible to have roster entries that are associated with an arbitrary uuid rather than with a userid, for use cases like guests who don't have a full account). I'll prepare that today.

[davisagli]

All right @pysailor, feel free to make a prerelease! I'd probably call it a beta rather than alpha, we've been using this approach for a long time in production, but it's possible there have been some issues in rebasing changes from our client-specific branch.

[ale-rt]

Thanks you all!

[pysailor]

All right, thanks! I just released 2.0b1

[davisagli]

Thanks for doing that and for the reminder to pay some attention to this addon :)