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

Remove old pre-join UTD logic #12464

Merged
merged 6 commits into from
May 10, 2024
Merged

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Apr 30, 2024

and add a playwright test for new pre-join UTD

The playwright test depends on the docker image for Synapse to include element-hq/synapse#17104

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

and add a playwright test for new pre-join UTD
@uhoreg uhoreg requested a review from a team as a code owner April 30, 2024 01:56
@uhoreg uhoreg added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Apr 30, 2024
playwright/e2e/crypto/crypto.spec.ts Outdated Show resolved Hide resolved
// - second message sent by Bob (decryptable)
// - third message sent by Bob (undecryptable)
const tiles = await page.locator(".mx_EventTile").all();
expect(tiles.length).toBeGreaterThanOrEqual(5);
Copy link
Contributor

Choose a reason for hiding this comment

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

OOI when will there be more than 5 tiles?

Copy link
Member Author

Choose a reason for hiding this comment

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

It uses an EventTile for things like the thing that indicates that encryption is enabled. By the time we get to this point in the test, that tile will have disappeared. I'm not sure if other previous events will get a tile (I think they all get thrown in a group). But basically, by checking that the length is at least 5, if other bits of the react-sdk change so that other tiles are shown, this test won't immediately break.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

Copy link
Member

Choose a reason for hiding this comment

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

I already said this on a previous PR, but:

Could we just set msc4115_membership_on_events: true on the default template, rather than have all this copypasta?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I forgot about that previous comment when I was splitting this PR out.

I wasn't sure what was the policy about enabling experimental features on the default template, but since Synapse is going to enable it by default anyways, it shouldn't be a problem here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants