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

Unified sharing options #4136

Merged
merged 19 commits into from
Apr 7, 2017
Merged

Unified sharing options #4136

merged 19 commits into from
Apr 7, 2017

Conversation

schiessle
Copy link
Member

@schiessle schiessle commented Mar 29, 2017

  • password protected mail shares
  • secure drop for mail shares
  • expire date for all shares
  • unit tests

This is how it looks like:

A folder shared by mail

Now with "secure drop", expiration date and password option

permissions-mail-share-folder

if you set a password the recipient will get a additional mail which contain the password.

A file shared by mail

Now with expiration date and password option

permissions-file-by-mail

if you set a password the recipient will get a additional mail which contain the password.

A file/folder shared to a user/group

Now with expire date

permissions-folder-share-internal

@mention-bot
Copy link

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

@jancborchardt
Copy link
Member

@schiessle let me know when it’s ready for review or if you need design fixes.

Is there still the issue that clicking further down on the date picker will close the menu? Maybe then @nextcloud/javascript folks can help you. :)

@schiessle schiessle force-pushed the expire-date-for-all-shares branch from f3f3e17 to bf64e31 Compare March 29, 2017 17:25
@schiessle
Copy link
Member Author

schiessle commented Mar 29, 2017

@jancborchardt you can already try it out and review it. All the functionality is there. At the moment I just adjust and fix some unit tests in the background.

Following issues exists:

  • If you select a date on the datepicker the permissions popover closes. Would be nice if we could find a way to keep it open, cc @nextcloud/javascript

  • If you set a password for a mail share you see a spinner on save which is not positioned correctly, maybe you or @nextcloud/designers can help here with some css magic

@TeeDizzle
Copy link

@schiessle and @jancborchardt: Thank you for your work and pushing this into the right direction.
I just set up an instance based on the new branch and found the password function not to work for me. I can set a password for a share via webmailer and the share will be protected but the password will not be accepted (The password is wrong. Try again.) trying to open the link. The same behaviour I experience sharing and password protecting a link manually, here aditionally a never disappearing spinner shows up indicating some incomplete action.
Last but not least I really hope you will find a way to make the current automatic sending of the share password via webmailer optional and not a default to meet previous security standards. Please refer to my comments on #2357 for this.

@schiessle
Copy link
Member Author

@TeeDizzle Thanks for testing. I will look into the password check issue. The plan is also to have a admin setting "Send password by mail" which you can disable if you wish.

@TeeDizzle
Copy link

@schiessle: Great to hear that, thank you so much. I will keep the test instance updated as you proceed and continue to report back.

@schiessle schiessle force-pushed the expire-date-for-all-shares branch from bf64e31 to dbd64ba Compare March 30, 2017 10:47
@nickvergessen
Copy link
Member

I get

Error while sharing
Invalid date. Format must be YYYY-MM-DD

When setting a password for a mail share.

Works perfectly. Nice roundhouse kick for all the sharing feature requests 🎉

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Needs an entry in capabilities

@schiessle schiessle force-pushed the expire-date-for-all-shares branch from 9bed8c9 to ce8b876 Compare March 30, 2017 16:22
@schiessle
Copy link
Member Author

@nickvergessen capabilities added

@schiessle schiessle force-pushed the expire-date-for-all-shares branch from ce8b876 to 3242ca0 Compare March 30, 2017 16:57
@schiessle schiessle added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 30, 2017
@diopgm
Copy link

diopgm commented Mar 31, 2017

hi - wanted to give it a try, but can't get it patched:
https://help.nextcloud.com/t/how-do-i-implement-a-change-pullrequest-available-on-github/10827/2
any help appreciated!

@supremesyntax
Copy link

@diopgm
This is how i did it. This downloads the complete server code to set up a new virtual host for testing for example

  1. git clone https://github.com/nextcloud/server.git
  2. cd server
  3. git submodule update --init
  4. git checkout -t origin/expire-date-for-all-shares
  5. git pull

@diopgm
Copy link

diopgm commented Mar 31, 2017

sorry, i dont want to hijack this thread for my troubles :) but privatemessages seem not availiable on github...
but thanks supremesyntax for this input, although i would like to update my regular nextcloud installation, not create a new virtual host - as this is still not a productive system yet
also, i'm not familiar with what happens behind the curtain through these commands :)
but thanks for your input 👍

@supremesyntax
Copy link

supremesyntax commented Mar 31, 2017

@schiessle @jancborchardt Just got a port forward from my admin for e-mail smtp access. I have tested the e-mail function after a fresh "git pull" and it looks good to me. seems like a good compromise between bringing back the old "send password protected link" function and unifying the input fields. i will talk to some end-user to get their point of view and see if this would get accepted and will report back
next week. good work!

Edit: Important! It is still possible to share via e-mail without the password is being set!! This should not be possible. I have set 'Enforce password protection'. Somehow you should respect this. So do not send out the e-mail with the link before password is set when admin settings say so!
Edit2: The option 'Set default expiration date' is also not respected when sharing stuff via e-mail. In my understanding the expiration date should be automatically preset and ticked when sharing an object if admin made the choice.
Edit3: Enforcing the expiration date is also not respected.

Edit4: Just a 'nice to have' would be a separate activity when setting a password like 'You set a password for share XYZ by mail with xyz@wasd.com'

@diopgm i think patching your instance is not possible without breaking stuff. the commits are based on nextcloud 12 on github and it is not recommended as productive system yet. i don't know though if backporting this to nextcloud 11 is possible atm... the commands i wrote are explained here: https://git-scm.com/docs
(tl;dr; the commands copy the 'nextcloud master repository'+'3rd party apps'+'branch with the changes @schiessle made' to your local computer. you need to install git of course depending on your OS)

@schiessle schiessle force-pushed the expire-date-for-all-shares branch from 3242ca0 to eca6fc4 Compare March 31, 2017 08:17
@diopgm
Copy link

diopgm commented Mar 31, 2017

version 12?
well, thats a bit disappointing :(
its probably miles away still, and i would like to have the feature in the documentation back (send mail links) and thought this here would be my best try :(
seems like i have to switch to a different sw then, to fulfill the users request in time now :( too bad, liked nc so far
thanks supremesyntax for your input
anyone let me know if there is a way to get that documented feature back into 11.0.2

@schiessle
Copy link
Member Author

schiessle commented Apr 7, 2017

@MorrisJobke I can't reproduce it. This is how it looks for me after I set the expire date, closed and re-opened the permission drop down.

permissions

Tested with Firefox 52.0.2 and Chromium 57.0.2987.98. Which Browser did you used?

I saw some screenshots from @jospoortvliet yesterday which also looked OK.

@rullzer
Copy link
Member

rullzer commented Apr 7, 2017

Mmm why does it display the timestamp... I can look into that part... it should just show the date.

@schiessle
Copy link
Member Author

@rullzer good question. It shows what the share API returns. Maybe we need to re-format it before we show it. Or change the share API that it returns the right format right away.

@rullzer
Copy link
Member

rullzer commented Apr 7, 2017

Nah we need to parse it trough moment... I can do that later

@rullzer
Copy link
Member

rullzer commented Apr 7, 2017

Basically I the date should be localized. So our american users get their weird date format as well

@rullzer rullzer self-assigned this Apr 7, 2017
@rullzer
Copy link
Member

rullzer commented Apr 7, 2017

okidoki date now looks sane...

@rullzer rullzer removed their assignment Apr 7, 2017
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

We'll probabaly find some bugs later. But for now lets do this!

@schiessle schiessle force-pushed the expire-date-for-all-shares branch from 158f120 to df141dd Compare April 7, 2017 13:28
Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
…rative editing documents

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
@schiessle schiessle force-pushed the expire-date-for-all-shares branch from df141dd to 11b4638 Compare April 7, 2017 13:44
@MorrisJobke
Copy link
Member

Tested with Firefox 52.0.2 and Chromium 57.0.2987.98. Which Browser did you used?

Chrome and Safari. But let me recheck.

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt
Copy link
Member

Fixed the transparency issue – just made everything full opacity because the checkmarks indicate if it’s checked.

Also fixed the font-weight of the »Unshare« entry.

@MorrisJobke
Copy link
Member

okidoki date now looks sane...

Not here. I guess @jancborchardt has overwritten your changes.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
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 now 👍

@MorrisJobke MorrisJobke merged commit ca9d251 into master Apr 7, 2017
@MorrisJobke MorrisJobke deleted the expire-date-for-all-shares branch April 7, 2017 22:14
@jancborchardt
Copy link
Member

Not here. I guess @jancborchardt has overwritten your changes.

Just for the record: I didn’t overwrite anything. ;)

@schiessle
Copy link
Member Author

Was my mistake :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet