Fix expand active order in ReviewChapters #267

jbalboni · 2018-09-14T16:30:00Z

This swaps the order of getActivePages and expandArrayPages to match routing.js.

<Addresses/Closes> #266

Types of changes

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

I've reviewed the guidelines for contributing to this repository.
I've checked style and lint with npm run lint.
I built the production version with npm run build.
I added tests to verify a bug fix or new functionality.
All tests pass locally with npm test.
My change requires a change to the documentation, and I've updated the documentation accordingly.

jbalboni · 2018-09-14T16:30:55Z

Will push up a test and prod files shortly.

dmethvin-gov

Might there be similar problems with SubmitController, getPageKeys, or getActiveChapters since they use the old order? Should there be a getExpandedActivePages defined to prevent someone calling these in the wrong order?

dmethvin-gov · 2018-09-14T17:40:11Z

src/js/review/ReviewChapters.jsx

@@ -125,8 +125,8 @@ function mapStateToProps(state, ownProps) {
  const chapters = chapterNames.map(chapterName => {
    const pages = pagesByChapter[chapterName];

-    const activePages = getActivePages(pages, formData);
-    const expandedPages = expandArrayPages(activePages, formData);
+    let expandedPages = expandArrayPages(pages, formData);


Why let here and not const? You can always nest the expandArrayPages() in the next line.

Nesting seemed less readable than this, but I'm happy to change it if you'd prefer.

jbalboni · 2018-09-14T18:27:51Z

@dmethvin-gov Yeah, looks like those need changing and also the helper function seems like a good idea.

jbalboni · 2018-09-14T18:29:44Z

src/js/review/ReviewChapters.jsx

@@ -106,7 +104,7 @@ class ReviewChapters extends React.Component {
  }
 }

-function mapStateToProps(state, ownProps) {
+export function mapStateToProps(state, ownProps) {


Not sure how you all feel about this. We made the changes in here to put more code in mapStateToProps because it's more testable, but in order to test it directly, we need to export it.

More testing is always good. In this case it's testing a lot of implementation and plumbing details so not ideal, but the test is clearly marked so if it breaks during a refactor it should be clear why.

jbalboni · 2018-09-14T19:03:53Z

@dmethvin-gov Should be ready for another look.

dmethvin-gov · 2018-09-14T20:27:39Z

This duo of calls seems to be everywhere, here's another one in FormNav. Can you look around and see if I missed any others? Would it be possible to not export one or both of the two individual functions, are they used separately for any reason?

jbalboni · 2018-09-14T20:31:37Z

I found the FormNav one when I took another pass through, but didn't see any others. I did see some uses of the functions on their own, but I didn't look too deeply at if that was valid. I will take another look at that on Monday.

jbalboni · 2018-09-17T18:10:43Z

@dmethvin-gov I looked through for uses of just getActivePages and it looks like the default transformForSubmit was using it without expanding array pages first. So I've updated that, too.

I think all this code was missing because itemFilter instead of depends was initially meant for filtering out items in pagePerItem pages, but then the index was added to depends for one of our forms and wasn't full fleshed out. On vets we don't have any uses of depends and pagePerItem pages outside of one form, which is only now going to beta.

annekainicUSDS

One question, otherwise LGTM!

annekainicUSDS · 2018-09-18T02:41:17Z

src/js/helpers.js

+ */
+export function transformForSubmit(formConfig, form, replacer = stringifyFormReplacer) {
+  const expandedPages = expandArrayPages(createFormPageList(formConfig), form.data);
+  const activePages = getActivePages(expandedPages, form.data);


Why are we not using getActiveExpandedPages here if it's doing these same 2 checks in this order? Because we need to calculate expandedPages separately for the next function?

Yeah, I thought it was useful to used the expandedPage array for both of the following function calls and make it clear that we're using the same base data to get the active and inactive page lists.

Yep, that's what it seemed like. Feel free to merge this when you're ready @jbalboni!

jbalboni · 2018-09-18T20:32:11Z

@annekainicUSDS I think you'll have to merge it, I don't have write access here.

annekainicUSDS · 2018-09-19T03:03:46Z

@jbalboni Oops sorry! Will merge now.

) Fix expand active order (cherry picked from commit b667241) Add build files (cherry picked from commit 7b8b325) Add tests and export (cherry picked from commit 56c8743) Add helper function (cherry picked from commit 31fd754) Fix typo (cherry picked from commit e999396) Fix bad change (cherry picked from commit ccd28fa) Run prod build (cherry picked from commit 4fb0362) Update nav with new helper Update transform to handle pagePerItem depends functions (cherry picked from commit 5ece651) Cleanup usds#267 PR cherry pick (dmethvin-gov)

Fix expand active order

b667241

Add build files

7b8b325

dmethvin-gov reviewed Sep 14, 2018

View reviewed changes

jbalboni added 2 commits September 14, 2018 14:13

Add tests and export

56c8743

Add helper function

31fd754

Fix typo

e999396

jbalboni commented Sep 14, 2018

View reviewed changes

jbalboni added 3 commits September 14, 2018 14:30

Fix bad change

ccd28fa

Run prod build

4fb0362

Update nav with new helper

6632780

Update transform to handle pagePerItem depends functions

5ece651

annekainicUSDS approved these changes Sep 18, 2018

View reviewed changes

annekainicUSDS mentioned this pull request Sep 18, 2018

Create a 1.2.0 release #272

Closed

annekainicUSDS merged commit 45b600a into usds:master Sep 19, 2018

jcmeloni-usds mentioned this pull request Sep 19, 2018

On the review page, pagePerItem pages with depends functions are not correctly passed index #266

Closed

dmethvin-gov mentioned this pull request Sep 24, 2018

Fix bad expect()ations in tests #284

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix expand active order in ReviewChapters #267

Fix expand active order in ReviewChapters #267

jbalboni commented Sep 14, 2018 •

edited

Loading

jbalboni commented Sep 14, 2018

dmethvin-gov left a comment •

edited

Loading

dmethvin-gov Sep 14, 2018

jbalboni Sep 14, 2018

jbalboni commented Sep 14, 2018

jbalboni Sep 14, 2018

dmethvin-gov Sep 14, 2018

jbalboni commented Sep 14, 2018

dmethvin-gov commented Sep 14, 2018

jbalboni commented Sep 14, 2018

jbalboni commented Sep 17, 2018

annekainicUSDS left a comment

annekainicUSDS Sep 18, 2018

jbalboni Sep 18, 2018

annekainicUSDS Sep 18, 2018

jbalboni commented Sep 18, 2018

annekainicUSDS commented Sep 19, 2018

Fix expand active order in ReviewChapters #267

Fix expand active order in ReviewChapters #267

Conversation

jbalboni commented Sep 14, 2018 • edited Loading

Types of changes

Checklist:

jbalboni commented Sep 14, 2018

dmethvin-gov left a comment • edited Loading

Choose a reason for hiding this comment

dmethvin-gov Sep 14, 2018

Choose a reason for hiding this comment

jbalboni Sep 14, 2018

Choose a reason for hiding this comment

jbalboni commented Sep 14, 2018

jbalboni Sep 14, 2018

Choose a reason for hiding this comment

dmethvin-gov Sep 14, 2018

Choose a reason for hiding this comment

jbalboni commented Sep 14, 2018

dmethvin-gov commented Sep 14, 2018

jbalboni commented Sep 14, 2018

jbalboni commented Sep 17, 2018

annekainicUSDS left a comment

Choose a reason for hiding this comment

annekainicUSDS Sep 18, 2018

Choose a reason for hiding this comment

jbalboni Sep 18, 2018

Choose a reason for hiding this comment

annekainicUSDS Sep 18, 2018

Choose a reason for hiding this comment

jbalboni commented Sep 18, 2018

annekainicUSDS commented Sep 19, 2018

jbalboni commented Sep 14, 2018 •

edited

Loading

dmethvin-gov left a comment •

edited

Loading