-
Notifications
You must be signed in to change notification settings - Fork 72
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
PROD-1869 - Support config for pre-response webhooks #4795
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
1c919b6
to
7aa417f
Compare
Passing run #7341 ↗︎
Details:
Review all test suite changes for PR #4795 ↗︎ |
Starting review! |
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.
@eastandwestwind before I dig into your endpoints, passing it back early to get the relationships set up and the endpoints registered -
tests/ops/api/v1/endpoints/test_pre_approval_webhook_endpoints.py
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4795 +/- ##
==========================================
+ Coverage 86.27% 86.65% +0.38%
==========================================
Files 339 342 +3
Lines 20100 20223 +123
Branches 2587 2596 +9
==========================================
+ Hits 17341 17524 +183
+ Misses 2293 2225 -68
- Partials 466 474 +8 ☔ View full report in Codecov by Sentry. |
Starting review! |
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.
@eastandwestwind endpoints looking good, mostly just some trivial copy/paste issues.
The main thing I see missing are model-level tests (like tests/ops/models/test_connectionconfig.py). They don't need to be large, because you're testing some of this through your endpoints, but I'm particularly looking for tests that create a PreApprovalWebhook and PreApprovalWebhookReply and assert their relationships on both sides. For example, assert PreApprovalWebhook.connection_config and ConnectionConfig.pre_approval_webhooks are the types you expect, same for PreApprovalWebhookReplay and the privacy request relationship.
src/fides/api/api/v1/endpoints/pre_approval_webhook_endpoints.py
Outdated
Show resolved
Hide resolved
src/fides/api/api/v1/endpoints/pre_approval_webhook_endpoints.py
Outdated
Show resolved
Hide resolved
src/fides/api/api/v1/endpoints/pre_approval_webhook_endpoints.py
Outdated
Show resolved
Hide resolved
src/fides/api/api/v1/endpoints/pre_approval_webhook_endpoints.py
Outdated
Show resolved
Hide resolved
"PreApprovalWebhook", | ||
back_populates="connection_config", | ||
cascade="delete", | ||
uselist=False, |
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.
I think we should remove uselist=False
because a connection config can have multiple pre approval webhooks right? If uselist=False, this will just return one object.
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.
Model-level tests are good for flushing these out -
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.
Ahhh I see, that makes sense! I'll remove this
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 means I likely should add uselist=False
to the privacy_request
field on my PreApprovalWebhookReply
api model:
privacy_request = relationship(
"PrivacyRequest", back_populates="pre_approval_webhook_reply", uselist=False # type: ignore
)
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.
great @eastandwestwind thank you for taking the time to write these nice model tests. My last question is about the naming of the privacy request <> pre approval webhook reply relationship because it maybe should be plural and represented in the test
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.
Great work @eastandwestwind
@pattisdr have you seen these codecov/path CI check failures before? Is this something I'll need to address via more unit tests? |
Yes, I don't always aim for 100%, depending on the circumstance, you can look at the failures and see if there are any lines you think should be covered - I just try to be reasonable with test coverage - |
Closes https://ethyca.atlassian.net/browse/PROD-1869
Description Of Changes
Backend infra to support future pre-response hook functionality.
This PR:
Code Changes
Steps to Confirm
pre_approval_webhook_endpoints.py
, but if you wish to manually test, confirm all the new endpoints are successful./api/v1/connection
with:/api/v1/dsr/webhook/pre_approval
with:/api/v1/dsr/webhook/pre_approval
should return this item/api/v1/dsr/webhook/pre_approval/my_webhook
should also return this item/api/v1/dsr/webhook/pre_approval/my_webhook
with, for example,{"name": "Some other name"}
/api/v1/dsr/webhook/pre_approval/my_webhook
should reflect this change/api/v1/dsr/webhook/pre_approval/my_webhook
should be successful, and GET/api/v1/dsr/webhook/pre_approval
should not return that webook at all.Pre-Merge Checklist
CHANGELOG.md