-
Notifications
You must be signed in to change notification settings - Fork 5
Feat/privilege numbers #510
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
Feat/privilege numbers #510
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.
Looks really good, just one small thing I noticed in the test data setup and one question.
Also, this is a breaking change right? We will want to note that in the PR description so @jlkravitz is aware
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/tests/__init__.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Outdated
Show resolved
Hide resolved
@jlkravitz This one is ready for your review. Thanks |
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.
Looks great! Love the new decorator- very clean. Thanks for the thorough comments.
I had one question below on the privilege ID.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
Show resolved
Hide resolved
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.
@isabeleliassen Good to merge!
Description List
Breaking change
With this PR, privileges will all be expected to include
privilegeId
in their schema, so any existing privileges (I think there are a total of 0 in CSG environments) will need updating to include one or just be deleted.Testing List
Closes #466
Closes #436