Page MenuHomePhabricator

Clarify when usernames in Participants list are saved or unsaved
Open, NormalPublic

Description

Steps to reproduce:

  1. Go to this event.
  2. In the participants box, add:
oeiuretye
iwruteyewior
Doc
Doc2
DoctorWho

Out of these the last three are valid but the first two are not.

  1. Hit Save participants
  2. The first two show up in red.
  3. Navigate back to the program view.
  4. Come back to the event

Expected: The 3 valid names were saved and the invalid 2 were removed.
Actual: All 5 named are removed.

It can also be reproduced in prod.

Event Timeline

Niharika triaged this task as High priority.Aug 10 2018, 7:44 PM
Niharika created this task.
Restricted Application added a project: Community-Tech. · View Herald TranscriptAug 10 2018, 7:44 PM
Niharika updated the task description. (Show Details)Aug 10 2018, 7:47 PM
MusikAnimal added a subscriber: MusikAnimal.EditedAug 10 2018, 7:54 PM

We discussed this a while back, and modified the error message to say "N usernames are invalid. Please edit or remove and then re-submit to save."

The issue is the transaction was rolled back due to the invalid usernames. There is not an easy fix, if you consider it broken. Many (most?) websites will function in the same way, where form errors mean nothing was saved.

Niharika lowered the priority of this task from High to Normal.Aug 10 2018, 7:58 PM
Niharika added subscribers: aezell, Samwilson, Mooeypoo, MaxSem.

We discussed this a while back, and modified the error message to say "N usernames are invalid. Please edit or remove and then re-submit to save."
The issue is the transaction was rolled back due to the invalid usernames. There is not an easy fix, if you consider it broken. Many websites will function in the same way, where form errors mean nothing was saved.

Hmm, I thought that was only for the broken usernames and not the valid ones. We show the valid ones as accepted (with the green check mark). That's definitely sending out a mixed signal.

I would like to get this fixed. Let's brainstorm the technical solutions here before we estimate this.

CC @Samwilson @Mooeypoo @aezell @MaxSem

MusikAnimal added a comment.EditedAug 10 2018, 8:12 PM

I don't remember why we added the green tick marks. They are pretty but I agree they can be misleading in this case.

It's not very efficient to save each form field one at a time. The larger issue is we'd have to build this out ourselves rather than using the built-in Symfony/Doctrine model interface, where it does all the work for us. Getting the errors from the model back to the form may involve considerable effort. It would also make the code quite hard to work with as we'd be going against convention.

I would instead try to improve the UI so that it is not misleading.

I can't think of a better example right now, but for instance on Phabricator if you edit a task, change the point value, but also remove the title -- you get an error message "Tasks must have a title". The change to the point value meanwhile was not saved. This is somewhat normal behaviour for form submission.

It's not very efficient to save each form field one at a time.

Never mind, under the hood it is doing an INSERT or UPDATE one a time, but we aren't coding it like that -- Doctrine is doing it for us. There is probably also a performance impact but the primary issue is having to implement our own system for saving forms. The idea is to attach Participants to the Event model and flush that to the database, rather than looping through and saving each Participant, one at a time. Getting the errors back to the form I think would be hardest part, since (basically) they exist on the Event, not individual Participants.

Hmm, so there's no easy way to go through the list, pluck out the invalid ones and ask Doctrine to save the rest?

I fear we'll have to make a quite radical change to the UI if we want to effectively communicate that all participants are in un-saved state until the error ones are removed or fixed.

MusikAnimal added a comment.EditedAug 10 2018, 8:54 PM

Hmm, so there's no easy way to go through the list, pluck out the invalid ones and ask Doctrine to save the rest?

No, not easy :( I'm sure it's doable but it will likely require some nasty hacks.

I fear we'll have to make a quite radical change to the UI if we want to effectively communicate that all participants are in un-saved state until the error ones are removed or fixed.

I think we could not show the green tick for usernames that weren't saved, going by whether or not the Participant object has an associated ID. The unsaved participants could be buried somewhere far down in the list, so not sure if this is really a solution (or maybe list the unsaved ones below the invalid ones?). In my opinion we could do without the green ticks, though I do think they are very pretty!

I do recall us talking about this with Sati and I think that's why we added in "Please edit or remove and then re-submit to save". See T182086#3898397. We can always modify the error message further, if that helps.

Taking out the green ticks would probably not help because the valid but unsaved names are still lost between the valid but saved names. The problem is that we want two different kinds of distinctions here:

  1. Valid vs Invalid
  2. Saved vs Unsaved

I have a meeting setup with @Prtksxna on Monday and I'll take him on a whirlwind tour of Grant metrics and see if he has any thoughts on this ticket. We don't have to rush on it.

Yeah, well let me reiterate an idea: List the unsaved participants below the invalid ones, and only those will lack the tick marks. Would that help? I think this still involves a little hackery but won't be as bad.

aezell added a comment.EditedAug 10 2018, 9:17 PM

@MusikAnimal I don't know a ton about Symfony/Doctrine. Could you short-circuit the form->model processing for Event and don't let it save participants. Instead, grab each value from the form field and run them through a new function on the Participant model like addToEvent().

You could then build a hash of participant->status and return that to the page so the UI could respond accordingly.

In other words, don't use the Event model to save participants but do them one-by-one via the Participant model.

EDIT:
I know this breaks the really nice form->model interaction that Symfony/Doctrine give us. But, it would seem to make for fairly logical code while allowing the kind of data integrity and UX we want.

I'm trying to think in my head how this could work. The biggest pain is getting the Symfony From component to cooperate. The error message you see now "N usernames are invalid" is being attacked to the Event as a violation. Symfony knows to show it as the form represents the Event. I'm not even sure how we'd make a form that doesn't have an associated entity. We'd probably have to ditch the FormBuilder entirely... which will be a huge pain (though it was just as much of a pain getting it to work in the first place!). A lot of Symfony devs refrain from the FormBuilder for this reason. Now I get it :) Maybe I went down the wrong path trying to go by convention. It does make for clean code, though.

I'm not against looping through the participants and saving one by one, but you should know it won't be easy. If we can alleviate confusion through the UI, either by putting unsaved participants below the invalid ones, or changing the error message, that might be better just to make best use of time. The other thing is forms across the web generally work in the same way, which is why I wonder if we're overthinking it. Wouldn't the organizers attempt to fix or remove the invalid usernames anyway, especially since we say to do this in the error message?

^ And that's just form-related challenges. Looping through and flushing each participant object one at a time probably won't be nearly as bad. I do recall forums saying to flush all your transactions at once, because... Doctrine. Or maybe we'd still be able to persist each entity and flush all at once, not sure. It certainly will involve more lines of code, since the form<->entity mapping is done automatically, and I'd have to handle each Participant iteratively rather than acting on the Event as a whole and letting it handle relationships and such. I'd need time to investigate this part before I can say how hard it would be.

I think that's a valid point. If we want it to work as a single form, maybe the UI needs to be different or at least have some different messaging.

To follow up on my idea, I think there's a middle ground.

Could we check validity in a loop in the callback and just drop the invalid names out of the array we try to persist? Then, we could return the invalid names back to the UI while letting the rest of the form persist correctly?

That way, we aren't flushing single participants but we are avoiding the whole Form erroring because we are cleaning the input ahead of the persistence bit.

That way, we aren't flushing single participants but we are avoiding the whole Form erroring because we are cleaning the input ahead of the persistence bit.

Yeah, that way we'd at least have the FormBulder tied to an Event (as opposed to manually building a form). I think we'd still have to manually add the invalid Participant objects back to the Form object, and flag them as invalid. I'm not sure how to do that but I'm sure it's possible.

No matter which route we go I foresee headaches. But as a developer I do have plenty of ibuprofen lying around!

I'm still curious what y'all think of this:

List the unsaved participants below the invalid ones, and only those will lack the tick marks.

Between that and the error message, would it not be clear enough to the user how it works?


We should discuss at T194703: Add support for categories on the frontend, but note that with the current setup the category form will be separate than the participant form. I suspect some people will try to edit both sections and assume they save together. So we probably have some reworking to do to get around that. While I'm in there I can look into this task as well.

List the unsaved participants below the invalid ones, and only those will lack the tick marks.
Between that and the error message, would it not be clear enough to the user how it works?

I could see how this might work, especially if adding participants to an event that already has some.

You'd have the invalid folks at the top marked with red. In the middle, the unsaved, valid, new folks marked with yellow. Below those two sets, all the preexisting valid folks marked with green.

With a slight change to the alert message, this might work.

aezell added a comment.EditedAug 10 2018, 10:48 PM

Or, none of that. Regardless of what's new or preexisting, we leave everything unmarked except the invalid ones.

Change the alert to something like, "Your participants were not saved. Please fix the invalid usernames marked with red and submit the form again."

I think @MusikAnimal suggested this above but I wanted to restate to make sure I understood it.

I tried to save one good and one bad username, and the UI a) didn't let me save, while b) clearly telling me to remove the bad name. Am I in the wrong place in Grant Metrics? Because this seems pretty clear to me (and believe me, I'm not usually the one to say that).

