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

Support for non-legacy attachments #136

Open
lightbody opened this issue Jan 4, 2020 · 2 comments
Open

Support for non-legacy attachments #136

lightbody opened this issue Jan 4, 2020 · 2 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@lightbody
Copy link
Contributor

Slack's language around attachments is a bit confusing... on this page they seem to indicate that the entire concept of attachment might go away. But on this reference page they clearly define "legacy" attachment fields separate from non-legacy fields: namely color (only remaining field) and blocks.

In short: it looks like Slack wants us to to stop using all the bespoke formatting you could get in attachments and simply use Blocks, which makes sense to me.

The problem is: this projects AttachmentIF only reflects the legacy stuff (+ color) and doesn't have support for List<Block>. I tried adding List<Block> getBlocks() hoping that would be enough to get the job done, but unfortunately Slack returns with an invalid_attachments error (which, btw, isn't documented anywhere) with additional context of [ERROR] invalid_keys.

Through trial-and-error, I learned that the error happens when you include legacy fields (ex: text, mrkdwn_in, fields, etc) even if they are null. So AttachmentsIF needs a bit more work than just adding a new getter. It needs a serializer that omits all other legacy fields if blocks are provided. Or perhaps we need to further compose Attachment into LegacyAttachment and BlockAttachment.

I'll try to get a PR for this soon, but this Jackson serialization and composition stuff is slow going for me so maybe you'll get to it before I do :)

@szabowexler
Copy link
Collaborator

Chiming in on the style - I think the fastest thing you could do here would actually be to add a validator on the attachment block.

If you go the custom serde (serialization/deserialization) route, users might be confused since the serialized JSON won't match what they think they put in. On the other hand, if we add an @Check annotated method that validates that we're either all in on the new block format, or all in on the legacy format, then we'll fail with a clear runtime exception if someone tries to do something illegal.

I think that will (A) be less work for you, and (B) be clearer in how it fails for users in what they need to do.

@szabowexler szabowexler added bug Something isn't working enhancement New feature or request labels Jan 8, 2020
@lightbody
Copy link
Contributor Author

Thanks for the note. I intend to tackle this soon, just been tied up on other things the last few days :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants