Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional sections don't validate correctly #121

Closed
hmpf opened this issue Sep 30, 2020 · 8 comments · Fixed by #133
Closed

Optional sections don't validate correctly #121

hmpf opened this issue Sep 30, 2020 · 8 comments · Fixed by #133
Assignees
Labels
bug Something isn't working

Comments

@hmpf
Copy link
Owner

hmpf commented Sep 30, 2020

If the section is optional and the planner has chosen to skip the question, the section is valid.

@hmpf
Copy link
Owner Author

hmpf commented Sep 30, 2020

See also #122 and #123. The three might all be solved with one trick. Maybe.

@hmpf hmpf changed the title Optional sections doesn't validate correctly Optional sections don't validate correctly Sep 30, 2020
@hmpf hmpf self-assigned this Sep 30, 2020
@vbhavdal
Copy link
Contributor

vbhavdal commented Oct 5, 2020

@hmpf Not able to recreate this one, if I make a plan with an optional section and choose No for the section, it gets green check mark in the summary at the end. Could you try it once more on master?

@hmpf
Copy link
Owner Author

hmpf commented Oct 5, 2020

Closes #88.

@hmpf
Copy link
Owner Author

hmpf commented Oct 6, 2020

I make a new plan based on MBT and go directly to the middle, optional section. Click no on the toggle question, which leads to a green checkmark. When I later click Yes on the same question, the section stays green. It shouldn't unless all the questions in the activated path after the toogle also are valid.

@vbhavdal
Copy link
Contributor

vbhavdal commented Oct 6, 2020

I see. And this works correctly for other, manual branches?

@hmpf
Copy link
Owner Author

hmpf commented Oct 6, 2020

I think so. It's really one of the areas where we need more tests.

@hmpf
Copy link
Owner Author

hmpf commented Oct 6, 2020

Okay, my investigation has revelead:

  1. There was a bug in section.is_skipped. Will be fixed by the fix to this ticket.
  2. The MBT section2 (optional) branching structure was insufficient to test the branching in an optional section. I've updated it already.
  3. section.validate_data is insufficiently thorough. With the old structure of the branching in MBT section 2, it was necessary to check the answers to validate that the branching structure was correct, and validate_data doesn't do that, yet. This has it's own issue, Section.validate_data needs to check the answers when validating a branching section #132.

Also, MBT should exist as a fixture of sorts so we can run automated tests on it. It should also be included in the devfixtures somehow. This should be made easier by having template export, we could store an export as the fixture. This should have its own issue.

@hmpf
Copy link
Owner Author

hmpf commented Oct 6, 2020

I'm bitten by #130 now so I'll make a PR for what I have so we can release the next version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants