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

Include non-matching DMs in Spotlight recent conversations when the DM's userId is part of the search API results #11374

Merged

Conversation

mgcm
Copy link
Contributor

@mgcm mgcm commented Aug 4, 2023

This PR continues the work started in #9556 and #11290 and fixes a couple of issues associated with how search results are filtered and handled in the Spotlight search dialog.

Specifically, it handles the situation when there is no match in the search term query to existing DMs but the user search directory results includes a userId that does have an existing DM with the user. With this change, the DM rooms will appear as items in the "Recent Conversations" section at the top of the results.

The PR also changes how users are sorted in the "Suggestions" section when there is no significant activity between them. This assumes that the server is already sorting users by some relevant property, as per the spec which states "results: Ordered by rank and then whether or not profile info is available".

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Signed-off-by: Milton Moura miltonmoura@gmail.com

Type: defect


Here's what your changelog entry will look like:

🐛 Bug Fixes

  • Include non-matching DMs in Spotlight recent conversations when the DM's userId is part of the search API results (#11374). Contributed by @mgcm.

     1. Include non-matching DMs in Spotlight suggestions if the userId of the DM is included in the user directory search results
     2. The user directory search results order is kept when there is no relevant activity between users, instead of sorting by MXID
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Aug 4, 2023
@github-actions github-actions bot added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Aug 4, 2023
@mgcm mgcm marked this pull request as ready for review August 4, 2023 22:52
@mgcm mgcm requested a review from a team as a code owner August 4, 2023 22:52
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand this. You're talking about the user directory but I don't see that being used here? It's just referring to the list of DMs. Also, I can't reproduce the bug I think you're implying, in that if I search for a someone's user ID (where their user ID is different from their display name), the DM room with them appears in my spotlight search results.

@mgcm
Copy link
Contributor Author

mgcm commented Aug 7, 2023

I'm not sure I understand this. You're talking about the user directory but I don't see that being used here? It's just referring to the list of DMs.

The Spotlight People results are built from the list of users that you have a DM with, the list of users that are in other rooms with you and the list of users returned from the user directory search API, filtered by the search terms that are used.

The proposed change affects the final results list by not excluding DMs from the "Recent Conversations" section that might not match the terms but are included in the possible result list.

Also, I can't reproduce the bug I think you're implying, in that if I search for a someone's user ID (where their user ID is different from their display name), the DM room with them appears in my spotlight search results.

I can point you to the test case I added (

it("show non-matching query members with DMs if they are present in the server search results", async () => {
) - where searching for terms that are not the user's MXID or the user's Display Name should not exclude a DM result if and only if the user ID is part of the possible result list (most likely retrieved from the API).

This is difficult to replicate in a standard homeserver deployment but let's you have user metadata, like office location, job title, organization or department, and your user directory search implementation is aware of that and can use it as search terms for returning a ranked result set.

Another example - You (David) and me (Milton) have a DM. We have a custom search API implementation that will match location metadata. If you search for "Milton Moura Azores", the custom search API implementation will return my MXID as part of the result set but you will only get a result in the "Suggestions" section, with no indication of an existing DM. With this patch, the DM with me will appear in the "Recent Conversations" section instead.

Hope this clears up things a bit.

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Ah, thanks for the explanation! I entirely did not get that users was the results from the user directory search (wish we'd used a clearer variable name...).

Could we possibly expand the comment to something like, "If the room is a DM with a user that was included in the user directory search results, we can assume the user is a relevant result, so include the DM with them too."?

Also if you felt like renaming the users variable to userDirectorySearchResults or something, I certainly wouldn't complain.

Other than that, I think this looks sensible.

@dbkr
Copy link
Member

dbkr commented Aug 8, 2023

Oh, also you have some eslint failures which look relevant (and it's a required check).

1. Updated comments
2. Renamed users to userDirectorySearchResults
3. Makes sure linter is happy
…rdeck/matrix-react-sdk into nic/fix/PZD-210-spotlight-user-search
@mgcm
Copy link
Contributor Author

mgcm commented Aug 9, 2023

I've updated the PR taking @dbkr 's comments into consideration.

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Thanks, this feels a lot easier to understand now.

@dbkr dbkr added this pull request to the merge queue Aug 10, 2023
Merged via the queue into matrix-org:develop with commit d240f06 Aug 10, 2023
70 checks passed
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Aug 31, 2023
Changes in [1.11.40](https://github.com/vector-im/element-web/releases/tag/v1.11.40) (2023-08-29)
=================================================================================================

## ✨ Features
 * Hide account deactivation for externally managed accounts ([\#11445](matrix-org/matrix-react-sdk#11445)). Fixes #26022. Contributed by @kerryarchibald.
 * OIDC: Redirect to delegated auth provider when signing out ([\#11432](matrix-org/matrix-react-sdk#11432)). Fixes #26000. Contributed by @kerryarchibald.
 * Disable 3pid fields in settings when `m.3pid_changes` capability is disabled ([\#11430](matrix-org/matrix-react-sdk#11430)). Fixes #25995. Contributed by @kerryarchibald.
 * OIDC: disable multi session signout for OIDC-aware servers in session manager ([\#11431](matrix-org/matrix-react-sdk#11431)). Contributed by @kerryarchibald.
 * Implement updated open dialog method of the Module API ([\#11395](matrix-org/matrix-react-sdk#11395)). Contributed by @dhenneke.
 * Polish & delabs `Exploring public spaces` feature ([\#11423](matrix-org/matrix-react-sdk#11423)).
 * Treat lists with a single empty item as plain text, not Markdown. ([\#6833](matrix-org/matrix-react-sdk#6833)). Fixes element-hq/element-meta#1265.
 * Allow managing room knocks ([\#11404](matrix-org/matrix-react-sdk#11404)). Contributed by @charlynguyen.
 * Pin the action buttons to the bottom of the scrollable dialogs ([\#11407](matrix-org/matrix-react-sdk#11407)). Contributed by @dhenneke.
 * Support Matrix 1.1 (drop legacy r0 versions) ([\#9819](matrix-org/matrix-react-sdk#9819)).

## 🐛 Bug Fixes
 * Fix path separator for Windows based systems ([\#25997](element-hq/element-web#25997)).
 * Fix instances of double translation and guard translation calls using typescript ([\#11443](matrix-org/matrix-react-sdk#11443)).
 * Fix export type "Current timeline" to match its behaviour to its name ([\#11426](matrix-org/matrix-react-sdk#11426)). Fixes #25988.
 * Fix Room Settings > Notifications file upload input being shown superfluously ([\#11415](matrix-org/matrix-react-sdk#11415)). Fixes #18392.
 * Simplify registration with email validation ([\#11398](matrix-org/matrix-react-sdk#11398)). Fixes #25832 #23601 and #22297.
 * correct home server URL ([\#11391](matrix-org/matrix-react-sdk#11391)). Fixes #25931. Contributed by @NSV1991.
 * Include non-matching DMs in Spotlight recent conversations when the DM's userId is part of the search API results ([\#11374](matrix-org/matrix-react-sdk#11374)). Contributed by @mgcm.
 * Fix useRoomMembers missing updates causing incorrect membership counts ([\#11392](matrix-org/matrix-react-sdk#11392)). Fixes #17096.
 * Show error when searching public rooms fails ([\#11378](matrix-org/matrix-react-sdk#11378)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants