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

Handle decoding problems with passed in fides_string #4350

Merged
merged 12 commits into from
Oct 31, 2023
Merged

Handle decoding problems with passed in fides_string #4350

merged 12 commits into from
Oct 31, 2023

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Oct 27, 2023 •
edited
Loading

Closes https://ethyca.atlassian.net/browse/PROD-1281

Description Of Changes

Passing in an invalid fides_string will now ignore it instead of breaking the app!

In the video, both with and without a fides string give the same result.

Screen.Recording.2023-10-27.at.10.42.01.AM.mov

Code Changes

  • Add try/catch around decoding the tc string
  • If the decoding fails, reset the passed in fides_string to null

Steps to Confirm

  • See steps in the ticket

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Issue Requirements are Met
  • Update CHANGELOG.md

@cypress
Copy link

cypress bot commented Oct 27, 2023 •
edited
Loading

Passing run #4929 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge bee9558 into 262394e...
Project: fides Commit: fa87522867 ℹ️
Status: Passed Duration: 00:48 💡
Started: Oct 31, 2023 7:24 PM Ended: Oct 31, 2023 7:25 PM

Review all test suite changes for PR #4350 ↗︎

@allisonking allisonking marked this pull request as ready for review October 27, 2023 18:49
NevilleS
NevilleS requested changes Oct 27, 2023
View reviewed changes
Copy link
Contributor

@NevilleS NevilleS 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 solid, but I have a question in there...

clients/fides-js/src/fides-tcf.ts Outdated Show resolved Hide resolved
NevilleS
NevilleS requested changes Oct 27, 2023
View reviewed changes
Copy link
Contributor

@NevilleS NevilleS left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for clarifying - OK, some nits

clients/fides-js/src/fides-tcf.ts Outdated Show resolved Hide resolved
clients/fides-js/src/fides-tcf.ts Outdated Show resolved Hide resolved
@allisonking
Copy link
Contributor Author

Okay @NevilleS I've refactored, hopefully it is a little more streamlined now. thank goodness for all the tests you and @eastandwestwind put in 🙏

NevilleS
NevilleS requested changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@NevilleS NevilleS left a comment

Choose a reason for hiding this comment

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

This works, but I feel like we've hit a level of complexity here that gives me pause- let's take a beat right now and see what we could do to simplify this!

clients/fides-js/src/fides-tcf.ts Show resolved Hide resolved
clients/fides-js/src/fides-tcf.ts Outdated Show resolved Hide resolved
clients/fides-js/src/fides-tcf.ts Show resolved Hide resolved
@allisonking
Copy link
Contributor Author

We talked about this separately and decided:

  • the initialization logic as a whole is too complicated!
  • this PR makes it marginally more understandable
  • let's make a separate ticket to simplify the initialization logic
  • this PR can continue down its release process (I tried to poke at it more to simplify it, but it ended up requiring larger changes I'd rather do as part of an initialization refactor)

eastandwestwind
eastandwestwind approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@eastandwestwind eastandwestwind left a comment

Choose a reason for hiding this comment

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

Overall looks good, nothing blocking, just a couple minor suggestions 🚀

clients/fides-js/src/fides-tcf.ts Outdated Show resolved Hide resolved
clients/fides-js/src/fides-tcf.ts Outdated Show resolved Hide resolved
clients/fides-js/src/fides-tcf.ts Outdated Show resolved Hide resolved
clients/privacy-center/cypress/e2e/consent-banner-tcf.cy.ts Show resolved Hide resolved
NevilleS
NevilleS approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@NevilleS NevilleS left a comment

Choose a reason for hiding this comment

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

Thanks for the extra TLC on this one 👍

@allisonking allisonking merged commit fb1ccdc into main Oct 31, 2023
11 checks passed
@allisonking allisonking deleted the aking/prod-1281/fides-string-decode branch October 31, 2023 19:46
Kelsey-Ethyca pushed a commit that referenced this pull request Nov 1, 2023
@allisonking @Kelsey-Ethyca
Handle decoding problems with passed in fides_string (#4350)
97dbb02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eastandwestwind eastandwestwind eastandwestwind approved these changes

@NevilleS NevilleS NevilleS approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@allisonking @NevilleS @eastandwestwind