Follow up questions #381

allait · 2017-02-16T10:45:46Z

Split multiquestion schema generation

Adds helper methods to generate schema for flat (storing all properties
in a top-level object) and nested (storing properties in a nested object)
multiquestions.

Adds support for 'radios' question type to the followup schema
generation, and refactors it to use a merge_schemas helper to
make it easier to join multiple follow-up schemas.

Schema generation was using an older version of content-loader, which
didn't have DynamicList question class. Since DynamicList constructor
switches question type to 'multiquestion' when updating content-loader
we need to add additional dispatch logic to multiquestion so it can
call dynamic_list generator for DynamicList questions.

Don't add lead question to the oneOf subschema required

Always listing lead-in question as required in a subschema makes the
validation fail when used with enforce_required=False in the API.

This wasn't a problem for brief responses, since both the lead-in and
the followup questions are nested inside one key, which is missing in
unrelated updates. But when storing them as separate top-level key we
need to be able to unset the required fields using the schema required
list.

So instead of listing the lead-in question as required in the subschemas
we add it to the main schema required list.

Let multiquestions extend main schema fields

Allows property schema generators to return a schema addition to be
merged with the main schema. This enables multiquestions to add
allOf/oneOff subschemas for followup questions and modify the
required list on the main schema.

Changes the existing specialists anyOf/dependency additions to use
merge_schemas.

Splits dynamic_list from nestd_multiquestion output

Dynamic lists are serialized as an array, but other multiquestions
aren't, so it makes sense to move the array part of the schema to
the _dynamic_list function.

And since we can no longer dispatch to the dynamic_list property
based on the question type we need to remove it from the types
mapping.

samuelhwilliams · 2017-02-16T11:09:06Z

README.md

@@ -71,8 +71,7 @@ Question keys
 * `dynamic_field` defines the field in the filter context that will be used to generate dynamic list questions (eg when passing
  a brief as filter context setting `dynamic_field` to `brief.niceToHaveRequirements` will generate questions for nice-to-have
  requirements) (only valid for `dynamic_list` questions)
-* `followup` sets the name of the question that will be required/shown if the current question is answered with "Yes" (only valid
-  for `boolean` questions)
+* `followup` is a dictionary where each key is the name of the question that will be required/shown and the value for the key is a list of question answers that should display the follow up (only valid for `boolean` and `radios` questions)


Followups are required for checkboxes, too.

samuelhwilliams · 2017-02-16T14:05:38Z

schema_generator/utils.py

@@ -0,0 +1,16 @@
+def merge_schemas(a, b):
+    if not (isinstance(a, dict) and isinstance(b, dict)):
+        raise ValueError("Error merging '{}' and '{}'".format(


This should probably be a TypeError, and the error message could probably be more descriptive: "Unable to merge disparate types: '{}' and '{}'."

allait · 2017-02-16T15:31:26Z

@samuelhwilliams added support for checkbox question with follow-ups and optional follow-up questions. The tests need a new content-loader version to get merged, which I'm adding the checkbox support to now.

samuelhwilliams · 2017-02-17T11:07:02Z

Probably need to add some tests around the followup schema generation.

Adds helper methods to generate schema for flat (storing all properties in a top-level object) and nested (storing properties in a nested object) multiquestions. Adds support for 'radios' question type to the followup schema generation, and refactors it to use a `merge_schemas` helper to make it easier to join multiple follow-up schemas. Schema generation was using an older version of content-loader, which didn't have DynamicList question class. Since DynamicList constructor switches question type to 'multiquestion' when updating content-loader we need to add additional dispatch logic to `multiquestion` so it can call `dynamic_list` generator for `DynamicList` questions.

Getting a list of required fields for multiquestions is complicated by the addition of followups and the fact that dynamic list / nested multiquestions have to deal with two required fields lists: one for the main schema and one for the nested object schema. Allowing property schema functions to return an optional list of required fields allows us to move that logic closer to the followup subschema generation without having to make any changes to the `.form_field` methods on the question classes, that could potentially affect other parts of the application code.

Always listing lead-in question as required in a subschema makes the validation fail when used with `enforce_required=False` in the API. This wasn't a problem for brief responses, since both the lead-in and the followup questions are nested inside one key, which is missing in unrelated updates. But when storing them as separate top-level key we need to be able to unset the required fields using the schema required list. So instead of listing the lead-in question as required in the subschemas we add it to the main schema required list.

Allows property schema generators to return a schema addition to be merged with the main schema. This enables multiquestions to add allOf/oneOff subschemas for followup questions and modify the required list on the main schema. Changes the existing specialists anyOf/dependency additions to use merge_schemas.

Dynamic lists are serialized as an array, but other multiquestions aren't, so it makes sense to move the array part of the schema to the `_dynamic_list` function. And since we can no longer dispatch to the dynamic_list property based on the question type we need to remove it from the types mapping.

Fixes the schema accepting an answer for the follow up when the lead in question wasn't answered. Since the subschema without the follow-up doesn't list the lead-in question in required list we can still use page_questions without failing the oneOf.

Since checkboxes are answered with a list of strings some of which can have a followup we need a way to check that the question answer list contains a given value. There's no direct way to do this using JSON schema, but we can check whether the list of answers contains only the values that don't have a follow-up. So using that and the inverse of that schema we can separate answers that only have no-followup values and answers that contain something else, which we assume IS the follow-up value we wanted to check for (it could be some other invalid value, in which case the schema will still require an answer for the followup but the parent property schema will also flag the value as invalid).

This requires us to use `.get_question` on the multiquestion, but has a nice side-effect of validating that follow-up question IDs are correct and belong to the same multiquestion as the lead-in.

allait · 2017-02-17T11:29:37Z

@samuelhwilliams fair point, I've added a few tests

samuelhwilliams · 2017-02-17T11:35:05Z

Other than that, from local testing and reading through, looks good. 👍

allait requested a review from samuelhwilliams February 16, 2017 10:46

samuelhwilliams suggested changes Feb 16, 2017

View reviewed changes

samuelhwilliams reviewed Feb 16, 2017

View reviewed changes

allait force-pushed the follow-up-questions branch 2 times, most recently from 1bdb2bb to 8863942 Compare February 16, 2017 15:30

allait force-pushed the follow-up-questions branch 2 times, most recently from a676cdc to 292f9d9 Compare February 16, 2017 16:34

allait added 18 commits February 17, 2017 11:27

Bump content-loader to 3.0.0 to add new followup format

7d60950

Use new followup property format in existing questions

6cd6337

Update multiquestion schema generation tests

6ac5e07

Update question followup property schema and README

5727ba6

Allow optional followup questions to remain optional

e2d5ec7

This requires us to use `.get_question` on the multiquestion, but has a nice side-effect of validating that follow-up question IDs are correct and belong to the same multiquestion as the lead-in.

Add checkboxes to the list of types that allow followups

8704181

Add followup keys to sample G9 questions

f234932

Change merge_schemas exception to TypeError

880f663

Use .filter(..., dynamic=False) instead of static=True

eedb98f

Add tests for followup schema generation

87ff028

Bump version to 6.0.0

8d77435

allait force-pushed the follow-up-questions branch from 292f9d9 to 8d77435 Compare February 17, 2017 11:27

samuelhwilliams approved these changes Feb 17, 2017

View reviewed changes

allait merged commit 8c00adc into master Feb 17, 2017

allait deleted the follow-up-questions branch February 17, 2017 11:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow up questions #381

Follow up questions #381

allait commented Feb 16, 2017

samuelhwilliams Feb 16, 2017

samuelhwilliams Feb 16, 2017

allait commented Feb 16, 2017

samuelhwilliams commented Feb 17, 2017

allait commented Feb 17, 2017

samuelhwilliams commented Feb 17, 2017

Follow up questions #381

Follow up questions #381

Conversation

allait commented Feb 16, 2017

Split multiquestion schema generation

Don't add lead question to the oneOf subschema required

Let multiquestions extend main schema fields

Splits dynamic_list from nestd_multiquestion output

samuelhwilliams Feb 16, 2017

Choose a reason for hiding this comment

samuelhwilliams Feb 16, 2017

Choose a reason for hiding this comment

allait commented Feb 16, 2017

samuelhwilliams commented Feb 17, 2017

allait commented Feb 17, 2017

samuelhwilliams commented Feb 17, 2017