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

Check room membership before processing events in room resource #2179

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lukemelia
Copy link
Contributor

No description provided.

Copy link

github-actions bot commented Feb 20, 2025

Host Test Results

    1 files  ±0      1 suites  ±0   22m 59s ⏱️ -28s
771 tests ±0  769 ✔️ ±0  2 💤 ±0  0 ±0 
776 runs  ±0  774 ✔️ ±0  2 💤 ±0  0 ±0 

Results for commit e8d8ef8. ± Comparison against base commit 868be56.

♻️ This comment has been updated with latest results.

@lukemelia lukemelia marked this pull request as draft February 20, 2025 21:27
@lukemelia lukemelia force-pushed the check-room-members-before-processing-events branch from d78af75 to ebb034f Compare February 20, 2025 21:45
@lukemelia lukemelia force-pushed the check-room-members-before-processing-events branch from ebb034f to e8d8ef8 Compare February 20, 2025 22:36
@lukemelia lukemelia marked this pull request as ready for review February 20, 2025 22:36
@lukemelia lukemelia requested review from IanCal and a team February 20, 2025 22:36
Comment on lines +119 to +122
if (!memberIds) {
return;
}
if (!memberIds.includes(this.matrixService.aiBotUserId)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you could collapse this into a single if using an "or"

@@ -44,6 +44,14 @@ export default class Room {
?.name;
}

get memberIds(): string[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like something that should be using @cached no? every call to this property will run this logic

assert.dom('[data-test-profile-display-name]').hasText(''); // No display name set yet
assert
.dom('[data-test-profile-icon]')
.hasStyle({ backgroundColor: 'rgb(34, 221, 152)' });
.hasStyle({ backgroundColor: 'rgb(88, 226, 29)' });
Copy link
Contributor

Choose a reason for hiding this comment

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

what happened here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "server" of the testuser changed from staging to localhost, which results in a different color for the avatar background

@@ -1030,7 +1030,7 @@ module('Acceptance | operator mode tests', function (hooks) {
assert.dom('[data-test-profile-icon]').hasText('T');
assert
.dom('[data-test-profile-icon]')
.hasStyle({ backgroundColor: 'rgb(34, 221, 152)' });
.hasStyle({ backgroundColor: 'rgb(88, 226, 29)' });
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

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.

2 participants