Repository navigation
Conversation
|
hi @jmanian - I think this makes sense to add here. My only quibble is that the same comment may not really be appropriate for But I have to defer to @dblock as he's the real owner here and I'm just helping out when I can so far. |
| attachments = options[:attachments] | ||
| attachments = JSON.dump(attachments) unless attachments.is_a?(String) | ||
| options = options.merge(attachments: attachments) | ||
| if options[:attachments].nil? |
There was a problem hiding this comment.
This file is generated by a rake task (see the top line), you have to apply this as a patch.
| if options[:attachments].nil? | ||
| # sometimes sending "null" for "attachments" leads to an error | ||
| # when it works it has the same effect as sending no attachments at all | ||
| options.delete(:attachments) |
There was a problem hiding this comment.
You should not be modifying the incoming options Hash, modify a copy, options = options.except(:attachments) for example.
There was a problem hiding this comment.
Any such changes should be in patches, https://github.com/slack-ruby/slack-ruby-client/tree/018dcfdea5a4bfb63310ac16e0dfceb1827816f4/lib/slack/web/api/patches.
Generally it would maybe make sense to have a kind of option in templates that takes standard treatment such as being removed if null or blank?.
|
I got word from Slack that the bug causing "null" attachments to yield |
|
Want to just close this? |
In our app we started receiving some
invalid_attachments_formaterrors for certain workspaces, and I tracked it down to a few spots where we passattachments: nilintochat_postMessage. I took a look in here to see what happens when you passattachments: niland the answer is that the JSON string forattachmentsisnull. I was able to get around this error by simply omittingattachmentsinstead.We've actually have been doing this for a long time and it's never been a problem before, so I contacted Slack about it and they've identified it as a bug related to a beta feature for certain workspaces.
But there's really no good reason to ever send
nullfor attachments, so I thought it might make sense to avoid that.The change to
chat_updateis perhaps less self-explanatory thanchat_postMessageandchat_ephemeral. Here's howchat_updatebehaves withattachments:attachments:attachmentsas"[]"ornull:Anyway, I'm curious if you think this is an appropriate safeguard to add to this library, or not.