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 this to Needs idea feedback / initial review in Spec Core Team Backlog via automation Sep 27, 2022
@turt2live turt2live moved this from Needs idea feedback / initial review to Requires spec review (post-FCP) in Spec Core Team Backlog Sep 27, 2022
@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
Contributor

@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
Contributor

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
Contributor

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
Contributor

@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
@turt2live turt2live moved this from Requires spec review (post-FCP) to Done to some definition in Spec Core Team Backlog Sep 28, 2022
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 <zecakeh@tedomum.fr>
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 <zecakeh@tedomum.fr>

* changelog

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

* Fix missing backtick

* Apply suggestion for error description

Co-authored-by: Travis Ralston <travpc@gmail.com>

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Co-authored-by: Travis Ralston <travpc@gmail.com>
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 <zecakeh@tedomum.fr>

* changelog

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

* Fix missing backtick

* Apply suggestion for error description

Co-authored-by: Travis Ralston <travpc@gmail.com>

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Co-authored-by: Travis Ralston <travpc@gmail.com>
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 <zecakeh@tedomum.fr>

* changelog

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

* Fix missing backtick

* Apply suggestion for error description

Co-authored-by: Travis Ralston <travpc@gmail.com>

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Co-authored-by: Travis Ralston <travpc@gmail.com>
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
Spec Core Team Backlog
  
Done to some definition
Development

Successfully merging this pull request may close these issues.

None yet

5 participants