USGS-WiM / SiGLDMS

Data management system for SiGL
Other
0 stars 5 forks source link

Validation and validation marking don't work consistently #210

Closed jlbruce closed 7 years ago

jlbruce commented 7 years ago

I was retesting the remove row issue, and I noticed that the validation isn't working as consistently, even on save.

When I go to an empty row and start with a bad longitude (80, instead of -80) and then move to another cell, the validation doesn't catch the error (but if I enter an incorrect HUC, it catches that right away). If I leave the bad long and hit save (with nothing else entered), it throws all the appropriate errors (a bunch of "required" errors and the "must be neg long" error), but it doesn't highlight the long cell in red. But if I fix all the other errors except for the long, then it gives me the "must be neg" error AND marks the cell red. So I think it won't actually let the bad value save, but it has trouble marking it.

Another scenario - I enter a bad long (no error), I pick Michigan for state, and then I enter a bad HUC (error and red). If I don't fix anything and hit save, the only error I get is "need req fields", and nothing new is highlighted (only the HUC cell is red).

troddick commented 7 years ago

For some reason that I cannot recall, I added a check on all the validators (except watershed and url I guess) that checks to see if there's anything else in the row first. If there is, then the validation is thrown, otherwise ignored. I can't recall why I added this without playing around with it after removing the check. I'll look at the other scenario too.

troddick commented 7 years ago

Regarding the 'Another scenario' .. bad long (there's nothing else at the row, no validation thrown), bad HUC (doesn't check for other values, throws validation.. it's added to the list of inValids). Don't do anything and hit save, it checks the list of inValids first .. if some in there, shows "need req fields" modal. Fix the red. Save. Then the inValids doesn't have anything, so it continues to the next step, which validates all Cells and throws up all the validations. So... I can: (A) move the validate all Cells first when you click save, but then you'd get a whole bunch of validations, or (B) you can keep it as is: fix what you see in red (removes inValids from list) and then when you save, it will see no inValids, run through all the cells that got missed and throw up all the necessary validation messages.

jlbruce commented 7 years ago

Hmm. Not quite sure I understand exactly what processes are going on and what my options are, but from a user perspective, here's what would be ideal...

  1. If I enter a bad value, I want to know about it when I enter it. (I don't want to wait until save, because I may have done it repetitively and could have saved effort had I known earlier.) That will also cut down on the amount (and more importantly, breadth) of errors that happen on save.
  2. When I save, if there are problems, I want to know about them all at once. I will likely be frustrated if I fix a bunch of errors and then more pop-up when I try again. Also, I will mostly rely on the visual markers, because if I get a lot of errors, I won't know which one pertains to which cell.

Does that help enough? If not, can we talk through the options?

troddick commented 7 years ago

I will remove the check that looks for other values in the same row, so that an error will appear regardless if it's the only entry in the row. This will give the red cell color and modal right away. I'm trying to remember why I added that check. Hopefully, it wasn't for a very important reason. :)

troddick commented 7 years ago

I remembered why I put that in there as soon as I removed it and tested. Without the check on if there's value in any other cell in that row, the required and lat/long validations are flagged for all the empty rows on the bottom. Looking into another solution, but there may not be one...

troddick commented 7 years ago

fixed it. I had to hack the library's code to ignore validating if the row was empty. Now validation message appears right away.

jlbruce commented 7 years ago

Sorry, but this still isn't fixed. (I cleared my cache and tested in both Mac Chrome and Firefox to make sure.) The longitude validation happens right away (yay!), but if you delete the value, the validation won't clear. Even if you resolve the validation with a good value and then delete the value, it errors again.

AND the right-click option doesn't work anymore, so you can't remove the row.

So essentially, once you've entered something into that cell, it can never be empty again. Even if you have no other values in the row and that cell is empty, the validation is triggered, it won't save - you can't get past the validation. The HUC cell validation doesn't have this issue.

troddick commented 7 years ago

Duplicating the issue. This is probably another reason why I had the check to see if the row was empty besides the validating cell. Also, the right-click is still there, but I've submitted an issue with them (the library's creator on their github page). When you scroll the page down (not the table, but the page the table sits within) and right-click, the context menu options are still up higher where it was before scrolling down on the page. If you scroll just the table (and not the containing page), the right-click always stays positioned correctly. The HUC cell doesn't do this because it is not a required field. Lat/Long both have 2 validation checks on it.

jlbruce commented 7 years ago

Hmm... is the right-click behavior a new thing? I could have sworn that I've been able to scroll down and use it before; for example, I think I was in the 20+ site number when I was testing that remove-row-didn't-clear thing.

troddick commented 7 years ago

I'm not sure when it started. I noticed it when I changed the 'remove row' to 'remove this row'. I thought it was the way I changed it, so I undid the change and still saw the behavior. I even checked what was on sigldev (vs running locally) and sure enough it's doing it there too.

As for the validation, the right-click remove does work (if you can find the menu options... I've been looking for a work-around for this, with no success so far) to stop it from failing validation. Or I put back in the check row for other fields. The lat/long are required and they must be a number between a certain range. So, once you enter something, it triggers the validation. If you remove something, it triggers the validation. The HUC is not required so having an empty value is ok. The lat long check looks at the value being within a range, if that passes (which an empty string will not pass). This is why I was also checking the rest of the row.

So.... long story but you can either have it the way it was (not showing validation on lat/long for new row until you saved, or you have to remove the row / revert to stop seeing the validation.

jlbruce commented 7 years ago

Well, I definitely think a lot of people won't know to right-click to remove a row (I'm still surprised at how many experienced scientists here still go to File > Save instead of hitting ctrl-S, so it's likely many people don't know the opportunities embedded in the right-click). So I guess we're back to the long not validating until save.

You mentioned above that it was the page scrolling (not table scrolling) that affected the positioning of the right-click Remove window, right? If we moved some of that new text into a little "Help" message to the right of the mode toggle that would open up a pop-up message, would that help shift the table up enough to prevent more people from having the issue? If this is possible and you think that would help, let me know and I'll rewrite all that help text into a single "Help" message.

troddick commented 7 years ago

I will revert back to lat/long validation on new row happening at save.

Yes, it is the page scroll that causes the weirdness with the right-click. Scrolling within the table area keeps the right-click in the correct opening position beside the mouse cursor. Moving the text in to a help message would bump the table up and reduce the necessity to scroll down on the page.

jlbruce commented 7 years ago

Ok. For SSE, leave the initial text line.. "Use single-site editing..." For MSE, delete all text above the table. (If the library creator ever fixes the bug, I'd like to add a line up there, but until that gets fixed we'll leave as much room as we can for the table.)

Then, next to the toggle, and visible from both SSE and MSE views, add the word "Help" in text (no button), which opens up a pop-up that says...

Single-site editing Use single-site editing to modify one site at a time. Click on the icons at the left to edit, duplicate, or delete an individual site.

Spreadsheet mode Select spreadsheet mode to edit multiple sites at a time (similar to Excel); each row is an individual site.

troddick commented 7 years ago

Fixed and updated on sigldev server

jlbruce commented 7 years ago

Can you add a little more space between the toggle and the "Help", and make the "Help" a little smaller (it's bigger type than then toggle type). Also, in the modal, can you make all the type the same size? The bullet type is bigger than the body.

Otherwise, looks great!

troddick commented 7 years ago

Done.