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

Discussion: Improvements around app.message(...) #465

Open
4 of 9 tasks
seratch opened this issue Apr 6, 2020 · 11 comments
Open
4 of 9 tasks

Discussion: Improvements around app.message(...) #465

seratch opened this issue Apr 6, 2020 · 11 comments
Assignees
Labels
discussion M-T: An issue where more input is needed to reach a decision semver:major
Milestone

Comments

@seratch
Copy link
Member

seratch commented Apr 6, 2020

Proposal

As described at #463, app.message(...)'s behavior with the latest payloads is not intuitive for developers. It's surprising for developers to receive more events than the number of messages in a channel.

I propose to change the behavior to receive only message typed events that don't have a subtype. With this, Bolt apps can receive only events telling the arrival of new messages.

Also. there is one additional thing we may need to care about in the future. As far as I know, the server-side no longer sends bot_message subtyped events. Instead, it sends no-subtype events with bot_id and bot_profile properties. If the server-side gets back to the previous behavior in the future, we may need to add bot_message events as well.

For reference: @aoberoi 's slackapi/hubot-slack#589

What happened to the subtype: 'bot_message'? It seems to be missing on at least some, but maybe all, events where it should be.

References

The list of sutyped message events

There are a number of subtype events for messages. Some of them may have a quite different data structure compared with message typed events without a subtype.

  • bot_message
  • ekm_access_denied
  • me_message
  • message_changed
  • message_deleted
  • message_replied
  • reply_broadcast
  • thread_broadcast

Bolt for Java's approach

Bolt for Java's app.message listeners receive only message events that don't have a subtype.
https://github.com/slackapi/java-slack-sdk/blob/v1.0.3/bolt/src/main/java/com/slack/api/bolt/App.java#L509-L526

There are two reasons for that. It's technically impossible to include other events due to the Reason 2.

Regarding bot_message events, Bolt doesn't invoke listeners given to app.message due to the difference in the structure of payloads. If the server-side is changed in the future, Bolt for Java may need to consider introducing a new routing method.

What type of issue is this? (place an x in one of the [ ])

  • bug
  • enhancement (feature request)
  • question
  • documentation related
  • testing related
  • discussion

Requirements (place an x in each of the [ ])

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've searched for any related issues and avoided creating a duplicate issue.
@seratch seratch added the discussion M-T: An issue where more input is needed to reach a decision label Apr 6, 2020
@aoberoi
Copy link
Contributor

aoberoi commented Apr 13, 2020

This is a well thought out and thorough proposal!

Question: In Bolt for Java, would it be possible to use constraints properties to vary the type of arguments passed to a listener? This is how we've been able to solve this problem in Bolt for JavaScript in the app.event<EventType>() method. I think this is a clean solution that doesn't expand the App API too much, but it might depend on unique capabilities of the TypeScript type system.

@seratch
Copy link
Member Author

seratch commented Apr 14, 2020

Question: In Bolt for Java, would it be possible to use constraints properties to vary the type of arguments passed to a listener?

Yes, it's possible and even is necessary for Bolt for Java apps. Developers are not allowed to make the type ambiguous.

Here is a simple example demonstrating how to use it:
https://github.com/slackapi/java-slack-sdk/blob/v1.0.3/bolt-servlet/src/test/java/samples/EventsSample.java#L21-L29
The first one in the listener args is typed with the given event class like MessageEvent. The type constraint is implemented this way: https://github.com/slackapi/java-slack-sdk/blob/v1.0.3/bolt/src/main/java/com/slack/api/bolt/App.java#L492

@stevengill
Copy link
Member

Thanks for writing this out @seratch!

So the proposal is to have app.message only handle message type events. Sounds fine to me.

Questions:

  1. would this be a breaking change?
  2. Is there any other way for bolt users to still handle events such as message_changed?

@seratch
Copy link
Member Author

seratch commented Apr 16, 2020

