Skip to content
This repository was archived by the owner on Nov 5, 2025. It is now read-only.

Conversation

@d-gubert
Copy link
Member

@d-gubert d-gubert commented Oct 14, 2019

Apps will need to update their slash commands for them to send messages correctly aware of the thread.

Implementation provided by RocketChat/Rocket.Chat#15574

@codecov
Copy link

codecov bot commented Oct 14, 2019

Codecov Report

Merging #164 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #164      +/-   ##
==========================================
+ Coverage   55.42%   55.51%   +0.09%     
==========================================
  Files          68       68              
  Lines        2396     2401       +5     
  Branches      360      360              
==========================================
+ Hits         1328     1333       +5     
  Misses       1068     1068
Impacted Files Coverage Δ
src/server/managers/AppSlashCommandManager.ts 99.38% <100%> (ø) ⬆️
src/server/accessors/MessageBuilder.ts 100% <100%> (ø) ⬆️

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 53ab0aa...9a582ef. Read the comment docs.

Copy link
Contributor

@graywolf336 graywolf336 left a comment

Choose a reason for hiding this comment

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

I like it overall. I understand this is just for slash commands, but is the thread id now being sent as part of the regular messages? Aka will the MessageSent events which are fired include the thread id?

@d-gubert
Copy link
Member Author

Yes, now they include the threadId.

I've opted to not send the actual thread message object as this would add another trip to the db, and then if some app needs the data from the thread message they can call MessageReader.getById()

@graywolf336
Copy link
Contributor

Wonder if we should add documentation to the property about that, that way people don't have to searching through issues and pull requests to get that information.


/**
* Retrieves the thread to which this message belongs,
* if any.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* if any.
* if any. Use the message reader to get the thread's message.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated it (see below). WDYT? 😛

@d-gubert d-gubert merged commit bf1c191 into master Oct 19, 2019
@d-gubert d-gubert deleted the thread-support branch October 19, 2019 16:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants