Create a 1.2.0 release

[annekainicUSDS]

There are a couple of changes the Vets.gov team needs more urgently than the next 2.0.0 release. Create a stable 1.2.0 branch with the following PRs:

1. Fix expand active order in ReviewChapters #267
2. Make pre-submit privacy agreement more generic and configurable #209

[dmethvin-gov]

@annekainicUSDS @jbalboni For the record, the code related changes made since 1.1 are:

• Make pre-submit privacy agreement more generic and configurable #209 Make pre-submit privacy agreement more generic and configurable (with [WIP] Followup to review comments on #209 #242)
• DateRange: Allow a range to be a single date #239 DateRange: Allow a range to be a single date
• [WIP] Move Google Analytics and Sentry to hooks that apps can call #196 Move Google Analytics and Sentry to hooks that apps can call
• Use date-fns instead of moment #258 Use date-fns instead of moment
• Fix expand active order in ReviewChapters #267 Fix expand active order in ReviewChapters

I've created a 1.x-stable branch by starting at current master (45b600a), reverting #258 + #196, and rebuilding. If you want I can revert #239 as well but that seemed like a bugfix, if you have code that breaks it may indicate some incorrect assumption on one side or the other.

@jbalboni do you want to test a bit with this branch? It has built files so you should be able to pull the hash. Note that since #209/#242 is there you'll need to add that section to your `formConfig.

[jbalboni]

That looks good to me. Is #258 a breaking change? Doesn't need to be in the release either way, but I am definitely looking forward to it.

I will test this out tomorrow. Thanks @dmethvin-gov and @annekainicUSDS

[dmethvin-gov]

I don't think #258 is a breaking change and it makes things smaller. If you're not using moment anywhere inside your own code, or want to refactor to use date-fns as well, we can leave it in.

[dmethvin-gov]

Actually I know at least some of the forms do use moment, I saw it in the Form 1990.

[annekainicUSDS]

We're also missing these PRs related to definitions I think @dmethvin-gov, which I assume we don't want to include in the 1.x stable branch:

1. Remove some definitions and tests #214
2. Extract definitions 1 built #215
3. Add schemas to remaining definitions #219
4. Make definitions consistent #225

[dmethvin-gov]

It looks like master does not have the commits in the right order. I started from the 1.1.0 commit there and all the ones mentioned above are actually before that. To be safe I think I'll start from the release in production and only pick over #267 and #209. Before I do that though, @jbalboni do you really want #209? If you're happy with your current checkbox-before-submit setup it will save work to not have to touch every form to add the config for this.

[jbalboni]

@dmethvin-gov Yeah, we have an item on our side to update that checkbox to the default required field notation and tweak the error message. It's probably a general enough change to be in USFS, but that would mean another PR to submit and merge. Seems like including #209 would be easier, though if not I can submit a PR for my change.

[dmethvin-gov]

@jbalboni no problem putting it in, I just wanted to make sure you really wanted it so I won't have to take it out. Sometimes there are conflicts in these cherry picks , especially while we're still committing built files, so I just want to commit what's really needed. If it's not working right when you test we can update it of course.

[jbalboni]

Makes sense. We do want it in. Let me know when the branch is updated and I'll take a look.

[dmethvin-gov]

Okay, 1.x-stable has been updated. It was a bit of a tough fight because #267 was landed as a merge commit and it was a little messy to cherry pick against 1.1.0. I tried merging it but that didn't work well. I ended up cherry picking the individual commits and then rebasing to create a single commit. Most of the problem was due to all the build artifacts, I now wish I'd prioritized fixing that sooner!

Since nobody was using the branch yet and it wasn't at all what we're going to end up with, I forced-pushed it (the git equivalent of the five-second rule for food that falls on the floor). If there are minor changes from here we should be able to avoid further force-pushes. Ready for your team's review @jbalboni!

[annekainicUSDS]

Oops, I'm sorry, that was my fault. I knew I would forget to switch back to the squash button.

[jbalboni]

So far testing is going pretty well: department-of-veterans-affairs/vets-website#8503

I did realize a couple things:

1. There's a breaking change in the 1.x branch, since filterInactivePages was renamed to filterInactivePageData and the parameters were changed. This is fine, we only have one place in our code to update
2. There are a couple more issues for me to open (or PRs to submit if there are already issues) to fully fix the items I've been working on. I'm not sure on the timing of those, but I guess we can coordinate how to get them back into vets.gov when they're done.

[jbalboni]

@dmethvin-gov It looks like something about the pre-submit change isn't working. It appears that it just never shows up. It looks like the PreSubmitSection component is conditional on this.preSubmitInfo being truthy in SubmitController, but I don't see that set anywhere. It looks like master's SubmitController is different and doesn't have that same check.

[dmethvin-gov]

The reason master looks different is because the 1.x-stable branch is missing f6edfe6 which was a followup. I don't think it changed the logic (mostly it was docs and test cleanup) but that really should be put into the branch if #209 stays.

However, that is the correct behavior if you totally leave out a preSubmitInfo in the formConfig, it doesn't show any checkbox or notice below the submit button. To put it another way, you need to go through your 11 or so formConfigs and add a preSubmitInfo section, else you won't have a check box to the effect of "I agree to the privacy policy" on the form. That's why I asked whether this was a change you wanted. Should I drop in the followup commit, or would you prefer I back out #209?

[jbalboni]

SubmitController definitely has logic changes between master and the 1.x branch. In the master version, you're always rendering PreSubmitSection and you're merging the preSubmitInfo config object on top of a default set of data. The 1.x version doesn't do either of those things and also has a bug where it's checking for this.preSubmitInfo, which never exists (should be formConfig.preSubmitInfo, I think).

You can drop in the follow up commit. I'm fine with updating all the configs, but it looks you all changed the logic so that at least the checkbox will always show up, which isn't currently happening.

[dmethvin-gov]

Yeah, it sounds like there may have been some code things then. I've added that commit (which includes documentation) so let us know if it's working.

[jbalboni]

@dmethvin-gov The checkboxes show up now, but it looks like some of the onChange wiring is mismatched. I get a console error when clicking the checkbox:

It looks like this is because onChange is being passed a boolean instead of the event.

[dmethvin-gov]

Looking into this now.

[dmethvin-gov]

Okay, it should be ready to try again. It only took me 10 minutes to fix the problem but then I decided to write a test for it and it took me 2 hours among other things to rediscover how to fake out the tests without Redux involved.

[jbalboni]

@dmethvin-gov I think it's missing updates to the lib/ files.

[dmethvin-gov]

@jbalboni sorry! I have been switching between branches too much.

[jbalboni]

@dmethvin-gov Thanks! That fixed the console error, though there is a warning that says:

Warning: ErrorableCheckbox is changing an uncontrolled input of type checkbox to be controlled. Input elements should not switch from uncontrolled to controlled (or vice versa). Decide between using a controlled or uncontrolled input element for the lifetime of the component. More info: https://fb.me/react-controlled-components

I don't think that's an issue right now, but something to note for later.

The other thing I'm seeing now is an errant } that I wasn't seeing before:

The pre submit config I'm using is:

{
   required: true,
   notice: (
     <div>
      <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)
     </div>
   ),
   field: 'privacyAgreementAccepted',
   label: <span>I have read and accept the <a target="_blank" href="/privacy/">privacy policy</a></span>,
   error: 'You must accept the privacy policy before continuing.'
}

I thought maybe it was using JSX in the label, but looks like that's not the issue.

[dmethvin-gov]

That errant brace is here leftover from an earlier commit. I removed it in the first attempt at this branch but forgot on the second. Let me look at the ErrorableCheckbox message to see if I can figure out why it has the vapors over that, and see if I can fix them both at once.

[dmethvin-gov]

OK, both problems should be gone now.

[jbalboni]

It looks like it's working correctly so far, but I need to update some more tests on our side and I haven't had time to do that today. Hopefully tomorrow I can give you a full answer on if it's all set on our side.

[jcmeloni-usds]

Just to confirm, we're waiting on @jbalboni to call this good on the Vets.gov side, then we can cut this release -- correct?

[dmethvin-gov]

Yes. I will probably push it manually since we don't have a release script in place that can do it and the chances are that a new script wouldn't get it right the first time.

[jcmeloni-usds]

@dmethvin-gov Great - just wanted to make sure I was reading everything right.

[jbalboni]

@dmethvin-gov This has been in production on vets.gov for a couple days and I haven't seen any issues come through, so I think we can call this good.

[dmethvin-gov]

@jbalboni I just published [email protected], we still need to create the GitHub release page but that doesn't hold you back from using it now. We can keep the 1.x-stable branch and apply future bugfix changes to that.

[dmethvin-gov]

This has been on npm for a while now, I just published the GitHub release so they're not out of sync anymore.