Add first and last name to journalist table and web app

[sssoleileraaa]

Description of Changes

Resolves #4251

• Add first and last name columns to the Journalists table
• Add ability to specify first and last name when creating a new account as an admin
• Add ability to change first and last name of a user as an admin
• Add ability to change first and last name as a user

Test plan for reviewers

• Successfully add a new account as an admin from the journalist interface
• Successfully change the name of an account that is not yours as an admin from the admin page
• Successfully change your own name as a non-admin from the accounts page
• Successfully change your own name to empty strings a non-admin from the accounts page

Checklist

If you made changes to the server application code:

• Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

If you made changes to securedrop-admin:

• Linting and tests (make -C admin test) pass in the admin development container

[heartsucker]

Also I should have noted in the above review that this does pass the test plan described.

[sssoleileraaa]

In case @redshiftzero finishes this PR while I work on finishing another outstanding PR, here's what's left:

• Test (as you can see :))
• Deciding on how to handle null first and last names, see notes below and refer to @ninavizz for UX feedback, this may or may not require an update to the code.

---

We currently (in production) have a username field that cannot be None. And we use a placeholder to show the current username in gray when editing the form. When you edit the field, the placeholder text does not disappear if you try to delete it. This is fine for usernames since we don't allow null values, but it is confusing for first name and last name where we allow null values.

The problem

If we want to implement first and last name like username, it's confusing, as you see here:

Solution A

Clicking "Update" with only the placeholder text erases the name. See the gif above to imagine how that might look. The two downsides to this solution are (1) it's inconsistent with username because saving a username field with None does not erase the username (username cannot be None anyway), and (2) it may look confusing to users trying to erase the first and last name fields when the placeholder text is still in the background.

Solution B

This is the solution currently implemented in this PR.

The two downsides to this solution are (1) it's inconsistent with username, and (2) i can't figure out a CSS (non-javascript) way of making it so that the field remains black text after editing it and clicking outside of the textbox. To address (1), we could implement username this way too.

[ninavizz]

I'm not sure exactly what the question is, here—but I would prefer no placeholder text exist for first name or last name fields (or even the username field), and to make the text black inside all 3.

Allie, I know we discussed the color-change weirdness between the click-into and static states of that placeholder text, after you approached me about how to resolve that issue last week... though zooming-out from the broader interaction, no placeholder text at all makes more sense to me, in all 3 fields.

Placeholder texts can be really nice to have, but too often cause more confusion than they add value. They really need to be done well, or not done at all—and I know they're done inconsistently throughout the Admin/Journalist UIs. For the name things, I'd rather just be safe and not have any placeholders—and then have the text be black, when clicked-into and typed with.

Also, a lot more padding to the left of the label last name, so there's a clear margin between it and the first name field.

Feeling like an inconsistent jerk, recommending the above. Let me know if it makes things harder or easier, or if you'd like to chat this through.

[sssoleileraaa]

My description of the problem isn't super clear. I tried!

The problem is that we've been trying to figure out a way to add the first and last name fields using the current design, or something similar, so that we can keep the scope of this change small.

If we stick with the current design of showing saved database values in text boxes, then I think it's desirable to allow the user to clear and see an empty text box when they want to erase their name.

If we remove placeholders all together, then there should be a different place to show what's currently saved in the db. This is a larger user-facing change that needs to be discussed further - next UX meeting perhaps!

I'm curious what @redshiftzero thinks. Any other ideas on how to get this change out and keep it small? Do you think we should discuss a redesign of how we show current database values and placeholders before moving forward with this change?

[ninavizz]

Oyy... facepalm. Thank you for the additional explanation, Allie! Your initial one was plenty clear to anyone familiar with the code. I should be more familiar with the code. Also: ouch wrt the problem at hand. Also wanting to avoid scope-creep. Will fiddle around with Erik's SD instance today, to see what makes sense for now. Also curious what others think; and, wanting to keep any "redesign" of things out of this 'lil ticket. <3

[codecov-io]

Codecov Report

Merging #4425 into develop will decrease coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           develop   #4425     +/-   ##
=========================================
- Coverage    83.79%   83.6%   -0.2%     
=========================================
  Files           44      45      +1     
  Lines         2956    3055     +99     
  Branches       321     330      +9     
=========================================
+ Hits          2477    2554     +77     
- Misses         402     418     +16     
- Partials        77      83      +6

