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

Add context 'user_id' extraction for 'message_changed' and 'message_deleted' events #736

Closed
wants to merge 4 commits into from
Closed

Add context 'user_id' extraction for 'message_changed' and 'message_deleted' events #736

wants to merge 4 commits into from

Conversation

eddyg
Copy link
Contributor

@eddyg eddyg commented Oct 9, 2022

I noticed that context.user_id was not being set for message_changed or message_deleted message subtype events. user is located in payloadeventmessage for message_changed events, and in payloadeventprevious_message for message_deleted events.

The first and third commits are for message_changed (with tests); the fourth commit is for message_deleted (with tests).

The second commit addresses a typing issue I noticed by ensuring that in is not being compared to None (the original code would generate "in" not supported for type "None" warnings. I also removed the # type: ignore comments on the associated return statements as they don't seem to be required.

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 [ ])

  • 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 self-assigned this Oct 9, 2022
@seratch seratch added enhancement New feature or request area:async area:sync labels Oct 9, 2022
@seratch seratch added this to the 1.15.2 milestone Oct 9, 2022
@seratch
Copy link
Member

seratch commented Oct 9, 2022

Hi @eddyg, thanks for sending this PR! Could you add the unit tests covering the pattern? Adding new files like the following would be easier to maintain for us. Please note that we need tests for both App and AsyncApp. Thanks!

@codecov
Copy link

codecov bot commented Oct 9, 2022

Codecov Report

Merging #736 (e38b7d1) into main (87dccda) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #736   +/-   ##
=======================================
  Coverage   92.05%   92.06%           
=======================================
  Files         172      172           
  Lines        5869     5873    +4     
=======================================
+ Hits         5403     5407    +4     
  Misses        466      466           
Impacted Files Coverage Δ
slack_bolt/request/internals.py 93.87% <100.00%> (+0.17%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@eddyg
Copy link
Contributor Author

eddyg commented Oct 9, 2022

Hi @eddyg, thanks for sending this PR! Could you add the unit tests covering the pattern? Adding new files like the following would be easier to maintain for us. Please note that we need tests for both App and AsyncApp. Thanks!

Alright, I took a shot at a simple test case (including an async version) to verify context.user_id in message_changed events...

@eddyg eddyg changed the title Add context 'user_id' extraction for 'message_changed' events Add context 'user_id' extraction for 'message_changed' and 'message_deleted' events Oct 10, 2022
@seratch
Copy link
Member

seratch commented Oct 14, 2022

@eddyg Thanks for adding tests! I have a few things to verify before merging this. I will work on it early next week.

@seratch
Copy link
Member

seratch commented Oct 17, 2022

@eddyg Thanks again for this contribution! We've merged your changes at 5c8e2f2

@seratch seratch closed this Oct 17, 2022
@eddyg eddyg deleted the improve-user-id-extraction branch October 17, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants