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.

Sign up for GitHub

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

Jump to bottom

Share email to ocs #1801

Closed
wants to merge 2 commits into from
Closed

Share email to ocs #1801

wants to merge 2 commits into from

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Oct 19, 2016

Fixes #1277

  • Adds a new endpoint to send email for a link share
  • Removed the possibility to notify recipients of a share
    • They can configure themselfs if they want notifications
    • Long term plan is still to also accept local shares
    • Would require adding a new endpoint that we'd have to support for long time

Currently we use the mailer as is. Which is not really testable. But I want a working version in sooner rather than later.

CC: @MorrisJobke @nickvergessen @LukasReschke @icewind1991

@rullzer rullzer added this to the Nextcloud 11.0 milestone Oct 19, 2016
@mention-bot
Copy link

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

@rullzer
Copy link
Member Author

rullzer commented Oct 19, 2016

Also it seems the autocomplete for e-mail addresses is broken. So I disabled it for now. @nickvergessen we can't just search by e-mail on the sharee api right?

@MorrisJobke
Copy link
Member

@nickvergessen we can't just search by e-mail on the sharee api right?

I don't think so, because the "email address style" is actually a remote share, right? cc @schiessle

@nickvergessen
Copy link
Member

@MorrisJobke it just sends the link from the link share field.

We can't just search by e-mail on the sharee api right?

we can make it do so. As far as I can see it just looks into the contacts app for remote users atm.

@rullzer
Copy link
Member Author

rullzer commented Oct 22, 2016

@nickvergessen yeah it looks trough all the contacts for e-mail completion it seems. Would be nice if we could add that. I guess we could abuse the SHARE_TYPE_EMAIL for that. Bjorn might also use that in his share by email PR. But searching there is the same I'd say.

@rullzer rullzer force-pushed the share_email_to_OCS branch from 92788cc to 5ce02d1 Compare October 22, 2016 11:55
@jancborchardt
Copy link
Member

Removed the possibility to notify recipients of a share
They can configure themselves if they want notifications

Yeah! Thanks a lot @rullzer :)

@rullzer rullzer mentioned this pull request Oct 24, 2016
Merged
MorrisJobke
MorrisJobke requested changes Oct 24, 2016
View reviewed changes
core/js/shareitemmodel.js
dataType: 'json'
}). fail(function(xhr) {
// FIXME: a model should not show dialogs
OC.dialogs.alert(t('core', result.data.message), t('core', 'Warning'));
Copy link
Member

Choose a reason for hiding this comment

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

Where is the result coming from?

core/js/shareitemmodel.js
}
);
dataType: 'json'
}). fail(function(xhr) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the xhr should be named result? 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

yes copy paste error

@rullzer rullzer added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 25, 2016
@rullzer rullzer force-pushed the share_email_to_OCS branch from 5ce02d1 to 578ac48 Compare October 25, 2016 13:33
@rullzer rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 25, 2016
@codecov-io
Copy link

codecov-io commented Oct 25, 2016 •
edited
Loading

Current coverage is 57.78% (diff: 0.00%)

Merging #1801 into master will increase coverage by 0.26%

@@             master      #1801   diff @@
==========================================
  Files          1076       1076          
  Lines         61306      61854   +548   
  Methods        6871       7020   +149   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          35264      35742   +478   
- Misses        26042      26112    +70   
  Partials          0          0

Sunburst

Diff Coverage File Path
0% apps/files_sharing/appinfo/routes.php
0% .../files_sharing/lib/Controller/ShareAPIController.php
•••••••••• 100% settings/templates/admin/sharing.php
•••••••••• 100% lib/private/Share/MailNotifications.php

Powered by Codecov. Last update 8957436...578ac48

@rullzer
Remove notify recipient option
57e6125 
Signed-off-by: Roeland Jago Douma <[email protected]>
@rullzer
Remove internal share mail function
b7046d3 
Signed-off-by: Roeland Jago Douma <[email protected]>
@rullzer rullzer force-pushed the share_email_to_OCS branch from 578ac48 to b7046d3 Compare October 26, 2016 18:54
@rullzer
Copy link
Member Author

rullzer commented Oct 26, 2016

Ok lets close this in favor of #1916

@rullzer rullzer closed this Oct 26, 2016
@LukasReschke LukasReschke deleted the share_email_to_OCS branch October 27, 2016 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MorrisJobke MorrisJobke MorrisJobke requested changes

Assignees
No one assigned
Labels
3. to review Waiting for reviews bug feature: sharing
Projects
None yet
Milestone
Nextcloud 11.0
Development

Successfully merging this pull request may close these issues.
6 participants
@rullzer @mention-bot @MorrisJobke @nickvergessen @jancborchardt @codecov-io