Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Refactor shield display logic; changed rules for DMs #4290

Merged
merged 7 commits into from
Mar 30, 2020

Conversation

foldleft
Copy link
Contributor

@foldleft foldleft commented Mar 27, 2020

@foldleft foldleft requested a review from a team March 27, 2020 13:46
@foldleft
Copy link
Contributor Author

foldleft commented Mar 27, 2020

This should represent a truth-table for all the behaviours we want to check, given the number of users in a room, DM status, and self-trust status:

  shieldStatusForMembership self-trust behaviour
    ✓ 2 unverified: returns 'normal', self-trust = true, DM = true
    ✓ 2 unverified: returns 'normal', self-trust = true, DM = false (1ms)
    ✓ 2 unverified: returns 'normal', self-trust = false, DM = true
    ✓ 2 unverified: returns 'normal', self-trust = false, DM = false
    ✓ 2 verified: returns 'verified', self-trust = true, DM = true
    ✓ 2 verified: returns 'verified', self-trust = true, DM = false
    ✓ 2 verified: returns 'verified', self-trust = false, DM = true (1ms)
    ✓ 2 verified: returns 'warning', self-trust = false, DM = false
    ✓ 2 mixed: returns 'normal', self-trust = true, DM = true
    ✓ 2 mixed: returns 'normal', self-trust = true, DM = false
    ✓ 2 mixed: returns 'normal', self-trust = false, DM = true
    ✓ 2 mixed: returns 'warning', self-trust = false, DM = false (1ms)
    ✓ 0 others: returns 'verified', self-trust = true, DM = true
    ✓ 0 others: returns 'verified', self-trust = true, DM = false
    ✓ 0 others: returns 'warning', self-trust = false, DM = true
    ✓ 0 others: returns 'warning', self-trust = false, DM = false
    ✓ 1 verified: returns 'verified', self-trust = true, DM = true
    ✓ 1 verified: returns 'verified', self-trust = true, DM = false (1ms)
    ✓ 1 verified: returns 'verified', self-trust = false, DM = true
    ✓ 1 verified: returns 'verified', self-trust = false, DM = false
    ✓ 1 unverified: returns 'normal', self-trust = true, DM = true
    ✓ 1 unverified: returns 'normal', self-trust = true, DM = false (1ms)
    ✓ 1 unverified: returns 'normal', self-trust = false, DM = true
    ✓ 1 unverified: returns 'normal', self-trust = false, DM = false
  shieldStatusForMembership other-trust behaviour
    ✓ 1 verified/untrusted: returns 'warning', DM = true
    ✓ 1 verified/untrusted: returns 'warning', DM = false
    ✓ 2 verified/untrusted: returns 'warning', DM = true (1ms)
    ✓ 2 verified/untrusted: returns 'warning', DM = false
    ✓ 2 unverified/untrusted: returns 'normal', DM = true
    ✓ 2 unverified/untrusted: returns 'normal', DM = false

NOTE: we don't flip "verified" to "normal" if we don't trust ourselves

@jryans jryans requested review from jryans and removed request for a team March 27, 2020 15:54
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! It's very helpful to have the tests and truth table for this! 😁

✓ 1 verified: returns 'verified', self-trust = false, DM = false

Shouldn't this case be "warning"? The room has someone other than yourself who you have verified, you don't trust yourself, and it's not a DM.

NOTE: we don't flip "verified" to "normal" if we don't trust ourselves

Can you be more specific about which case(s) you mean here?

src/utils/ShieldUtils.ts Show resolved Hide resolved
src/utils/ShieldUtils.ts Show resolved Hide resolved
src/utils/ShieldUtils.ts Outdated Show resolved Hide resolved
@foldleft
Copy link
Contributor Author

Thanks for working on this! It's very helpful to have the tests and truth table for this! 😁

✓ 1 verified: returns 'verified', self-trust = false, DM = false

It's a 1:1 room so it's treated as a DM

NOTE: we don't flip "verified" to "normal" if we don't trust ourselves

Can you be more specific about which case(s) you mean here?

Yeah. If it's a DM or a 1:1 room

@foldleft foldleft requested a review from jryans March 30, 2020 09:25
@jryans
Copy link
Collaborator

jryans commented Mar 30, 2020

✓ 1 verified: returns 'verified', self-trust = false, DM = false

It's a 1:1 room so it's treated as a DM

Ah right, makes sense then!

NOTE: we don't flip "verified" to "normal" if we don't trust ourselves

Can you be more specific about which case(s) you mean here?

Yeah. If it's a DM or a 1:1 room

Okay, I believe this is implementing the logic as expected. For 1:1 / DM rooms, your view of the room trust is meant to "effectively be the same as" your trust of the other user in the room, so nothing about yourself should affect it.

It does mean that each side will see a different shield for the room in 1:1 / DM rooms, but that's known and by design. We'll have to wait for feedback to see if it ends up being confusing.

Copy link
Collaborator

@jryans jryans 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, thanks! 😁

@foldleft foldleft merged commit 4e8cec3 into develop Mar 30, 2020
@foldleft foldleft deleted the foldleft/12484-user-shields branch March 30, 2020 13:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only include current user status in shield rules for topic rooms (not DM rooms)
2 participants