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.

Sign up for GitHub

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

Jump to bottom

Don't include guest users in the search results #10

Merged
merged 3 commits into from
Sep 15, 2023
Merged

Don't include guest users in the search results #10

merged 3 commits into from
Sep 15, 2023

Conversation

dhenneke
Copy link
Contributor

@dhenneke dhenneke commented Sep 15, 2023 •
edited
Loading

The module uses the check_username_for_spam hook to exclude all guest users from the user search endpoint of Synapse. It might still be added by Element as a "suggestion", but we need to work on that separately.

✔️ Checklist

  • A changeset describing the change and affected packages (more info).
  • Added or updated documentation.
  • Tests for new functionality and regression tests for bug fixes.
  • Screenshots or videos attached (for UI changes).
  • All your commits have a Signed-off-by line in the message (more info).

@dhenneke
Deactivate all users before a test run
720f9bc 
Signed-off-by: Dominik Henneke <dominik.henneke@nordeck.net>
@dhenneke
Don't include guest users in the search results
00c92e0 
Signed-off-by: Dominik Henneke <dominik.henneke@nordeck.net>
@dhenneke dhenneke force-pushed the nic/feat/hide-guest-users-from-search branch from c8db19a to 00c92e0 Compare September 15, 2023 08:22
@dhenneke dhenneke marked this pull request as ready for review September 15, 2023 09:07
@dhenneke dhenneke requested a review from a team as a code owner September 15, 2023 09:07
charlynguyen
charlynguyen reviewed Sep 15, 2023
View reviewed changes
Copy link
Contributor

@charlynguyen charlynguyen left a comment

Choose a reason for hiding this comment

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

Should we be worried about the timeouts in the E2E tests? Are they flaky?

@dhenneke
Copy link
Contributor Author

Should we be worried about the timeouts in the E2E tests? Are they flaky?

These are e2e tests, they are flaky by design! (just kidding)

Yes I am a bit worried about that. This specific situation (user clicks "Continue with guest", user sees "Creating Account...", dialog closes, user sees "loading spinner", user finally is in the room) is very prone to race conditions. Playwright either (1) doesn't find the spinner because the process was too fast and the spinner is already hidden, or (2) waits for the room name to appear, but then the page sometimes reloads and it fails with a "page closed" error. I struggled to get it succeeding in the CI in the first place, but now it's still not as stable as I would like it to be...

@charlynguyen
Copy link
Contributor

E2E tests will be addressed later on.

@dhenneke dhenneke enabled auto-merge September 15, 2023 09:54
@dhenneke dhenneke merged commit be17bc5 into main Sep 15, 2023
5 checks passed
@dhenneke dhenneke deleted the nic/feat/hide-guest-users-from-search branch September 15, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charlynguyen charlynguyen charlynguyen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dhenneke @charlynguyen