Resend notification email #6322
Conversation
This is simply a generalization of the current "displayNamesInGroup" method; it will be needed in a following commit to get extended data for a user in a method in which "displayNamesInGroup" is currently used. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
This will make possible to conditionally add other fields like the e-mail address in a following commit. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
This will be needed in a following commit to return extended user data in the result. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
When searching the sharees only the name of the sharee and its type was returned. Now, if the sharee is a user, its e-mail address is also returned (if it is set for that user); this will be used by the front-end to provide more actions without needing to query the server again for each matched user. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Mail notifications can be resent only to shares with users that have a known e-mail address; e-mail shares are not taken into account, as they have a different workflow. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
This makes possible to trigger the resend from the front-end. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
MorrisJobke
requested review from
LukasReschke,
jancborchardt,
nickvergessen and
rullzer
|
@MorrisJobke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @schiessle and @rullzer to be potential reviewers. |
|
@nickvergessen @rullzer @danxuliu Please review - I would like to get this ASAP into stable12. |
danxuliu
left a comment
danxuliu
left a comment
There was a problem hiding this comment.
Only the two last commits (Add method to send again a mail notification for a share and Add API endpoint to resend an e-mail notification for a share) should be included; the other four commits are required only to provide extended data about the shares, like the e-mail of the sharee, in the share dropdown. However, if the action to resend the notification is not included in the dropdown those changes are not needed.
Note, however, that whether an e-mail address is set in the user profile or not should be provided by formatShare in ShareAPIController in order for the front-end to be able to know whether to show the action to resend e-mail notification or not in the share menu (it should be taken into account in sharedialogshareelistview.js and shareitemmodel.js), but I would change that in a different pull request, even for the PHP part.
In any case, even if we said we will add the extended data for the dropdown anyway just in case it is useful in the future (personally I would not do that), the commit Extend getUsers to return the e-mail addresses if available must be amended before merging; in order to make it as flexible as possible I returned the e-mail address for the user if available, but I forgot to take into account the visibility settings set for the e-mail address in the user profile. Therefore, even if the user set the e-mail address as Private and thus only visible to herself her e-mail address would be sent anyway to the sharer (it would not be shown in the UI, but it could be get by checking the HTTP response)! Instead of sending the e-mail address it should be changed to just sending whether the user has an e-mail address set or not (but, again, personally I would drop the first four commits entirely).
|
I'm unsure if it's necessary from a UX point of view. It makes it seem like notifications could have errors sending … and on the other hand, if the first notification wasnt seen, why would the new one? Would you not then ask the person via a different channel to verify? |
|
This needs fixing after #6414 So it also injects the language |
nickvergessen
added
2. developing
This is an extract of the PHP backend code for #6276
I would like to hear your feedback on this. The idea is to have then later an entry in the share list inside of the 3 dots menu that says "resend share notification" and is an way for the sharer to resend the notification. This PR basically is the base for this, to provide the needed API endpoints.
cc @jancborchardt form a UX point of view: does it make sense to introduce such a functionality?
I tested #6276 and it worked fine there. I had a view at the code and it looks fine. 👍