allow to specify a link to a legal notice by blizzz · Pull Request #9437 · nextcloud/server

blizzz · 2018-05-09T13:57:06Z

Adds another input field to theming settings:

the legal notice is added to the footer (default is empty):

Also fixes a bug where changes to input[type=url] where not saved.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

jancborchardt · 2018-05-09T19:55:52Z


+a.legal {
+	font-size: smaller;
+	text-decoration: underline;


No underline on links please. We don't do that anywhere, compare to "Nextcloud" in the footer.

Actually we have a style for external links that does this: https://github.com/nextcloud/server/blob/master/core/css/styles.scss#L65-L68 I am fine with removing it however.

jancborchardt · 2018-05-09T19:56:20Z

 	<div class="advanced-options">
+ <div>
+ <label>
+ <span><?php p($l->t('Legal notice address')) ?></span>


"Legal notice link"

Further up is also "Web address", I used this for conistency.
server/apps/theming/templates/settings-admin.php
Lines 50 to 51 in 62f1156
<span><?phpp($l->t('Web address')) ?></span>
<input id="theming-url" type="url" placeholder="<?phpp($l->t('Web address https://…')); ?>" value="<?phpp($_['url']) ?>" maxlength="500" />

We should change that one to »Website link« also. »Address« is just too ambiguous, especially for a legal notice. It might as well be the postal address of an imprint. (I know it says »https://…« but let’s make sure to use simple language.)

MariusBluem · 2018-05-09T20:06:20Z

What about a seperate „Privacy“ link?

blizzz · 2018-05-09T20:20:40Z

What about a seperate „Privacy“ link?

Requirement was link to imprint/legal notice on login page.

If more should be placed there, we need a different, flexible approach, as I am not gonna add links for also Cookies, Contact us, Security, Cialis, Blog, etc. Perhaps with some pre-defined labels to allow easy translation, that's a significantly more work however.

MariusBluem · 2018-05-10T14:38:58Z

The external-app has options for positions (where login-page could be one I think). This would make it easily possible to change the link for specific languages (show it only in EU for example) and integrate it into the clients too... Maybe this is something for the future ....

However, it is working fine - THX 👍

juliusknorr

Although I would prefer to have this integrated in the external sites app, since it would make the whole mechanism a lot more flexible and fit better to the concept of external sites than theming, i'd be fine to merge this to have it available quickly.

Looks good besides some small comments.

juliusknorr · 2018-05-11T10:10:18Z

+ <div>
+ <label>
+ <span><?php p($l->t('Legal notice address')) ?></span>
+ <input id="theming-imprintUrl" type="url" placeholder="<?php p($l->t('Legal notice address https://…')); ?>" value="<?php p($_['imprintUrl']) ?>" maxlength="500" />


I'd probably remove the Legal notice address from the placeholder, since that is duplicate to the label.

Same for web address then ^

juliusknorr · 2018-05-11T10:12:47Z

+ <label>
+ <span><?php p($l->t('Legal notice address')) ?></span>
+ <input id="theming-imprintUrl" type="url" placeholder="<?php p($l->t('Legal notice address https://…')); ?>" value="<?php p($_['imprintUrl']) ?>" maxlength="500" />
+ <div data-setting="imprintUrl" data-toggle="tooltip" data-original-title="<?php p($l->t('Reset to default')); ?>" class="theme-undo icon icon-history"></div>


The undo button is not working, as in not visible to the user. You probably need to add the empty default value here: https://github.com/nextcloud/server/pull/9437/files#diff-7336f3dab52c02ef94f856620590f56bR80

the key issue was missing CSS rules for url-input fields, undoing "web address" was also broken. fixed with a coming commit :)

juliusknorr · 2018-05-11T10:17:15Z

There were 4 failures:
1) OCA\Theming\Tests\Settings\AdminTest::testGetFormNoErrors
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
- 'iconDocs' => null
+ 'iconDocs' => ''
'images' => Array ()
+ 'imprintUrl' => null
/drone/src/github.com/nextcloud/server/apps/theming/tests/Settings/AdminTest.php:111
2) OCA\Theming\Tests\Settings\AdminTest::testGetFormWithErrors
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
+ 'imprintUrl' => null
/drone/src/github.com/nextcloud/server/apps/theming/tests/Settings/AdminTest.php:160
3) OCA\Theming\Tests\Controller\ThemingControllerTest::testGetJavascript
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
+ imprintUrl: null,\n
/drone/src/github.com/nextcloud/server/apps/theming/tests/Controller/ThemingControllerTest.php:825
4) OCA\Theming\Tests\Controller\ThemingControllerTest::testGetJavascriptInverted
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
+ imprintUrl: null,\n
/drone/src/github.com/nextcloud/server/apps/theming/tests/Controller/ThemingControllerTest.php:859

blizzz · 2018-05-11T13:34:32Z

