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 error for showConsentManager when viewState is TCF_EU #157

Merged
merged 11 commits into from
Feb 8, 2024

Conversation

linh-transcend
Copy link
Member

@linh-transcend linh-transcend commented Feb 7, 2024

Related Issues

  • Links https://transcend.height.app/T-33647
  • When the input to showConsentManager is TCF_EU show and error that tells the user to update regimes to see that view state. When input is not a valid view state, error too.

Screenshot 2024-02-08 at 10 38 05 AM

Security Implications

[none]

System Availability

[none]

Copy link

height bot commented Feb 7, 2024

This pull request has been linked to 1 task:

  • T-33647 Throw nicer error if viewState is invalid in consent-manager-ui and consent-manager-tcf-ui repos (unlink task)

💡Tip: Add "Close T-33647" to the pull request title or description, to a commit message, or in a comment to mark this task as "Pending Deploy" when the pull request is merged.

Copy link

vercel bot commented Feb 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
consent-manager-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 8, 2024 4:31pm

@linh-transcend linh-transcend changed the title [WIP] Add error for showConsentManager when viewState is TCF_EU Add error for showConsentManager when viewState is TCF_EU Feb 8, 2024
src/api.ts Outdated
return;
}
logger.warn(
'TCF_EU view state is not valid. Please configure your regime to use this view state.',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'TCF_EU view state is not valid. Please configure your regime to use this view state.',
'TCF_EU view state is not valid for this user experience. Please configure your Regional Experience to use this view state and try again.',

Copy link
Member

@anotherminh anotherminh Feb 8, 2024

Choose a reason for hiding this comment

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

I think we should also only throw this error for TCF_EU explicitly, and if the userpass in any other view state, we should throw a different error & log out the list of valid view states for them

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to confirm, if a user uses 'banana' as a view state, that's when we list out the valid view state?

Copy link
Member

Choose a reason for hiding this comment

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

@linh-transcend yea, that's when i think we say "'${input}' is not a valid view state. Valid view states are: ..."

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, updated!
Screenshot 2024-02-08 at 10 22 28 AM

src/api.ts Outdated
options?.viewState &&
!Object.values(ViewState).includes(options.viewState)
) {
logger.warn(
Copy link
Member

Choose a reason for hiding this comment

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

Can we do error instead of warn?

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry i missed that. just updated it to an error. i need to do the same for the warning in the tcf-ui

anotherminh
anotherminh previously approved these changes Feb 8, 2024
src/api.ts Outdated
`${
options.viewState
} is not a valid view state. Valid view states include ${Object.values(
ViewState,
Copy link
Member

Choose a reason for hiding this comment

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

@linh-transcend actually let's only list the "open" view states and not closed view states like closed/collapsed/hidden

Copy link
Member Author

Choose a reason for hiding this comment

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

Screenshot 2024-02-08 at 10 53 47 AM

Copy link
Member

Choose a reason for hiding this comment

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

LanguageOptions doesn't seem right either, hmm

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated!
Screenshot 2024-02-08 at 11 23 50 AM

src/settings.ts Outdated
@@ -63,7 +63,7 @@ const inheritLogLevels = (logLevels: LogLevel[]): LogLevel[] => {
};

const wildcardLogLevels = ['all', '*'];
const logLevelsSetting = (settings.log ?? '').toLowerCase();
const logLevelsSetting = (settings.log ?? 'warn error').toLowerCase();
Copy link
Member

Choose a reason for hiding this comment

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

hmm let's not change this

Copy link
Member Author

Choose a reason for hiding this comment

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

oi i forgot to remove this from testing. thanks for catching!

anotherminh
anotherminh previously approved these changes Feb 8, 2024
@linh-transcend linh-transcend merged commit 03918d9 into main Feb 8, 2024
10 checks passed
@delete-merged-branch delete-merged-branch bot deleted the linh-transcend/invalid-view-state-error branch February 8, 2024 22:43
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.

3 participants