make ChangePasswordController and UI Code respect Message and Hint fr… #1787

blizzz · 2016-10-18T16:39:10Z

…om HintException

Needed for #1715

Please review @GitHubUser4234 @nickvergessen @ChristophWurst

mention-bot · 2016-10-18T16:39:13Z

@blizzz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @MorrisJobke, @rullzer and @michag86 to be potential reviewers.

nickvergessen · 2016-10-19T07:17:34Z

settings/js/users/users.js

+									if(!_.isEmpty(result.data.hint)) {
+										message += "<br/>" + _.escape(result.data.hint);
+									}
+									OC.Notification.showTemporary(t('admin', message), {isHTML: true});


Just mentioning, when the strings are not used in any other t method, they will not be translated.

Maybe it's better to use t in line 667 and 669?

@michag86 this does not help. To make sure that a string is translated, it must be provided as plain text to the method. That way it can be parsed and fed into the translation tool.

Basically I did not change the behavior much. If translation worked before, it is pure luck. This should happen on the server side code where the string is passed (if applicable). Best thing to do here is actually remove the transaltion of received strings.

Removed the translation call.

codecov-io · 2016-10-19T11:12:59Z

Current coverage is 57.07% (diff: 60.00%)

Merging #1787 into master will increase coverage by 0.01%

@@             master      #1787   diff @@
==========================================
  Files          1191       1191          
  Lines         71912      71909     -3   
  Methods        7299       7300     +1   
  Messages          0          0          
  Branches       1213       1213          
==========================================
+ Hits          41036      41045     +9   
+ Misses        30876      30864    -12   
  Partials          0          0

Diff Coverage	File Path
•••••• 60%	settings/Controller/ChangePasswordController.php

Powered by Codecov. Last update 789e6e7...376d34b

MorrisJobke · 2016-10-20T10:47:30Z

Changing the password on the personal page causes this: (it shows the message and the hint(localized))

MorrisJobke · 2016-10-20T10:48:40Z

Same for the admin user management (when changing the password of an existing user):

nickvergessen · 2016-10-20T12:00:39Z

settings/js/personal.js

