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

udpate OneTrustGetUserResponse reference, create OneTrustEnrichedAssessments, and improve OneTrustGetRiskResponse #209

Merged
merged 7 commits into from
Jan 22, 2025

Conversation

abrantesarthur
Copy link
Member

Related Issues

  • [none]

Security Implications

[none]

System Availability

[none]

lvonessen
lvonessen previously approved these changes Jan 21, 2025
Copy link
Member

@lvonessen lvonessen left a comment

Choose a reason for hiding this comment

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

Overall, LGTM!Though, does the version number in package.json need to get bumped regarding your change?

@abrantesarthur abrantesarthur changed the title udpate OneTrustGetUserResponse reference and create OneTrustEnrichedAssessments codec udpate OneTrustGetUserResponse reference, create OneTrustEnrichedAssessments, and improve OneTrustGetRiskResponse Jan 22, 2025
lvonessen
lvonessen previously approved these changes Jan 22, 2025
Copy link
Member

@lvonessen lvonessen left a comment

Choose a reason for hiding this comment

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

Had a question and a suggestion, but overall LGTM

src/oneTrust/endpoints/getRisk.ts Outdated Show resolved Hide resolved
Comment on lines +247 to +255
sourceType: t.union([
t.literal('PIA'),
t.literal('RA'),
t.literal('GRA'),
t.literal('INVENTORY'),
t.literal('INCIDENT'),
t.literal('GENERIC'),
t.null,
]),
Copy link
Member

Choose a reason for hiding this comment

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

Is it intentional that source.type (directly above these lines) and sourceType are nearly the same but slightly different? One has t.null in the list

Copy link
Member Author

Choose a reason for hiding this comment

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

it is intentional but you do give me a good idea to extract these into their own variables!

@abrantesarthur
Copy link
Member Author

Overall, LGTM!Though, does the version number in package.json need to get bumped regarding your change?

great point! Will do

@abrantesarthur abrantesarthur merged commit ec34eb7 into main Jan 22, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants