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 support for compliance_topics in the app TOML #3392

Merged
merged 24 commits into from
Mar 22, 2024

Conversation

gonzaloriestra
Copy link
Contributor

@gonzaloriestra gonzaloriestra commented Feb 7, 2024

WHY are these changes introduced?

Fixes https://github.com/Shopify/develop-app-management/issues/1600

WHAT is this pull request doing?

  • It allows to deploy privacy compliance webhooks through [[webhooks.subscriptions]] with compliance_topics
  • It keeps working with the legacy format, by defining them under [webhooks.privacy_compliance]
  • It raises an error if both ways are used together
  • It allows defining subscriptions with compliance_topics, but without topics. It raises if none is present.
  • When running link, it rewrites the TOML with the new format and simplifies it by grouping the compliance topics by URI. Only when the declarative_webhooks beta is enabled (code taken from Specs reverse transform receives betas #3416)

How to test your changes?

With declarative_webhooks flag:

  • Deploy with legacy format >> it gets updated in the dashboard
[webhooks.privacy_compliance]
customer_deletion_url = "https://app.com/customer_deletion"
customer_data_request_url = "https://app.com/customer_request"
shop_deletion_url = "https://app.com/shop_deletion"
  • Link updates the TOML to the new format
  • Update all the URIs to the same one with the new format >> it gets updated in the dashboard
  • Link simplifies the TOML, merging both topics and compliance_topics with the same URI
  • Deploy raises an error when both formats are present:
  [webhooks.privacy_compliance]
  customer_data_request_url = "https://app.com/webhook"

  [[webhooks.subscriptions]]
  uri = "https://app.com/webhook"
  compliance_topics = [ "customers/data_request" ]
  • Deploy raises an error if topics and compliance_topics are missing:
  [[webhooks.subscriptions]]
  uri = "https://app.com/webhook"
  • Deploy raises an error if there are duplicated compliance_topics:
  [[webhooks.subscriptions]]
  uri = "https://app.com/webhook"
  complicante_topics = [ "customers/data_request" ]
  
  [[webhooks.subscriptions]]
  uri = "https://app.com/other"
  complicante_topics = [ "customers/data_request" ]  

Without declarative_webhooks flag:

  • Deploy with legacy format >> it gets updated in the dashboard
[webhooks.privacy_compliance]
customer_deletion_url = "https://app.com/customer_deletion"
customer_data_request_url = "https://app.com/customer_request"
shop_deletion_url = "https://app.com/shop_deletion"
  • Update the URLs in the dashboard
  • Link updates the TOML keeping the format

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

Copy link
Contributor

github-actions bot commented Feb 7, 2024

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

  • Themes: @shopify/advanced-edits
  • UI extensions: @shopify/ui-extensions-cli
    • Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
  • Hydrogen: @shopify/hydrogen
  • Other: @shopify/cli-foundations

Copy link
Contributor

github-actions bot commented Feb 7, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.21% (+0.14% 🔼)
6740/9334
🟡 Branches
69.34% (+0.14% 🔼)
3295/4752
🟡 Functions
70.86% (+0.29% 🔼)
1797/2536
🟡 Lines
73.38% (+0.09% 🔼)
6355/8660
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app_config_webhook.ts
93.75% (-6.25% 🔻)
85.71% (-14.29% 🔻)
100%
93.33% (-6.67% 🔻)
🟢
... / app_config_webhook.ts
95.24% (-0.6% 🔻)
69.23% (-13.12% 🔻)
100% 100%

Test suite run success

1615 tests passing in 754 suites.

Report generated by 🧪jest coverage report action from 1be6420

Copy link
Contributor

@gracejychang gracejychang left a comment

Choose a reason for hiding this comment

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

the code looks mostly good to me! 😄 Just some small comments here and there. Another note is that I think we should maybe start keeping track of all the places we are changing due to beta flags, as I assume we want to rollback a number of these changes once we declarative-webhooks beta is rolled out 100%?

},
{
compliance_topics: ['shop/redact'],
uri: 'https://example.com/shop-deletion',
Copy link
Contributor

Choose a reason for hiding this comment

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

will the uri's here change to something like uri: '/shop-deletion' with Alex's PR where relative paths are supported, or compliance webhooks should be unaffected here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I guess we should support relative paths here as well. We can leave it for a later PR once both are merged. Cc @alexanderMontague

packages/app/src/cli/services/dev/select-app.test.ts Outdated Show resolved Hide resolved
@gonzaloriestra gonzaloriestra force-pushed the compliance_topics branch 2 times, most recently from 6f4c649 to 619dd6a Compare February 14, 2024 15:04

This comment has been minimized.

@gonzaloriestra

This comment was marked as resolved.

Copy link
Contributor

@alexanderMontague alexanderMontague 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, we should try to get this merged! Will need a rebase, tophat looks great too but lmk if you want to go over this again once cleaned up.

Copy link
Contributor

@alvaro-shopify alvaro-shopify left a comment

Choose a reason for hiding this comment

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

LGTM, amazing jog with this 🎉 !

Copy link
Contributor

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/public/common/array.d.ts
@@ -20,6 +20,13 @@ export declare function getArrayRejectingUndefined<T>(array: (T | undefined)[]):
  * @returns True if the array contains duplicates.
  */
 export declare function getArrayContainsDuplicates<T>(array: T[]): boolean;
+/**
+ * Removes duplicated items from an array.
+ *
+ * @param array - The array to inspect.
+ * @returns Returns the new duplicate free array.
+ */
+export declare function uniq<T>(array: T[]): T[];
 /**
  * This method is like  except that it accepts  which is
  * invoked for each element in  to generate the criterion by which
packages/cli-kit/dist/public/common/object.d.ts
@@ -69,4 +69,11 @@ export declare function setPathValue(object: object, path: string, value?: unkno
  * @param object - The value to check.
  * @returns - Returns true if value is empty, else false.
  */
-export declare function isEmpty(object: object): boolean;
\ No newline at end of file
+export declare function isEmpty(object: object): boolean;
+/**
+ * Removes the undefined elements.
+ *
+ * @param object - The object whose undefined will be deleted.
+ * @returns A copy of the object with the undefined elements deleted.
+ */
+export declare function compact(object: object): object;
\ No newline at end of file

Copy link
Contributor Author

@gonzaloriestra gonzaloriestra left a comment

Choose a reason for hiding this comment

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

I've been testing it and everything works as expected 👌

@gracejychang thanks for taking care of it, great job!

@gonzaloriestra gonzaloriestra added this pull request to the merge queue Mar 22, 2024
Merged via the queue into main with commit 199bafc Mar 22, 2024
@gonzaloriestra gonzaloriestra deleted the compliance_topics branch March 22, 2024 11:19
@gonzaloriestra gonzaloriestra added the #gsd:36745 Declarative Webhooks label Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#gsd:36745 Declarative Webhooks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants