-
Notifications
You must be signed in to change notification settings - Fork 226
Add work objects support #1512
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
base: main
Are you sure you want to change the base?
Add work objects support #1512
Conversation
4d45e21
to
0c5ae6a
Compare
0c5ae6a
to
8be1e14
Compare
…o feat-work-objects
…o feat-work-objects
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (5.88%) is below the target coverage (30.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1512 +/- ##
============================================
- Coverage 73.03% 72.69% -0.35%
Complexity 4381 4381
============================================
Files 475 476 +1
Lines 14222 14293 +71
Branches 1447 1456 +9
============================================
+ Hits 10387 10390 +3
- Misses 2976 3043 +67
- Partials 859 860 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8270f1a
to
dcc9b69
Compare
.threadTs(req.getPayload().getMessage().getThreadTs()) | ||
.text("OK, I will generate numbers for you!") | ||
.metadata(new Message.Metadata("assistant-generate-numbers", eventPayload)) | ||
.metadata(Message.Metadata.builder().eventType("assistant-generate-numbers").eventPayload(eventPayload).build()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to node-slack-sdk, I originally updated the Message.Metadata object that was previously event-metadata-only. Planning on updating this PR to use a separate EntityAndEventMessageMetadata object as well
cc: @zimeg and @mwbrooks to sanity check this; any apps calling chat.postMessage and constructing the Message.Metadata object will need to update that type to Message.EntityAndEventMessageMetadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 @vegeris This is so interesting! Thank you for calling it out 🙏 ✨
If I'm understanding this right, changing the metadata
type from Message.Metadata
might error during compilation of app code for this:
Message.Metadata metadata = message.getMetadata();
It makes sense that this argument can be either metadata type for the API but I'm wondering if we workaround changing this in the SDK for now with an additional argument for the method input and generous javadoc too?:
/**
* Metadata of entities attached to a message. Used instead of the "metadata" attribute, which sends event-based message metadata. In an upcoming major version, the "metadata" attribute will accept both types of metadata.
*/
private Message.EntityMessageMetadata entityMessageMetadata;
IIRC we have logic to parse various cases of metadata
that can be extended in meantimes-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this!
additional argument for the method input
I'm not sure I follow 👀
Edit: Ohh, gotcha; potentially have a separate property for entity+event metadata and check for it in FormBody.Builder toForm(ChatPostMessageRequest req)
I believe @WilliamBergamin is also back this week; it would be great to also get your input 🙏
dcc9b69
to
8ea0ffc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌟 @vegeris Praises all throughout these changes! I tried to keep comments focused to thoughts and suggestions. This is looking solid!
The metadata
typings you've called out caught my attention and I left some rambles on a workaround to avoid changing the existing types. I'm not sure if it's a good idea but I wonder if some changes might be left as TODO
for now?
So curious about what you think! I plan to test this more after updates to metadata typings that might be in progress at the moment too 🫡
// and the Conversations API will become available to classic Slack apps over | ||
// the coming months. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🪬 note: TIL!
// ------------------------------ | ||
// work object entities | ||
// ------------------------------ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧮 quibble: This method might've been renamed in recent times. Can I request a realphabeticalization before merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do; 'work objects' was the internal name which was potentially going to change (so we were avoiding using it in external API names etc.) but we ended up keeping it
Here I'll just use 'entity' as the header to be consistent with the API naming
slack-api-client/src/main/java/com/slack/api/methods/Methods.java
Outdated
Show resolved
Hide resolved
slack-api-client/src/main/java/com/slack/api/methods/AsyncMethodsClient.java
Outdated
Show resolved
Hide resolved
slack-api-client/src/main/java/com/slack/api/methods/AsyncMethodsClient.java
Outdated
Show resolved
Hide resolved
// https://docs.slack.dev/changelog/2021-08-changes-to-unfurls | ||
private String source; | ||
|
||
private String rawMetadata; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private String rawMetadata; | |
private String metadataAsString; |
🔍 suggestion: To match chat.postMessage
formats!
Line 66 in e3f79e0
private String metadataAsString; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📚 suggestion: Adding similar javadoc might be nice too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong feelings but I called it rawMetadata
to be consistent with the naming for the unfurls param (unfurls
and rawUnfurls
)
(Added a javadoc comment!)
.threadTs(req.getPayload().getMessage().getThreadTs()) | ||
.text("OK, I will generate numbers for you!") | ||
.metadata(new Message.Metadata("assistant-generate-numbers", eventPayload)) | ||
.metadata(Message.Metadata.builder().eventType("assistant-generate-numbers").eventPayload(eventPayload).build()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 @vegeris This is so interesting! Thank you for calling it out 🙏 ✨
If I'm understanding this right, changing the metadata
type from Message.Metadata
might error during compilation of app code for this:
Message.Metadata metadata = message.getMetadata();
It makes sense that this argument can be either metadata type for the API but I'm wondering if we workaround changing this in the SDK for now with an additional argument for the method input and generous javadoc too?:
/**
* Metadata of entities attached to a message. Used instead of the "metadata" attribute, which sends event-based message metadata. In an upcoming major version, the "metadata" attribute will accept both types of metadata.
*/
private Message.EntityMessageMetadata entityMessageMetadata;
IIRC we have logic to parse various cases of metadata
that can be extended in meantimes-
slack-api-client/src/main/java/com/slack/api/methods/RequestFormBuilder.java
Outdated
Show resolved
Hide resolved
* JSON object containing the data that will be displayed in the flexpane for | ||
* the entity. | ||
*/ | ||
private String rawMetadata; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private String rawMetadata; | |
private String metadataAsString; |
🧵 suggestion: To match another comment, but I also think duplicating the javadoc for the more explicit typings that follow might be solid too for reference generation? I haven't experimented with such edge-
private String mimetype; | ||
private String url; | ||
private AttachmentMetadata metadata; | ||
private Object workObjectEntity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👾 question: Is this particular to a specific Object
import, or can it be cast to something else? IIRC adjustments to these models are common if fields are adjusted, but having such type might make accessing easier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The work object attachment is the internal client schema, which we don't want users to consume, but in the integration test for chat.unfurl
I'm verifying that the work object was successfully added by checking for this property
} | ||
} | ||
|
||
String blocksAsString = "[\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like my formatter in VSCode updated this whole file; is there a particular formatter I should be using? (My current default is Language Support for Java(TM) by Red Hat
)
03b1b45
to
c18db30
Compare
c18db30
to
55656ec
Compare
…o feat-work-objects
"provided": "", | ||
"arg": "" | ||
"arg": "", | ||
"callstack": "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the callstack param be left out of these json-logs files? (I don't see it included for any other APIs)
34d137b
to
1105f3c
Compare
This PR adds the following support for work objects:
chat.unfurl
orchat.postMessage
to accept entity metadataCategory (place an
x
in each of the[ ]
)Requirements
Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.