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

MSC2153: Add a default push rule to ignore m.reaction events #2153

Closed
wants to merge 2 commits into from

Conversation

jryans
Copy link
Contributor

@jryans jryans commented Jul 4, 2019

Reactions are considered "metadata" that annotates an existing event and thus they should not by default trigger notifications. This adds a new default override rule that ignores reaction events for the purpose of notifications.

Rendered

FCP Checkboxes


Implementations:

@jryans jryans added proposal A matrix spec change proposal proposal-in-review labels Jul 4, 2019
@jryans jryans changed the title Add reaction push rule proposal MSC2153: Add reaction push rule proposal Jul 4, 2019
@jryans jryans changed the title MSC2153: Add reaction push rule proposal MSC2153: Add a default push rule to ignore m.reaction events Jul 4, 2019
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

Looks very plausible.

proposals/2153-reaction-push-rule.md Outdated Show resolved Hide resolved
Co-Authored-By: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
jryans added a commit to matrix-org/matrix-js-sdk that referenced this pull request Jul 4, 2019
This adds a default push rule to ignore reactions as proposed in
[MSC2153](matrix-org/matrix-spec-proposals#2153). By adding it here
in the client directly, we can try out the idea early even if it hasn't appeared
in the user's HS yet.

Part of element-hq/element-web#10208
@turt2live turt2live self-requested a review July 4, 2019 14:30
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

lgtm

soliciting review from @dbkr as the push rule expert would also be ideal.

@jryans jryans requested a review from dbkr July 4, 2019 17:07
@anoadragon453
Copy link
Member

Seems like people are generally happy with this one, so:

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Jul 5, 2019

This FCP proposal has been cancelled by #2153 (comment).

Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people:

Concerns:

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Jul 5, 2019
jryans added a commit to matrix-org/synapse that referenced this pull request Jul 5, 2019
@uhoreg
Copy link
Member

uhoreg commented Jul 5, 2019

Looks reasonable, but as a housekeeping matter:

@mscbot concern should not be merged until MSC1849 is ready

jryans added a commit to matrix-org/synapse that referenced this pull request Jul 5, 2019
@KitsuneRal
Copy link
Member

I wonder if it makes sense to just merge this as a section in MSC1849.

@turt2live turt2live added the kind:maintenance MSC which clarifies/updates existing spec label Apr 20, 2020
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

One small thing.

}
```

## Tradeoffs
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Tradeoffs
## Alternatives

@richvdh
Copy link
Member

richvdh commented Jul 6, 2020

as a general note, I see that, as with the rest of reactions, this is already implemented and we have servers and clients relying on it.

@richvdh
Copy link
Member

richvdh commented May 23, 2022

This is, aiui, superceded by msc2677, so

@mscbot fcp close

@mscbot
Copy link
Collaborator

mscbot commented May 23, 2022

Team member @mscbot has proposed to close this. The next step is review by the rest of the tagged people:

Concerns:

  • Reactions don't exist

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

@mscbot mscbot added disposition-close proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels May 23, 2022
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

I'll also link to the implementations in the PR description to clear the label.

"Requested changes" to note that there's things which need to be done before merge, but not before FCP.

proposals/2153-reaction-push-rule.md Show resolved Hide resolved
}
],
"actions": [
"dont_notify"
Copy link
Member

Choose a reason for hiding this comment

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

proposals/2153-reaction-push-rule.md Show resolved Hide resolved
@turt2live turt2live removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label May 31, 2022
@turt2live
Copy link
Member

though, we should land/FCP reactions themselves first, which is why we originally cancelled FCP. For now let's just block it:

@mscbot concern Reactions don't exist

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label May 31, 2022
@turt2live
Copy link
Member

except I'm blind and this is for an FCP closure. Instead of chasing 8 checkboxes let's see if @jryans is okay with this MSC being incorporated into MSC2677 and close as obsolete?

@mscbot resolve Reactions don't exist

@mscbot mscbot removed the unresolved-concerns This proposal has at least one outstanding concern label May 31, 2022
@mscbot
Copy link
Collaborator

mscbot commented Jul 19, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Jul 19, 2022
@mscbot
Copy link
Collaborator

mscbot commented Jul 24, 2022

The final comment period, with a disposition to close, as per the review above, is now complete.

@mscbot mscbot closed this Jul 24, 2022
@mscbot mscbot added finished-final-comment-period and removed disposition-close final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Jul 24, 2022
@turt2live turt2live added obsolete A proposal which has been overtaken by other proposals and removed finished-final-comment-period labels Jul 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:maintenance MSC which clarifies/updates existing spec obsolete A proposal which has been overtaken by other proposals proposal A matrix spec change proposal
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants