front-end-password-change capability

[diddledan]

• refactor clientManager.js to allow configuration parsing as a serparate function.
• refactor clientManager.js to add configuration writing function.
• add new plugin for /setpass command.
• add server.js changes to allow for new "profile" screen
• add password change ui to "profile" screen
• modify css, front-end shout.js and index.html to add "profile" ui
• refactor client.js to use new clientManager functionality for saving the configuration files

fixes #359

[floogulinc]

If we are making a GUI for the password changing, why also make a command. I think when you use a / command, it is expected that it's a command being sent to the connected server.

[JocelynDelalande]

If we are making a GUI for the password changing, why also make a command.

Because some prefer commands, others GUI, best of both worlds :-)

I think when you use a / command, it is expected that it's a command being sent to the connected server.

The /clear command, already implemented is not sent to IRC server. For myself, I'm not against having / commands that are not mapped to IRC protocol command. After all, the use of "/" commands is a bare convention, not the IRC protocol.

@diddledan thanks for your patch, I will get a review at it later (quite big patch !)

I have two remarks already:

• I would tend to prefer separating iso-functional refactorings into dedicated commits, for better readability (same PR is fine), but others may disagree, we never talked about it @astorije ? @erming ?
• Generally in password change interfaces, you have to provide your old password as well, and it's a good thing, cause if someone "steals your seat", it cannot change your password, at least.

[astorije]

@diddledan, thanks a lot for your contribution! Could you rebase this and make sure that the CI passes?

@JocelynDelalande, it looks like you have already taken a look at this, so I'm going to add you as first reviewer, and then I'll happily take over for second review when you are happy.
Hopefully @diddledan can get back to your remarks and we can have this in Shout :-)

[astorije]

I would tend to prefer separating iso-functional refactorings into dedicated commits, for better readability (same PR is fine), but others may disagree, we never talked about it @astorije ? @erming ?

I generally agree, but what refactoring are you talking about here?
It's true that this is a big PR, and splitting changes in commits will help with reviews as well as future code reads.

Generally in password change interfaces, you have to provide your old password as well, and it's a good thing, cause if someone "steals your seat", it cannot change your password, at least.

Yes, let's have that on Shout too!

[diddledan]

ok, I think that I've addressed the main issue of having the old password be required before changing the password to prevent hot-seat attacks. Travis also now passes - there are a few places in the refactoring that I mentioned where the try{}catch{} blocks are in the wrong function now that the contents have been moved to individual reusable functions. (specifically in client.updateUser and client.loadUser IIRC)

[JocelynDelalande]

@diddledan Thanks for taking care.

I tested it, and well... it works, that's for sure :-)

We're getting better and better !

(I haven't done code review yet)

Some change still have to be done IMHO, from user point-of-view:

• the form is a bit messy-looking, if you need some inspiration, the "connect to network" page is a nicely organized form
• same applies for the message on successful change or error, it's not nicely fitting the UI nicely, what would you think about using bootstrap alerts ?
• clear form after submit (wether it's error or success)

and on a meta (but important) side:

• The git history is still too chatty: we don't want style commit like e8b92c3 or 7d51a33 in the git history. Just squash those modifications with the commit where the original mistake was made.
• conform the commit naming guidelines, I'm speaking for example about "per comments on pull request #506, I've added the requirement for the current password to be specified" which first line would better be something like "Require old password on password change form"

By the way, I prefer rebasing over merging, but I don't want to make it too difficult, so let's say merging is ok for this one (I'm speaking about 34aa52b).

[JocelynDelalande]

@diddledan sorry for huge delay on my side, good to see things that changes, password change UI is way better as you changed it :-)

For git history, several commits (with some containing several of your first-try commits) would have been better and clearer, but one commit is better than the handful that existed in your first try, so I'm ok with thig git history :-)

Two issues remain, that I left unchecked in my previous message, and another appeared:

• no feedback at all on UI, wether it's a success or failure

[JocelynDelalande]

@diddledan bump ?

[diddledan]

sorry about the delay. The alert dialogs now work correctly. Still need to clear the form after submittal though.

[JocelynDelalande]

@diddledan cool, getting close :)

[JocelynDelalande]

@diddledan gentle bump ? :)

[diddledan]

thanks for the reminder. I thought I'd committed and pushed, but it turns out I did neither :-p The latest commit from a moment ago should fix the remaining issue (clearing the fields after submittal).

[JocelynDelalande]

@diddledan perfect :)

👍

Waiting for another review here to merge.

@diddledan Final touch : would-you please mind :

• squashing commits into one
• submiting the same PR to lounge (which is community fork of shout)

Thanks by advance

side-note : I will no longer be active on shout master, in favor of lounge once PRs I started taking of are done

[diddledan]

done.

lounge pr is thelounge/lounge#57

[JocelynDelalande]

ok, as others commented on thelounge/lounge#57, I will continue discussion there.

(sadly, I have been too quick giving my 👍)