feat(sharing): allow email-based labels for lookup results#60375
Open
n-iv wants to merge 3 commits into
Open
Conversation
When the new system config 'shareapi_lookup_label_show_email' is set to true, the LookupPlugin composes suggestion labels with the user's email (from the lookup server response) instead of the federation ID. Default behavior is strictly preserved when the flag is absent or false. Motivation: in Global Scale deployments, federation IDs carry sharded identity that is not human-readable (e.g. 'user@node05.example.org'). Operators can opt-in to email-based labels to improve UX without affecting other deployments. Signed-off-by: niv <nicolas.varlot@ac-versailles.fr>
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in system configuration flag (shareapi_lookup_label_show_email) to change how lookup-server share suggestions are labeled in Global Scale setups: when enabled and the lookup response contains an email, the suggestion label shows Name (email) instead of Name (federationId), while keeping the federation ID as the actual share target.
Changes:
- Read new system config flag
shareapi_lookup_label_show_emailinLookupPluginand use lookup-responseemailfor the suggestion label when enabled. - Update existing
LookupPluginTestexpectations for the additional config read. - Add a new unit test covering the “email label” path when the flag is enabled and email is present.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/private/Collaboration/Collaborators/LookupPlugin.php | Adds config-driven logic to compose lookup suggestion labels using email when available. |
| tests/lib/Collaboration/Collaborators/LookupPluginTest.php | Updates mocks for the new config read and adds a new test for the email-label behavior. |
Comments suppressed due to low confidence (1)
lib/private/Collaboration/Collaborators/LookupPlugin.php:47
- This config read happens even when Global Scale is disabled (the method returns early right after), so it adds an unnecessary system-config lookup on every non-GS search call. Consider moving the
shareapi_lookup_label_show_emailread to after the!$isGlobalScaleEnabledearly-return to avoid extra work in the common non-GS case.
$isGlobalScaleEnabled = $this->config->getSystemValueBool('gs.enabled', false);
$isLookupServerEnabled = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no') === 'yes';
$hasInternetConnection = $this->config->getSystemValueBool('has_internet_connection', true);
$showFederatedEmail = $this->config->getSystemValueBool('shareapi_lookup_label_show_email', false);
// If case of Global Scale we always search the lookup server
// TODO: Reconsider using the lookup server for non-global scale
// if (!$isGlobalScaleEnabled && (!$isLookupServerEnabled || !$hasInternetConnection || $disableLookupServer)) {
if (!$isGlobalScaleEnabled) {
return false;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $isGlobalScaleEnabled = $this->config->getSystemValueBool('gs.enabled', false); | ||
| $isLookupServerEnabled = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no') === 'yes'; | ||
| $hasInternetConnection = $this->config->getSystemValueBool('has_internet_connection', true); | ||
| $showFederatedEmail = $this->config->getSystemValueBool('shareapi_lookup_label_show_email', false); |
Comment on lines
+82
to
+88
| $email = $lookup['email']['value'] ?? ''; | ||
|
|
||
| if ($showFederatedEmail && !empty($email)) { | ||
| $label = empty($name) ? $email : $name . ' (' . $email . ')'; | ||
| } else { | ||
| $label = empty($name) ? $lookup['federationId'] : $name . ' (' . $lookup['federationId'] . ')'; | ||
| } |
| ->willReturn(json_encode($resultBody)); | ||
| $client = $this->createMock(IClient::class); | ||
| $client->expects($this->once()) | ||
| ->method('get') |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Nicolas Varlot <nicolas.varlot@ac-versailles.fr>
- rename variable $showFederatedEmail to $showEmailInLabel for clarity - add testSearchWithEmailLabelFallbackWhenEmailMissing covering the fallback path when flag is enabled but lookup response has no email - tighten HTTP expectation in testSearchWithEmailLabel to validate the requested URL Signed-off-by: niv <nicolas.varlot@ac-versailles.fr>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an opt-in system config flag shareapi_lookup_label_show_email that, when set to true, makes the LookupPlugin compose suggestion labels with the user's email (from the lookup server response) instead of the federation ID.
Default behavior is strictly preserved when the flag is absent or
false.Motivation
In Global Scale deployments, federation IDs are sharded across nodes (e.g.
user@node05.example.org) and do not carry human-readable signal. When the sharing dialog shows two accounts of the same person who happens to be hosted on different nodes, users see:Jane Doe (jdoe@node05.example.org)Jane Doe (janedoe1@node10.example.org)The node suffix is implementation detail and does not help the user pick. The email, which the lookup server already exposes in its response, is far more discriminating.
With the flag enabled, the same suggestions become:
Jane Doe (jane.doe@site-a.example)Jane Doe (jane.doe@site-b.example)Behavior
false: identical to currentmaster.trueandemailpresent in lookup response: label usesName (email)oremailif no name.trueandemailmissing: graceful fallback toName (federationId)orfederationId.Tests