(By the way, this is an excellent example of how including a little more info in tickets—in this case a screenshot would have been nice—helps more of us to be able to collaborate on an issue.)

Or, none of that. Regardless of what's new or preexisting, we leave everything unmarked except the invalid ones.

This would be inline with traditional forms on the web. The green tick marks are more confusing than helpful, to me.

@jmatazzoni That's the one. I think the rub here is two-fold.

  • The green check mark could lead the user to thinking that user was saved. If they don't care about the invalid one, they navigate away and lose all changes.
  • I think @Niharika would like it to go ahead and save the ones it can while allowing the user to remove or fix the ones it can't.

To me, the pattern here is a very familiar one and the messaging is clear. The pattern is:

  1. I tried to save but,
  2. Some of my data was bad so,
  3. The system told me what I need to fix to save.

The messaging clearly says "please edit or remove and then re-submit to save." So it seems unlikely that people will assume the green-checked names are saved.

Given the difficulty (as I understand it) of making the change to save the data, it's worth asking: do we have reason to think that users are stumbling on this? Have we had comments or do we see it in the logs? If not, maybe we can make a few minor UI tweaks and then wait to see if we get any negative feedback about this process?

I do see a couple of small possible UI tweak; the first is the most serious, I think:

  • When i go in to fix the bad username, I get confirmation that the edited name is valid when it shows up as a suggestion in a dropdown menu from the entry box. But when I select the valid name, the red triangle doesn't go away or change to a green check (see screenshot below). That could make me unsure about whether I can now resubmit. When I substitute a valid name, I should get the positive confirmation—or at least no longer see the negative feedback.
  • A very minor point: the bad data in the list is clearly labeled, but if the list is long and the bad names are at the bottom, the user might not understand what's happening at first—especially given the all the green checks she'll be seeing. So a small refinement to the messaging might help. Something like: One username is invalid. Please check below for the highlighted name, then edit or remove and re-submit..."

I like the green checks as giving positive confirmation of valid names. But if we have reason to think the checks are an issue for some, then removing them would be fine.

I think @MusikAnimal and @aezell's suggestions are good ones to go with:

  • Move unsaved rows to the top (below the invalid ones)
  • The invalid rows have the red mark. ❗️
  • The valid but unsaved rows have the yellow mark ⚠️ which on hover tells them that the participant is not saved.
  • The valid and saved ones have the usual green mark.

And we tweak the wording to x participant(s) is/are invalid. Please edit or remove the invalid ones and re-submit the form to save all new participants. Check for incorrect spelling, capitalization and extra formatting.

@MusikAnimal Does that sound like a viable solution?

I tried to save one good and one bad username, and the UI a) didn't let me save, while b) clearly telling me to remove the bad name. Am I in the wrong place in Grant Metrics? Because this seems pretty clear to me (and believe me, I'm not usually the one to say that).

It doesn't clearly specify that it did not save the good username either.

(By the way, this is an excellent example of how including a little more info in tickets—in this case a screenshot would have been nice—helps more of us to be able to collaborate on an issue.)

Thanks for pointing it out. I'll keep that in mind.

