Add UI for switching public mail notification language #31869

VicDeo · 2018-06-21T21:52:11Z

Description

Make lang selector reusable
Reuse it for a new option in the Sharing section

Related Issue

https://github.com/owncloud/enterprise/issues/2435#issuecomment-396551153

Motivation and Context

Allows admin users change a language used for public mail notifications

How Has This Been Tested?

Language selector in Settings->Personal -> General -> Language should NOT change it's behavior
A new option should be visible only when Allow users to send mail notification for shared files is checked in Settings->Admin->Sharing
Switching the dropdown to the value Owner language should reset to the current behavior when the notification is sent in the language of the file owner
expected:
php occ config:app:get core shareapi_public_notification_lang produces an empty output in this case (no value set)
Switching the dropdown to any other value except Owner language should update shareapi_public_notification_lang with a respective language code

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-22T18:26:56Z

Codecov Report

Merging #31869 into master will increase coverage by 0.05%.
The diff coverage is 97.72%.

@@             Coverage Diff              @@
##             master   #31869      +/-   ##
============================================
+ Coverage     63.53%   63.58%   +0.05%     
+ Complexity    18555    18553       -2     
============================================
  Files          1167     1169       +2     
  Lines         69566    69604      +38     
  Branches       1264     1264              
============================================
+ Hits          44201    44260      +59     
+ Misses        24996    24975      -21     
  Partials        369      369

Flag	Coverage Δ	Complexity Δ
#javascript	`52.59% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`64.84% <97.72%> (+0.05%)`	`18553 <15> (-2)`	⬇️

Impacted Files	Coverage Δ	Complexity Δ
settings/Application.php	`56.49% <ø> (+0.86%)`	`33 <0> (-1)`	⬇️
settings/Panels/Personal/Profile.php	`100% <100%> (+29.82%)`	`4 <0> (-17)`	⬇️
lib/private/Settings/SettingsManager.php	`72.9% <100%> (+0.17%)`	`44 <0> (ø)`	⬇️
settings/templates/panels/personal/profile.php	`60% <100%> (+1.37%)`	`0 <0> (ø)`	⬇️
lib/private/Helper/LocaleHelper.php	`100% <100%> (ø)`	`15 <15> (?)`
settings/templates/language.php	`100% <100%> (ø)`	`0 <0> (?)`
settings/templates/panels/admin/filesharing.php	`19.04% <50%> (+1.54%)`	`0 <0> (ø)`	⬇️
settings/Panels/Admin/FileSharing.php	`98.52% <95.45%> (-1.48%)`	`7 <0> (+1)`

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 43f921c...813f5d4. Read the comment docs.

PVince81 · 2018-07-03T14:57:08Z

as discussed, ready for review again

phil-davis · 2018-07-03T15:54:15Z

lib/private/Helper/LocaleHelper.php

+			// TRANSLATORS this is a self-name of your language for the language switcher
+			$endonym = (string)$l->t('__language_name__');
+			//Check if the language name is in the translation file
+			// Fallback to hardcoded language name if translation is


if translation is ???

'if it is not'

PVince81

Looks good so far.

I'm missing unit tests for the helper. This is important due to the complexity of the code you have simplified.

PVince81 · 2018-07-04T11:09:47Z

settings/Panels/Personal/Profile.php

-			$l = $this->lfactory->get('settings', $lang);
-			// TRANSLATORS this is the language name for the language switcher in the personal settings and should be the localized version
-			$potentialName = (string) $l->t('__language_name__');
-			if ($l->getLanguageCode() === $lang && \substr($potentialName, 0, 1) !== '_') {//first check if the language name is in the translation file


it looks like this complex logic got simplified in getNormalizedLanguages ?

please make sure you cover all these old code paths in a unit test in LocaleHelperTest

@PVince81 I just made it more readable by moving L79-83 into the getLanguageNameByCode method. The logic itself was not that complex, but it needed cutting into smaller pieces and renaming some variables for the sake of better readability.

basically we check if the current lang has a self-name in it's translation file, then fallback to harcoded language self-names and when everything else fails use language code as a self-name.

PVince81 · 2018-07-04T11:12:23Z

Codecov didn't complain about missing tests for LocaleHelper because the code in question seems to be touched by the other tests. Would still be good to have a test class there to enforce (and document) the many different locale formats

phil-davis · 2018-07-04T11:30:23Z

It would be nice to have acceptance tests for the UI settings page. I made a general issue for that, and when @paurakhsharma gets initial tests, then it will be easy to add tests for this UI element.
(This is just a note FYI, not a blocker of this PR!)

VicDeo · 2018-07-05T23:04:00Z

@PVince81 added some tests with explanations

PVince81 · 2018-07-06T07:49:44Z

lib/private/Helper/LocaleHelper.php

+	 * @var string[]
+	 */
+	private $languageCodes = [
+		'el' => 'Ελληνικά',


I wonder if we can get these from a PHP library in the future

PVince81

👍 looks good, thanks

PVince81 · 2018-07-06T07:50:46Z

@VicDeo please backport

mmattel · 2018-07-06T08:44:58Z

I guess that this is documentation relevant at least because of:
php occ config:app:get core shareapi_public_notification_lang
In case, please open a doc issue and reference it

mmattel · 2018-07-06T08:46:50Z

This language definition for eMails could be useful for eMails sent when creating a new user!
@PVince81 @VicDeo

VicDeo · 2018-07-06T14:19:40Z

Stable10: #32004

lock · 2019-07-30T12:02:35Z

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 21, 2018

VicDeo force-pushed the language-selector branch from 692ca98 to 0a7f9c4 Compare June 22, 2018 18:26

VicDeo force-pushed the language-selector branch 5 times, most recently from d3419bc to a6b515d Compare June 23, 2018 09:55

PVince81 added this to the development milestone Jun 28, 2018

VicDeo added 2 commits July 3, 2018 14:00

Extract lang-related logic from the personal panel

dcd7ffd

Extract template for selector

91c0a21

VicDeo force-pushed the language-selector branch from a6b515d to a1e15c2 Compare July 3, 2018 12:31

VicDeo changed the title ~~[WIP] Make lang selector reusable~~ Add UI for switching public mail notification language Jul 3, 2018

VicDeo added 3 - To Review feature:sharing and removed 2 - Developing labels Jul 3, 2018

VicDeo requested review from PVince81 and pmaier1 July 3, 2018 12:49

VicDeo self-assigned this Jul 3, 2018

VicDeo force-pushed the language-selector branch from a1e15c2 to 915f80f Compare July 3, 2018 13:36

phil-davis reviewed Jul 3, 2018

View reviewed changes

VicDeo force-pushed the language-selector branch from 915f80f to 0dff292 Compare July 3, 2018 19:06

PVince81 reviewed Jul 4, 2018

View reviewed changes

phil-davis mentioned this pull request Jul 4, 2018

UI Tests for Settings page #31983

Closed

5 tasks

Reuse selector

813f5d4

VicDeo force-pushed the language-selector branch from 0dff292 to 813f5d4 Compare July 5, 2018 23:01

VicDeo added the backport-request label Jul 5, 2018

PVince81 reviewed Jul 6, 2018

View reviewed changes

PVince81 approved these changes Jul 6, 2018

View reviewed changes

PVince81 merged commit 5a9909d into master Jul 6, 2018

PVince81 deleted the language-selector branch July 6, 2018 07:50

VicDeo mentioned this pull request Jul 6, 2018

[Stable10] Add UI for switching public mail notification language #32004

Merged

11 tasks

mmattel mentioned this pull request Jul 6, 2018

[Feature Request] Add UI for switching email language when user is generated #32005

Closed

VicDeo removed the backport-request label Jul 6, 2018

mmattel mentioned this pull request Nov 19, 2018

Doc improvements for commands occ config:xxx necessary owncloud/docs#275

Closed

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

Add UI for switching public mail notification language #31869

Add UI for switching public mail notification language #31869

VicDeo commented Jun 21, 2018 •

edited

Loading

codecov bot commented Jun 22, 2018 •

edited

Loading

PVince81 commented Jul 3, 2018

phil-davis Jul 3, 2018

VicDeo Jul 3, 2018

PVince81 left a comment

PVince81 Jul 4, 2018

VicDeo Jul 5, 2018

VicDeo Jul 5, 2018 •

edited

Loading

PVince81 commented Jul 4, 2018

phil-davis commented Jul 4, 2018 •

edited

Loading

VicDeo commented Jul 5, 2018

PVince81 Jul 6, 2018

PVince81 left a comment

PVince81 commented Jul 6, 2018

mmattel commented Jul 6, 2018 •

edited

Loading

mmattel commented Jul 6, 2018

VicDeo commented Jul 6, 2018

lock bot commented Jul 30, 2019

Add UI for switching public mail notification language #31869

Add UI for switching public mail notification language #31869

Conversation

VicDeo commented Jun 21, 2018 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Jun 22, 2018 • edited Loading

Codecov Report

PVince81 commented Jul 3, 2018

phil-davis Jul 3, 2018

Choose a reason for hiding this comment

VicDeo Jul 3, 2018

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 Jul 4, 2018

Choose a reason for hiding this comment

VicDeo Jul 5, 2018

Choose a reason for hiding this comment

VicDeo Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

PVince81 commented Jul 4, 2018

phil-davis commented Jul 4, 2018 • edited Loading

VicDeo commented Jul 5, 2018

PVince81 Jul 6, 2018

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Jul 6, 2018

mmattel commented Jul 6, 2018 • edited Loading

mmattel commented Jul 6, 2018

VicDeo commented Jul 6, 2018

lock bot commented Jul 30, 2019

VicDeo commented Jun 21, 2018 •

edited

Loading

codecov bot commented Jun 22, 2018 •

edited

Loading

VicDeo Jul 5, 2018 •

edited

Loading

phil-davis commented Jul 4, 2018 •

edited

Loading

mmattel commented Jul 6, 2018 •

edited

Loading