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

Threads: Read receipts & notifications #1255

Merged
merged 4 commits into from
Sep 28, 2022
Merged

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Sep 27, 2022

Specifies matrix-org/matrix-spec-proposals#3771
Specifies matrix-org/matrix-spec-proposals#3773

This is reviewable commit-by-commit, for each of the MSCs listed above.

Note: This PR references a non-existent section called "Threading". That is specified by #1254 and thus this PR cannot land until 1254 does. Please see 1254 for details on what the "Threading" section entails.

Requires matrix-org/matrix-spec-proposals#3899 for clarifications.

Preview: https://pr1255--matrix-spec-previews.netlify.app

Note: this builds on a (as of writing) non-existent "threading" section, which is part of a different commit.
@turt2live turt2live mentioned this pull request Sep 27, 2022
@turt2live turt2live marked this pull request as ready for review September 27, 2022 04:36
@turt2live turt2live requested a review from a team as a code owner September 27, 2022 04:36
@turt2live turt2live added the release-blocker Blocks the next release from happening label Sep 27, 2022
@turt2live turt2live mentioned this pull request Sep 27, 2022
13 tasks
Copy link

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

Looks great! I added non blocking minor comments

content/client-server-api/modules/receipts.md Show resolved Hide resolved
content/client-server-api/modules/receipts.md Show resolved Hide resolved
content/client-server-api/modules/receipts.md Outdated Show resolved Hide resolved
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Looks good overall! Thanks for writing this up! 👍

@@ -22,33 +22,68 @@ that the user had read all events *up to* the referenced event. See the
[Receiving notifications](#receiving-notifications) section for more
information on how read receipts affect notification counts.

{{< added-in v="1.4" >}} Read receipts exist in three major forms:
Copy link
Member

Choose a reason for hiding this comment

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

MSC3771 doesn't specify that unthreaded/threaded applies only to read receipts, but there isn't currently any other type of receipts...

Copy link
Member Author

Choose a reason for hiding this comment

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

yea, we're very slowly moving this module to talk about only read receipts at the moment (at least until another receipt shows up)

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, just wanted to ensure this was on purpose.

content/client-server-api/modules/receipts.md Outdated Show resolved Hide resolved
content/client-server-api/modules/receipts.md Show resolved Hide resolved
content/client-server-api/modules/receipts.md Show resolved Hide resolved
@turt2live turt2live requested review from dbkr, erikjohnston, clokep and germain-gg and removed request for clokep and germain-gg September 27, 2022 19:33
@turt2live
Copy link
Member Author

@dbkr not sure if your approval was unconditional or not, between yourself and @erikjohnston I'll take whatever though :)

@clokep @gsouquet for some reason github hates me and will only let me re-request review from one of you. Given you both had feedback, please take a look at the changes.

@turt2live turt2live requested review from clokep and removed request for germain-gg September 27, 2022 19:36
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me.

@dbkr
Copy link
Member

dbkr commented Sep 28, 2022

Yep, still lgtm

@turt2live turt2live merged commit 227757d into main Sep 28, 2022
@turt2live turt2live deleted the travis/spec/threads-rr branch September 28, 2022 20:49
zecakeh added a commit to zecakeh/matrix-spec that referenced this pull request Oct 1, 2022
zecakeh added a commit to zecakeh/matrix-spec that referenced this pull request Oct 1, 2022
It seems to have been omitted in matrix-org#1255

Signed-off-by: Kévin Commaille <[email protected]>
turt2live added a commit that referenced this pull request Oct 4, 2022
* Receipts: Add thread_id to the /receipt endpoint

It seems to have been omitted in #1255

Signed-off-by: Kévin Commaille <[email protected]>

* changelog

Signed-off-by: Kévin Commaille <[email protected]>

* Fix missing backtick

* Apply suggestion for error description

Co-authored-by: Travis Ralston <[email protected]>

Signed-off-by: Kévin Commaille <[email protected]>
Co-authored-by: Travis Ralston <[email protected]>
turt2live added a commit that referenced this pull request Oct 4, 2022
* Receipts: Add thread_id to the /receipt endpoint

It seems to have been omitted in #1255

Signed-off-by: Kévin Commaille <[email protected]>

* changelog

Signed-off-by: Kévin Commaille <[email protected]>

* Fix missing backtick

* Apply suggestion for error description

Co-authored-by: Travis Ralston <[email protected]>

Signed-off-by: Kévin Commaille <[email protected]>
Co-authored-by: Travis Ralston <[email protected]>
clokep pushed a commit to clokep/matrix-spec that referenced this pull request May 3, 2023
* Spec MSC3771: Threaded read receipts

Note: this builds on a (as of writing) non-existent "threading" section, which is part of a different commit.

* Spec MSC3773: Threaded notifications

* changelog

* Various clarifications per review
clokep pushed a commit to clokep/matrix-spec that referenced this pull request May 3, 2023
* Receipts: Add thread_id to the /receipt endpoint

It seems to have been omitted in matrix-org#1255

Signed-off-by: Kévin Commaille <[email protected]>

* changelog

Signed-off-by: Kévin Commaille <[email protected]>

* Fix missing backtick

* Apply suggestion for error description

Co-authored-by: Travis Ralston <[email protected]>

Signed-off-by: Kévin Commaille <[email protected]>
Co-authored-by: Travis Ralston <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocker Blocks the next release from happening
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants