Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

Invalid topic in addHandler bricks registerAll (Mandatory GDPR Webhooks) #363

Closed
bishpls opened this issue Apr 13, 2022 · 3 comments · Fixed by #371
Closed

Invalid topic in addHandler bricks registerAll (Mandatory GDPR Webhooks) #363

bishpls opened this issue Apr 13, 2022 · 3 comments · Fixed by #371

Comments

@bishpls
Copy link

bishpls commented Apr 13, 2022

Issue summary

Adding a webhook handler for a topic that does not exist via addHandler/addHandlers causes the registerAll function to error.

This is undesirable behavior, as the Mandatory GDPR webhook topics (customers/data_request, customers/redact, and shop/redact) are considered invalid topics; this means that if you attempt to add handlers for the GDPR webhooks, the registerAll function no longer works.

Now, this is complicated by the fact that you aren't SUPPOSED to add handlers for the GDPR topics...but you can, and doing so is actually entirely functional!

Given that this package lacks a built-in method to validate the mandatory GDPR webhooks (or any documentation on doing so), this is a pretty severe gap in usability for many developers.

Now, one thing you CAN do is addHandler the GDPR topics, and then manually call Shopify.Webhooks.register for each non-GDPR topic you subscribe to for a shop...but this is extremely unintuitive, and again -- undocumented behavior.

Expected behavior

Two points of expected/desired behavior:

  1. There is a way to register the mandatory GDPR webhooks without manually validating them per the comments here: Mandantory GDPR webhooks #256

  2. Attempting to add a handler for an invalid topic does not completely break functionality of registerAll.

Actual behavior

Calling addHandler for GDPR webhook topics (or any other invalid topic) renders registerAll unusable.

Two options for developers: either manually validate GDPR webhooks, or manually call register for each non-GDPR webhook.

Steps to reproduce the problem

  1. Add a handler for an invalid webhook topic. (I.E. Mandatory GDPR webhooks)
  2. Add a handler for any valid webhook topic.
  3. Call Shopify.Webhooks.Registry.registerAll (Note that individually calling register on each non-GDPR topic DOES work)
  4. Error

Screen Shot 2022-04-13 at 6 09 50 PM

Reduced test case

 function demoHandler() {
    console.log('hello');
  }

  Shopify.Webhooks.Registry.addHandler("APP_UNINSTALLED", {
    path: '/webhooks',
    webhookHandler: demoHandler,
  });
  Shopify.Webhooks.Registry.addHandler("CUSTOMERS_DATA_REQUEST", {
    path: '/webhooks',
    webhookHandler: demoHandler,
  }); 

followed by, as part of shop auth:

      const webhookRegistrationResult = await Shopify.Webhooks.Registry.registerAll({ shop, accessToken });

Checklist

  • [x ] I have described this issue in a way that is actionable (if possible)
@paulomarg
Copy link
Contributor

paulomarg commented Apr 14, 2022

Thank you for reporting this, you make a great point. I think the ideal way for this to go would be for us to allow the GDPR topics in addHandler, but skip over them in registerAll. That way, apps can use the lib's webhook features in a consistent way.

As you mentioned, a temporary workaround would be to use the process method by adding all the handlers but "manually" looping over them and calling register instead of registerAll. But we'll make sure registerAll deals with this scenario for you.

The reason why this fails is that the API itself doesn't support subscribing to those events (it needs to happen in the partners dashboard).

@zirkelc
Copy link

zirkelc commented May 2, 2022

@paulomarg does addHandler support the GDPR topics now?

@bishpls
Copy link
Author

bishpls commented May 2, 2022

@zirkelc It technically did before, as long as you didn't call registerAll! It's just easier to use out of the box now. :)

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants