-
Notifications
You must be signed in to change notification settings - Fork 152
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 tests for various forms of rich replies #1799
Conversation
391d19c
to
f1267ba
Compare
Signed-off-by: Tadeusz „tadzik” Sośnierz <tadeusz@sosnierz.com>
f1267ba
to
19bf636
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable, thanks for improving the test coverage!
src/bridge/MatrixHandler.ts
Outdated
@@ -386,6 +393,9 @@ export class MatrixHandler { | |||
* @param {Object} event : The Matrix member event. | |||
*/ | |||
private _onMemberEvent(req: BridgeRequest, event: OnMemberEventData) { | |||
if (event.origin_server_ts) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only joins, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, should be, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, also we need to bound these by leave time too. Or maybe just delete the map entry, given you won't be replying to anything after you've left.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, it still works in this state, since your subsequent join will reset the map entry –so you won't be able to see messages from before your most recent join.
Still, I'd rather be explicit and have a test for it than rely on this quirk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(ffcba6f)
Cleaned up and expanded #1797