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.

Sign up for GitHub

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

Jump to bottom

refactor: change use consent hook to be powered by api hook #479

Merged
merged 1 commit into from
Dec 1, 2023
Merged

refactor: change use consent hook to be powered by api hook #479

merged 1 commit into from
Dec 1, 2023
+67 −41

Conversation

jkdowdle
Copy link
Contributor

@jkdowdle jkdowdle commented Dec 1, 2023

Changes

  • use rest api hook to power useConsent
  • support the type differences of includeForm: boolean in looking up consent directives for user

Screenshots

jkdowdle
jkdowdle commented Dec 1, 2023
View reviewed changes
src/types/rest-types.ts Outdated
};
Response: {
items: ConsentDirective[];
};
};

'GET /v1/consent/directives/me?includeForm=true': {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't quite sure if this would work, but I did test it and appears to work.

I couldn't think of another way to differentiate the response type (generic didn't seem appropriate since it would be on the top level Rest types) - but part of me kinda likes this

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a simpler change. There are currently no use cases in the SDK for fetching without the form. So, we can just update this line to be includeForm: true, and make the response always ConsentAndForm[].

Does that make sense?

@coveralls
Copy link

coveralls commented Dec 1, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 7062980908

  • 8 of 9 (88.89%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 83.741%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/hooks/useConsent.ts 8 9 88.89%
Totals Coverage Status
Change from base Build 7062509134: -0.02%
Covered Lines: 3246
Relevant Lines: 3766
💛 - Coveralls

@jkdowdle jkdowdle requested review from swain, aymanenadi, markdlv, nicolls1 and aecorredor and removed request for aymanenadi December 1, 2023 16:33
@swain
Copy link
Contributor

swain commented Dec 1, 2023

@jkdowdle Also, this should definitely be a fix: or feat: commit, not a refactor:.

refactor: commits don't cause a new release to be cut.

@jkdowdle
Copy link
Contributor Author

jkdowdle commented Dec 1, 2023

@swain I did a force push to fix the semantic commit message, sorry about that. but I do think it is better to review that way since some of the other changes are just not necessary any more

@swain swain merged commit bcfb860 into master Dec 1, 2023
3 checks passed
@swain swain deleted the refactor-use-consent branch December 1, 2023 17:20
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 1, 2023

🎉 This PR is included in version 9.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released Has been released to the package repository (NPM, etc) label Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swain swain swain approved these changes

@markdlv markdlv Awaiting requested review from markdlv

@nicolls1 nicolls1 Awaiting requested review from nicolls1

@aecorredor aecorredor Awaiting requested review from aecorredor

@aymanenadi aymanenadi Awaiting requested review from aymanenadi

Assignees
No one assigned
Labels
released Has been released to the package repository (NPM, etc)
Projects
None yet
Milestone
No milestone
3 participants
@jkdowdle @coveralls @swain