Impacted Files	Coverage Δ
securedrop/securedrop/models.py	91.29% <0%> (-1.35%)	⬇️
securedrop/securedrop/manage.py	77.1% <0%> (-1.31%)	⬇️
securedrop/securedrop/qa_loader.py	86.16% <0%> (ø)	⬆️
securedrop/securedrop/journalist_app/forms.py	100% <0%> (ø)	⬆️
.../b58139cfdc8c_add_checksum_columns_revoke_table.py	36.36% <0%> (ø)	⬆️
...bic/versions/a9fe328b053a_migrations_for_0_14_0.py	57.14% <0%> (ø)
securedrop/securedrop/journalist_app/utils.py	88.7% <0%> (+0.44%)	⬆️
securedrop/securedrop/journalist_app/admin.py	87.95% <0%> (+0.61%)	⬆️
securedrop/securedrop/journalist_app/account.py	93.87% <0%> (+0.69%)	⬆️

---

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 82eed07...720e7cf. Read the comment docs.

[redshiftzero]

I think what makes sense is that the "placeholder" text here just provides the current value of that field if it's defined. If there is no value defined, then there is no placeholder text. If we were to get rid of this placeholder text then we need to display elsewhere what the current values of these fields are. In terms of the discrepancy between the username and first/last name, I agree that we should be consistent so I would update the username field to behave as the first/last name fields do.

[ninavizz]

Is it possible to make text that is a "Current value in data table" vs a "placeholder," black? Placeholder text, being standardized text that all users see when the data table value for a field is null.

That would be my only real ask with this—but I don't know if that's possible. Placeholder = gray, existing value previously saved to account = black, is the standard pattern for this that I'd like to see followed as much as can be possible w/o scope-creep.

[heartsucker]

I think what makes sense is that the "placeholder" text here just provides the current value of that field if it's defined. If there is no value defined, then there is no placeholder text.

I think this makes the most sense. @ninavizz, does this work? I can't tell if you're in agreement or not.

Dismissing review because I don't want to block and I'm working on other things.

[ninavizz]

So: I don't want to hold-up merging this PR! I agree with the essense of what Jen's proposing @heartsucker, but it is sooooo confusing when saved data table values are reflected back to the user in gray text. I see that a lot, and it's a bad, highly confusing anti-pattern.

I'm only just now learning of all the nuanced non-standard patterns employed by the Admin and Journalist experiences, and while I want to ensure they get "corrected," I don't want to encourage wasted cycles on experiences that may not exist in 6-12mos. And tbh, I don't know what it would take to make all saved data table values black, and ONLY placeholder text grey. Which is how it's supposed to be.

[sssoleileraaa]

I think part of the reason this conversation has been so confusing is because of the term "placeholder." Just to make sure everyone understands, a placeholder describes the expected value of an input field. By default, this appears gray and when you click into the empty box is remains visible.

In production, we use it in two different ways:

(1) to describe the expected value, e.g. showing "Username" in the username textbox of the new user form

(2) to display what is currently stored in the database, e.g. showing the actual username stored for the current user for the existing user form

There are a couple different ways to show default values in the text boxes. You can use the value attribute or you can use the placeholder attribute as we currently do. They have different behaviors. The value attribute is simple. The text appears black, and you can delete it completely. The placeholder attribute I described above: it appears gray and remains visible when the textbox is empty (think of it like a ghost value).

I originally wanted to make it so that when you clicked a textbox the placeholder would disappear, but since we don't use javascript, I came up with Solution B using CSS. This solution also makes it so that the text boxes are empty when there is no database value saved -- this was never an issue, but people have brought it up in comments I noticed.

Hopefully with all the feedback here, @redshiftzero can come to a reasonable solution, whether that be change the username field to act like Solution B or just use value attributes and ditch placeholders. Or something fancier!? 🐩

[ninavizz]

@creviera I've appreciated all the depth and clarity you've provided in this PR, around patterns that are both non-standard and inconsistent across the experience; an experience largely built by volunteer contributors, or a likely overworked dev (or two) wearing too many hats. I mean, the current experience is a vast improvement upon command-line only stuff, right?

Your attention to detail in making sure it all gets done right, and asking questions where quick assumptions could otherwise be made, I feel are so important in the product's current growth phase. Your communication has been very clear, to me—I'm just the only kid on the thread w/o any familiarity with how all this is coded. Which was missing context, early in the convo, that you promptly and clearly shed light upon. Thank you for not giving this PR the shove out the door, go go bye treatment. I've appreciated it. 😸

[redshiftzero]

