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

Implement MSC3531: Hiding messages during moderation #2041

Merged
merged 1 commit into from
Jan 12, 2022
Merged

Conversation

Yoric
Copy link

@Yoric Yoric commented Nov 29, 2021

Matrix-js-sdk-level support for hiding messages.

This is my first patch in matrix-js-sdk, please accept my apologies for anything I have done wrong.

MSC: matrix-org/matrix-spec-proposals#3531

Signed-off-by: David Teller [email protected]


This PR currently has no changelog labels, so will not be included in changelogs.

A reviewer can add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is, or add Type: [enhancement/defect/task] to the description and I'll add them for you.

@Yoric Yoric requested a review from a team as a code owner November 29, 2021 16:26
@turt2live turt2live changed the title MSC3531: Hiding messages during moderation Implement MSC3531: Hiding messages during moderation Nov 30, 2021
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.

As a first pass there's mostly spec-related workflow concerns on this, but the change overall seems sensible. I've also left a review on the MSC itself which might ultimately impact the review of the actual visibility change code.

src/models/event.ts Outdated Show resolved Hide resolved
src/models/event.ts Outdated Show resolved Hide resolved
src/models/event.ts Outdated Show resolved Hide resolved
src/models/event.ts Outdated Show resolved Hide resolved
src/models/event.ts Outdated Show resolved Hide resolved
src/models/event.ts Outdated Show resolved Hide resolved
src/models/room.ts Show resolved Hide resolved
src/models/room.ts Outdated Show resolved Hide resolved
src/models/room.ts Outdated Show resolved Hide resolved
@Yoric
Copy link
Author

Yoric commented Nov 30, 2021

Apologies, I realized too late that I was missing a big part of the patch to handle redactions. Added.

@Yoric Yoric requested a review from turt2live December 2, 2021 11:09
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.

overall this is looking fine - just a few more nitpicks, mostly around the project structure

src/@types/event.ts Outdated Show resolved Hide resolved
src/@types/event.ts Outdated Show resolved Hide resolved
src/models/event.ts Outdated Show resolved Hide resolved
src/models/event.ts Outdated Show resolved Hide resolved
src/models/event.ts Outdated Show resolved Hide resolved
src/models/room.ts Outdated Show resolved Hide resolved
src/models/room.ts Outdated Show resolved Hide resolved
src/models/room.ts Outdated Show resolved Hide resolved
src/models/room.ts Outdated Show resolved Hide resolved
src/models/room.ts Outdated Show resolved Hide resolved
@Yoric Yoric requested a review from turt2live December 9, 2021 08:43
src/@types/event.ts Outdated Show resolved Hide resolved
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.

almost there :)

There's also MSC feedback which will need addressing ftr. Those aspects are not covered in code review.

src/models/event.ts Outdated Show resolved Hide resolved
src/models/room-state.ts Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
src/@types/event.ts Outdated Show resolved Hide resolved
@Yoric Yoric requested review from turt2live and t3chguy January 6, 2022 09:36
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.

one more step, and then we're there :)

src/models/room-state.ts Outdated Show resolved Hide resolved
src/models/room.ts Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
@Yoric Yoric requested a review from turt2live January 11, 2022 11:46
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.

alright, let's send it.

@turt2live
Copy link
Member

... after you fix merge conflicts and the build :D (try merging develop - should fix both, hopefully)

@turt2live
Copy link
Member

(please don't force push onto reviewed branches - it invalidates review, thus making it required for re-review. In this case, the changes appear fine)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants