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

Fides GTM & Event Origination. #5821

Merged
merged 26 commits into from
Mar 6, 2025
Merged

Fides GTM & Event Origination. #5821

merged 26 commits into from
Mar 6, 2025

Conversation

tvandort
Copy link
Contributor

Closes []

Description Of Changes

Write some things here about the changes and any potential caveats

Code Changes

  • list your code changes here

Steps to Confirm

  1. list any manual steps for reviewers to confirm the changes

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
  • Followup issues:
    • Followup issues created (include link)
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

@vercel Vercel
Copy link

vercel bot commented Feb 28, 2025 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
fides-privacy-center ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 6, 2025 0:09am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Mar 6, 2025 0:09am

@tvandort tvandort marked this pull request as ready for review March 5, 2025 20:21
@tvandort tvandort force-pushed the fides-js-event-wip branch from 6441f54 to 773973a Compare March 5, 2025 20:54
gilluminate
gilluminate reviewed Mar 5, 2025
View reviewed changes
clients/fides-js/src/components/tcf/TcfPurposes.tsx
Comment on lines +185 to +186
// @ts-ignore
modelType: item.bestTranslation
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this isn't your doing, but it would be nice to make this a bit safer while we're here

Suggested change
// @ts-ignore
modelType: item.bestTranslation
modelType: (item as PrivacyNoticeWithBestTranslation).bestTranslation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't that a 6 of one, half a dozen of another sort of change?

Copy link
Contributor

@gilluminate gilluminate Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sort of. If there were other TS issues besides this one, they would not get seen. This is easier for future devs, basically. ts-ignore should be avoided at all costs.

Copy link
Contributor

Choose a reason for hiding this comment

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

An example of where this could be problematic is if bestTranslation were also removed from PrivacyNoticeWithBestTranslation and that wouldn't get picked up here because of the //@ts-ignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok.

@tvandort @gilluminate
Update clients/fides-js/__tests__/integrations/gtm.test.ts
44f0439 
Co-authored-by: Jason Gill <jason.gill@ethyca.com>
@tvandort @gilluminate
Update clients/fides-js/src/lib/events.ts
2dfd636 
Co-authored-by: Jason Gill <jason.gill@ethyca.com>
@NevilleS
Copy link
Contributor

NevilleS commented Mar 6, 2025

OK @gilluminate I pushed some updates to the Cypress tests to update those to include the new GTM events and do some (light) cleanup.

NevilleS
NevilleS approved these changes Mar 6, 2025
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.

Overall, this is looking great. Thanks gang

gilluminate
gilluminate approved these changes Mar 6, 2025
View reviewed changes
Copy link
Contributor

@gilluminate gilluminate left a comment

Choose a reason for hiding this comment

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

Great detail added in the tests @NevilleS!

clients/privacy-center/cypress/e2e/consent-banner.cy.ts
@@ -2122,7 +2122,19 @@ describe("Consent overlay", () => {

// Toggle the notice, but don't save yet
cy.getByTestId("toggle-Advertising").click();
cy.get("@FidesUIChanged").should("have.been.calledOnce");
cy.get("@FidesUIChanged").its("callCount").should("equal", 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

@NevilleS Just curious what the difference is here? Just for consistency?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's all

@tvandort tvandort merged commit fe14b8b into main Mar 6, 2025
18 checks passed
@tvandort tvandort deleted the fides-js-event-wip branch March 6, 2025 00:37
tvandort added a commit that referenced this pull request Mar 6, 2025
@tvandort @NevilleS @gilluminate
Fides GTM & Event Origination. (#5821)
347282f 
Co-authored-by: Neville Samuell <neville@ethyca.com>
Co-authored-by: Jason Gill <jason.gill@ethyca.com>
@cypress Cypress
Copy link

cypress bot commented Mar 6, 2025

fides    Run #12630

Run Properties:  status check passed Passed #12630  •  git commit fe14b8bb75: Fides GTM & Event Origination. (#5821)
Project fides
Branch Review main
Run status status check passed Passed #12630
Run duration 00m 46s
Commit git commit fe14b8bb75: Fides GTM & Event Origination. (#5821)
Committer Tom Van Dort
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 5
View all changes introduced in this branch ↗︎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NevilleS NevilleS

@gilluminate gilluminate

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
@tvandort @NevilleS @gilluminate