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

Fix Shopify masking #5282

Merged
merged 19 commits into from
Sep 26, 2024
Merged

Fix Shopify masking #5282

merged 19 commits into from
Sep 26, 2024

Conversation

Vagoasdf
Copy link
Contributor

@Vagoasdf Vagoasdf commented Sep 12, 2024

Closes #2617

Description Of Changes

Updating the Shopify dataset to avoid sending masked values for country and province
Adding tests for masking values

Code Changes

  • updated Changelog

Steps to Confirm

We've added Shipping Address to the shopify fixtures, and a new test that has a strict policy so it would mask all the values before sending it to Shopify. That checks that the values that we marked as read_only are not masked

  • Check Response output on the newly added test at test_shopify_erasure_accepts_masking_task

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

@Vagoasdf Vagoasdf requested a review from a team as a code owner September 12, 2024 13:39
Copy link

vercel bot commented Sep 12, 2024

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

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Sep 25, 2024 5:56pm

Copy link

cypress bot commented Sep 12, 2024

fides    Run #10159

Run Properties:  status check passed Passed #10159  •  git commit a30ebfedd8 ℹ️: Merge 1699b0a53c7d9f99a1e772fbf2cf57fed329935e into 077d26642df0359b60d351f8f252...
Project fides
Branch Review refs/pull/5282/merge
Run status status check passed Passed #10159
Run duration 00m 38s
Commit git commit a30ebfedd8 ℹ️: Merge 1699b0a53c7d9f99a1e772fbf2cf57fed329935e into 077d26642df0359b60d351f8f252...
Committer Bruno Gutierrez Rios
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 4
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

@@ -503,7 +503,7 @@ async def test_shopify_erasure_request_task(
f"{dataset_name}:customers": 1,
f"{dataset_name}:blogs": 0,
f"{dataset_name}:customer_orders": 1,
f"{dataset_name}:customer_addresses": 1,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we're adding a new address at shipping address, this increases by 1

@galvana galvana changed the title Fix shoppify masking Fix Shiopify masking Sep 25, 2024
@galvana galvana changed the title Fix Shiopify masking Fix Shopify masking Sep 25, 2024
Copy link
Contributor

@galvana galvana 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 good! Just make sure to bump the version on the shopify_config.yml so any existing datasets auto-update. For example version: 0.0.4 -> version: 0.0.5

CHANGELOG.md Outdated
### Fixed
- Fix wording in tooltip for Yotpo Reviews [#5274](https://github.com/ethyca/fides/pull/5274)
- Updated Shopify Datamap in order to ignore country and province values that are required on the erasure API
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Updated Shopify Datamap in order to ignore country and province values that are required on the erasure API
- Updated Shopify dataset in order to flag country, province, and other location values as read-only

@Vagoasdf Vagoasdf merged commit dba1a56 into main Sep 26, 2024
37 checks passed
@Vagoasdf Vagoasdf deleted the fix-shoppify-masking branch September 26, 2024 13:29
Copy link

cypress bot commented Sep 26, 2024

fides    Run #10190

Run Properties:  status check passed Passed #10190  •  git commit dba1a567fd: Fix Shopify masking (#5282)
Project fides
Branch Review main
Run status status check passed Passed #10190
Run duration 00m 37s
Commit git commit dba1a567fd: Fix Shopify masking (#5282)
Committer Bruno Gutierrez Rios
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 4
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants