Allow different language in public link share email #31656

VicDeo · 2018-06-05T21:03:13Z

Description

Allow set another language for email notification when sharing with link

Related Issue

https://github.com/owncloud/enterprise/issues/2435

Motivation and Context

How Has This Been Tested?

Case 1

Share via link and send an email
Check your mailbox

expected

email notification has language in accordance with language set in owner personal settings

Case 2

php occ config:app:set core shareapi_public_notification_lang --value 'ru'
Share via link and send an email
Check your mailbox

expected

email notification is in Russian

Note: in both cases personal message is not translated

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

codecov · 2018-06-06T17:18:02Z

Codecov Report

Merging #31656 into master will increase coverage by 0.01%.
The diff coverage is 94.87%.

@@             Coverage Diff              @@
##             master   #31656      +/-   ##
============================================
+ Coverage     62.89%   62.91%   +0.01%     
- Complexity    18418    18422       +4     
============================================
  Files          1154     1154              
  Lines         69157    69177      +20     
  Branches       1260     1260              
============================================
+ Hits          43499    43521      +22     
+ Misses        25289    25287       -2     
  Partials        369      369

Flag	Coverage Δ	Complexity Δ
#javascript	`52.39% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`64.12% <94.87%> (+0.01%)`	`18422 <11> (+4)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
core/ajax/share.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/private/Share/MailNotifications.php	`100% <100%> (ø)`	`35 <8> (+4)`	⬆️
lib/private/legacy/template.php	`49.68% <100%> (ø)`	`39 <3> (ø)`	⬇️
core/templates/mail.php	`100% <0%> (+20%)`	`0% <0%> (ø)`	⬇️
core/templates/altmail.php	`100% <0%> (+25%)`	`0% <0%> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 649c57c...405ec37. Read the comment docs.

VicDeo · 2018-06-06T17:26:39Z

@PVince81 @pmaier1 should it also be configurable via settings page or CLI is enough?

pmaier1 · 2018-06-07T07:10:40Z

@PVince81 @pmaier1 should it also be configurable via settings page or CLI is enough?

Unless @PVince81 has a different opinion I think CLI should be enough for this. Default value should be "English" btw. And for CLI to be a good solution, this needs documentation (for the command and possible values)!! Please take care. /cc @settermjd

PVince81 · 2018-06-07T08:50:24Z

If not too much effort we can add a setting in the settings page. I expect this to be a matter of copying the existing language selection box and adjusting. Let's do this in a separate PR (optional task).

@pmaier1 we need to keep in mind the difference between the sysadmin and OC admin roles. Only the sysadmin has access to CLI and OC admin needs to setup everything from the web UI.

PVince81

Looks good, see my comment for clarification

PVince81 · 2018-06-07T08:55:23Z

lib/private/legacy/template.php

+		// fix translation when app is something like core/lostpassword
+		$parts = \explode('/', $app);
+
+		$languageCode = $l10n === null ? null : $l10n->getLanguageCode();


if the language code of the injected is different, doesn't it mean you can simply reuse that instance instead of re-getting it ?

@PVince81 I did it this way in the previous PR, but this was incorrect as l10n instance depends both on the app and locale. So we need to create an l10n instance with app matching $app ctor arg

can you add a PHP comment so future onlookers will not wonder ?

also adjust the constructor PHPDoc and clarify which IL10N instance needs to be injected (generic vs app specific vs ...)

@PVince81 I thought it over, let's pass just a language code instead of an object that is not fully suitable.

VicDeo · 2018-06-07T12:28:20Z

@PVince81 @pmaier1

Default value should be "English"

This way we will change the current behavior (notification in the lang of the owner) with no fallback option.

As for UI this selector makes sense only when mail notifications are enabled

pmaier1 · 2018-06-07T13:30:41Z

@pmaier1 we need to keep in mind the difference between the sysadmin and OC admin roles. Only the sysadmin has access to CLI and OC admin needs to setup everything from the web UI.

Hm, yes, agreed. Taking this into account a dropdown in admin "General" settings would be good.

VicDeo · 2018-06-07T13:55:10Z

disagree. Why in "General", when it is related to sharing and applicable only in case mail notifications for public links are enabled?

pmaier1 · 2018-06-07T14:02:38Z

disagree. Why in "General", when it is related to sharing and applicable only in case mail notifications for public links are enabled?

Hmm ok, good point. I thought it would apply to other notifications or potential future notifications as well. If it's just for link shares and will continue to be then probably next to the setting to enable link share sending in the "Sharing" settings fits better.

PVince81 · 2018-06-11T15:50:46Z

@VicDeo add UI selector in this PR or separately ?

VicDeo · 2018-06-11T22:01:54Z

@PVince81 better separately, unless you want me to c&p most of code from https://github.com/owncloud/core/blob/master/settings/Panels/Personal/Profile.php

PVince81 · 2018-06-12T11:01:28Z

@VicDeo please backport

PVince81 · 2018-06-12T16:06:24Z

I just tried a quick backport but there are conflicts on the template level.
Leaving this up to you to solve them, might need to find out why this part of code is so different from master.

VicDeo · 2018-06-13T17:57:53Z

Stable10: #31767
not that match differences, all related to php-cs in master: spaces, global namespace for functions, etc

lock · 2019-07-30T16:02:24Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

VicDeo added the 2 - Developing label Jun 5, 2018

VicDeo added this to the development milestone Jun 5, 2018

VicDeo self-assigned this Jun 5, 2018

VicDeo force-pushed the implement-e2435-public-shares branch 2 times, most recently from 0a85bfa to 91ace1c Compare June 6, 2018 17:17

VicDeo force-pushed the implement-e2435-public-shares branch from 91ace1c to 115cac1 Compare June 6, 2018 17:24

VicDeo added p3-medium Normal priority enhancement labels Jun 6, 2018

PVince81 approved these changes Jun 7, 2018

View reviewed changes

VicDeo force-pushed the implement-e2435-public-shares branch from 115cac1 to d96956b Compare June 7, 2018 12:23

VicDeo added 3 - To Review and removed 2 - Developing labels Jun 7, 2018

Allow different language in public link share email

405ec37

VicDeo force-pushed the implement-e2435-public-shares branch from d96956b to 405ec37 Compare June 7, 2018 13:58

PVince81 merged commit ae6fbf6 into master Jun 12, 2018

PVince81 deleted the implement-e2435-public-shares branch June 12, 2018 11:01

PVince81 added the backport-request label Jun 12, 2018

PVince81 removed this from the development milestone Jun 13, 2018

PVince81 added this to the QA milestone Jun 13, 2018

VicDeo mentioned this pull request Jun 13, 2018

[Stable10] Allow different language in public link share email #31767

Merged

9 tasks

VicDeo removed the backport-request label Jun 13, 2018

lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow different language in public link share email #31656

Allow different language in public link share email #31656

VicDeo commented Jun 5, 2018 •

edited

Loading

codecov bot commented Jun 6, 2018 •

edited

Loading

VicDeo commented Jun 6, 2018

pmaier1 commented Jun 7, 2018

PVince81 commented Jun 7, 2018

PVince81 left a comment

PVince81 Jun 7, 2018

VicDeo Jun 7, 2018

PVince81 Jun 7, 2018

VicDeo Jun 7, 2018

VicDeo commented Jun 7, 2018

pmaier1 commented Jun 7, 2018

VicDeo commented Jun 7, 2018

pmaier1 commented Jun 7, 2018 •

edited

Loading

PVince81 commented Jun 11, 2018

VicDeo commented Jun 11, 2018

PVince81 commented Jun 12, 2018

PVince81 commented Jun 12, 2018

VicDeo commented Jun 13, 2018 •

edited

Loading

lock bot commented Jul 30, 2019

Allow different language in public link share email #31656

Allow different language in public link share email #31656

Conversation

VicDeo commented Jun 5, 2018 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

expected

expected

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Jun 6, 2018 • edited Loading

Codecov Report

VicDeo commented Jun 6, 2018

pmaier1 commented Jun 7, 2018

PVince81 commented Jun 7, 2018

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 Jun 7, 2018

Choose a reason for hiding this comment

VicDeo Jun 7, 2018

Choose a reason for hiding this comment

PVince81 Jun 7, 2018

Choose a reason for hiding this comment

VicDeo Jun 7, 2018

Choose a reason for hiding this comment

VicDeo commented Jun 7, 2018

pmaier1 commented Jun 7, 2018

VicDeo commented Jun 7, 2018

pmaier1 commented Jun 7, 2018 • edited Loading

PVince81 commented Jun 11, 2018

VicDeo commented Jun 11, 2018

PVince81 commented Jun 12, 2018

PVince81 commented Jun 12, 2018

VicDeo commented Jun 13, 2018 • edited Loading

lock bot commented Jul 30, 2019

VicDeo commented Jun 5, 2018 •

edited

Loading

codecov bot commented Jun 6, 2018 •

edited

Loading

pmaier1 commented Jun 7, 2018 •

edited

Loading

VicDeo commented Jun 13, 2018 •

edited

Loading