Skip to content

Conversation

@nickvergessen
Copy link
Member

  • Allow type-ahead of local users via email address
  • If we match a complete email address, use a local user share and hide email + remote share options

I guess tests may fail, will adjust them then later.

Signed-off-by: Joas Schilling <coding@schilljs.com>
@mention-bot
Copy link

@nickvergessen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rullzer, @schiessle and @icewind1991 to be potential reviewers.

@schiessle
Copy link
Member

It looks like two things are missing:

  • We should hide the email option if we match a exact federated cloud id (we query the local address books and the lookup server for it)
  • We should hide the remote share option if we match a exact email address of non-local users.

@nickvergessen
Copy link
Member Author

Those two things should already be the case, did you test them?

@schiessle
Copy link
Member

schiessle commented Jun 14, 2017

I tested it now with a user who where email address === login name. I think that's quite common on some LDAP setups and the reason why we didn't searched the system address book.

Setup:

  • a user with the username "user@server.com" and a email address "user@server.com"
  • create system address book sudo -u www-data ./occ dav:sync-system-addressbook

Now I typed "user@server":

autocomplete1

Since there is no exact match this seems to be fine.

Then after I completed the email address it looks like that:

appmenu2

Now we have a exact match and the result is a bit confusing to me. We have two options but what are the differences? I assume that user@server.com (user@server.com) is the email address like in the first screenshot, but where is the (email) gone? The first option user@server.com is the local user, right? I think in this case we shouldn't show the email option, right?

@nickvergessen
Copy link
Member Author

Ah okay, yeah I didn't think about name === email, will have a look

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member Author

Done and added a test

@codecov
Copy link

codecov bot commented Jun 14, 2017

Codecov Report

Merging #5384 into master will increase coverage by <.01%.
The diff coverage is 50%.

@@             Coverage Diff              @@
##             master    #5384      +/-   ##
============================================
+ Coverage     54.14%   54.15%   +<.01%     
- Complexity    22312    22323      +11     
============================================
  Files          1381     1380       -1     
  Lines         85449    85497      +48     
  Branches       1325     1325              
============================================
+ Hits          46267    46298      +31     
- Misses        39182    39199      +17
Impacted Files Coverage Δ Complexity Δ
...es_sharing/lib/Controller/ShareesAPIController.php 66.03% <50%> (-2%) 110 <5> (+9)
apps/dav/lib/CalDAV/Calendar.php 73.23% <0%> (-1.03%) 52% <0%> (ø)
lib/base.php 2.97% <0%> (-0.01%) 173% <0%> (ø)
config/config.sample.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
apps/user_ldap/appinfo/app.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
settings/personal.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
settings/templates/personal.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
apps/user_ldap/templates/settings.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
apps/files/templates/appnavigation.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
lib/private/Comments/ManagerFactory.php 100% <0%> (ø) 2% <0%> (ø) ⬇️
... and 5 more

Copy link
Member

@schiessle schiessle left a comment

Choose a reason for hiding this comment

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

looks good now!

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works:

bildschirmfoto 2017-06-14 um 17 53 48

And then the share drop down only shows the display name:

bildschirmfoto 2017-06-14 um 18 02 26

👍

@MorrisJobke MorrisJobke merged commit 2398d40 into master Jun 14, 2017
@MorrisJobke MorrisJobke deleted the allow-to-share-to-local-users-via-email branch June 14, 2017 23:03
@MorrisJobke
Copy link
Member

Ping @nickvergessen for the backport

blizzz pushed a commit that referenced this pull request Jun 15, 2017
Allow to find local users by their email address

Signed-off-by: Joas Schilling <coding@schilljs.com>

Make sure to only add system users once

Signed-off-by: Joas Schilling <coding@schilljs.com>

Add unit test

Signed-off-by: Joas Schilling <coding@schilljs.com>
@blizzz
Copy link
Member

blizzz commented Jun 15, 2017

I backported it quickly → #5428

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.

6 participants