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

feat(Stripe Trigger Node): Add Stripe webhook descriptions based on the workflow ID and name #9956

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

JavierCane
Copy link
Contributor

Summary

Right now n8n creates the needed Stripe Webhooks while adding Stripe Trigger nodes without specifying a description for that Stripe Webhook.

That is, you can end up having a list of Stripe Webhooks which do not reveal any kind of contextual information. In case of receiving an errors spike alert from Stripe, which demands a quick response, you do not have any more information rather than the webhook URL where it points to:

2024_07_06-12_31_11

With this PR n8n will start adding the following description to these webhooks:

`Created by n8n for workflow ID $workflowId: $workflowName`;

This allows to have a better idea of the implications of an error spike in webhook calls, or just knowing why you have N declared Stripe Webhooks and where they come from.

Take into account that, even if you manually add the Stripe Webhook description from the Stripe Workbench, it will be removed while updating n8n because it de-register the webhooks and create them again. So this is the only approach I can think of in order to maintain the webhook descriptions in place 😊

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@Joffcom
Copy link
Member

Joffcom commented Jul 6, 2024

Hey @JavierCane

I really like this idea, the version of the node doesn't need increasing where you have done it so that can be reverted.

As workflow names can contain any character including emojis does this cause any issues on the Stripe side? It may be safer to just use the workflow ID which solves the issue, what do you think?

@n8n-assistant n8n-assistant bot added community Authored by a community member node/improvement New feature or request in linear Issue or PR has been created in Linear for internal review labels Jul 6, 2024
@JavierCane
Copy link
Contributor Author

JavierCane commented Jul 6, 2024

They do seem to actually support emojis:
2024_07_06-13_07_09

If you ask my opinion, I would prefer to provide the workflow name as it will be more semantic than just the ID. However, no strong opinions here. If you prefer so I can remove the title and go ahead with the ID 😊

Edit: Thanks for the feedback on the node version increase. My bad 😅. Fixed!

@JavierCane
Copy link
Contributor Author

Hi, would it make it easy to merge this PR if I remove the workflow name from the Stripe Webhook description?

Thanks!

@Joffcom
Copy link
Member

Joffcom commented Jul 10, 2024

Hey @JavierCane,

This is waiting for my next PR review session, I typically look at PRs twice a week. I think I would be more comfortable with just the ID which solves the same problem with less risk.

@JavierCane
Copy link
Contributor Author

No problem then @Joffcom, workflow name removed 🤟

@Joffcom
Copy link
Member

Joffcom commented Jul 22, 2024

👍🏻 , I will add this to the review list to test this week.

@Joffcom Joffcom merged commit 3433465 into n8n-io:master Aug 7, 2024
9 checks passed
@github-actions github-actions bot mentioned this pull request Aug 7, 2024
@janober
Copy link
Member

janober commented Aug 7, 2024

Got released with n8n@1.54.0

MiloradFilipovic added a commit that referenced this pull request Aug 7, 2024
* master:
  refactor(core): Centralize scaling mode (no-changelog) (#9835)
  fix(editor): Remove body padding from storybook previews (no-changelog) (#10317)
  feat(MySQL Node): Return decimal types as numbers (#10313)
  🚀 Release 1.54.0 (#10315)
  feat(Elasticsearch Node): Add bulk operations for Elasticsearch (#9940)
  feat(Stripe Trigger Node): Add Stripe webhook descriptions based on the workflow ID and name (#9956)
  feat(MongoDB Node): Add projection to query options on Find (#9972)
  fix(Invoice Ninja Node): Fix payment types (#10196)
  feat(HTTP Request Tool Node): Use DynamicStructuredTool with models supporting it (no-changelog) (#10246)
  feat: Return scopes on executions (no-changelog) (#10310)
  feat(Webflow Node): Update to use the v2 API (#9996)
  feat(Lemlist Trigger Node): Update Trigger events (#10311)
  feat(Calendly Trigger Node): Update event names (no-changelog) (#10129)
  refactor(core): Reorganize webhook related components under src/webhooks (no-changelog) (#10296)
  docs: Fix links to license files in readme (no-changelog) (#10257)
  fix(editor): Update design system Avatar component to show initials also when only firstName or lastName is given (#10308)
  fix(editor): Update tags filter/editor to not show non existing tag as a selectable option (#10297)
  fix(editor): Update project tabs test (no-changelog) (#10300)
  fix(core): VM2 sandbox should not throw on `new Promise` (#10298)

# Conflicts:
#	packages/design-system/src/components/N8nAvatar/Avatar.vue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Authored by a community member in linear Issue or PR has been created in Linear for internal review node/improvement New feature or request Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants