Make pre-submit privacy agreement more generic and configurable #209

dmethvin-gov · 2018-08-08T17:11:47Z

Essentially, make the pre-submit notification, checkbox, label, and error message configurable via the formConfig.preSubmitInfo. If someone wants to leave out the validation completely they just leave out the label of the field (another trigger field can be chosen). So for example, you could fill out the notice to say, "By submitting this form you agree to the privacy agreement" and not have the checkbox or label.

I still need to make a change to actually set the form field name to fieldName in the outgoing data but figured I'd wait until we decided whether this should land and if so what the right names would be. (In other words, the current starter app always sends privacyAgreementAccepted: true in the submitted form data, but with this PR if they use formConfig.preSubmitInfo: { fieldName: 'licenseAccepted' } it should send licenseAccepted: true instead.)

This is passing the current unit tests but I think that's more an indictment of the unit tests than an assurance I got it right. 😸 Like the other cleanup I did to the unit tests, this file has a lot of unnecessary duplication and I just piled mine on top for now.

I'll gladly change the field and variable names used here too, I'm not super attached to them.

This fixes #53

annekainicUSDS

This approach makes sense to me, so far! Curious to see how we'll update the fieldName throughout. We'll probably also want to add some docs on how this works too.

annekainicUSDS · 2018-08-13T13:28:21Z

src/js/components/PreSubmitSection.jsx

+  return (
+    <div>
+      {preSubmitInfo.notice || ''}
+      {preSubmitInfo.label &&


It's smart to use a field to trigger whether or not the checkbox is shown at all, but I think there is value in making this field explicit, creating an additional prop called showCheckbox or something like that. I think it makes it more clear to the user what conditions cause the checkbox to be shown.

annekainicUSDS · 2018-08-13T14:08:13Z

src/js/state/reducers.js

-  [SET_PRIVACY_AGREEMENT]: (state, action) => {
-    return _.set('data.privacyAgreementAccepted', action.privacyAgreementAccepted, state);
+  [SET_PRE_SUBMIT]: (state, action) => {
+    return _.set('data.preSubmitAccepted', action.preSubmitAccepted, state);


data.preSubmitAccepted may need to change if we change fieldName? I'm curious how we're going to set this to a variable, because the user could name this field anything they want in the form config, right?

annekainicUSDS · 2018-08-13T14:09:35Z

src/js/components/PreSubmitSection.jsx

+import React from 'react';
+import ErrorableCheckbox from './ErrorableCheckbox';
+
+// formConfig.preSubmitInfo = {


It might be useful to add some comments about how preSubmitInfo comes from the form config, and how the other props (onChange, checked, showError) get passed, to distinguish them from one another.

dmethvin-gov · 2018-08-16T16:46:43Z

I think I've cleaned this up so that it's ready, and I added some docs that could use a review. No hurry on landing it tho.

bernars-usa · 2018-08-16T16:56:27Z

docs/building-a-form/available-form-features-and-usage-guidelines.md

@@ -419,7 +419,17 @@ For the code implementation, see the [`review` folder](../../src/js/review).

 ### Required checkbox before form submission

-Use this feature to require a user to indicate they have read terms and conditions, a privacy policy, or any other text before submitting your form. This includes a checkbox and short-form text that can include relevant links to more verbose text on separate pages on your site.
+Use this feature to require a user to indicate they have read terms and conditions, a privacy policy, or any other text before submitting your form. It optionally includes a checkbox and short-form text that can include relevant links to more verbose text on separate pages on your site. To configure this feature, place a `preSubmitInfo` object in the `formConfig`:


It optionally includes a checkbox and short-form text

Which part of this is optional? By my understanding, the checkbox itself was already optional, but now the text that accompanies it is what's customizable, is that correct?

Before this PR the checkbox was required and it had to be checked before the user could submit successfully. Also, the notice was hard-wired text that couldn't be changed by the user.

With this PR the user can change the notice text or leave it out completely (by not specifying the notice property), and have a mandatory checkbox+label or leave it out completely (by setting required: false or leaving out the required/field/label properties entirely). The two features can be used independently, so you can just have a notice over the submit button for example that says "By submitting this form I agree to the privacy policy and represent that all the information here is true to the best of my knowledge."

I just added the required field this go-round and it brings up some unusual error cases because you can now say required: true but what if you don't specify the label or field? For now just pick some defaults to keep the ball rolling but this is more of a form developer error and we haven't addressed those issues although #176 is open to address such things.

I chose the settings in the example config to match the screen shot so we wouldn't need to shoot it again, but I'll need to verify that is actually true and there aren't any layout issues or whatever.

I don't think we need the "optionally" here, then. Unfortunately, as it was originally written, it already seemed like the feature was optional. We can just go with:

Use this feature to require a user to indicate they have read terms and conditions, a privacy policy, or any other text before submitting your form. It includes a checkbox and short-form text that can include relevant links to more verbose information on separate pages on your site. To configure this feature, place a preSubmitInfo object in the formConfig:

bernars-usa

Thanks for keeping docs in mind!

dmethvin-gov · 2018-08-27T19:24:25Z

@annekainicUSDS this should be ready for review. I feel like the mocking of so many things in all of the tests is a sign that the architecture does not encapsulate things very well but that can wait for a later refactor.

bernars-usa · 2018-08-27T20:41:34Z

It looks like you have a merge conflict here, @dmethvin-gov.

dmethvin-gov · 2018-08-27T21:12:11Z

it was close to working and I managed to mess it up while resolving this spurious conflict with the map file. This is a result of #218 where our build artifacts don't track the real source files. I'll mess with it and try to get something I can commit.

dmethvin-gov · 2018-08-28T16:37:38Z

@annekainicUSDS This should be ready for review now, I cleaned up the tests a bit in the process.

annekainicUSDS

A few questions for you, @dmethvin-gov!

annekainicUSDS · 2018-08-29T17:12:36Z

docs/building-a-form/available-form-features-and-usage-guidelines.md

@@ -419,7 +419,17 @@ For the code implementation, see the [`review` folder](../../src/js/review).

 ### Required checkbox before form submission

-Use this feature to require a user to indicate they have read terms and conditions, a privacy policy, or any other text before submitting your form. This includes a checkbox and short-form text that can include relevant links to more verbose text on separate pages on your site.
+Use this feature to require a user to indicate they have read terms and conditions, a privacy policy, or any other text before submitting your form. It includes a checkbox and short-form text that can include relevant links to more verbose information on separate pages on your site. To configure this feature, place a `preSubmitInfo` object in the `formConfig`:


This is super nit picky, but I wonder if we should move the last sentence in this paragraph that starts with "To configure this feature" to a new line, so that the instructions for how to use it are a little easier to find and more apparent.

Should we also specify that the object takes specific elements, listed below? Just so they know what goes in it and that you can't just pass anything in there?

At the moment you can pass anything, just like the rest of formConfig we're not validating it against a schema so anything that is not specified is ignored. The example here is showing all the valid properties and what they might be set to.

annekainicUSDS · 2018-08-29T17:35:06Z

src/js/review/SubmitController.jsx

+    let isValid;
+    let errors;
+
+    // If a pre-submit agreement was specified, it has to be accepted first


This logic took me a little while to reason through and understand. Perhaps a more descriptive comment would be helpful here? Something like:

"If a pre-submit agreement was passed to the form config AND if it is a required field, capture the field name. We use this to check in the form data if the field with that name has been submitted as true before checking if the entire form submission is valid."

Sounds good, I'll make a followup PR.

annekainicUSDS · 2018-08-29T17:36:15Z

src/js/review/SubmitController.jsx

+    // If a pre-submit agreement was specified, it has to be accepted first
+    const preSubmitField = formConfig.preSubmitInfo &&
+        formConfig.preSubmitInfo.required && (formConfig.preSubmitInfo.field || 'AGREED');
+    if (preSubmitField && !form.data[preSubmitField]) {


Am I understanding the addition of this logic to be pulling this check out of isValidForm and instead checking it first here before running the isValidForm check? Is there a reason for pulling the preSubmitInfo check out of the entire isValidForm check?

If we left that check in isValidForm we'd need to pass in the information about the preSubmit checkbox and its value just so it could check them. If we weren't treating this checkbox specially, for example if it were just a plain required checkbox with an error message, I think it would be natural to let isValidForm deal with it. However, since this is a special component that is actually part of SubmitController it seems unnatural to pass this information to a utility function to check it. It didn't stick out as much before since the name of the field and the value (true) was hard-wired, but it still was a special case.

annekainicUSDS · 2018-08-29T17:41:42Z

src/js/review/SubmitController.jsx

      submission
    } = this.props;
    return (
      <div>
-        <p><strong>Note:</strong> According to federal law, there are criminal penalties, including a fine and/or imprisonment for up to 5 years, for withholding information or for providing incorrect information. (See 18 U.S.C. 1001)</p>
-        <PrivacyAgreement
+        { this.preSubmitInfo && <PreSubmitSection


Is this check supposed to be formConfig.preSubmitInfo instead of this.preSubmitInfo? I can't figure out what this.preSubmitInfo is referring to?

Yipes, that looks like a mistake. It's completely possible that the tests aren't catching this, given that I've found the same problem in several other cases.

annekainicUSDS · 2018-08-29T17:44:19Z

src/js/review/SubmitController.jsx

-          checked={privacyAgreementAccepted}
-          showError={showPrivacyAgreementError}/>
+          preSubmitInfo={formConfig.preSubmitInfo}
+          onChange={() => this.props.setPreSubmit(formConfig.preSubmitInfo.field, this.value)}


I'm also a little confused where this.value comes from. In the past I've seen us use event.target.value to grab the value of a field, which is what I assume we're trying to grab here?

I'll look at this as well.

annekainicUSDS · 2018-08-29T17:56:38Z

src/js/state/helpers.js

+  // Initialize the preSubmit flag if one was specified
+  const preSubmitInfo = formConfig.preSubmitInfo;
+  if (preSubmitInfo && preSubmitInfo.field) {
+    pageAndDataState.data[preSubmitInfo.field] = false;


So this is just adding the field to the form data and initializing it to false? Is there a reason for doing this separately as opposed to how we were doing it before where it was just adding above in the data object?

When this field had a hardwired name it could easily be initialized above. Now that the name can be configured and the field doesn't even need to be present in some cases it gets messier to try and define it in the initial object. I could have done it but it wouldn't be that readable IMO.

The only reason I initialized this field at all is because there are tests that demand a false initial value and I was hesitant to change existing tests. However, given my experience with the tests I'm more inclined to change them now.

What I would prefer to do is not initialize this field at all since we only really care if the value is true if the formConfig indicates it's required. The form cannot be submitted unless the value is true. An undefined or missing field is not true which works fine and it saves a bunch of ugly code like this.

* Followup to review on #209. Improve docs; refactor tests * Move to enzyme for everything in SubmitController test * Add test for just a notice with no checkbox * Clarify docs; make sample match the existing screen shot * Changes based on review feedback

…#209) * Make pre-submit privacy agreement more generic and configurable * Make preSubmit field name configurable; fix unit tests * Remove debugging; add docs * Fix second unit test with orphaned expect() * Fix failing unit test * Add test case for when preSubmitInfo isn't specified (cherry picked from commit d3cd6f0)

* Followup to review on #209. Improve docs; refactor tests * Move to enzyme for everything in SubmitController test * Add test for just a notice with no checkbox * Clarify docs; make sample match the existing screen shot * Changes based on review feedback (cherry picked from commit f6edfe6)

dmethvin-gov requested a review from annekainicUSDS August 8, 2018 17:12

annekainicUSDS reviewed Aug 13, 2018

View reviewed changes

dmethvin-gov requested a review from bernars-usa August 16, 2018 16:46

bernars-usa reviewed Aug 16, 2018

View reviewed changes

bernars-usa approved these changes Aug 16, 2018

View reviewed changes

dmethvin-gov added 6 commits August 27, 2018 16:58

[WIP] Make pre-submit privacy agreement more generic and configurable

e0baf48

Make preSubmit field name configurable; fix unit tests

0000c71

Remove debugging; add docs

07e99ac

Fix second unit test with orphaned expect()

9de01ac

Reword

e99a3d2

Equals

6e3f673

dmethvin-gov force-pushed the 129-presubmit branch from 576ee3c to 6e3f673 Compare August 27, 2018 21:03

dmethvin-gov added 2 commits August 28, 2018 11:10

Fix failing unit test

ac71bab

Add test case for when preSubmitInfo isn't specified

fa75074

dmethvin-gov changed the title ~~[WIP] Make pre-submit privacy agreement more generic and configurable~~ Make pre-submit privacy agreement more generic and configurable Aug 28, 2018

dmethvin-gov mentioned this pull request Aug 28, 2018

[WIP] Move Google Analytics and Sentry to hooks that apps can call #196

Merged

3 tasks

dmethvin-gov merged commit d3cd6f0 into usds:master Aug 29, 2018

dmethvin-gov deleted the 129-presubmit branch August 29, 2018 17:49

annekainicUSDS reviewed Aug 29, 2018

View reviewed changes

dmethvin-gov added a commit to dmethvin-gov/us-forms-system that referenced this pull request Aug 31, 2018

Followup to review on usds#209. Improve docs; refactor tests

0157e74

dmethvin-gov mentioned this pull request Sep 4, 2018

Remove federal law language in SubmitController #129

Closed

annekainicUSDS mentioned this pull request Sep 18, 2018

Create a 1.2.0 release #272

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make pre-submit privacy agreement more generic and configurable #209

Make pre-submit privacy agreement more generic and configurable #209

dmethvin-gov commented Aug 8, 2018 •

edited

Loading

annekainicUSDS left a comment

annekainicUSDS Aug 13, 2018

annekainicUSDS Aug 13, 2018

annekainicUSDS Aug 13, 2018

dmethvin-gov commented Aug 16, 2018

bernars-usa Aug 16, 2018

dmethvin-gov Aug 16, 2018

bernars-usa Aug 16, 2018

bernars-usa left a comment

dmethvin-gov commented Aug 27, 2018

bernars-usa commented Aug 27, 2018

dmethvin-gov commented Aug 27, 2018

dmethvin-gov commented Aug 28, 2018

annekainicUSDS left a comment

annekainicUSDS Aug 29, 2018

annekainicUSDS Aug 29, 2018

dmethvin-gov Aug 30, 2018

annekainicUSDS Aug 29, 2018

dmethvin-gov Aug 30, 2018

annekainicUSDS Aug 29, 2018

dmethvin-gov Aug 30, 2018

annekainicUSDS Aug 29, 2018

dmethvin-gov Aug 30, 2018

annekainicUSDS Aug 29, 2018

dmethvin-gov Aug 30, 2018

annekainicUSDS Aug 29, 2018

dmethvin-gov Aug 30, 2018

Make pre-submit privacy agreement more generic and configurable #209

Make pre-submit privacy agreement more generic and configurable #209

Conversation

dmethvin-gov commented Aug 8, 2018 • edited Loading

annekainicUSDS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmethvin-gov commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernars-usa left a comment

Choose a reason for hiding this comment

dmethvin-gov commented Aug 27, 2018

bernars-usa commented Aug 27, 2018

dmethvin-gov commented Aug 27, 2018

dmethvin-gov commented Aug 28, 2018

annekainicUSDS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmethvin-gov commented Aug 8, 2018 •

edited

Loading