-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Sharing link & mail parity #24364
Conversation
bba5d88
to
8b9f171
Compare
This comment has been minimized.
This comment has been minimized.
96c1adc
to
b43fde9
Compare
This comment has been minimized.
This comment has been minimized.
e985224
to
fadb430
Compare
fadb430
to
3d30347
Compare
There was a problem hiding this 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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Please let get this in! 😭 |
ba5d8d5
to
4a0d3cb
Compare
This comment has been minimized.
This comment has been minimized.
4a0d3cb
to
0915c10
Compare
2cd9976
to
2e59c31
Compare
There was a problem hiding this 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 👍
Let me fix the integration tests |
060b481
to
830d1e8
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
830d1e8
to
78bcd6d
Compare
Rebased and integration tests fixed! 🤖 |
Now PHPUnit and Psalm are not happy 🙈 |
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: 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>
78bcd6d
to
1576764
Compare
Rebased™ |
@PVince81 @rullzer @nickvergessen @MorrisJobke GREEN! |
@tobiasKaminsky @marinofaggiana you might be interested in this! |
Fix #24209
Fix #19562
Put the mail shares into he same check as link shares and unify settings
➡️ we now show the dropdown automatically with the random password (if enforced)