Skip to content

Fix #644 app.message listener does not handle events when a file is attached - #645

Merged
seratch merged 1 commit into
slackapi:mainfrom
seratch:issue-644
May 11, 2022
Merged

seratch merged 1 commit into
slackapi:mainfrom
seratch:issue-644

Conversation

@seratch

@seratch seratch commented May 11, 2022

Copy link
Copy Markdown
Contributor

This pull request resolves #644

When it comes to the feature parity with bolt-js, we may want to remove the subtype constraints in bolt-python. bolt-js does not check any subtypes when matching message text in app.message listeners: https://github.com/slackapi/bolt-js/blob/%40slack/bolt%403.11.0/src/App.ts#L602-L615

That being said, changing bolt-python app.message listeners to handle other message patterns by Slack product (e.g., channel_join, channel_posting_permissions) could be a breaking change to existing apps. Also, more importantly, passing message_changed and so on would require code changes on the bolt-python user side for sure plus the behavior should not be desired at all.

For this reason, I didn't change the part drastically and went with just adding file_share subtype to the allowed list.

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

  • slack_bolt.App and/or its core components
  • slack_bolt.async_app.AsyncApp and/or its core components
  • Adapters in slack_bolt.adapter
  • Document pages under /docs
  • Others

Requirements (place an x in each [ ])

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.

  • 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 run ./scripts/install_all_and_run_tests.sh after making the changes.

@seratch seratch added bug Something isn't working area:async area:sync labels May 11, 2022
@seratch seratch added this to the 1.13.2 milestone May 11, 2022
@seratch
seratch requested review from filmaj, mwbrooks and srajiang May 11, 2022 00:56
@seratch seratch self-assigned this May 11, 2022
@codecov

codecov Bot commented May 11, 2022

Copy link
Copy Markdown

Codecov Report

Merging #645 (5a02496) into main (20bfec8) will not change coverage.
The diff coverage is n/a.

❗ Current head 5a02496 differs from pull request most recent head 138abe1. Consider uploading reports for the commit 138abe1 to get more accurate results

@@           Coverage Diff           @@
##             main     #645   +/-   ##
=======================================
  Coverage   92.04%   92.04%           
=======================================
  Files         170      170           
  Lines        5793     5793           
=======================================
  Hits         5332     5332           
  Misses        461      461           
Impacted Files Coverage Δ
slack_bolt/app/app.py 87.27% <ø> (ø)
slack_bolt/app/async_app.py 94.20% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20bfec8...138abe1. Read the comment docs.

@filmaj filmaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this change!

Regarding the potential breaking change to line up behaviour with bolt-js, my view on this is that consistency between bolt-js and bolt-python is more a nicety for us as maintainers rather than for bolt users. I think bolt users would probably stay with one language, generally, and therefore not expect identical behaviour if they move to a different language for bolt? What does everyone else think?

If the above assumption is accurate or agreed on by the rest of the team, then releasing a breaking change just to improve maintainers lives seems heavy-handed and should not be done just for that one change. However, if we have other, user-facing breaking changes we would like to land, then adding the subtype matching change in together with other breaking changes in the next major release may make sense.

@celestinojones

celestinojones commented May 11, 2022 •

Copy link
Copy Markdown

Just wanted to add to the discussion for some context. I can confirm that there are teams who work with a combination of our SDKs across sectors, and one of them noticed the inconsistency first and brought it up to support, so this is in response to user feedback.

@filmaj

filmaj commented May 11, 2022

Copy link
Copy Markdown
Contributor

Thank you for that additional context @celestinojones, your input helps make sure we are in line with customer expectations 🙏

@seratch

seratch commented May 11, 2022

Copy link
Copy Markdown
Contributor Author

@filmaj @celestinojones Thanks for the comments!

As for other subtypes apart from file_share, we may want to revisit in the future if it's worth. However, I think that we should not decide to simply change the app.message listener behavior without any options to opt-in (for existing bolt-python apps). I have to say that the subtypes in message events are tricky in the nature. We can be more careful than usual for the behavior change.

Either way, this file_share subtype pattern is an obvious bug, so that we should fix this as early as possible. Thus, we can merge this PR for the upcoming patch version release now. Thanks for the review and discussion!

@seratch
seratch merged commit 97e5b15 into slackapi:main May 11, 2022
@seratch
seratch deleted the issue-644 branch May 11, 2022 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:async area:sync bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

app.message listener does not handle events when a file is attached

4 participants