@juliushaertl external sites did not come up to my mind, and i have not looked at it for ages… last time i hada glance, you could only insert a link. This looks more interesting now :) But getting this to work there needs to be wired with templates at least, if not theming (pre-/post-footer vs. general footer). And, as you say, cannot be just shipped to 13 out of its maintenance release.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

and minor adjustments Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

codecov · 2018-05-11T13:58:51Z

Codecov Report

Merging #9437 into master will decrease coverage by <.01%.
The diff coverage is 47.82%.

@@ Coverage Diff @@## master #9437 +/- ##
============================================
- Coverage 51.61% 51.61% -0.01% - Complexity 25684 25699 +15 
============================================
Files 1638 1638 Lines 96283 96341 +58 Branches 1393 1393 ============================================
+ Hits 49695 49723 +28 - Misses 46588 46618 +30

Impacted Files	Coverage Δ	Complexity Δ
apps/theming/templates/settings-admin.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
apps/theming/lib/Settings/Admin.php	`82.85% <100%> (+0.5%)`	`5 <0> (ø)`	⬇️
apps/theming/lib/ThemingDefaults.php	`89.79% <100%> (+0.58%)`	`51 <1> (+3)`	⬆️
apps/theming/lib/Controller/ThemingController.php	`72.96% <20%> (-2.38%)`	`31 <0> (+2)`
lib/private/legacy/json.php	`0% <0%> (-12%)`	`19% <0%> (ø)`
lib/private/AppFramework/OCS/BaseResponse.php	`75.86% <0%> (-4.91%)`	`14% <0%> (+10%)`
lib/private/legacy/api.php	`34.37% <0%> (-1.05%)`	`31% <0%> (ø)`
apps/files_versions/lib/Sabre/RootCollection.php	`0% <0%> (ø)`	`5% <0%> (ø)`	⬇️
apps/files_trashbin/lib/Sabre/RootCollection.php	`0% <0%> (ø)`	`5% <0%> (ø)`	⬇️
apps/files_trashbin/lib/Expiration.php	`91.93% <0%> (+1.61%)`	`29% <0%> (ø)`	⬇️

blizzz · 2018-05-11T13:58:51Z

I left "address" instead calling it link to keep it consistent with the web url. otherwise, all remarks are addressed.

blizzz · 2018-05-15T15:06:45Z

@jancborchardt 🏓

jancborchardt · 2018-05-15T20:17:05Z

@blizzz see my follow-up comment to the wording link. Let’s change both, otherwise it’s fine. :)

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz · 2018-05-15T22:56:51Z

@jancborchardt done

rullzer

LGTM

allow to specify a link to a legal notice Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de> fix tests Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de> fix undo for url-typed inputs and minor adjustments Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de> Use link not address in labels for URLs Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de> this file does not belong here Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

allow to specify a link to a legal notice
62f1156
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz added enhancement 3. to review Waiting for reviews backport-request feature: theming labels May 9, 2018

blizzz added this to the Nextcloud 14 milestone May 9, 2018

blizzz requested review from juliusknorr, nickvergessen, rullzer, schiessle and skjnldsv May 9, 2018 13:57

jancborchardt requested changes May 9, 2018
View reviewed changes

juliusknorr requested changes May 11, 2018
View reviewed changes

blizzz added 2 commits May 11, 2018 15:37

fix tests
850e225
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

fix undo for url-typed inputs
84ab102
and minor adjustments Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

juliusknorr approved these changes May 14, 2018
View reviewed changes

Use link not address in labels for URLs
feff9f5
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

rullzer approved these changes May 17, 2018
View reviewed changes

rullzer merged commit 9236c20 into masterMay 17, 2018

rullzer deleted the feature/noid/imprint branch May 17, 2018 19:16

blizzz mentioned this pull request May 18, 2018
[stable13] allow to specify a link to a legal notice #9516
Merged

MorrisJobke removed the backport-request label May 22, 2018

MariusBluem mentioned this pull request May 24, 2018
theming app should offer links to imprint AND privacy policy #9577
Closed

	<span><?phpp($l->t('Web address')) ?></span>
	<input id="theming-url" type="url" placeholder="<?phpp($l->t('Web address https://…')); ?>" value="<?phpp($_['url']) ?>" maxlength="500" />

Uh oh!

Uh oh!

Conversation

blizzz commented May 9, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blizzzMay 9, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MariusBluem commented May 9, 2018

Uh oh!

blizzz commented May 9, 2018

Uh oh!

MariusBluem commented May 10, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juliusknorr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusknorr commented May 11, 2018

Uh oh!

blizzz commented May 11, 2018

Uh oh!

codecovBot commented May 11, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blizzz commented May 11, 2018

Uh oh!

blizzz commented May 15, 2018

Uh oh!

jancborchardt commented May 15, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blizzz commented May 15, 2018

Uh oh!

rullzer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

blizzz commented May 9, 2018•
edited
Loading

blizzzMay 9, 2018•
edited
Loading

MariusBluem commented May 10, 2018•
edited
Loading

codecovBot commented May 11, 2018•
edited
Loading

jancborchardt commented May 15, 2018•
edited
Loading