Issue 22 - Add a way to store user names

[kzar]

So this isn't ready to be merged yet but hopefully it's close. A few notes/questions:

• It seems that for Chrome at least the \0 is lost when saving / loading to storage.
• Do you think the interface around revisions looks OK? For example what about how I've made that automatic for legacy passwords?
• I'm not too happy about how the password's revision property exists to start with when the password is created, but after that the code constantly has to split it out of the password's name. However since the password's key is it's ID I didn't want to mess with it either.
• I've passed in the revision when actually saving the passwords, I'm not sure if that's the right thing to do or not.
• So far I didn't strip the revision part of the user name for messages such as "Are you sure you want to delete this?".

[kzar]

Maybe the interfaces would be simpler if revision wasnt used as part of the key, but stored with the other password parameters like the length? Revision isn't really needed for legacy passwords anyway, and I think conflating the need to generate a different password for the same user name with needing to store multiple entries with the same user name kind of confusing. (I can't personally think of any cases where I need two entries for the same domain that both have the same user name anyway. If I did need that one day I wouldn't mind making a note of the second password in the Secure Notes section instead of having two entries.)

[kzar]

The more I think about it the more that would make sense. In the future I hoped we'd add a convenient way for the user to bump the revision number for a password up/down. If we use the revision number as part of the password key that would get extremely confusing.

Take one example. Let's suppose you have two passwords for a username, one legacy(2) and one generated(1) and you try to bump the revision for the generated one. The number would have to jump up to 3 to skip the legacy password's revision. Then all the interfaces start showing the passwords with the revision numbers (2) and (3) by the names, and the user has to figure out the 2 means nothing since the password is legacy, but the 3 is very important since that password is generated!

[palant]

I can't personally think of any cases where I need two entries for the same domain that both have the same user name anyway.

Of course not - this is a temporary situation, necessary when you are changing your password. Most websites will require both your old password and the new password for that.

Concerning legacy passwords I agree, these shouldn't have revisions. As to bumping revisions, we can add a simpler UI for that but only after the main functionality stands.

I think you underestimated the complexity of introducing revisions quite significantly. I definitely wouldn't try to squeeze it all into one commit. First step is merely renaming "password name" into "user name", this will already generate lots of changes. Then we could modify the backend to deal with revisions (assisted by unit tests). And only then I would modify the UI.

[kzar]

Fair enough, I will update this review to just rename the fields when I get a chance. (About to reinstall my laptop, remind me not to apt upgrade when I've got stuff to do...)

What do you think to storing the password revision along with other parameters like password length, instead of appending it onto the end of the password's name for the key? Since we agree that having two passwords for a website with the same username is kind of pointless why not just use the user name as the key passwords? (We can add the nice UI to increment / decrement the revision for an existing generated password in the future, then there is really no need to have two separate records, not even during the process of changing the password on the website.)

[palant]

Any such process would be rather complicated and error prone. It can be implemented as an addition but should not be a requirement. So the key should really be the combination of name and revision.

[kzar]

Well really it's the same password entry, just the next revision. That's how the user will think of it, I mean things like username and password length etc all need to stay the same, just this number needs to change.

Wouldn't it actually be simpler to add +/- buttons for existing passwords by their revision number? Simpler than adding the "I need another password with the same name" button, the revision field at creation time, the revision numbers in parentheses after the user names, the warnings like "you can have two usernames which are the same but first change that number over there" etc.

[palant]

Adding a + button is possible once the remaining functionality stands, but it won't solve the issue that you need two revisions of the password at the same time.

[kzar]

Well if the buttons were +/- like I suggest the user could easily jump back to the old revision if it was needed again.

[palant]

but it won't solve the issue that you need two revisions of the password at the same time.

[kzar]

Well I don't think it's hard to click copy for the current revision and then click + and then click copy again. (Or to even realise you need the old one, click - and then click copy.) But I guess we just disagree about this one, I'll shut up about it now, sorry!

[kzar]

(Note I just pushed my updated commit out of habit, I didn't actually want it to show up in the review yet because I didn't test it yet!)

[palant]

I merged the unrelated state fix already, thank you for catching this. Need to test the rename fix before merging (only noticed now, GitHub doesn't notify you when a pull request is updated).

[kzar]

@palant Ah right I didn't realise that, I guess I'll comment with an @mention to you when I've posted a new patch set next time. (I've tested the user name change now, it works fine.)

[palant]

This has been merged, please create new pull requests for additional work related to #22.