fix: replacing access_types with access_tags adding transactions, removing direct celery calls#79
Merged
Merged
Conversation
…into user_identity_access_processing
Adding a new grant and revoke method, adding transactions scope for offboarding
…into user_identity_access_processing
… unrequired params, using the new method for access grant
…into fixes-and-refactor
…calls, adding error messages, replacing access type with access tag
…user to be added to group
Kartikkumar-Shetty
changed the title
vedharish
previously approved these changes
Feb 21, 2023
|
|
||
| def change_state(self, final_state): | ||
| user_states = dict(USER_STATUS_CHOICES) | ||
| user_states = dict(self. USER_STATUS_CHOICES) |
Collaborator
There was a problem hiding this comment.
need to run linter on this file
Contributor
Author
There was a problem hiding this comment.
yeah, will run after first round of reviews are done
| ) | ||
| new_memberships.append(member) | ||
| user_not_added.append(user.email) | ||
| if not group.needsAccessApprove: |
Contributor
There was a problem hiding this comment.
should it be "if group.needsAccessApprove:"?
niveshm
reviewed
Feb 21, 2023
| user_not_added = [] | ||
| for user in selected_users: | ||
| try: | ||
| with transaction.atomic(): |
Contributor
There was a problem hiding this comment.
shouldn't this transaction be for the loop because even if one user failed to add then revert?
Contributor
Author
There was a problem hiding this comment.
no because the transaction will remain open for too long if we do it for all users. Since for every user we are also adding access
| group_members = self.membership_group.all() | ||
| return group_members | ||
|
|
||
| def get_all_approved_members(self): |
Contributor
Author
There was a problem hiding this comment.
fixed, removed
niveshm
reviewed
Feb 21, 2023
| } | ||
| ), | ||
| ) | ||
| revoke_request(user_access_mapping = access, revoker = request.user.user) |
Contributor
There was a problem hiding this comment.
here access is an object of AccessV2, but no issues I have fixed it in my "revoke access to group" PR
larsendsouza
approved these changes
Feb 21, 2023
niveshm
approved these changes
Feb 21, 2023
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixing the instances where access types is used instead of access tags
refactoring the code to user auth_user to improve code readability
Adding transactions for the add a user to a group
refactoring the code, the request will be moved to processing by the calling function instead accept_request.
removing instances where the celery methods are called directly.