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

Adding support and check for the event subtypes #451

Conversation

gaspardpetit
Copy link
Contributor

Signed-off-by: Gaspard Petit gaspard.petit@eidosmontreal.com

Summary

Right now the RTM client does not consider the subtype when deserializing messages and dispatching them. This results in class conversion exceptions when multiple handlers are registered for different events of the same types, ex. MessageEvent and MessageChangedEvents. Both receive the notification possibly with the wrong object type.

Requirements (place an x in each [ ])

Signed-off-by: Gaspard Petit <gaspard.petit@eidosmontreal.com>
@CLAassistant
Copy link

CLAassistant commented May 8, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thanks, I will take a look at this later. Check one comment I left first.

@seratch seratch added the project:slack-api-client project:slack-api-client label May 8, 2020
Signed-off-by: Gaspard Petit <gaspard.petit@eidosmontreal.com>
@codecov
Copy link

codecov bot commented May 8, 2020

Codecov Report

Merging #451 into master will decrease coverage by 0.35%.
The diff coverage is 10.81%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #451      +/-   ##
============================================
- Coverage     83.99%   83.64%   -0.36%     
- Complexity     2385     2386       +1     
============================================
  Files           250      250              
  Lines          6406     6438      +32     
  Branches        579      586       +7     
============================================
+ Hits           5381     5385       +4     
- Misses          667      694      +27     
- Partials        358      359       +1     
Impacted Files Coverage Δ Complexity Δ
...c/main/java/com/slack/api/rtm/RTMEventHandler.java 45.45% <0.00%> (-19.77%) 4.00 <0.00> (ø)
...ava/com/slack/api/rtm/RTMEventsDispatcherImpl.java 47.36% <14.81%> (-11.90%) 13.00 <1.00> (+1.00) ⬇️

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 9dcb2e6...a30b173. Read the comment docs.

seratch added a commit that referenced this pull request May 12, 2020
@seratch seratch merged commit 5743a92 into slackapi:master May 12, 2020
@seratch
Copy link
Member

seratch commented May 12, 2020

Thanks for your contribution 🙇‍♂️ I've verified your change works and merged into the master branch 🎉

@gaspardpetit gaspardpetit deleted the java_slack_sdk_support_subtypes_when_deserializing branch May 16, 2020 21:39
@gaspardpetit
Copy link
Contributor Author

Thanks for adding a test and accepting my PR! I will do my best to include one next time I contribute.

seratch added a commit that referenced this pull request May 28, 2020
* [slack-api-model] Add missing fields in objects (confirm.style in blocks, user.is_invited_user: boolean, message.hidden, Slack post related fields in file objects) - thanks @seratch
* [slack-api-client etc] #466 #462 Calls API support - thanks @seratch
* [slack-api-client] #475 #474 Make redirect_uri for oauth.access / oauth.v2.access optional - thanks @natevaughan @seratch
* [slack-api-client] #476 Bump dependencies (okhttp, micronaut, tyrus-standalone-client) - thanks @seratch
* [slack-api-client] #459 Adding ping message and pong event to RTM client - thanks @gaspardpetit
* [slack-api-client] #451 Add support and check for the event subtypes in RTM client - thanks @gaspardpetit
* [bolt] #455 Improve OAuth flow module to consider the cases where team is missing in oauth.v2.access responses - thanks @seratch
* [bolt] #476 Bump dependencies (aws-java-sdk-s3) - thanks @seratch
emanguy pushed a commit to emanguy/java-slack-sdk that referenced this pull request Jun 22, 2020
emanguy pushed a commit to emanguy/java-slack-sdk that referenced this pull request Jun 22, 2020
* [slack-api-model] Add missing fields in objects (confirm.style in blocks, user.is_invited_user: boolean, message.hidden, Slack post related fields in file objects) - thanks @seratch
* [slack-api-client etc] slackapi#466 slackapi#462 Calls API support - thanks @seratch
* [slack-api-client] slackapi#475 slackapi#474 Make redirect_uri for oauth.access / oauth.v2.access optional - thanks @natevaughan @seratch
* [slack-api-client] slackapi#476 Bump dependencies (okhttp, micronaut, tyrus-standalone-client) - thanks @seratch
* [slack-api-client] slackapi#459 Adding ping message and pong event to RTM client - thanks @gaspardpetit
* [slack-api-client] slackapi#451 Add support and check for the event subtypes in RTM client - thanks @gaspardpetit
* [bolt] slackapi#455 Improve OAuth flow module to consider the cases where team is missing in oauth.v2.access responses - thanks @seratch
* [bolt] slackapi#476 Bump dependencies (aws-java-sdk-s3) - thanks @seratch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants