-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
allow admins to enable medial search on group and user #34659
Conversation
Codecov Report
@@ Coverage Diff @@
## master #34659 +/- ##
============================================
+ Coverage 65.25% 65.25% +<.01%
- Complexity 18458 18463 +5
============================================
Files 1207 1207
Lines 69895 69914 +19
Branches 1280 1280
============================================
+ Hits 45608 45621 +13
- Misses 23915 23921 +6
Partials 372 372
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #34659 +/- ##
============================================
+ Coverage 65.25% 65.25% +<.01%
- Complexity 18458 18463 +5
============================================
Files 1207 1207
Lines 69895 69914 +19
Branches 1280 1280
============================================
+ Hits 45608 45621 +13
- Misses 23915 23921 +6
Partials 372 372
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #34659 +/- ##
============================================
- Coverage 65.3% 65.28% -0.02%
- Complexity 18478 18487 +9
============================================
Files 1209 1210 +1
Lines 69964 70009 +45
Branches 1280 1280
============================================
+ Hits 45689 45707 +18
- Misses 23903 23930 +27
Partials 372 372
Continue to review full report at Codecov.
|
lib/private/Group/Database.php
Outdated
@@ -313,7 +325,13 @@ public function countUsersInGroup($gid, $search = '') { | |||
$parameters = [$gid]; | |||
$searchLike = ''; | |||
if ($search !== '') { | |||
$parameters[] = '%' . $this->dbConn->escapeLikeParameter($search) . '%'; | |||
$search = \OC::$server->getDatabaseConnection()->escapeLikeParameter($search); | |||
$allowMedialSearches = \OC::$server->getConfig()->getSystemValue("accounts.enable_medial_search", true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to inject the services
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I injected services in this class and applied fixDI
hack on changed functions. But, User/Database
class a bit tricky, I don't prefer to touch this class for dependency injection in the scope of this pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karakayasemi you can simply add a "TODO: inject" then
If the option will be used only for DB groups, it should be clarified in the sample file. |
b550a54
to
7a58236
Compare
A sentence was added for this purpose to sample.config.php. |
If not sure if it's clear enough... |
7a58236
to
106930e
Compare
Codecov Report
@@ Coverage Diff @@
## master #34659 +/- ##
============================================
+ Coverage 65.3% 65.3% +<.01%
- Complexity 18479 18481 +2
============================================
Files 1209 1209
Lines 69971 69981 +10
Branches 1280 1280
============================================
+ Hits 45695 45704 +9
- Misses 23904 23905 +1
Partials 372 372
Continue to review full report at Codecov.
|
@jvillafanez, as far as I understand from the following sentences of this comment #33883 (comment), it should apply to only DB backend:
If you have any suggestion, we can improve the explanation line of |
The "accounts.enable_medial_search" key has to apply only to account searches, so using it against an specific backend is wrong. I'm not sure if using medial search for users inside the DB backend (not in accounts) has any use. We'll need to sync the users into the account table, and the user manager checks against the account table, so a "userbackend.db.enable_medial_search" key affecting just the DB backend has a very limited usage.
What about
It's more clear that it will apply on the group id (there is no other usage right now), and also that it will apply only to the DB backend. |
106930e
to
c3e7f03
Compare
@jvillafanez I changed the |
lib/private/User/Database.php
Outdated
@@ -184,8 +184,14 @@ public function getDisplayNames($search = '', $limit = null, $offset = null) { | |||
$searchLike = ''; | |||
if ($search !== '') { | |||
$search = \OC::$server->getDatabaseConnection()->escapeLikeParameter($search); | |||
$parameters[] = '%' . $search . '%'; | |||
$parameters[] = '%' . $search . '%'; | |||
$allowMedialSearches = \OC::$server->getConfig()->getSystemValue("accounts.enable_medial_search", true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still very concern about using this key here. I'd prefer to use a new one just to be used for this case instead of reusing another key which was created for another purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't touch / reuse the old keys and only work with new ones as discussed here: #33883 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that this function is only used in the account sync operation. I believe, this time I understand @jvillafanez's concern.
@PVince81, IMHO, there is no much benefit on adding a key like userbackend.db.enable_medial_search
as @jvillafanez said. I removed User/Database
changes from PR.
c3e7f03
to
a1b2fe7
Compare
@ownclouders rebase |
Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently |
a1b2fe7
to
7fe2f35
Compare
Automated rebase with GitMate.io was successful! 🎉 |
Description
allow admin to enable medial search on group and user
Related Issue
Fixes #33883 and https://github.com/owncloud/enterprise/issues/2311
Motivation and Context
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.
How Has This Been Tested?
'groups.enable_medial_search' => true
line to config'groups.enable_medial_search' => false
Types of changes
Checklist:
Open tasks: