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

Improvement: Added more context to Shopify webhook registering errors #1090

Merged
merged 5 commits into from May 8, 2024

Conversation

ramadanomar
Copy link
Contributor

About Issue

When initializing the Shopify Client with invalid parameters (excluding apiKey), errors are not explicitly thrown, resulting in a silent failure in the background. The user only sees an error message stating, "Error: Missing webhook ID for delete operation." This occurs because the current logic attempts to delete a webhook subscription first if its creation fails, possibly to avoid the "address already in use" exception. However, this logic is poorly documented, and only users running a trigger client with verbose mode enabled will see the debug message, "ERROR CREATING WEBHOOK RESTARTING WITH DELETE FIRST." In cases of invalid secret keys, this approach is ineffective and does not resolve the issue.

✅ Checklist

  • I have followed every step in the contributing guide
  • The PR title follows the convention.
  • I ran and tested the code works

Testing

Localhost & Shopify Dev Store


Changelog

  • Added input validation for constructor parameters to ensure required arguments are correct.
  • Verified that the store domain uses the .myshopify.com domain instead of a custom primary domain.
  • Integrated logger.error statements into the webhook created/update/delete logic for improved error tracking.

Screenshots

Old Behaviour

image

New Behaviour

image

💯

Co-authored-by: Ahmed Ramadan <ahmedramadan1337@gmail.com>
Copy link

changeset-bot bot commented May 7, 2024

🦋 Changeset detected

Latest commit: 2ed1eb9

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@matt-aitken matt-aitken requested a review from nicktrn May 7, 2024 21:55
Copy link
Collaborator

@nicktrn nicktrn 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 this! Just one concern about backwards compat re the hostname option. Let me know what you think. Only accept the code suggestions if you think they make sense. 🙏

integrations/shopify/src/index.ts Outdated Show resolved Hide resolved
@ramadanomar ramadanomar requested a review from nicktrn May 8, 2024 12:49
Copy link
Collaborator

@nicktrn nicktrn 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 changes! Made some small edits and will merge later if all goes well.

@nicktrn nicktrn merged commit 1fd26ff into triggerdotdev:main May 8, 2024
0 of 2 checks passed
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.

None yet

2 participants