[#112216471] Shove the logic for boolean_list questions into Content Loader
#248
Conversation
pcraig3
changed the title
|
You're seriously asking us to look at this? |
|
Wouldn't dream of it, Kev. |
pcraig3
force-pushed
the
pc_supplier_reply_responses
branch
from
a4a9f24 to
8aa5355
Compare
Adds the capability to inject brief data into `boolean_list` questions, and then, afterwards, to return the correct error messages for each individual `boolean_list` question by asking for error messages from `ContentQuestionSummary` objects rather than `ContentQuestions` themselves. There's kind of a depenency in that `boolean_list` question error messages expect brief data to have been added to the question before the errors are generated, but nothing that would result in a fatal error.
pcraig3
force-pushed
the
pc_supplier_reply_responses
branch
from
8aa5355 to
ec4aec4
Compare
Order of messages is important for the new error message stuff. Seems harmless to use ordered dictionaries everywhere.
Also updated CHANGELOG detailing breaking change.
pcraig3
force-pushed
the
pc_supplier_reply_responses
branch
from
ec4aec4 to
b462162
Compare
pcraig3
changed the title
boolean_list questions into Content Loader
|
|
| @@ -319,6 +318,10 @@ def get_question_by_slug(self, question_slug): | |||
| if question.get('slug') == question_slug: | |||
| return question | |||
|
|
|||
| def inject_brief_questions_into_boolean_list_question(self, brief): | |||
There was a problem hiding this comment.
Not sure if this name should be more generic - we could in theory have boolean lists that aren't to do with briefs?
e.g. inject_list_of_questions_into_boolean_list or similar. (In which case the parameter would be something more generic than brief too).
There was a problem hiding this comment.
Yeah, I tend to agree with your final comment. We only need these for briefs, so with our really specific method naming it's obvious what they're doing.
|
Nice work on the test coverage. I'm not sure about the naming stuff - maybe just stick with briefs for now as it makes it clear what the methods expect and what it's all used for, and it can be made generic if it's ever re-used for something else? Gets a 👍 from me, but would be nice to have someone else look it over too I reckon. |
[#112216471] Shove the logic for `boolean_list` questions into Content Loader
After a few false starts on how to handle the
boolean_list_questiontype, this seems to me to be the neatest way to get all the non-reusable code into the ContentLoader.Two main things: (1) adding brief questions to the section data and (2) getting the right error messages.
.inject_brief_questions_into_boolean_list_question()methods to both theContentSectionand theContentQuestionis a fairly straightforward way to do this..get_error_messages()on aContentQuestionSummaryreturns the right error messages for boolean listsself.idkey is returned by API's validation messagesself.type == 'boolean_list'boolean_list_questionskey existsLogic in the supplier app is wayyy simpler.
Wrote the tests. Should be good to go.