always query the lookup server in a global scale setup #11714
Conversation
I'm a bit unsure about this, but for now it's fine. We should rethink how to show email, remote, global scale, user and group share presentation anyways as we discussed it at the conf. |
| $isGlobalScaleEnabled = $this->config->getSystemValue('gs.enabled', false); | ||
| $isLookupServerEnabled = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no'); | ||
| // if case of Global Scale we always search the lookup server | ||
| if ($isLookupServerEnabled !== 'yes' && $isGlobalScaleEnabled) { |
There was a problem hiding this comment.
Shouldn't this be like that?
$isLookupServerEnabled !== 'yes' && !$isGlobalScaleEnabled
Because it is:
neither lookup server is enabled nor global scale is enabled
Or did I miss something here?
There was a problem hiding this comment.
hm, it worked during my tests as expected, but now I'm confused as well.. let me test it again
…er label Signed-off-by: Bjoern Schiessle <[email protected]>
schiessle
force-pushed
the
lookupserver-and-global-scale
branch
from
734d382 to
3fa13e7
Compare
Signed-off-by: Bjoern Schiessle <[email protected]>
| $isGlobalScaleEnabled = $this->config->getSystemValue('gs.enabled', false); | ||
| $isLookupServerEnabled = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no'); | ||
| // if case of Global Scale we always search the lookup server | ||
| if ($isLookupServerEnabled !== 'yes' && !$isGlobalScaleEnabled) { |
There was a problem hiding this comment.
Would be a lot more readable if the === 'yes' is added to line 60 and then the if statement only holds !$isLookupServerEnabled && !$isGlobalScaleEnabled.
Sorry for nitpicking, but readable code is worth it IMO.
There was a problem hiding this comment.
good idea... will add it.
| $body = json_decode($response->getBody(), true); | ||
|
|
||
| foreach ($body as $lookup) { | ||
| $remote = $this->cloudIdManager->resolveCloudId($lookup['federationId'])->getRemote(); |
|
|
||
| foreach ($body as $lookup) { | ||
| $remote = $this->cloudIdManager->resolveCloudId($lookup['federationId'])->getRemote(); | ||
| if ($this->currentUserRemote === $remote) continue; |
| foreach ($body as $lookup) { | ||
| $remote = $this->cloudIdManager->resolveCloudId($lookup['federationId'])->getRemote(); | ||
| if ($this->currentUserRemote === $remote) continue; | ||
| $name = isset($lookup['name']['value']) ? $lookup['name']['value'] : ''; |
There was a problem hiding this comment.
use ?? operator, see backport
There was a problem hiding this comment.
yes, but honestly I consider the other one more readable so I will stick to it for now. All other points are addressed... Thanks for all the feedback!
Signed-off-by: Bjoern Schiessle <[email protected]>
|
@MorrisJobke @nickvergessen all comments are addressed |
| $this->config->expects($this->once()) | ||
| $this->config->expects($this->at(0)) | ||
| ->method('getSystemValue') | ||
| ->with('gs.enabled', false) |
There was a problem hiding this comment.
can we have tests with gs.enabled = true, too, please?
There was a problem hiding this comment.
This will produce the exact same results, because the lookup server is enabled in all test cases 😉 But I added another test (see last commit) which might do what you meant...
Signed-off-by: Bjoern Schiessle <[email protected]>
Check if global scale is enabled, in this case we should always query the lookup server. Further I improved the label of the result to look like
User Name (uid@nextcloudserver)