Skip to content

allow SpotlightDialog person search on non-matrix attributes#247

Closed
4www wants to merge 2 commits into
developfrom
feature/PZD-210_user-search-non-matrix-attributes
Closed

allow SpotlightDialog person search on non-matrix attributes#247
4www wants to merge 2 commits into
developfrom
feature/PZD-210_user-search-non-matrix-attributes

Conversation

@4www
Copy link
Copy Markdown

@4www 4www commented Apr 19, 2023

Follow-up on the PR and discussion here: matrix-org#9556

@4www 4www requested a review from HarHarLinks April 19, 2023 15:17
for (const user of [...findVisibleRoomMembers(cli, msc3946ProcessDynamicPredecessor), ...users]) {

const lcQuery = trimmedQuery.toLowerCase();
const members = findVisibleRoomMembers(cli);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should probably keep this:

Suggested change
const members = findVisibleRoomMembers(cli);
const members = findVisibleRoomMembers(cli, msc3946ProcessDynamicPredecessor);

user_id: "@earthling:matrix.org",
display_name: "Earth Person",
avatar_url: undefined,
location: "Earth", // this attribute could come from a matrix homeserver connected to LDAP
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this test case is not clear: with this, searching by location would require searching for (a substring of) Earth, which is also a substring of earthling and Earth Person. That means it would also match in the current implementation. However with our change, the dialogue should be able to successfully find a user such as

            user_id: "@earthling:matrix.org",
            display_name: "Earth Person",
            avatar_url: undefined,
            location: "Mars",

with a query of Mars.

Comment on lines +472 to +481
const results = users.filter(
(it) => {
return (
!searchTerm ||
it.user_id.toLowerCase().includes(searchTerm) ||
it.display_name?.toLowerCase().includes(searchTerm) ||
it.location?.toLowerCase().includes(searchTerm)
)
}
);
Copy link
Copy Markdown

@HarHarLinks HarHarLinks Apr 20, 2023

Choose a reason for hiding this comment

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

it seems superfluous to not just do

Suggested change
const results = users.filter(
(it) => {
return (
!searchTerm ||
it.user_id.toLowerCase().includes(searchTerm) ||
it.display_name?.toLowerCase().includes(searchTerm) ||
it.location?.toLowerCase().includes(searchTerm)
)
}
);
const results = users;

while this illustrates how the server works, i feel it overcomplicates things without being useful in this place. unless i'm overlooking something?

@HarHarLinks HarHarLinks closed this Aug 4, 2023
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