Fix demo generator #1034
Merged
Fix demo generator #1034
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The functions do not require the `UserRuntimeInterviewAttributes` and some functions, like the `enableConditional` and `completionConditional`, which could call them, receive a `UserInterviewAttributes` object.
by a call to the isSectionCompleted in the navigation helpers
fixes chairemobilite#1032 Since the enableConditional depend on the completion of the current section, when we navigate forward from a completed section, we need to set the section as completed, otherwise it won't be possible to navigate to the next section. The actual section completion is currently done on the section _after_ the section navigation. We'll need to revisit the section changes, when implementing chairemobilite#969.
There was a problem hiding this comment.
Pull Request Overview
Updates the demo generator to use the renamed section-completion helper, aligns interview attribute types, and adds runtime state-setting for completed sections.
- Swapped out
isSectionCompletefor the newisSectionCompletedhelper in the generator script - Updated type references from
UserRuntimeInterviewAttributestoUserInterviewAttributesin navigation helpers - Introduced a private
setSectionCompletedmethod inNavigationServiceusinglodash/setto mark sections complete
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/evolution-generator/src/scripts/generate_section_configs.py | Changed import and calls from isSectionComplete to isSectionCompleted |
| packages/evolution-common/src/services/questionnaire/sections/navigationHelpers.ts | Switched interview type in imports and JSDoc to UserInterviewAttributes |
| packages/evolution-common/src/services/questionnaire/sections/NavigationService.ts | Added _set import and setSectionCompleted method; inserted call after navigation |
Comments suppressed due to low confidence (3)
packages/evolution-generator/src/scripts/generate_section_configs.py:55
- Ensure the new import path correctly matches the published API and update any documentation or downstream consumers still referencing
isSectionComplete.
ts_code += f"import { isSectionCompleted } from 'evolution-common/lib/services/questionnaire/sections/navigationHelpers';\n"
packages/evolution-common/src/services/questionnaire/sections/navigationHelpers.ts:19
- The JSDoc
@paramannotations have been updated, but verify that descriptions remain accurate and reflect any new assumptions inUserInterviewAttributes.
* @param {UserInterviewAttributes} options.interview The interview object
packages/evolution-common/src/services/questionnaire/sections/NavigationService.ts:509
- The new
setSectionCompletedmethod lacks associated unit tests. Adding tests here will help prevent regressions in navigation logic.
private setSectionCompleted({
| @@ -6,6 +6,7 @@ | |||
| */ | |||
| import _isEqual from 'lodash/isEqual'; | |||
| import _shuffle from 'lodash/shuffle'; | |||
| import _set from 'lodash/set'; | |||
There was a problem hiding this comment.
[nitpick] Directly mutating nested interview state with lodash/set can be error-prone; consider encapsulating this in a higher-level helper or service to maintain consistency and ease testing.
Copilot uses AI. Check for mistakes.
Comment on lines
+516
to
+532
| // FIXME Not sure this is the place to set this, but since some enable | ||
| // conditional may depend on preceding section's completion status, we | ||
| // need to know that a section is completed. Using _set directly is not | ||
| // the best way, see if we should add a helper for this is we need to | ||
| // set completion of a section more generally. The backend also has | ||
| // similar code. Eventually, when implementing | ||
| // https://github.com/chairemobilite/evolution/issues/969, the section | ||
| // navigation history and completion won't be stored in the responses | ||
| // anymore and we'll have a better way to set it, so we can go for the | ||
| // easy fix for now. | ||
| _set( | ||
| interview, | ||
| `response._sections.${section.sectionShortname}${section.iterationContext ? '.' + section.iterationContext.join('/') : ''}._isCompleted`, | ||
| true | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
The FIXME highlights technical debt—consider tracking a ticket or refactoring soon to avoid leaving this workaround in production.
Suggested change
| // FIXME Not sure this is the place to set this, but since some enable | |
| // conditional may depend on preceding section's completion status, we | |
| // need to know that a section is completed. Using _set directly is not | |
| // the best way, see if we should add a helper for this is we need to | |
| // set completion of a section more generally. The backend also has | |
| // similar code. Eventually, when implementing | |
| // https://github.com/chairemobilite/evolution/issues/969, the section | |
| // navigation history and completion won't be stored in the responses | |
| // anymore and we'll have a better way to set it, so we can go for the | |
| // easy fix for now. | |
| _set( | |
| interview, | |
| `response._sections.${section.sectionShortname}${section.iterationContext ? '.' + section.iterationContext.join('/') : ''}._isCompleted`, | |
| true | |
| ); | |
| } | |
| this.markSectionAsCompleted(interview, section); | |
| } | |
| /** | |
| * Helper method to mark a section as completed in the interview responses. | |
| * | |
| * @param {UserRuntimeInterviewAttributes} interview The interview object | |
| * @param {NavigationSection} section The section to mark as completed | |
| */ | |
| private markSectionAsCompleted( | |
| interview: UserRuntimeInterviewAttributes, | |
| section: NavigationSection | |
| ): void { | |
| const path = `response._sections.${section.sectionShortname}${ | |
| section.iterationContext ? '.' + section.iterationContext.join('/') : '' | |
| }._isCompleted`; | |
| _set(interview, path, true); | |
| } |
Copilot uses AI. Check for mistakes.
samuel-duhaime
approved these changes
Jun 15, 2025
|
Make sure to remove the double click that I had to insert in the next button test in #1010, so we can test this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See individual commits for details