I do see a couple of small possible UI tweak; the first is the most serious, I think:

  • When i go in to fix the bad username, I get confirmation that the edited name is valid when it shows up as a suggestion in a dropdown menu from the entry box. But when I select the valid name, the red triangle doesn't go away or change to a green check (see screenshot below). That could make me unsure about whether I can now resubmit. When I substitute a valid name, I should get the positive confirmation—or at least no longer see the negative feedback.

If the red turns to green, it would give users the impression that the field was auto-saved while it wasn't. The red icon in that state is a reminder that they need to click 'Save'. Once they do, it turns green.

  • A very minor point: the bad data in the list is clearly labeled, but if the list is long and the bad names are at the bottom, the user might not understand what's happening at first—especially given the all the green checks she'll be seeing. So a small refinement to the messaging might help. Something like: One username is invalid. Please check below for the highlighted name, then edit or remove and re-submit..."

The red ones are always at the top of the list.

Prtksxna updated the task description. (Show Details)Aug 13 2018, 2:41 AM

I am a bit confused about the UI right now. Are we showing saved and unsaved changes together?

One participant already addedAdding more participantsErrorComment
No difference between saved and unsaved. Being able to tell them apart with icons (❗️⚠️) is useful, but they shouldn't be shown together in the first place.

I don't fully understand the tech and user limitations well enough yet, but is it possible to show the unsaved and the saved changes separately?

One participant already addedAdding more participantsErrorComment
Changes that can be saved are saved and put below (not sure if this is technically possible), those with errors are kept in the edit interface

or

One participant already addedAdding more participantsErrorComment
This one doesn't tell us which usersnames are the invalid ones. Quite useless.

I have a meeting setup with @Prtksxna on Monday and I'll take him on a whirlwind tour of Grant metrics and see if he has any thoughts on this ticket. We don't have to rush on it.

Thanks @Niharika! That would be really helpful.

I was wondering about this, and thought that perhaps already-saved usernames could be shown just in a list (there's lots of space at the right side of the form). Each would still have a cross for removal. The idea being that it's not very common to want to change a username: that would be removing one and adding a different one. This way, anything in a text entry box would not be saved, and perhaps the UI would be simpler?

I realize we may be moving away from the partial save idea here.

@MusikAnimal I didn't know this, but I happen to know one of the authors of Doctrine. I asked him about this and showed him the Event controller. He said, "After the handleRequest but before the if ($form->isSubmitted() && $form->isValid()) {, just run your validation and call your persist() and flush() there. Then, you can process the rest of the form the way it is effectively having already saved the usernames you want."

This is the code he referenced: https://github.com/wikimedia/grantmetrics/blob/master/src/AppBundle/Controller/EventController.php#L174

MusikAnimal added a comment.EditedAug 13 2018, 8:17 PM

Wow, a Doctrine author was reviewing my code! :D

The participant form is actually handled with the handleParticipantForm() method, but same deal. Relatedly, maybe handleFormSubmission should be given a better name, as it's only used for the event create/edit forms.

I still don't quite grasp how we could get it to work. We're not running validations directly, rather it does it when you attempt to save the Event because of the @Assert\Callback annotation. I can't persist and flush if $form->isValid() is false or else I get an "integrity constraint violation" error because it's trying to put in NULL as the user ID, which is NOT NULL column. I think the issue is that the Form represents an Event. Symfony (and/or Doctrine) is running the validations on the whole model and it's relationships (Participants, in this case), and will attach errors to the Form object, one for each invalid field and one for the Event as a whole. Outside of ditching Symfony Forms entirely, I don't know how to validate each Participant individually and still get the errors assigned the way Symfony wants it. Yay, Symfony Forms... =p

I had a feeling it was more nuanced than he suggested. It seems like this convo is actually fine with it invalidating the whole form. It does make "web sense" to me. So, I think we can leave the form-handling and validation the way it is because it's nice and clean. We can focus on getting the UX streamlined and it will likely be fine.

jmatazzoni added a comment.EditedDec 13 2018, 1:10 AM

So I see the problem here now, having experienced this a little more. We don't want to go to any great lengths to clarify this, but I have a couple of suggestions that I think are pretty simple:

Do these definitely

  • Clarify meaning of green check: One problem here is that the meaning of the green check is ambiguous. It appears to mean "saved" but it also means "valid (but not saved)." So my suggestion: during the saving process, for names that are valid but not saved, do not show any icon. Thus the system becomes a simple, binary yes or no: green icon means saved, red icon means problem. While it sounds like that might leave the status of valid but unsaved names unclear, I think in action it will make perfect sense—see the image below-right. We could have a third icon, but I think it actually just clutters things up and is one more thing to learn. Also, using only the red icon puts the user's focus where it needs to be.
  • Warn users who try to leave with unsaved changes: A second problem currently is that when a user with unsaved changes leaves the page, there is no warning. In such situations it's standard to throw up a dialog that says something like "Leave page? You have unsaved changes that will be lost. [Leave] [Cancel]"

Also do one of these

  • Separate the Saved and Unsaved changes: Right now, all valid changes, Saved and Unsaved, are mixed together in alphabetical order (see screenshot below-left). Invalid changes, however, go to the top of the column. If we could put all unsaved but valid names at the top as well, under a label stating that they are "Unsaved", that would clarify quite a bit. "Saved" names would be in a second alphabetized list after the Unsaved—see sketch below-right. In this way, all the items needing further action are at the top. I prefer this to the suggestion below, but if this proves to complex then we can do the third symbol. @Prtksxna, if this makes sense to you, please do a proper design for the Saved/Unsaved labels.
  • Add a third icon for Valid-but-unsaved": I like putting all the items that need the user's attention at top. But if that can't be done, then we should probably go ahead and label the valid-but-unsaved items. My suggestion would be to avoid a symbol that is bound to be unclear and just use the word "Unsaved" or "Valid" for these. As I say, though, I greatly prefer the above. If we can't do the idea above, then Prateek will design something.
CURRENT DESIGN with saved and unsaved mixed together PROPOSED CONCEPT with labeled, separate lists (and no green check for unsaved but valid)
jmatazzoni renamed this task from [BUG] Grant metrics deletes valid usernames when invalid usernames are submitted too to Clarify when usernames in Participants list are saved or unsaved, and add a dialog box to warn users.Dec 13 2018, 5:52 PM

After discussing this with @jmatazzoni, here is an option that keeps the saves what we can immediately, and keeps the unsaved changes in the textarea and shows the error indicator there.

InitialAdding new itemsAfter hitting save

I understand that partial saving might be difficult to implement at the moment. Would it be better to wait and implement this, or do we need to figure out a temporary solution?

I like the design provided in Prateek's post. As he says, this depends on being able to do partial save, which would be great but which I'm told may be a challenge. @MusikAnimal and @Mooeypoo, we have at least a couple of workable proposals on the table (I also like my suggestion here). How can we get to a decision here about which one provides the best value for the least effort?

I think to support Prateek's design, we'd need to avoid using the handy model saving functions and write some direct DB insert/update code in the model to handle each participant individually. Is that what this looks like @MusikAnimal?

MusikAnimal added a comment.EditedDec 20 2018, 6:06 PM

I like Prateek's design too! I need to revisit this, but I feel like removing the misleading green ticks is the easy short-term solution. The error message already says "please edit or remove [the invalid usernames] and then re-submit to save", which is how most websites work (invalid means nothing was saved). It's a bit crazy to write to the db for each one individually, it should be saved on the Event level in one go, letting Doctrine handle the relationships and cascading. Maybe we can give the view the full Participant objects, which should make it easier to separate out the invalid ones. (nevermind, I think it will still rollback the transactions that saved the valid participants). More investigation needed...

jmatazzoni renamed this task from Clarify when usernames in Participants list are saved or unsaved, and add a dialog box to warn users to Clarify when usernames in Participants list are saved or unsaved.Jan 2 2019, 7:38 PM

In our team meeting, we decided to hold action on the UI issues relating to red and green checks (the issue of signaling clearly which usernames are valid/invalid and saved/unsaved). This is because Symphony can easily see which names are valid/invalid but can't tell which ones are saved/unsaved. That means we can't distinguish saved+valid from unsaved+valid—something that would be helpful in clarifying the experience.

Meanwhile, we decided to split off the most pressing issue—which is that we don't warn users when they try to leave the page with unsaved changes—and address that in a separate ticket, which I just wrote: T212804.