-
Notifications
You must be signed in to change notification settings - Fork 35
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
Group Perms update for argus client #34
base: master
Are you sure you want to change the base?
Conversation
This reverts commit 9283776.
This reverts commit 6019781.
Co-authored-by: j-ma-sf <[email protected]>
Co-authored-by: j-ma-sf <[email protected]>
Thanks for the contribution! It looks like @sanjana-sfdc @snehabas is an internal user so signing the CLA is not required. However, we need to confirm this. |
:: | ||
|
||
logging.info("Looking up existing group with groupID %s", groupID) | ||
groupObj = |
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 required? Looks incomplete
params=self.get_all_req_opts.get(REQ_PARAMS, None), | ||
dataObj=self.get_all_req_opts.get(REQ_BODY, None)))) | ||
self.get_all_req_opts.get(REQ_PATH, None), | ||
params=self.get_all_req_opts.get(REQ_PARAMS, |
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.
formatting is off
if not get_all_req_opts: | ||
get_all_req_opts = {} | ||
get_all_req_opts.setdefault(REQ_PATH, "grouppermission") | ||
print("init happening") |
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.
remove
return updatedGroup | ||
|
||
def delete_permissions_for_group(self, grouppermission): | ||
deleted_permission = None |
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.
Define this closer to where it is used like near line 501
perms_to_delete = grouppermission.permissionIds | ||
if perms_to_delete == []: | ||
raise ValueError("Permission is not already assigned and hence cant be deleted") | ||
if tuple(perms_to_delete) in self._coll: |
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.
do we need this check? I think we should make the request anyway even if it is not in the collection
@@ -0,0 +1,119 @@ | |||
# |
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.
we don't need to check this file in
} | ||
groupPermission_E = { | ||
"type": "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.
You could use the variable groupPermissionIdentifier here
@snehabas I made some additional changes to your formatting PR and merged it. Could you please rebase this PR? |
No description provided.