-
-
Notifications
You must be signed in to change notification settings - Fork 144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use client groups for local role sharing #2301
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exciting as usual! Left some comments
"""Client group ID | ||
""" | ||
if not hasattr(self, self.GROUP_KEY): | ||
setattr(self, self.GROUP_KEY, self.getClientID() or self.getId()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to do not rely on getClientID
, but getId
only:
setattr(self, self.GROUP_KEY, '{}-group'.format(self.getId())
Reasoning is that the (internal) id of the client object will never change, while the value of ClientID
can be changed anytime, as long as it remains unique. Thus, ClientID
of two different clients could be shifted in future. This could cause confusion and inconsistencies as well. Since the group id is not "visible", but it's name only, it does not need to be human-friendly.
On another note, why to store the group_id as an attribute. Why to not simply do
return self.getClientID() or self.getId()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ClientID
as the Group ID was chosen because it makes more sense to the people and allows to search for the client ID in the groups panel. The getId
fallback was mainly because of the tests, where we do not provide the ClientID
on sample creation.
Said that, the stored attribute handles exactly that use case where the Client ID changed afterwards.
I agree that the getId
would be better, but having the people in mind who are using it, I decided to go with the ClientID
.
src/bika/lims/content/client.py
Outdated
portal_groups = api.get_tool("portal_groups") | ||
# Use the client ID for the group ID | ||
group_id = self.getClientID() | ||
portal_groups.removeGroup(group_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rely on the existing property self.group_id
:
portal_groups.removeGroup(self.group_id)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Fixed in 5e0bb01.
I also remove the attribute there.
""" | ||
group = self.get_group() | ||
if not group: | ||
group = self.create_group() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? Why to not return False
directly if there is no group?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically I want to ensure a group if it was either deleted, or not created due to the registry setting. Remember, the group has the global Client
role and the local Owner
role on the current client object.
<!-- for="*" --> | ||
<!-- provides="senaite.core.interfaces.IDynamicLocalRoles" --> | ||
<!-- factory=".localroles.ClientAwareLocalRoles" --> | ||
<!-- name="senaite.core.adapter.localroles.clientaware" /> --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done a quick search (extensions included) and ClientAwareLocalRoles
is not used anywhere else. I suggest to remove (instead of comment) this adapter declaration, as well as the adapter itself (senaite.core.adapters.localroles.ClientAwareLocalRoles
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in d953958
Description of the issue/feature this PR addresses
This PR changes how users that are linked to a client contact get the local
Owner
role in the following way:Owner
role assigned on their clientClient
role assignedCurrent behavior before PR
Owner
role is looked up dynamically for each user that is linked to a client contactDesired behavior after PR is merged
Owner
is granted by group membership of the client groupAnalyst
,Sampler
or any other custom role.--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.