OK this should be ready for re-review: previous review comments are addressed, the migration and migration tests are moved into a new migration for 0.14.0, and tests are added

To @ninavizz' point any placeholder text (e.g. "First name") is gray, and any db-populated values (e.g. "Alice") are black, thanks @creviera and @ninavizz both for the explanations 💖

[redshiftzero]

I'm signing off on Allie's changes here, just want my commits to get a review and then this should be good to merge

[rmol]

I ran through the test plan, and everything worked. Input coloring was as @redshiftzero last described. I don't see any problems with the code.

Two comments/questions:

• Should we add labels to the new user form inputs, for consistency and to eliminate some of the problems with the placeholder values? (Did I miss this being considered above?)
• Just gonna lob this little firecracker in: first and last names are a pretty shaky choice any more. It's not too late to go with a single name column. It'd be easier to change it now than in a migration after first and last have been deployed. (So glad we're distributed and I'm out of reach. 🙂)

[sssoleileraaa]

@rmol, ha, that firecracker has gone off a few times already. I'm glad you're bringing it up in written form, and I agree with you.

I think we should come to a consensus here, so I'll try to capture the arguments on both sides.

Arguments for two names:

• we want to be able to greet people by a preferred nickname rather than a full name; two fields make it users can provide a short version of how they want to be addressed
• journalists would have an easier time recognizing people's badges by first and last name initials.

Arguments for one name:

• see rmol's link: https://shinesolutions.com/2018/01/08/falsehoods-programmers-believe-about-names-with-examples/
• it simplifies rules like these: Add Identity text fields in Admin interface #4251 (comment)
• you can have two names in a single name field

[ninavizz]

@rmol I totally agree with you in spirit; however, the only reason for the two names, is to facilitate the first-initial/last-initial ID badge in Workstation Client. That was what got this ticket going in the first place. That's literally it. Likewise, it is nice to be able to address users by their first name only, in messaging (like "Welcome, Kushal!"). For this reason, most ux and content folks hate the single text field for a users name... despite developers preferring it.

For internationalization, where surnames are used first, we can also just swap the ordering in how the badges are done (if research would validate that—I've never heard of nor done this). Especially for internationalization, though, when folks have multiple names (most countries except for the US); "How you're called socially" and "Your surname" remain the two only identifiers anyone cares about, in the B2B or Enterprise context.

From the user's POV: imagine looking at a conversation thread in a shared client; one source, many colleagues. Would you rather see a single-letter on the ID badge, or two?

In enterprise and B2B contexts, two name fields is very standard; namely, because the stiffies in suits in corporate America would freak the f* out if people were tracked by a single db value. Also, legal reasons (like, employer lawsuits are easier when users are more clearly ID'd). This really is a business app, and needs to follow many of those paradigms for user comfort and org compliance—on a whole aside from how we speak to users in UI text.

You're missed in the weekly UX meetings, btw! 😄

[redshiftzero]

@rmol thanks for the review! I added a small HTML change to address your comment re: the new user form 🚀

Thanks for the background/explanation @ninavizz - let's leave as first name / last name :-)

[redshiftzero]

Approving once CI passes based on my review of @creviera's commits and @rmol's review of my changes

[rmol]

For this reason, most ux and content folks hate the single text field for a users name... despite developers preferring it. [...] "How you're called socially" and "Your surname" remain the two only identifiers anyone cares about

I think many sites that have a single field for the full name also add a "what should we call you" field. That's how I've done it where possible/permitted. It respects people two ways: getting their name exactly right, and asking them how they want to be addressed.

[ninavizz]

Yeah, I really love the "What should we call you?" field. A lot. For our purposes with newsroom teams, it's as much about us addressing the authenticated user in system messaging, as it is about surfacing each user's activity to be easily recognized by colleagues as being theirs.

I have seen the "What should we call you?" field go wrong in workplace situations like this, and it can run the gamut of simply confusing, to really funny. I'd love to implement it eventually, but see doing it right as still being a ways off.

Especially one gone-wrong experience, with a colleague of mine who put "Playful Guy" into that field... which then showed-up in Team Activity streams for others in the company to see. He was really into SF's queer swingers scene, so its inference was not work friendly—but fortunately it was a VERY small consultancy with a bunch of us that had all previously worked together, elsewhere, and we just thought it was hilarious... (he was soooo embarrassed, which admittedly we also all got a big kick out of!) but it has remained with me, since, as the best-ever example of "What should we call you?" going bad. Then again, maybe most professionals wouldn't think to put their S&M monikers, in such a field for work stuff.