Prevent submit before validation complete

[labkey-bpatel]

Rationale

https://github.com/LabKey/internal-issues/issues/828

Related Pull Requests

• Prevent submit before validation complete onprcEHRModules#1683
• https://github.com/LabKey/johnsHopkinsEHRModules/pull/628

Changes

• Disable submit buttons when validation is in progress.
• Add a 'validation' indicator to show when validation in is progress (esp. useful when adding animals in Bulk).

[bbimber]

@brentlogan, i'm not sure what's driving this, but I dont think this is necessarily the behavior you want. In a perfect world, there would be minimal lag time between validation and submission. The challenge is that is someone is interacting with a form, there are lots of events that trigger many, many queries to the server to validate.

The reason we didnt introduce this kind of behavior originally is b/c the consequence of missing a validation should generally be negligible. If the user entered something and hit submit before the validation returned, then in theory the submit request should undergo the same checks and fail if there was a problem. There are definitely warnings and some fringe cases where this isnt quite true, but most of the time it should be.

[labkey-martyp]

@brentlogan, i'm not sure what's driving this, but I dont think this is necessarily the behavior you want. In a perfect world, there would be minimal lag time between validation and submission. The challenge is that is someone is interacting with a form, there are lots of events that trigger many, many queries to the server to validate.

The reason we didnt introduce this kind of behavior originally is b/c the consequence of missing a validation should generally be negligible. If the user entered something and hit submit before the validation returned, then in theory the submit request should undergo the same checks and fail if there was a problem. There are definitely warnings and some fringe cases where this isnt quite true, but most of the time it should be.

@bbimber Agreed that in most cases the submit-time checks are a sufficient backstop, the problem is specifically with warnings, which are advisory and won't block a submit, so a missed warning silently produces subtler data issues rather than a clean failure. ONPRC has some form helpers for bulk entry where validation lag has been long enough that records are getting submitted before users ever see the warnings. Speeding up validation is worth pursuing too, but the validation paths continue to evolve, so it's hard to rely on perf alone. The plan is to keep the re-validate button enabled and show a "validating…" info message, so the user knows what's happening and can restart validation if a response is lost.

[bbimber]

@labkey-martyp, sure, I can see that. I did see the 'validating...' indicator which seems like a generally good idea.

my understanding of how this works is based purely off reading the code, so I might be partially wrong on what this does. However, if there are specific form helpers that trigger outsized problems (and the bulk entry ones probably are), could the code that does a bulk-add event simply add an explicit 'Loading..." popup that prevents user interaction until that first validation event completes? That would be in contrast to disabling a submit button in all cases, and better scope the change to the problematic situation. The title of this PR 'prevent submit until validation is complete' implies the impact is much more broad.

[labkey-martyp]

@bbimber this is intentionally broad because I think it's a good improvement for the EHR. There's continual development here with new form features and additional validation. Adding a loading modal on a per-feature basis seems like it could get missed and I doubt ONPRC will be the only to run into this. I assumed this was not done originally in these forms just because validation or bulk entry was not as extensive as they have become. Is the specific concern just that the UI will get stuck with missing validation responses?

[bbimber]

Yes, I get that features are added. I think my concerns could be addressed with sufficient testing, but here is why I brought this up:

• 100% agree on the scenario about bulk adding a lot of data all at once being a problem for current behavior. That said, the 'Bulk Load' button is not especially different from loading a previously saved form though, when that saved form has a lot of existing records. The latter case shows a 'loading..' modal until the initial one-time loading completes though. The act of bulk creating a bunch of records seems very similar to me.
• My primary user-facing concern is that this adds a potential annoyance to solve a problem that only seems to exist for the scenario above. That annoyance would be a disconnect that could happen if there are many validation events, and if the server becomes laggy in returning them. If memory serves, the forms were designed to assume that validation events could be delayed and would even ignore them if the client-side data changed more rapidly than they returned. It's possible the listeners being added are quite sound, but getting a scheme to work reliably on a slim, fast dev machine is a different scenario that a production server and a large DB. I see there is a 're-validate' button, but if this is needed more than very rarely, it seems like an unnecessary hurdle to introduce to the users to address one targeted problem.

Anyway, the concept of this is not crazy, but just depends on whether or not it creates real-world obstacles.

[labkey-martyp]

Looks good. I did manual testing as well. Just one comment below.

[labkey-martyp]

@labkey-bpatel Please hold off merging this in until we've done client testing.

[labkey-martyp]

Yes, I get that features are added. I think my concerns could be addressed with sufficient testing, but here is why I brought this up:

• 100% agree on the scenario about bulk adding a lot of data all at once being a problem for current behavior. That said, the 'Bulk Load' button is not especially different from loading a previously saved form though, when that saved form has a lot of existing records. The latter case shows a 'loading..' modal until the initial one-time loading completes though. The act of bulk creating a bunch of records seems very similar to me.
• My primary user-facing concern is that this adds a potential annoyance to solve a problem that only seems to exist for the scenario above. That annoyance would be a disconnect that could happen if there are many validation events, and if the server becomes laggy in returning them. If memory serves, the forms were designed to assume that validation events could be delayed and would even ignore them if the client-side data changed more rapidly than they returned. It's possible the listeners being added are quite sound, but getting a scheme to work reliably on a slim, fast dev machine is a different scenario that a production server and a large DB. I see there is a 're-validate' button, but if this is needed more than very rarely, it seems like an unnecessary hurdle to introduce to the users to address one targeted problem.

Anyway, the concept of this is not crazy, but just depends on whether or not it creates real-world obstacles.

Thanks for the feedback @bbimber. We've also added an experimental flag to immediately opt-out if there are issues on any server. If that is the case we'll loop back and look for any improvements or reduced scope.