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

Base support for MSC3840: Ignore invites #2483

Closed
wants to merge 15 commits into from

Conversation

Yoric
Copy link
Contributor

@Yoric Yoric commented Jul 1, 2022

This PR expects that most of the work will be done at higher level,
where the user can decide to look at a list of ignored invites,
un-ignore them individually, etc.

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


Here's what your changelog entry will look like:

✨ Features

This PR expects that most of the work will be done at higher level,
where the user can decide to look at a list of ignored invites,
un-ignore them individually, etc.

Signed-off-by: David Teller <davidt@element.io>
@Yoric Yoric requested a review from a team as a code owner July 1, 2022 14:21
src/client.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.

overall lgtm, though needs tests :)

@Yoric
Copy link
Contributor Author

Yoric commented Jul 6, 2022

Adding a few unit tests.
I'm not convinced that they're testing anything useful, unfortunately.

@Yoric Yoric requested a review from turt2live July 7, 2022 07:39
* Gets the list of currently ignored invites.
*/
public getIgnoredInvites(): IgnoredInvites {
const event = this.getAccountData(EventType.IgnoredInvites);
Copy link

Choose a reason for hiding this comment

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

what's the computational complexity of this function call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't been able to find exactly where this is implemented. However, as far as I understand, this.getAccountData should be fast (because everything is cached to memory).

Also note that you can subscribe to client.on(ClientEvent.AccountData, event => { ... }) to be informed whenever a piece of account data changes.

Copy link

@jesopo jesopo left a comment

Choose a reason for hiding this comment

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

for what we're currently aiming to do, this looks correct to me

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.

generally this seems fine - thanks for adding the test, and apologies for missing some things in my last review.

@@ -1297,4 +1297,37 @@ describe("MatrixClient", function() {
expect(result!.aliases).toEqual(response.aliases);
});
});

describe("ignoring invites", () => {
it('stores ignored invites', async function() {
Copy link
Member

Choose a reason for hiding this comment

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

we should be using should, and arrow functions:

Suggested change
it('stores ignored invites', async function() {
it('should store ignored invites', async () => {

it('stores ignored invites', async function() {
// Mockup account data storage.
const dataStore = new Map();
client.setAccountData = function(eventType, content) {
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
client.setAccountData = function(eventType, content) {
client.setAccountData = (eventType, content) => {

dataStore.set(eventType, content);
return Promise.resolve();
};
client.getAccountData = function(eventType) {
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
client.getAccountData = function(eventType) {
client.getAccountData = (eventType) => {

};

// Initially, the invite list should be empty but not `null`.
expect(await client.getIgnoredInvites()).toEqual({});
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
expect(await client.getIgnoredInvites()).toEqual({});
expect(await client.getIgnoredInvites()).toStrictEqual({});

@@ -81,6 +81,7 @@ export enum EventType {
PushRules = "m.push_rules",
Direct = "m.direct",
IgnoredUserList = "m.ignored_user_list",
IgnoredInvites = "org.matrix.msc3840.ignored_invites",
Copy link
Member

Choose a reason for hiding this comment

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

we're not supposed to define unstable event types here - we should be using UnstableValue().name

src/client.ts Show resolved Hide resolved
Comment on lines +9349 to +9350
* The instant the user decided to ignore this invite,
* as a Matrix timestamp.
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
* The instant the user decided to ignore this invite,
* as a Matrix timestamp.
* The timestamp for when the user decided to ignore this invite.

}

/**
* Invites to ignore across all devices/sessions, as per MSC 3840.
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
* Invites to ignore across all devices/sessions, as per MSC 3840.
* Invites to ignore across all devices/sessions, as per MSC3840.
*
* See https://github.com/matrix-org/matrix-spec-proposals/pull/3840

* This interface is expected to gain additional fields, all
* of them optional.
*/
export interface IgnoreInviteMetadata {
Copy link
Member

Choose a reason for hiding this comment

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

All of these interfaces will need a jsdoc line that says they can be removed at any point without notice due to being unstable.

*/
export interface IgnoredInvites {
/**
* A map of room ids => metadata for rooms the user does not wish to be invited to.
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
* A map of room ids => metadata for rooms the user does not wish to be invited to.
* A map of room IDs => metadata for rooms the user does not wish to be invited to.

@Yoric
Copy link
Contributor Author

Yoric commented Aug 30, 2022

Closing in favour of another implementation based on MSC3847.

@t3chguy
Copy link
Member

t3chguy commented Aug 31, 2022

@Yoric you said closing in favour of X but didn't actually close it?

@Yoric Yoric closed this Aug 31, 2022
@Yoric
Copy link
Contributor Author

Yoric commented Aug 31, 2022

(oops)

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

Successfully merging this pull request may close these issues.

5 participants