-
Notifications
You must be signed in to change notification settings - Fork 6
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!: authorization support for some (mostly "Bento Public") endpoints #529
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #529 +/- ##
===========================================
+ Coverage 88.63% 89.43% +0.80%
===========================================
Files 131 134 +3
Lines 4830 5094 +264
Branches 714 748 +34
===========================================
+ Hits 4281 4556 +275
+ Misses 395 386 -9
+ Partials 154 152 -2 ☔ View full report in Codecov by Sentry. |
test: test refactoring + testing of more discovery responses
@v-rocheleau still work to be done, but requesting an initial look at this code |
…d-scope-class refact(discovery)!: introduce container class for discovery scope
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.
Small questions, otherwise looks good!
discovery: DiscoveryConfig, | ||
request: DrfRequest, | ||
dt_permissions: DataTypeDiscoveryPermissions, | ||
queryset: QuerySet, | ||
scope_repr: str, # We pass disc |
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.
discovery
and scope_repr
arguments are derived from the same ValidatedDiscoveryScope
instance.
Might be better to replace those 2 params with a single ValidatedDiscoveryScope
param, otherwise someone could theoretically pass a discovery
and scope_repr
that don't belong together.
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 was to avoid having to deal with EmptyConfig
type in two places, since scopes can return empty configs. but yeah, i agree this also isn't the best (and the comment is truncated/unfinished)
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.
LGTM!
No description provided.