-
Notifications
You must be signed in to change notification settings - Fork 1
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: add studio write access validation #55
Conversation
9835cff
to
5f2911e
Compare
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.
needs a changelog entry and update of version number, probably to 3.5.0
thoughts about organization you can take or leave
also it might need the decorator that actually requires and decodes a user on the view calls
582c157
to
3bb68e0
Compare
💹 done |
3bb68e0
to
d797c7a
Compare
28fd65e
to
89798e4
Compare
* Add SessionAuthentication * Add JwtAuthentication
89798e4
to
9cfec12
Compare
can_change_summaries_settings.return_value = False | ||
self.access_mock = patch('ai_aside.platform_imports.can_change_summaries_settings', | ||
can_change_summaries_settings) | ||
self.access_mock.start() |
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.
this is a weird python resource leak technicality
in super.setup it makes self.accessmock and starts it, let's call that mock1
then here you reassign self.accessmock to a new mock and start it, that's mock2
there is no longer any reference available to mock1, self.accessmock is now mock2
in teardown we tear down mock2. mock1 is never torn down.
I don't think that mocks really leak anything significant especially if you never start them so this is just something to notice. Fixing it would just be not calling super setup or delegating mock creation into a function which reads self.can_change_summaries or a million other methods.
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.
this is worth knowing about but not a blocker to merge I think
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!
Add studio write access validation
Ticket: https://jira.2u.com/browse/ACADEMIC-16359
Merge checklist:
Check off if complete or not applicable: