alxndrsn commented 7 years ago

Observed behaviour

When validation has failed for a page, the Next > button still looks clickable:

Expected behaviour

Some visual clue should be given that we cannot proceed to the next page, e.g. button could be grey

newtewt commented 5 years ago

This is also an issue if something is wrong with z-score calculation. This form is missing the z-score calculate and stops the user from progressing with no error in console.

<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:ev="http://www.w3.org/2001/xml-events" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:orx="http://openrosa.org/xforms" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
  <h:head>
    <h:title>Treatment Enrollment</h:title>
    <model>
      <instance>
        <treatment_enrollment delimiter="#" id="treatment_enrollment" prefix="J1!treatment_enrollment!" version="2019-02-06 14-09">
          <inputs>
            <meta>
              <location>
                <lat/>
                <long/>
                <error/>
                <message/>
              </location>
            </meta>
            <source>user</source>
            <source_id/>
            <contact>
              <_id/>
              <name/>
              <patient_id/>
              <date_of_birth/>
            </contact>
          </inputs>
          <child_name/>
          <patient_id/>
          <dob/>
          <age_in_days/>
          <referral>
            <visit/>
            <admitted/>
          </referral>
          <zscore>
            <gender/>
            <admission_type/>
            <weight/>
            <height/>
            <muac/>
            <zscore_wfh/>
            <nn/>
          </zscore>
          <enrollment>
            <enroll/>
            <facility/>
            <program/>
            <n_1/>
            <n_2/>
            <n_3/>
            <target_weight/>
            <target_muac/>
            <reasons/>
            <false_positive_confirmation/>
          </enrollment>
          <additional_notes/>
          <summary>
            <n_1/>
            <n_2/>
            <n_3/>
            <n_4/>
            <n_5/>
            <n_6/>
            <n_7/>
            <n_8/>
            <n_9/>
            <n_10/>
            <n_11/>
            <n_12/>
            <n_13/>
            <n_14/>
            <n_15/>
            <n_16/>
            <n_17/>
            <n_18/>
            <n_19/>
            <n_20/>
            <n_21/>
          </summary>
          <meta tag="hidden">
            <instanceID/>
          </meta>
        </treatment_enrollment>
      </instance>
      <instance id="contact-summary"/>
      <bind nodeset="/treatment_enrollment/inputs" relevant="./source = 'user'"/>
      <bind nodeset="/treatment_enrollment/inputs/source" type="string"/>
      <bind nodeset="/treatment_enrollment/inputs/source_id" type="string"/>
      <bind nodeset="/treatment_enrollment/inputs/contact/_id" type="db:person"/>
      <bind nodeset="/treatment_enrollment/inputs/contact/name" type="string"/>
      <bind nodeset="/treatment_enrollment/inputs/contact/patient_id" type="string"/>
      <bind nodeset="/treatment_enrollment/inputs/contact/date_of_birth" type="string"/>
      <bind calculate="../inputs/contact/name" nodeset="/treatment_enrollment/child_name" type="string"/>
      <bind calculate="../inputs/contact/patient_id" nodeset="/treatment_enrollment/patient_id" type="string"/>
      <bind calculate="substr(../inputs/contact/date_of_birth, 0, 10)" nodeset="/treatment_enrollment/dob" type="string"/>
      <bind calculate="int(decimal-date-time(today()) - decimal-date-time(date( /treatment_enrollment/dob )))" nodeset="/treatment_enrollment/age_in_days" type="string"/>
      <bind nodeset="/treatment_enrollment/referral/visit" required="true()" type="select1"/>
      <bind nodeset="/treatment_enrollment/referral/admitted" relevant=" /treatment_enrollment/referral/visit  = 'no'" required="true()" type="select1"/>
      <bind nodeset="/treatment_enrollment/zscore" relevant=" /treatment_enrollment/referral/visit  = 'yes'"/>
      <bind nodeset="/treatment_enrollment/zscore/gender" type="select1"/>
      <bind nodeset="/treatment_enrollment/zscore/admission_type" required="true()" type="select1"/>
      <bind constraint=". &gt;= 0.8 and . &lt;= 68.5" jr:constraintMsg="Weight should be between 0.8 kg and 68.5 kg" nodeset="/treatment_enrollment/zscore/weight" required="true()" type="decimal"/>
      <bind constraint=". &gt;= 45 and . &lt;= 120" jr:constraintMsg="Height should be between 45 cm and 120 cm" nodeset="/treatment_enrollment/zscore/height" required="true()" type="decimal"/>
      <bind constraint=". &gt;= 5 and . &lt;= 30" jr:constraintMsg="MUAC should be between 5 and 30 cm" nodeset="/treatment_enrollment/zscore/muac" required="true()" type="decimal"/>
      <bind nodeset="/treatment_enrollment/zscore/zscore_wfh" required="true()" type="decimal"/>
      <bind nodeset="/treatment_enrollment/zscore/nn" readonly="true()" type="string"/>
      <bind nodeset="/treatment_enrollment/enrollment" relevant=" /treatment_enrollment/referral/visit  = 'yes'"/>
      <bind nodeset="/treatment_enrollment/enrollment/enroll" required="true()" type="select1"/>
      <bind nodeset="/treatment_enrollment/enrollment/facility" relevant=" /treatment_enrollment/enrollment/enroll  = 'yes'" required="true()" type="select1"/>
      <bind nodeset="/treatment_enrollment/enrollment/program" relevant=" /treatment_enrollment/enrollment/enroll  = 'yes' and  /treatment_enrollment/enrollment/facility  = 'clinic'" required="true()" type="select1"/>
      <bind nodeset="/treatment_enrollment/enrollment/n_1" readonly="true()" relevant=" /treatment_enrollment/enrollment/program  = 'otp'" type="string"/>
      <bind nodeset="/treatment_enrollment/enrollment/n_2" readonly="true()" relevant=" /treatment_enrollment/enrollment/program  = 'sfp'" type="string"/>
      <bind nodeset="/treatment_enrollment/enrollment/n_3" readonly="true()" relevant=" /treatment_enrollment/enrollment/program  = 'sc'" type="string"/>
      <bind constraint=". &gt;= 0.8 and . &lt;= 68.5" jr:constraintMsg="Weight should be between 0.8 kg and 68.5 kg" nodeset="/treatment_enrollment/enrollment/target_weight" relevant=" /treatment_enrollment/enrollment/enroll  = 'yes' and  /treatment_enrollment/enrollment/facility  = 'clinic'" type="decimal"/>
      <bind constraint=". &gt;= 5 and . &lt;= 30" jr:constraintMsg="MUAC should be between 5 and 30 cm" nodeset="/treatment_enrollment/enrollment/target_muac" relevant=" /treatment_enrollment/enrollment/enroll  = 'yes' and  /treatment_enrollment/enrollment/facility  = 'clinic'" type="decimal"/>
      <bind nodeset="/treatment_enrollment/enrollment/reasons" relevant=" /treatment_enrollment/enrollment/enroll  = 'no'" required="true()" type="select1"/>
      <bind nodeset="/treatment_enrollment/enrollment/false_positive_confirmation" relevant=" /treatment_enrollment/enrollment/reasons  = 'false_positive'" required="true()" type="select1"/>
      <bind nodeset="/treatment_enrollment/additional_notes" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_1" readonly="true()" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_2" readonly="true()" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_3" readonly="true()" relevant=" /treatment_enrollment/referral/visit  = 'no'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_4" readonly="true()" relevant=" /treatment_enrollment/referral/visit  = 'yes'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_5" readonly="true()" relevant=" /treatment_enrollment/referral/visit  = 'yes'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_6" readonly="true()" relevant=" /treatment_enrollment/referral/visit  = 'yes'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_7" readonly="true()" relevant=" /treatment_enrollment/referral/visit  = 'yes'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_8" readonly="true()" relevant=" /treatment_enrollment/referral/visit  = 'yes'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_9" readonly="true()" relevant=" /treatment_enrollment/referral/visit  = 'yes'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_10" readonly="true()" relevant=" /treatment_enrollment/referral/visit  = 'yes'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_11" readonly="true()" relevant=" /treatment_enrollment/referral/visit  = 'yes'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_12" readonly="true()" relevant=" /treatment_enrollment/referral/visit  = 'yes'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_13" readonly="true()" relevant=" /treatment_enrollment/referral/visit  = 'yes'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_14" readonly="true()" relevant=" /treatment_enrollment/enrollment/enroll  = 'yes'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_15" readonly="true()" relevant=" /treatment_enrollment/enrollment/enroll  = 'yes' and  /treatment_enrollment/enrollment/facility  = 'clinic'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_16" readonly="true()" relevant=" /treatment_enrollment/enrollment/enroll  = 'yes' and  /treatment_enrollment/enrollment/facility  = 'clinic'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_17" readonly="true()" relevant=" /treatment_enrollment/enrollment/enroll  = 'yes' and  /treatment_enrollment/enrollment/facility  = 'clinic'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_18" readonly="true()" relevant=" /treatment_enrollment/enrollment/enroll  = 'no'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_19" readonly="true()" relevant=" /treatment_enrollment/enrollment/reasons  = 'false_positive'" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_20" readonly="true()" type="string"/>
      <bind nodeset="/treatment_enrollment/summary/n_21" readonly="true()" type="string"/>
      <bind calculate="concat('uuid:', uuid())" nodeset="/treatment_enrollment/meta/instanceID" readonly="true()" type="string"/>
    </model>
  </h:head>
  <h:body class="pages">
    <group appearance="field-list" ref="/treatment_enrollment/inputs">
      <label>Patient</label>
      <input appearance="hidden" ref="/treatment_enrollment/inputs/source">
        <label>Source</label>
      </input>
      <input appearance="hidden" ref="/treatment_enrollment/inputs/source_id">
        <label>Source ID</label>
      </input>
      <group ref="/treatment_enrollment/inputs/contact">
        <label>Contact</label>
        <input appearance="db-object" ref="/treatment_enrollment/inputs/contact/_id">
          <label>What is the child's name?</label>
          <hint>Select a person from list</hint>
        </input>
        <input appearance="hidden" ref="/treatment_enrollment/inputs/contact/name">
          <label>Child Name</label>
        </input>
        <input appearance="hidden" ref="/treatment_enrollment/inputs/contact/patient_id">
          <label>Child ID</label>
        </input>
        <input appearance="hidden" ref="/treatment_enrollment/inputs/contact/date_of_birth">
          <label>Date of Birth</label>
        </input>
      </group>
    </group>
    <group ref="/treatment_enrollment/referral">
      <label></label>
      <select1 ref="/treatment_enrollment/referral/visit">
        <label>Did the person attend the referral visit</label>
        <item>
          <label>Yes</label>
          <value>yes</value>
        </item>
        <item>
          <label>No</label>
          <value>no</value>
        </item>
      </select1>
      <select1 ref="/treatment_enrollment/referral/admitted">
        <label>Is <output value=" /treatment_enrollment/child_name "/> admitted in any treatment program</label>
        <item>
          <label>Yes</label>
          <value>yes</value>
        </item>
        <item>
          <label>No</label>
          <value>no</value>
        </item>
      </select1>
    </group>
    <group appearance="zscore field-list" ref="/treatment_enrollment/zscore">
      <label></label>
      <select1 appearance="zscore-sex" ref="/treatment_enrollment/zscore/gender">
        <label>Child gender</label>
        <item>
          <label>Male</label>
          <value>male</value>
        </item>
        <item>
          <label>Female</label>
          <value>female</value>
        </item>
      </select1>
      <select1 ref="/treatment_enrollment/zscore/admission_type">
        <label>Type of admission</label>
        <item>
          <label>New case</label>
          <value>new_case</value>
        </item>
        <item>
          <label>Relapse/Readmission</label>
          <value>relapse</value>
        </item>
        <item>
          <label>Transfers from other OTP</label>
          <value>otp</value>
        </item>
        <item>
          <label>Transfers from SFP</label>
          <value>sfp</value>
        </item>
        <item>
          <label>Transfer from SC</label>
          <value>sc</value>
        </item>
        <item>
          <label>Returned defaulter</label>
          <value>defaulter</value>
        </item>
      </select1>
      <input appearance="zscore-weight" ref="/treatment_enrollment/zscore/weight">
        <label>Weight (kg)</label>
      </input>
      <input appearance="zscore-height" ref="/treatment_enrollment/zscore/height">
        <label>Height/Length (cm)</label>
      </input>
      <input ref="/treatment_enrollment/zscore/muac">
        <label>MUAC (cm)</label>
      </input>
      <input appearance="zscore-weight-for-height hidden" ref="/treatment_enrollment/zscore/zscore_wfh">
        <label>WFH</label>
      </input>
      <input ref="/treatment_enrollment/zscore/nn">
        <label>WFH: <output value=" /treatment_enrollment/zscore/zscore_wfh "/></label>
      </input>
    </group>
    <group appearance="field-list" ref="/treatment_enrollment/enrollment">
      <label></label>
      <select1 ref="/treatment_enrollment/enrollment/enroll">
        <label>Would you like to enroll the child into treatment?</label>
        <item>
          <label>Yes</label>
          <value>yes</value>
        </item>
        <item>
          <label>No</label>
          <value>no</value>
        </item>
      </select1>
      <select1 ref="/treatment_enrollment/enrollment/facility">
        <label>Facility of admission</label>
        <item>
          <label>At your clinic</label>
          <value>clinic</value>
        </item>
        <item>
          <label>Referral to another clinic</label>
          <value>another_clinic</value>
        </item>
      </select1>
      <select1 ref="/treatment_enrollment/enrollment/program">
        <label>Admission treatment program</label>
        <item>
          <label>OTP</label>
          <value>OTP</value>
        </item>
        <item>
          <label>SFP</label>
          <value>SFP</value>
        </item>
        <item>
          <label>SC</label>
          <value>SC</value>
        </item>
      </select1>
      <input ref="/treatment_enrollment/enrollment/n_1">
        <label>OTP admission criteria should be '+ or ++Oedema' or 'MUAC &lt; 11.5cm' or 'Weight for Height Z score &lt;-3'</label>
      </input>
      <input ref="/treatment_enrollment/enrollment/n_2">
        <label>SFP admission criteria should be 'MUAC between 11.5 to 12.4cm' or 'Weight for Height Z score ≥ -3 to &lt; -2'</label>
      </input>
      <input ref="/treatment_enrollment/enrollment/n_3">
        <label>SC admission criteria should be '+++ Oedema' or 'MUAC &lt; 11.5cm with complications' or 'Weight for Height Z score &lt;-3 with complications'</label>
      </input>
      <input ref="/treatment_enrollment/enrollment/target_weight">
        <label>Target Weight (kg)</label>
      </input>
      <input ref="/treatment_enrollment/enrollment/target_muac">
        <label>Target MUAC (cm)</label>
      </input>
      <select1 ref="/treatment_enrollment/enrollment/reasons">
        <label>Reasons for non enrollment</label>
        <item>
          <label>False positive</label>
          <value>false_positive</value>
        </item>
        <item>
          <label>Chronic malnutrition</label>
          <value>chronic_malnutrition</value>
        </item>
      </select1>
      <select1 ref="/treatment_enrollment/enrollment/false_positive_confirmation">
        <label>Are you sure that the child is a false postitive?</label>
        <item>
          <label>Yes</label>
          <value>yes</value>
        </item>
        <item>
          <label>No</label>
          <value>no</value>
        </item>
      </select1>
    </group>
    <input ref="/treatment_enrollment/additional_notes">
      <label>Additional notes</label>
    </input>
    <group appearance="field-list summary" ref="/treatment_enrollment/summary">
      <label>Click on submit to finalize this report</label>
      <input ref="/treatment_enrollment/summary/n_1">
        <label>&lt;h4&gt;<output value=" /treatment_enrollment/child_name "/>&lt;/h4&gt;</label>
      </input>
      <input ref="/treatment_enrollment/summary/n_2">
        <label>Did the child attend the referral visit: <output value=" /treatment_enrollment/referral/visit "/></label>
      </input>
      <input ref="/treatment_enrollment/summary/n_3">
        <label>Admitted into any treatment program: <output value=" /treatment_enrollment/referral/admitted "/></label>
      </input>
      <input appearance="h2 blue" ref="/treatment_enrollment/summary/n_4">
        <label>Visit Details</label>
      </input>
      <input ref="/treatment_enrollment/summary/n_5">
        <label>Gender: <output value=" /treatment_enrollment/zscore/gender "/></label>
      </input>
      <input ref="/treatment_enrollment/summary/n_6">
        <label>Date of birth: <output value=" /treatment_enrollment/dob "/></label>
      </input>
      <input ref="/treatment_enrollment/summary/n_7">
        <label>Admission Type: <output value=" /treatment_enrollment/zscore/admission_type "/></label>
      </input>
      <input ref="/treatment_enrollment/summary/n_8">
        <label>Weight: <output value=" /treatment_enrollment/zscore/weight "/> kg</label>
      </input>
      <input ref="/treatment_enrollment/summary/n_9">
        <label>Height: <output value=" /treatment_enrollment/zscore/height "/> cm</label>
      </input>
      <input ref="/treatment_enrollment/summary/n_10">
        <label>MUAC: <output value=" /treatment_enrollment/zscore/muac "/> cm</label>
      </input>
      <input ref="/treatment_enrollment/summary/n_11">
        <label>Weight for height z-score: <output value=" /treatment_enrollment/zscore/zscore_wfh "/></label>
      </input>
      <input ref="/treatment_enrollment/summary/n_12">
        <label>Would you like to enroll child into treatment: <output value=" /treatment_enrollment/enrollment/enroll "/></label>
      </input>
      <input appearance="h2 blue" ref="/treatment_enrollment/summary/n_13">
        <label>Treatment enrollment details</label>
      </input>
      <input ref="/treatment_enrollment/summary/n_14">
        <label>Facility of admission: <output value=" /treatment_enrollment/enrollment/facility "/></label>
      </input>
      <input ref="/treatment_enrollment/summary/n_15">
        <label>Program: <output value=" /treatment_enrollment/enrollment/program "/></label>
      </input>
      <input ref="/treatment_enrollment/summary/n_16">
        <label>Target weight: <output value=" /treatment_enrollment/enrollment/target_weight "/> kg</label>
      </input>
      <input ref="/treatment_enrollment/summary/n_17">
        <label>Target MUAC: <output value=" /treatment_enrollment/enrollment/target_muac "/> cm</label>
      </input>
      <input ref="/treatment_enrollment/summary/n_18">
        <label>Reason for non-enrollment: <output value=" /treatment_enrollment/enrollment/reasons "/></label>
      </input>
      <input ref="/treatment_enrollment/summary/n_19">
        <label>False positive confirmed: <output value=" /treatment_enrollment/enrollment/false_positive_confirmation "/></label>
      </input>
      <input ref="/treatment_enrollment/summary/n_20">
        <label>Additional notes:</label>
      </input>
      <input ref="/treatment_enrollment/summary/n_21">
        <label><output value=" /treatment_enrollment/additional_notes "/></label>
      </input>
    </group>
  </h:body>
</h:html>

eljhkrr commented 5 years ago

This form relies on calculation of z-scores via appearance attribute:

<input appearance="zscore-weight-for-height hidden" ref="/treatment_enrollment/zscore/zscore_wfh">
 <label>WFH</label>
</input>

This method of calculation has a short delay before the value is computed during which the form cannot proceed to the next page. It would be great if the 'NEXT' button was disabled if a user cannot proceed to the next page because of a missing required value. In this case, users should endeavor to compute z scores with explicit calculation which would be more responsive.

peter-ochieng commented 5 years ago

@garethbowen I'd really like your advice on this ticket.We worked on the first issue, disabling the Next button when there are validation errors.Whats confusing is what should happen to the form when the z-score is not working or having a delay as described by Nick and Elijah respectively. What would be the best solution for all the scenarios?

garethbowen commented 5 years ago

@newtewt I think the form you have is out of date - at some point we switched form using a zscore widget to using a zscore function as in the example from @eljhkrr .

I don't think there's much we can do about this error - you're referencing a widget the app knows nothing about.

newtewt commented 5 years ago

In the above form I had to add the z-score function for it to work properly. It would then calculate the zscore values and then I could continue on. However, the form as is would show no z-score calculations and the button would be enabled but wouldn't allow you to continue forward. If we are saying it's because it's out of date I'm ok with that. To me it seems like poor behavior to show a enabled button and be stuck.

garethbowen commented 5 years ago

I think what you've created is a hidden required field with no calculation to update it. I think the fix for the original issue may mean the Next button is disabled but there will still be no way to proceed with the form you have. It'd be good to investigate the behaviour and see if it's more sensible once the original bug is fixed.

peter-ochieng commented 5 years ago

Enketo isn't reevaluating the form Add Family form at this juncture genericForm.nextPage(8) as its supposed to do, to enable the Next button. More investigation required.

SCdF commented 5 years ago

So I took a quick look at the e2e test:

I fixed it here: https://travis-ci.org/medic/medic/builds/583147291 (hopefully, once it passes)
Apart from rebasing the only change I made was to the e2e test: https://github.com/medic/medic/commit/1b6c0487791057b7fe111dd7503ea0a3a59c1f7b

Having said that, I don't think we should actually merge this change. Unless I misunderstand the breadth of the problem this ticket aims to solve, it is a QOL improvement to disable the next button until the user is actually able to progress.

While this improvement is nice, it is somewhat undercut by us apparently only having input blur (or similar) as a hook to determine whether the next button is enabled, meaning you replace a next button that is sometimes enabled when it probably shouldn't be with a next button that is disabled when it probably shouldn't be (until you tab out or click away).

Thoughts @garethbowen ?

SCdF commented 5 years ago

Actually @vimago interested in your opinion here as well. Is there a smarter way of doing this?

garethbowen commented 5 years ago

I think you might be right. I haven't tried this solution out, but is the problem when...

You load a form
You click Next
Enketo finds a validation error and disables the Next button
You fix the validation error but the Next button doesn't enable until you blur

I've just tried this on my local branch and it looks like the field level validation clears on change without blur (I tested a select2 field). This should fire the valuechange.enketo event (and/or the validationcomplete.enketo event) so we should be able to reenable the next button right away. The issue may be that the PR is checking for CSS classes which may or may not have been removed yet - according to the documentation the valuechange.enketo event also passes whether or not validation failed which may be a more reliable value.

It looks like you're working on this so I haven't dug in and tried this out - let me know if I can help.

SCdF commented 5 years ago

It looks like the field level validation clears on change without blur (I tested a select2 field).

This works for select2 fields, checkboxes, radios. However, it doesn't work for input fields (try creating a patient and not filling in their name). I'm guessing because Enketo is hooked into onchange at a system level and that doesn't fire on text-based inputs every time the you type into them, only when the value is "committed".

So then, I think there are three choices:

Disable the next button when there is invalid input and be OK with users having to tap out of text input fields before the next button becomes available again
Patch enketo so that it supports the more modern oninput, taking care to consider what that would do for performance (since you'd be running validation on every key down effectively, unless you batch via debouncing etc)
Leave it as is

Without any input from users as to how annoying the current situation is, my vote would be for the last one.

garethbowen commented 5 years ago

A fourth option is that we listen for oninput on the whole form. Then we could either 4a) re-run validation and update the Next button, or 4b) enable the Next button right away and let the validation run when they click it.

My gut feeling is 4a would be too expensive to run every key press which may be why Enketo doesn't do it. I think 4b would be a reasonable compromise between correctness and performance.

SCdF commented 5 years ago

OK @garethbowen I've done 4b, and I've also done it on the submit button for consistency. Check our the PR above and have a play and see if you like it.

garethbowen commented 5 years ago

Ready for AT in 3399-next-button-enketo-fixed

ngaruko commented 5 years ago

LGTM on majority of forms. However:

Once the 'Next' button has been greyed out due to a missing required field, it does not get enabled after rectifying the error. Eg - trying to skip the Last Menstrual Period, then come back and chose a date... (see screenshot below). Does this have anything with the issue you raised in this comment @newtewt ?

(The following are also on master, so could be raised separately if needed).

Some forms (eg New Pregnancy)come with This field is required error message upon opening. It has been like this for a while and I think since we are on it, we could fix it, ie only flag the error message when the user attempt to proceed without selecting the patient's name
Other forms (like Immunization Report) don't have name selection as required, so a report can be submitted without a name (maybe a form or conf issue?)

SCdF commented 5 years ago

@ngaruko thanks for ATing this.

On 1, can you provide reproduction steps and details (ie the actual form you're seeing this on etc) so I can look at it

On 2, I've noticed this as well but it's a separate issue and so we should raise it as such.

On 3, I'm not clear on the problem here? If a form field isn't required then the code wouldn't treat it as such. If you are using some standard / default config that we support (it's not clear from your post) and you think that config is wrong that would like 2 warrant a separate ticket

SCdF commented 5 years ago

@ngaruko no worries on 1, I've managed to reproduce it. For the other 2 though if you want to raise issues that would be helpful

SCdF commented 5 years ago

OK I originally wrote how I was at a dead end but I took another look and I got a bit further.

This is now available in the linked PR, and should have been pushed to horti (though we are currently suffering through pretty massive e2e test stability so it's not clear).

This is somewhat of a compromise:

It should work correctly on a singular page, disabling and re-enabling as expected
Going back a page will always re-enable the next button so you no longer get stuck
However, because we've dropped all of the dodgy jQuery find usage I haven't been able to detect an invalid page upon arrival into it (e.g. you are on an invalid page and hit back then next). In this instance the next button will stay enabled. I think this is better than the alternative, which is to potentially get stuck with an endlessly disabled next button!

garethbowen commented 5 years ago

@ngaruko I think this is ready for review again.

ngaruko commented 5 years ago

Thanks @SCdF . That one seems to be work perfect. There are still a few areas where the app could do better in forms. 1 - If we have more than one radio buttons on the form and try to go Next without selecting any, then only select one upon prompt, the Next button is showing enabled, but not functional see screenshot and screen recording attached (from a delivery report)

delivery.mov.zip

2 - Maybe a caching issue, when after being prompted to make a selection, the user goes back or to a different tab, then comes to back to the form, the error message is still showing while the Next button shows enabled (see screenshot and screen recording - for a pregnancy form) To reproduce this one: Start new pregnancy form on Last Menstrual Period page, select no then click next without selecting any burron from Approximate start date of last cycle* Next button greyed out and error shows - as expected Click 'yes' radio button Click back 'No" button ==> Error shows, but Next button looks enabled preg.mov.zip

3 - Maybe a regression here, we lost the warning when we exit the form while filling it no-warning.mov.zip

garethbowen commented 4 years ago

Closing as won't fix as recommended by @SCdF - it's low priority and non-trivial to get right.

medic / cht-core

NEXT button on enketo forms is available even when it doesn't do anything #3399

Observed behaviour

Expected behaviour