Skip to content
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

Add connection key to execution log #2100

Merged
merged 12 commits into from
Jan 4, 2023
Merged

Conversation

sanders41
Copy link
Contributor

@sanders41 sanders41 commented Dec 21, 2022

Closes #1725

Code Changes

  • Added a connection_key field to the execution log table
  • Added connection_key to the ExecutionAndAuditLogResponse
  • write_execution_log includes the connection_key

Steps to Confirm

  • Create an invalid Mailchimp connector (or any other saas connector)
  • Create a privacy request
  • Verify that the privacy request fails
  • Check the execution log response for the privacy request using verbose mode and verify that it contains the connection_key for the failed saas connector

Pre-Merge Checklist

Description Of Changes

Write some things here about the changes and any potential caveats

@adamsachs adamsachs self-requested a review January 3, 2023 20:43
Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, this generally looks good to me, the only thing that i think may be missing is support of the connection_key field making it back onto the serialized response on the GET /api/v1/privacy-request endpoint, as seems to be suggested in the issue's AC.

i think it should be a pretty minor update, namely adding a connection_key field to the PrivacyRequestResponse schema class that's the response model for the GET /api/v1/privacy-request endpoint. and then i think adding/updating test coverage around this endpoint is also needed.

if this functionality is no longer needed, then i think this is good to go, we should just update the issue accordingly.

let me know what you think!

@sanders41
Copy link
Contributor Author

nice, this generally looks good to me, the only thing that i think may be missing is support of the connection_key field making it back onto the serialized response on the GET /api/v1/privacy-request endpoint, as seems to be suggested in the #1725 AC.

It is included in the PrivacyRequestVerboseResponse, so when verbose is True the key is included. My assumption was because this is to retrieve the information when an error occurs this is what was wanted, since the verbose contains the error information. This is a good question though, @chriscalhoun1974 is my assumption here correct, or do I need to add it to the non verbose response also?

@chriscalhoun1974
Copy link
Contributor

@sanders41 You are correct to add it to response payload given the verbose: true parameter passed into the GET /api/v1/privacy-request endpoint. As of right now there is not an outstanding requirement to add it to the non verbose response. However, there is an upcoming story which has a requirement to display a tooltip when hovered over a given DSR error status record when displayed within the grid. Not sure as to what sprint this has been targeted. I recall seeing this in a Figma design at one point.

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me now, thanks for making sure we've got the data we need in the privacy request endpoint!

@sanders41 sanders41 merged commit 1674a15 into main Jan 4, 2023
@sanders41 sanders41 deleted the ps-privacy-request-key-1725 branch January 4, 2023 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants