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(Discord Node): Remove requirement on message for webhooks #8377

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

nihaals
Copy link
Contributor

@nihaals nihaals commented Jan 17, 2024

Summary

This adds a missing change to #8060 which affects sending from webhooks.

Review / Merge checklist

  • PR title and summary are descriptive. Remember, the title automatically goes into the changelog. Use (no-changelog) otherwise. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.

    A bug is not considered fixed, unless a test is added to prevent it from happening again.
    A feature is not complete without tests.

@nihaals nihaals force-pushed the discord-webhook-message-required branch from 939cb49 to 7c65364 Compare January 17, 2024 23:38
@n8n-assistant n8n-assistant bot added community Authored by a community member node/improvement New feature or request labels Jan 18, 2024
@Joffcom
Copy link
Member

Joffcom commented Jan 19, 2024

@nihaals just looking at this again, If you don't specify a Message what happens?

@nihaals
Copy link
Contributor Author

nihaals commented Jan 19, 2024

just looking at this again, If you don't specify a Message what happens?

You can't trigger the node as the field is required

@Joffcom
Copy link
Member

Joffcom commented Jan 19, 2024

@nihaals in that case I am confused, Why would you remove the required option if it is actually required?

@nihaals
Copy link
Contributor Author

nihaals commented Jan 19, 2024

in that case I am confused, Why would you remove the required option if it is actually required?

Oh I meant before this patch. The reason it's not required is the same as #8060 (I just forgot to apply the change to webhooks too as I didn't realise they were implemented separately, them not being required is the same for both webhooks and Discord bot messages):

The PR removes the requirement on 2 fields: the content and the description of embeds. Discord requires neither of these fields and as they weren't required in the v1 node, it prevents migrating to the v2 node for users not providing one of them.

Without this patch it's still not possible to migrate to v2 unless you're able to switch to a bot token (and are unnecessarily forced to) as the change was only applied to bot messages and not also webhook messages.

@Joffcom Joffcom merged commit c64e893 into n8n-io:master Jan 22, 2024
8 checks passed
@Joffcom
Copy link
Member

Joffcom commented Jan 22, 2024

Hey @nihaals,

Perfect thanks 👍🏻

@janober
Copy link
Member

janober commented Jan 22, 2024

Got released with n8n@1.25.1

@github-actions github-actions bot mentioned this pull request Jan 24, 2024
@nihaals nihaals deleted the discord-webhook-message-required branch February 6, 2024 16:57
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 node/improvement New feature or request Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants