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] Broken slack compatible webhook #11742

Merged
merged 9 commits into from
Sep 20, 2018
Merged

[FIX] Broken slack compatible webhook #11742

merged 9 commits into from
Sep 20, 2018

Conversation

geekgonecrazy
Copy link
Contributor

@geekgonecrazy geekgonecrazy commented Aug 10, 2018

Closes #11506

I added integer as a safe type. Also added Boolean. Both of which I can see being used, and are safe toString.

Are there any other that are safe to string we could/should include?

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11742 August 10, 2018 15:49 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11742 August 10, 2018 15:59 Inactive
@geekgonecrazy geekgonecrazy added this to the 0.69.0 milestone Aug 10, 2018
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11742 August 10, 2018 16:24 Inactive
@RocketChat RocketChat deleted a comment Aug 14, 2018
@RocketChat RocketChat deleted a comment from kapkoi Aug 20, 2018
}));

if (typeof(attachmentField.value) !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to enforce it as String? I think it's fine to keep it saved in the original format, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rodrigok initially I didn't convert. But looking at slack's it looks like they are converting to string so added.

But yeah I don't think it matters so much since all accepted will easily be converted to string when displayed in UI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll ping mobile teams and see which is better compatible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rodrigok confirmed with mobile. Because both clients are using typed languages having as a string is the best way.

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11742 August 21, 2018 19:58 Inactive
@geekgonecrazy geekgonecrazy changed the title [FIX] Allow string or int in attachment field value [FIX] Allow string, int or bool in attachment field value Aug 21, 2018
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11742 September 5, 2018 06:30 Inactive
@geekgonecrazy geekgonecrazy modified the milestones: 0.69.0, 0.70.0 Sep 5, 2018
@geekgonecrazy
Copy link
Contributor Author

@RocketChat/core lets make sure we get this merged on next release. Having to personally use the script provided here: #11506 (comment) a script shouldn't be needed for a slack payload

@geekgonecrazy geekgonecrazy changed the title [FIX] Allow string, int or bool in attachment field value [FIX] Broken slack compatible webhook Sep 7, 2018
const validateAttachmentsFields = (attachmentFields) => {
check(attachmentFields, objectMaybeIncluding({
const validateAttachmentsFields = (attachmentField) => {
check(attachmentField, objectMaybeIncluding({
short: Boolean,
Copy link
Member

Choose a reason for hiding this comment

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

do you mind merging these two checks? 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11742 September 18, 2018 17:09 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11742 September 19, 2018 19:54 Inactive
@sampaiodiego sampaiodiego merged commit e599ab7 into develop Sep 20, 2018
@sampaiodiego sampaiodiego deleted the fix/11506 branch September 20, 2018 20:17
This was referenced Sep 28, 2018
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.

4 participants