search

collective / collective.workspace

Restricted workspaces for Plone
7 stars 11 forks source link

Fixes for the _update_groups method #28

Closed ale-rt closed 6 years ago

ale-rt commented 6 years ago

This PR adds some test coverage for the _update_groups method and fixes its behavior. For example, it was possible to have a user both in the Members and in the Guests groups, given that the condition was only checking on the existent groups. Also it does not remove the Members group when calling this method with add_auto_groups=False.

Closes #27

ale-rt commented 6 years ago

@davisagli thanks for the review! I am probably not deep enough in the code to understand the implications and starting from #27 I probably did too much :p Let me know if you have further questions or if my comments are based on false assumptions.

ale-rt commented 6 years ago

Anyway I think I managed to accept all the comments.

Just for a reference the old branch is here https://github.com/collective/collective.workspace/tree/27-fix-update-groups-original.

Ah! I will also add a line in the changelog :)

davisagli commented 6 years ago

I'm going to take your work a step further and try to clean up how groups are handled. Will post here soon for you to review...

davisagli commented 6 years ago

@ale-rt here are the additional changes I think we want: https://github.com/collective/collective.workspace/compare/27-fix-update-groups...27-fix-update-groups-davisagli?expand=1

Basically I made a separate function for removing from all groups, so that the purpose of _update_groups is more focused and we don't need the add_auto_groups option any more.

ale-rt commented 6 years ago

@davisagli I merged your changes in #30

ale-rt commented 6 years ago

@davisagli I think we can merge this one now, right?

davisagli commented 6 years ago

@ale-rt yeah, go for it. Sorry, I thought I already said you can go ahead and do that but I must not have been clear.

ale-rt commented 6 years ago

@davisagli you were clear, it is just me that I wanted to be extra sure :) @pysailor can we release a new version?

pysailor commented 6 years ago

@ale-rt Sure! I just released https://pypi.org/project/collective.workspace/2.0b3/