would this be a breaking change?

Good point. Yes, it is a breaking change. So, it may be better to consider a bit careful transition like providing an option to change the behavior in the next minor release, and then changing the default in the next major release.

Is there any other way for bolt users to still handle events such as message_changed?

I think app.event is the best way to handle those. Developers can check what the subtype is inside a listener or middleware.

@stevengill stevengill self-assigned this May 7, 2020
@seratch seratch added this to the 4.0.0 milestone Mar 23, 2021
@seratch
Copy link
Member Author

seratch commented Mar 23, 2021

As far as I know, the server-side no longer sends bot_message subtyped events.

This was a wrong assumption. If other bot users in the same workspace are still classic apps, they may generate message events with bot_message subtype.

As of version 1.4.4, bolt-python handles only non-subtyped and "bot_message" subtype events by app.message listeners. This is the behavior most developers should expect. We can revisit this issue and it's worth considering to apply the behavior changes in v4.0 (of course, with a clear mention about it in the migration guide).

@batflarrow
Copy link

We can revisit this issue and it's worth considering to apply the behavior changes i

Hey so if I am building a slack app currently and receiving events via the slack events web api should I handle the cases with subtype as bot_message or consider they will not be subtyped with the presence of bot_id and bot_profile properties?

@seratch
Copy link
Member Author

seratch commented May 26, 2022

@batflarrow As I mentioned above, both no subtype and "bot_message" subtype patterns can still co-exist. If other bots in your workspace are still based on legacy permission model (we call it "classic" app), their message events don't have the bot_message subtype.

@batflarrow
Copy link

batflarrow commented May 26, 2022

@batflarrow As I mentioned above, both no subtype and "bot_message" subtype patterns can still co-exist. If other bots in your workspace are still based on legacy permission model (we call it "classic" app), their message events don't have the bot_message subtype.

It's the other way round right "classic" app's messages with have a sub_type as bot_message and not the new apps this was what you mentioned above in this comment.

@batflarrow
Copy link

@batflarrow As I mentioned above, both no subtype and "bot_message" subtype patterns can still co-exist. If other bots in your workspace are still based on legacy permission model (we call it "classic" app), their message events don't have the bot_message subtype.

It's the other way round right "classic" app's messages with have a sub_type as bot_message and not the new apps?

Also is there a list of these "classic" apps, how can I test my code for cases with sub_type as bot_message or is there a way just to know the payload in cases where the events will be sub_typed as bot_message

@seratch
Copy link
Member Author

seratch commented May 26, 2022

If you don't have so many apps in your workspace, it may not be so hard to review the scopes of the apps listed at https://slack.com/apps/manage . However, I think that your goal is to safely respond to all new messages. If so, all you need to do are:

  • Check if a message event has "bot_message" subtype or does not have any subtype
  • If yes, extract text property from the event data and respond to it

@batflarrow
Copy link

batflarrow commented May 26, 2022

If you don't have so many apps in your workspace, it may not be so hard to review the scopes of the apps listed at https://slack.com/apps/manage . However, I think that your goal is to safely respond to all new messages. If so, all you need to do are:

  • Check if a message event has "bot_message" subtype or does not have any subtype
  • If yes, extract text property from the event data and respond to it

yeah but I am building a slack app myself for people to use and they can have any apps installed on their systems themselves so I will have to handle the cases with the "bot_message" subtype. It would be great if I could find a event with the sub_type as bot_message but anyways I'll look for it .The issue is the I need the bot_profile.app_id of the bot in all cases and is it available in the cases where the event is sub_typed as 'bot_message' Also can you confirm "classic" apps have the subtype right and not the new apps coz you mentioned it the otherway round here and here.

@filmaj filmaj modified the milestones: 4.0.0, 5.0.0 Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion M-T: An issue where more input is needed to reach a decision semver:major
Projects
None yet
Development

No branches or pull requests

5 participants