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

Take accounts.enable_medial_search into account while searching for remote users in share autocomplete #36225

Merged
merged 1 commit into from
Oct 4, 2019

Conversation

VicDeo
Copy link
Member

@VicDeo VicDeo commented Sep 30, 2019

Description

Add matchMode search option for carddav backend. Make it any by default and switch to start when accounts medial search is off

Related Issue

Motivation and Context

Use medial search only if it is enabled when searching for users synced via federation app

How Has This Been Tested?

  1. Setup 2 ownclouds and add them as trusted to each other at Settings -> Admin -> Sharing -> Federation
  2. Execute cron on both servers e.g. with occ system:cron
  3. Execute occ federation:sync-addressbooks
  4. Disable account medial search on both
    5 Try share autocomplete using the middle chars of remote users usernames

Expected

No remote users in autocomplete as medial search is off

Actual

Remote users with the search patterns in the middle of username are still proposed

Screenshots (if appropriate):

medial

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@VicDeo VicDeo added 2 - Developing p3-medium Normal priority labels Sep 30, 2019
@VicDeo VicDeo added this to the development milestone Sep 30, 2019
@VicDeo VicDeo self-assigned this Sep 30, 2019
@codecov
Copy link

codecov bot commented Sep 30, 2019

Codecov Report

Merging #36225 into master will increase coverage by <.01%.
The diff coverage is 76%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #36225      +/-   ##
============================================
+ Coverage     64.83%   64.84%   +<.01%     
- Complexity    19762    19768       +6     
============================================
  Files          1271     1271              
  Lines         74675    74697      +22     
  Branches       1309     1309              
============================================
+ Hits          48418    48434      +16     
- Misses        25871    25877       +6     
  Partials        386      386
Flag Coverage Δ Complexity Δ
#javascript 54% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.03% <76%> (ø) 19768 <6> (+6) ⬆️
Impacted Files Coverage Δ Complexity Δ
...files_sharing/lib/Controller/ShareesController.php 90.67% <100%> (+0.28%) 107 <0> (+1) ⬆️
apps/dav/lib/CardDAV/AddressBookImpl.php 72.61% <100%> (ø) 26 <0> (ø) ⬇️
apps/dav/lib/CardDAV/CardDavBackend.php 82.65% <60%> (-0.84%) 92 <6> (+5)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b58749a...3bc8057. Read the comment docs.

@VicDeo VicDeo force-pushed the fix/e3495 branch 2 times, most recently from 44cad55 to e55f4ba Compare October 1, 2019 15:47
@VicDeo VicDeo requested a review from micbar October 2, 2019 07:02
@VicDeo VicDeo changed the title Take medial search config into account while searching for remote users Take accounts.enable_medial_search into account while searching for remote users in share autocomplete Oct 2, 2019
Copy link
Contributor

@karakayasemi karakayasemi left a comment

Choose a reason for hiding this comment

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

The code looks okay and it is working like expected. But, I have some doubts about the architecture.

Since accounts.enable_medial_search is a system value, I feel like it should be forced in core implementation. In my POV, it should not be an option. The other usages of the same system value are also in this way. It is not selectable by API consumers like this one

$allowMedialSearches = $this->config->getSystemValue('accounts.enable_medial_search', true);
. Also, the following issue description is stating that this config is intended for Backend implementations #33883 (comment).

In this implementation, we expect every class which uses ContactManager or CardDAV backend search to be aware of this system config and send the correct option to obey the configured setting. Also when I look at ContactManager interface or its implementation, I can not see which are these search options.

I may have missed something, but I would keep it simple and just add a check in the CardDAV search for this config.

@VicDeo
Copy link
Member Author

VicDeo commented Oct 2, 2019

@karakayasemi there is an internal conflict there.
I don't think dav contacts search should depend on accounts.enable_medial_search directly
Contacts are NOT accounts, user backend, etc.

Such kind of dependency might have side effects while working with address books and doing an ordinary search in user contacts

Copy link
Contributor

@micbar micbar left a comment

Choose a reason for hiding this comment

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

LGTM

@micbar
Copy link
Contributor

micbar commented Oct 4, 2019

@VicDeo Could you rebase and push?

@micbar
Copy link
Contributor

micbar commented Oct 4, 2019

After rebase you will get the new changelog generator templates.
You have the honour to place the first changelog file into changelog. Just create a folder unreleased there and put a file there with your PR as a filename.
Write the changelog into that file according to https://github.com/owncloud/core/blob/master/changelog/TEMPLATE

@VicDeo
Copy link
Member Author

VicDeo commented Oct 4, 2019

@micbar done

@VicDeo VicDeo merged commit cc3c4a4 into master Oct 4, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/e3495 branch October 4, 2019 11:21
@davitol davitol mentioned this pull request Nov 26, 2019
37 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants