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

Sharing link & mail parity #24364

Merged
merged 8 commits into from
Mar 22, 2021
Merged

Sharing link & mail parity #24364

merged 8 commits into from
Mar 22, 2021

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Nov 25, 2020

Fix #24209
Fix #19562

Put the mail shares into he same check as link shares and unify settings

  • The password enforce was not making any sense here
  • Disabling the "send by mail" and enforcing password would lead to a random unknown password
    ➡️ we now show the dropdown automatically with the random password (if enforced)
  • Make sure the mail shares uses the proper expiration
  • Clear the input after adding a new share
  • Show the email on a second line if a label is set
    Capture d’écran_2020-11-25_10-15-05

Peek 25-11-2020 09-57

@skjnldsv

This comment has been minimized.

@skjnldsv skjnldsv force-pushed the fix/sharing-mail-link-parity branch 3 times, most recently from 96c1adc to b43fde9 Compare November 25, 2020 10:22
@skjnldsv

This comment has been minimized.

This was referenced Dec 14, 2020
@rullzer rullzer mentioned this pull request Dec 28, 2020
39 tasks
@rullzer rullzer mentioned this pull request Jan 4, 2021
5 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 21, Nextcloud 22 Jan 7, 2021
@danxuliu danxuliu force-pushed the fix/sharing-mail-link-parity branch from fadb430 to 3d30347 Compare January 7, 2021 16:42
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

I have extracted some of the changes to their own commits (please do not squash several fixes and changes in the same commit, as it makes reviewing more difficult both now and in the future in case a bisection is needed). I am not sure if it would be even better to extract them to different pull request to ease backporting (I have not checked if they need to be backported and to which versions, though, so maybe it is fine if they are all in the same pull request :-) ).

In any case, I have not addressed yet the integration test failures (although I think that they are legit and caused by an "overunification" :-) For example the password of mail shares can not be hashed before passing it to the provider (as otherwise it would be hashed in the e-mail too)).

I was wondering if it would be better not to unify the settings. Although similar, mail shares are not link shares, and the infrastructure to enforce passwords in mail shares independently of link shares is already in place and works, so unifying it would be removing a feature that may (or may not) be useful for some people.

Due to this I would not unify the enforced password setting for link and mail shares. However, in this case I guess that an expiration date field specific to mail shares should be added too (as using the setting from link shares for this would be counterintuitive).

Additionally, the fact that mail shares require link shares to be enabled seems to be just due to a check in the middleware. But I think that check should not be there :-) The public share page is generic and makes possible to view any share with a token, not only link shares. Due to this, if link shares are disabled, that check causes that mail shares and also shares in public Talk conversations to not be available.

Note that even if that check is removed link shares would still not work when disabled, but other token based shares will do (although in order to be able to create mail shares another check should be removed too). This is probably something for a different pull request, but I thought that it was worth mentioning it.

All in all I think that the settings for link and mail shares should not be unified (although the changes in the WebUI for the label or enforced passwords are fine). Thoughts?

Whether the settings are unified or not I think that @tobiasKaminsky would like to know about this pull request :-)

Independently of all that, when passwords for mail shares are enforced and a new mail share is created the Video verification checkbox should be initially disabled (exactly the same as after a password is set), as it will not be possible to enable it until a new password is typed.

@faily-bot

This comment has been minimized.

@rullzer

This comment has been minimized.

@skjnldsv
Copy link
Member Author

Please let get this in! 😭

@skjnldsv skjnldsv force-pushed the fix/sharing-mail-link-parity branch 2 times, most recently from ba5d8d5 to 4a0d3cb Compare March 17, 2021 17:34
@skjnldsv

This comment has been minimized.

@skjnldsv skjnldsv force-pushed the fix/sharing-mail-link-parity branch 2 times, most recently from 2cd9976 to 2e59c31 Compare March 18, 2021 11:00
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.

Beside my nitpick it works great 👍

@skjnldsv
Copy link
Member Author

Let me fix the integration tests

@skjnldsv skjnldsv force-pushed the fix/sharing-mail-link-parity branch 3 times, most recently from 060b481 to 830d1e8 Compare March 19, 2021 13:35
@PVince81

This comment has been minimized.

@skjnldsv

This comment has been minimized.

@skjnldsv skjnldsv force-pushed the fix/sharing-mail-link-parity branch from 830d1e8 to 78bcd6d Compare March 19, 2021 14:06
@skjnldsv
Copy link
Member Author

Rebased and integration tests fixed! 🤖

@MorrisJobke
Copy link
Member

Rebased and integration tests fixed! 🤖

Now PHPUnit and Psalm are not happy 🙈

skjnldsv and others added 8 commits March 22, 2021 06:34
Now the email is shown on a second line if a label is set.

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
This is needed to be able to use the function from other components.

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: npmbuildbot-nextcloud[bot] <npmbuildbot-nextcloud[bot]@users.noreply.github.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the fix/sharing-mail-link-parity branch from 78bcd6d to 1576764 Compare March 22, 2021 05:51
@skjnldsv
Copy link
Member Author

Rebased™

@skjnldsv
Copy link
Member Author

@PVince81 @rullzer @nickvergessen @MorrisJobke GREEN!
LGTM 🚢

@rullzer rullzer merged commit d3647dc into master Mar 22, 2021
@rullzer rullzer deleted the fix/sharing-mail-link-parity branch March 22, 2021 06:56
@skjnldsv
Copy link
Member Author

@tobiasKaminsky @marinofaggiana you might be interested in this!
I don't know on what you rely for email share on the clients, but now mails and link shares behaves the same! :)

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