@@ -231,6 +231,9 @@ $(document).ready(function () {
 					$('#pass2').val('').change();
 				} else {
 					if (typeof(data.data) !== "undefined") {
+						if(!_.isEmpty(data.data.hint)) {
+							data.data.message += "\n" + data.data.hint;


In normal cases hint is the translated version of message. That is at least the case for all HintExceptions I have seen so far.

blizzz · 2016-10-20T12:18:14Z

@MorrisJobke thx.

Wow. After acking through the HintException usages it seems that half of the time the hintparameter is being used as a translated message. Imho that does not make much sense, since this is supposed to be user facing anyway. Also, my perception is that hint should be a suggestion for an action, instead of the reason of the error.

However since it is vastly used in this manner i am hesitating to change this all, and this would include other repos… If this is expected to land into logExpection translated stuff does not make sense again either. Perhaps the best way is to extend (or add another Exception inheriting from it for backwards compatibility) for another parameter, translated message or so 🙈 🙉 🙊

This amazingly extend the PR, resulting in more work than I anticipated. Perhaps I postpone it for a little bit later and we need to go with a workaround solution in #1715 first.

MorrisJobke · 2016-10-20T12:53:06Z

@blizzz The hint is the reason that could be shown to the user and is therefore translated. The hint never lands in the logs. The message of an hint exception is untranslated and will never get to the user because it could contain sensitive data. So the usage of the HintException holding the translated string is correct.

cc @LukasReschke

blizzz · 2016-10-21T08:30:44Z

@MorrisJobke then we ought to rename the parameter and add some doc. This is confusing and misleading.

GitHubUser4234 · 2016-10-24T06:24:37Z

Yeah, at least adding some comments to the constructor / getHint() method would give a "hint" on how to use them =)

Alright, for the #1715, I'll adjust the code accordingly, thanks @blizzz though 👍

rullzer · 2016-11-03T19:33:47Z

So what are we going to do here? @blizzz @MorrisJobke ?

MorrisJobke · 2016-11-18T20:32:56Z

So what are we going to do here? @blizzz @MorrisJobke ?

At least resolve the conflicts :(

MorrisJobke · 2016-11-25T22:58:14Z

Needed for #1715

This was already merged in #2286 - how to proceed here?

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

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

* stings should be translated (if applicable) where they happen * ChangePasswordController returns all its fixed strings already translated * local user backend does not throw HintException at all Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

MorrisJobke · 2016-11-25T23:04:03Z

I fixed the conflicts and rebased on master ;)

blizzz · 2016-11-28T10:23:57Z

I fixed the conflicts and rebased on master ;)

That's kind, but apart from PHP Doc adjustments there is nothing else to do (cf. #1787 (comment)) I come up with a new PR instead.

blizzz added bug 3. to review Waiting for reviews feature: users and groups labels Oct 18, 2016

blizzz added this to the Nextcloud 11.0 milestone Oct 18, 2016

blizzz force-pushed the fix-hintexception-changepasswordcontroller branch from 4bd006f to 948d3d2 Compare October 18, 2016 21:59

nickvergessen reviewed Oct 19, 2016

View reviewed changes

nickvergessen reviewed Oct 20, 2016

View reviewed changes

GitHubUser4234 mentioned this pull request Oct 24, 2016

Enable changing passwords in user_ldap #1715

Closed

MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Nov 18, 2016

MorrisJobke modified the milestones: Nextcloud 12.0, Nextcloud 11.0 Nov 25, 2016

blizzz added 3 commits November 26, 2016 00:02

make ChangePasswordController and UI Code respect Message and Hint fr…

641fa45

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

adjust test

5d4e064

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

MorrisJobke force-pushed the fix-hintexception-changepasswordcontroller branch from b97fdcb to 376d34b Compare November 25, 2016 23:03

blizzz closed this Nov 28, 2016

blizzz deleted the fix-hintexception-changepasswordcontroller branch November 28, 2016 10:24

blizzz mentioned this pull request Nov 28, 2016

add missing phpdoc to HintException #2366

Merged

MorrisJobke removed this from the Nextcloud 12.0 milestone Nov 28, 2016

snyk-bot mentioned this pull request Apr 17, 2021

[Snyk] Upgrade @nextcloud/vue from 3.7.2 to 3.8.0 chankoba/server#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make ChangePasswordController and UI Code respect Message and Hint fr… #1787

make ChangePasswordController and UI Code respect Message and Hint fr… #1787

blizzz commented Oct 18, 2016

mention-bot commented Oct 18, 2016

nickvergessen Oct 19, 2016

michag86 Oct 19, 2016 •

edited

Loading

blizzz Oct 19, 2016

blizzz Oct 19, 2016

codecov-io commented Oct 19, 2016 •

edited

Loading

MorrisJobke commented Oct 20, 2016 •

edited by GitHubUser4234

Loading

MorrisJobke commented Oct 20, 2016

nickvergessen Oct 20, 2016

blizzz commented Oct 20, 2016

MorrisJobke commented Oct 20, 2016

blizzz commented Oct 21, 2016

GitHubUser4234 commented Oct 24, 2016

rullzer commented Nov 3, 2016

MorrisJobke commented Nov 18, 2016

MorrisJobke commented Nov 25, 2016

MorrisJobke commented Nov 25, 2016

blizzz commented Nov 28, 2016

make ChangePasswordController and UI Code respect Message and Hint fr… #1787

make ChangePasswordController and UI Code respect Message and Hint fr… #1787

Conversation

blizzz commented Oct 18, 2016

mention-bot commented Oct 18, 2016

nickvergessen Oct 19, 2016

Choose a reason for hiding this comment

michag86 Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

blizzz Oct 19, 2016

Choose a reason for hiding this comment

blizzz Oct 19, 2016

Choose a reason for hiding this comment

codecov-io commented Oct 19, 2016 • edited Loading

Current coverage is 57.07% (diff: 60.00%)

MorrisJobke commented Oct 20, 2016 • edited by GitHubUser4234 Loading

MorrisJobke commented Oct 20, 2016

nickvergessen Oct 20, 2016

Choose a reason for hiding this comment

blizzz commented Oct 20, 2016

MorrisJobke commented Oct 20, 2016

blizzz commented Oct 21, 2016

GitHubUser4234 commented Oct 24, 2016

rullzer commented Nov 3, 2016

MorrisJobke commented Nov 18, 2016

MorrisJobke commented Nov 25, 2016

MorrisJobke commented Nov 25, 2016

blizzz commented Nov 28, 2016

michag86 Oct 19, 2016 •

edited

Loading

codecov-io commented Oct 19, 2016 •

edited

Loading

MorrisJobke commented Oct 20, 2016 •

edited by GitHubUser4234

Loading