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

Functional members #1771

Merged
merged 21 commits into from
Jul 22, 2021
Merged

Functional members #1771

merged 21 commits into from
Jul 22, 2021

Conversation

jaller94
Copy link

@jaller94 jaller94 commented Jul 6, 2021

At Element Matrix Services we're experimenting with "functional members", e.g. bots you want to have in a DM room with someone else.

Currently, if I have a DM room with Alice and my bot "Helper Bot" the DM room will be called "Alice and 1 other" (or "Helper Bot" and 1 other").
We want to filter bots based on a state event so that the room name will be just "Alice".

The state event content for the type io.element.functional_members would look like this:

{
    "service_members": [
        "@helperbot:localhost",
        "@reminderbot:alice.tdl"
    ]
}

I'm writing this PR as an employee at Element, so I'm not sure the Sign-Off is required.

Signed-off-by: Christian Paul christianp@element.io

@jaller94 jaller94 marked this pull request as draft July 6, 2021 16:06
src/@types/event.ts Outdated Show resolved Hide resolved
src/models/room.ts Outdated Show resolved Hide resolved
@t3chguy t3chguy changed the base branch from master to develop July 6, 2021 16:37
@t3chguy
Copy link
Member

t3chguy commented Jul 6, 2021

(Updated base branch so it doesn't accidentally go to the wrong place.)

@jaller94 jaller94 changed the title WIP: Functional members Functional members Jul 6, 2021
Christian Paul and others added 4 commits July 6, 2021 18:58
@jaller94 jaller94 marked this pull request as ready for review July 6, 2021 17:19
src/@types/event.ts Outdated Show resolved Hide resolved
src/@types/event.ts Outdated Show resolved Hide resolved
@jaller94 jaller94 requested a review from a team July 8, 2021 15:12
@t3chguy
Copy link
Member

t3chguy commented Jul 8, 2021

Adding tests for this behaviour would be great

@jaller94 jaller94 removed the request for review from a team July 10, 2021 01:43
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 t3chguy says, tests would be appreciated for this. From my part, it'd be good to see tests for the abusive cases, and a paragraph somewhere on this PR acknowledging the room for abuse and how that is mitigated (if at all).

Some abusive cases include:

  • Existing DMs with participants where excluding the member might cause an empty name (eg: 1:1 DM and manually excluding a non-service user). Alternatively, putting all users from the DM into the excluded users list.
  • Excluding users which are not present in the room (would expect the performance to not be impacted, and for the code to not explode)

It's particularly concerning that the exclusion is based on user ID only as that isn't a reliable factor for whether or not the user is truly a service/bot user or not. It's largely fine for this case, I think, but acknowledging the vector is appreciated.

*/
export const UNSTABLE_ELEMENT_FUNCTIONAL_USERS = new UnstableValue(
"io.element.functional_members",
"io.element.functional_members");
Copy link
Member

Choose a reason for hiding this comment

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

I'd sooner hop into UnstableValue and mark the second field as optional, fixing any docs along the way.

Copy link
Author

Choose a reason for hiding this comment

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

I'd make it a type of its own, as UnstableValue has a different meaning, is exported and also requires unstable to be defined.

Or do you mean to set .unstable to the same value as .name if no second parameter is provided to the constructor?

src/models/room.ts Outdated Show resolved Hide resolved
src/@types/event.ts Outdated Show resolved Hide resolved
@jaller94 jaller94 requested a review from turt2live July 20, 2021 10:51
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.

Thanks!

@turt2live turt2live merged commit 2bd99b5 into develop Jul 22, 2021
@turt2live turt2live deleted the j94/functional-members branch July 22, 2021 00:25
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.

None yet

4 participants