Skip to content

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented Oct 24, 2016

  • Allow to search for SHARE_TYPE_EMAIL (4)
  • Added tests

Needed for #1801

@nickvergessen as discussed
@schiessle this is probabaly also usefull for #657

Please review

CC: @LukasReschke @MorrisJobke @icewind1991 @blizzz

@mention-bot
Copy link

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

'shareType' => Share::SHARE_TYPE_EMAIL,
'shareWith' => $email,
],
];
Copy link
Member

Choose a reason for hiding this comment

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

else if (!$this->shareeEnumeration)
and kill the if after the loop?

* @param string $search
*/
protected function getEmails($search) {
$this->result['emails'] = [];
Copy link
Member

Choose a reason for hiding this comment

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

emails not users

foreach ($emails as $email) {
if (strtolower($search) === $contact['FN'] ||
strtolower($search) === strtolower($email)
) {
Copy link
Member

Choose a reason for hiding this comment

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

missing strtolower

@rullzer rullzer force-pushed the shareesAPI_email branch 3 times, most recently from 47d8fd0 to 6d4a2c7 Compare October 24, 2016 12:57
* Allow to search for SHARE_TYPE_EMAIL (4)
* Added tests

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@codecov-io
Copy link

Current coverage is 57.32% (diff: 75.00%)

Merging #1876 into master will increase coverage by 0.06%

@@             master      #1876   diff @@
==========================================
  Files          1075       1077     +2   
  Lines         61317      61586   +269   
  Methods        6851       6866    +15   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          35110      35303   +193   
- Misses        26207      26283    +76   
  Partials          0          0          

Sunburst

Diff Coverage File Path
••••••• 75% ...iles_sharing/lib/Controller/ShareesAPIController.php

Powered by Codecov. Last update 729c065...a28528a

@rullzer
Copy link
Member Author

rullzer commented Oct 24, 2016

@nickvergessen addressed. All tests pass now.

@MorrisJobke MorrisJobke dismissed nickvergessen’s stale review October 24, 2016 19:44

issues are addressed

@nickvergessen
Copy link
Member

👍

@rullzer
Copy link
Member Author

rullzer commented Oct 25, 2016

@MorrisJobke @LukasReschke @schiessle mind a review?

@MorrisJobke
Copy link
Member

👍

@MorrisJobke MorrisJobke merged commit 01a85a9 into master Oct 25, 2016
@MorrisJobke MorrisJobke deleted the shareesAPI_email branch October 25, 2016 11:54
@schiessle
Copy link
Member

@rullzer I fear that this will conflict with #657 because here I only want to search for emails if the share-by-mail provider is loaded. At the point in the code where the search happens we can't distinguish if the request comes from the share auto-complete or from the email field for link shares. Wouldn't it be possible to use something else here than the "SHARE_TYPE_EMAIL"?

@rullzer
Copy link
Member Author

rullzer commented Oct 25, 2016

Uhm wel only if we would introduce another share type

@schiessle
Copy link
Member

do we really need a share provider or share type just to auto complete email addresses? I mean the email addresses added for link shares never become real shares.

@rullzer
Copy link
Member Author

rullzer commented Oct 25, 2016

No but we need to signal the sharee api that we want autocomplete of a certain type. And that uses the share types

@schiessle
Copy link
Member

schiessle commented Oct 25, 2016

The Share manager should handle shares. Asking for a "fake share type" just to auto-complete email addresses sounds wrong to me. I think this should be done either by a different API (ContactsMananger API) which is independent from sharing (because auto completing email addresses has nothing to do with sharing and could be done in many other places as well) or if it really needs to be part of sharing it should be part of the "link share provider/type" and not a separate share type.

I think you can judge best what can be done, because you know the sharing code best. But if we really need to use a "fake share type" we need a additional share type. So either you or I need to move to a new one. I think "SHARE_TYPE_EMAIL" describes perfectly my share provider. Maybe you could use something like "SHARE_TYPE_AUTOCOMPLE_EMAIL"? But already the name sounds really odd to me, to be honest.

@nickvergessen
Copy link
Member

I think this should be done either by a different API (ContactsMananger API) which is independent from sharing (because auto completing email addresses has nothing to do with sharing and could be done in many other places as well)

For the record I tend to agree. But lets continue discussion in #1916

@rullzer
Copy link
Member Author

rullzer commented Oct 26, 2016

Yeah probabaly solved in #1916 is in.

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.

7 participants