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

[FR] Global user and group medial search boolean option #33883

Closed
tomneedham opened this issue Dec 13, 2018 · 11 comments · Fixed by #34659
Closed

[FR] Global user and group medial search boolean option #33883

tomneedham opened this issue Dec 13, 2018 · 11 comments · Fixed by #34659
Assignees
Labels
Milestone

Comments

@tomneedham
Copy link
Contributor

tomneedham commented Dec 13, 2018

we have accounts.enable_medial_search but we should move to one option respected globally for users, and one for groups, making the results more consistent.

Eg: custom groups backend
Eg: local groups backend
Eg: local user backend
Eg: ldap user backend

@tomneedham
Copy link
Contributor Author

@pmaier1 @PVince81

@ownclouders

This comment has been minimized.

@pmaier1
Copy link
Contributor

pmaier1 commented Dec 13, 2018

Maybe we should just disable medial search for groups until we have the groups table instead of adding config options?

@PVince81
Copy link
Contributor

ref: #27623

@PVince81
Copy link
Contributor

disabling permanently could impair the UX for people to whom UX is more important than performance.

let's have this as a config.php only option

@PVince81 PVince81 added this to the backlog milestone Dec 14, 2018
@PVince81 PVince81 added the p3-medium Normal priority label Dec 14, 2018
@PVince81
Copy link
Contributor

@tomneedham if I understand correctly you mean we need two options:

  • "accounts.enable_medial_search_users": affects local, ldap and any other user backends
  • "accounts.enable_medial_search_groups": affects ldap group backend, custom groups, etc

now the groups option is not related to the "accounts" table.

what to do with the existing option, migrate it to the other two ? not possible in read-only config.php. we could state this change in release notes for admins to adjust.

@tomneedham
Copy link
Contributor Author

Move to new config options with better naming. Don't try 'auto migrate' them. We can document in release notes and normal documentation. Side effects are minimal / not critical at all. Easily changed.

@karakayasemi
Copy link
Contributor

karakayasemi commented Feb 26, 2019

@tomneedham @PVince81 Please, correct me if I'm wrong. I'm planning to do the followings.

  • Seperate 'accounts.enable_medial_search' as 'accounts.enable_medial_search_users' and 'accounts.enable_medial_search_groups' in sample.config.php.

  • Change 'accounts.enable_medial_search' usages to 'accounts.enable_medial_search_users' in all repositories. Also, there is 'user_ldap.enable_medial_search' option in config.apps.sample.php I will remove this option and use new 'accounts.enable_medial_search_users' option instead of this.

  • Currently, the implemented group search in the core is using medial search unconditionally. I will add 'accounts.enable_medial_search_groups' check to determine search type in group search methods like here: https://github.com/owncloud/core/blob/master/lib/private/Group/Database.php#L289. Also, I will check other group backend implementations to use this new option.

@tomneedham
Copy link
Contributor Author

tomneedham commented Mar 1, 2019

* Seperate 'accounts.enable_medial_search' as  'accounts.enable_medial_search_users' and  'accounts.enable_medial_search_groups' in sample.config.php.

Accounts represent users from a backend. We should keep this named as this.

* Change 'accounts.enable_medial_search' usages to  'accounts.enable_medial_search_users' in all repositories. Also, there is 'user_ldap.enable_medial_search' option in config.apps.sample.php I will remove this option and use new 'accounts.enable_medial_search_users' option instead of this.

The accounts.enable_medial_search is fine - group ones should be called groups.enable_medial_search and then when central group table is merged, we can use it there as well.

Also, there is 'user_ldap.enable_medial_search' option in config.apps.sample.php

We should see where this is used. The key thing should be that within ownCloud - all account searches are only hitting the database so we have control over the expectations for the search time. I would not bring LDAP into this if we can. We might be able to leave this separated for now.

@tomneedham
Copy link
Contributor Author

@karakayasemi Do you want to have a quick call Monday morning about this? I can explain some background

@owncloud owncloud deleted a comment from karakayasemi Mar 1, 2019
@karakayasemi
Copy link
Contributor

karakayasemi commented Mar 1, 2019

@tomneedham thank you for help, I read other related issues and patches. After your last explanations, I believe I got the idea for implementing necessities. We can move user_ldap part out of this issue's scope.
I am planning to open a pull request at the weekend. We can discuss further details on the PR. If we need, we can have a chat on rocket.chat or arrange a quick call.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants