-
Notifications
You must be signed in to change notification settings - Fork 10
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
141882981-Add-question-type-to-content-loader-and-summary-display #36
Changes from 13 commits
8c7825a
fc68dab
3ba2e4e
aead0f9
2c1aa52
7753666
dcefe3f
e9d4db2
a5bc981
ea3ac84
c9e96d2
7d7502b
f687cd5
43a8424
19f0a52
958d24a
ebb7e95
fc377ed
9294be4
1ac9929
359618c
08191f9
85a23f2
26da03e
d221162
9165547
1ed993c
666e986
c2f8b4d
c5dae54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,7 @@ | |
from werkzeug.datastructures import ImmutableMultiDict | ||
|
||
from .errors import ContentNotFoundError, QuestionNotFoundError | ||
from .questions import Question, ContentQuestion | ||
from .questions import Question, ContentQuestion, Date | ||
from .messages import ContentMessage | ||
from .utils import TemplateField, template_all, drop_followups | ||
|
||
|
@@ -353,7 +353,15 @@ def get_error_messages(self, errors, question_descriptor_from="label"): | |
|
||
return errors_map | ||
|
||
@staticmethod | ||
def unformat_assurance(key, data): | ||
return { | ||
key + '--assurance': data[key].get('assurance', None), | ||
key: data[key].get('value', None) | ||
} | ||
|
||
def unformat_data(self, data): | ||
|
||
"""Unpack assurance information to be used in a form | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we do decide to stick with this solution then documentation for this function needs updating. Also is it worth adding a test to cover There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doc block top line doesn't feel correct anymore. |
||
|
||
:param data: the service data as returned from the data API | ||
|
@@ -372,10 +380,16 @@ def unformat_data(self, data): | |
result = {} | ||
for key in data: | ||
if self._has_assurance(key): | ||
result[key + '--assurance'] = data[key].get('assurance', None) | ||
result[key] = data[key].get('value', None) | ||
# If it's an assurance question do the unformatting here. | ||
result.update(self.unformat_assurance(key, data)) | ||
else: | ||
result[key] = data[key] | ||
question = self.get_question(key) | ||
if question: | ||
# Otherwise if it is a legitimate question use the unformat method on the question. | ||
result.update(question.unformat_data(data)) | ||
else: | ||
# Otherwise it's not a question, default to returning the k: v pair. | ||
result[key] = data[key] | ||
return result | ||
|
||
def get_question(self, field_name): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,8 @@ | ||
from collections import OrderedDict, defaultdict | ||
from datetime import datetime | ||
|
||
from dmutils.formats import DATE_FORMAT, DISPLAY_DATE_FORMAT | ||
|
||
from .converters import convert_to_boolean, convert_to_number | ||
from .errors import ContentNotFoundError | ||
from .formats import format_price | ||
|
@@ -96,7 +100,6 @@ def get_error_messages(self, errors, question_descriptor_from="label"): | |
for field_name in sorted(error_fields): | ||
question = self.get_question(field_name) | ||
message_key = errors[field_name] | ||
|
||
validation_message = question.get_error_message(message_key, field_name) | ||
|
||
error_key = question.id | ||
|
@@ -182,6 +185,10 @@ def inject_brief_questions_into_boolean_list_question(self, brief): | |
def has_assurance(self): | ||
return True if self.get('assuranceApproach') else False | ||
|
||
@property | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we still need this? |
||
def is_date(self): | ||
return False | ||
|
||
def get_question_ids(self, type=None): | ||
return [self.id] if type in [self.type, None] else [] | ||
|
||
|
@@ -544,6 +551,43 @@ def update_expected_values(options, parents, expected_set): | |
return expected_values - selected_values_set | ||
|
||
|
||
class Date(Question): | ||
"""Class used as an interface for date data between forms, backend and summary pages.""" | ||
|
||
FIELDS = ('year', 'month', 'day') | ||
|
||
@property | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto here, is this now redundant? |
||
def is_date(self): | ||
return True | ||
|
||
def summary(self, service_data): | ||
return DateSummary(self, service_data) | ||
|
||
def get_data(self, form_data): | ||
"""Retreive the fields from the POST data (form_data). | ||
|
||
The d, m, y should be in the post as 'questionName-day', questionName-month ... | ||
Extract them and format as '\d\d\d\d-\d{1,2}-\d{1,2}'. | ||
https://code.tutsplus.com/tutorials/validating-data-with-json-schema-part-1--cms-25343 | ||
""" | ||
parts = [] | ||
for key in self.FIELDS: | ||
identifier = '-'.join([self.id, key]) | ||
value = form_data.get(identifier, '').replace('-', '').strip() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should have a test for the stripping |
||
parts.append(value or '') | ||
|
||
return {self.id: '-'.join(parts) if any(parts) else None} | ||
|
||
def unformat_data(self, data): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have any tests for this method? |
||
|
||
result = {} | ||
value = data[self.id] | ||
if data[self.id]: | ||
for partial_value, field in zip(value.split('-'), self.FIELDS): | ||
result['-'.join([self.id, field])] = partial_value | ||
return result | ||
|
||
|
||
class QuestionSummary(Question): | ||
def __init__(self, question, service_data): | ||
self.number = question.number | ||
|
@@ -626,6 +670,22 @@ def answer_required(self): | |
return self.is_empty | ||
|
||
|
||
class DateSummary(QuestionSummary): | ||
|
||
def __init__(self, question, service_data): | ||
super(DateSummary, self).__init__(question, service_data) | ||
self._value = self._service_data.get(self.id, '') | ||
|
||
@property | ||
def value(self): | ||
try: | ||
return datetime.strptime(self._value, DATE_FORMAT).strftime(DISPLAY_DATE_FORMAT) | ||
except ValueError: | ||
# We may need to fall back to displaying a plain string value in the case of briefs in draft before | ||
# the date field was introduced. | ||
return self._value | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be nice to include a very short comment explaining that we are falling back to original date format |
||
|
||
|
||
class MultiquestionSummary(QuestionSummary, Multiquestion): | ||
def __init__(self, question, service_data): | ||
super(MultiquestionSummary, self).__init__(question, service_data) | ||
|
@@ -758,6 +818,7 @@ def _get_options_recursive(options): | |
'list': List, | ||
'checkboxes': List, | ||
'checkbox_tree': Hierarchy, | ||
'date': Date | ||
} | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
[pep8] | ||
exclude = ./venv | ||
exclude = ./venv* | ||
max-line-length = 120 | ||
|
||
[pytest] | ||
norecursedirs = venv | ||
norecursedirs = venv* |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,5 +26,8 @@ | |
'Werkzeug==0.11.9', | ||
'inflection==0.3.1', | ||
'six==1.10.0' | ||
], | ||
dependency_links=[ | ||
'git+https://github.com/alphagov/[email protected]#egg=digitalmarketplace-utils==25.0.1' | ||
] | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -168,6 +168,45 @@ def test_get_question_ids_by_type(self): | |
assert self.question().get_question_ids(type='text') == ['example'] | ||
|
||
|
||
class TestDates(QuestionTest): | ||
def question(self, **kwargs): | ||
data = { | ||
"id": "example", | ||
"type": "date" | ||
} | ||
data.update(kwargs) | ||
return ContentQuestion(data) | ||
|
||
def test_get_data(self): | ||
assert self.question().get_data({ | ||
'example-day': '19', | ||
'example-month': '03', | ||
'example-year': '2017', | ||
}) == {'example': '2017-03-19'} | ||
|
||
def test_get_data_with_blank_data(self): | ||
assert self.question().get_data({ | ||
'example-day': '19', | ||
'example-month': '03', | ||
}) == {'example': '-03-19'} | ||
|
||
assert self.question().get_data({ | ||
'example-day': '19', | ||
'example-month': '03', | ||
'example-year': ' ', | ||
}) == {'example': '-03-19'} | ||
|
||
def test_get_data_with_blank_year(self): | ||
assert self.question().get_data({ | ||
'example-day': '19', | ||
'example-month': '03', | ||
'example-year': '', | ||
}) == {'example': None} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we expect this to be |
||
|
||
def test_get_data_unknown_key(self): | ||
assert self.question().get_data({'other': 'other value'}) == {'example': None} | ||
|
||
|
||
class TestBoolean(QuestionTest): | ||
def question(self, **kwargs): | ||
data = { | ||
|
@@ -676,6 +715,46 @@ def test_is_empty_optional_question(self): | |
assert question.is_empty | ||
|
||
|
||
class TestDateSummary(QuestionSummaryTest): | ||
|
||
def question(self, **kwargs): | ||
data = { | ||
"id": "example", | ||
"type": "date" | ||
} | ||
data.update(kwargs) | ||
|
||
return ContentQuestion(data) | ||
|
||
def test_date_is_formatted_into_user_friendly_format(self): | ||
question = self.question().summary({'example': '2016-02-18'}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worth having a test for where we don't include 0 padding i.e. Either we are saving the month input directly in which case we may see data like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, the test below now addresses this. |
||
assert question.value == 'Thursday 18 February 2016' | ||
|
||
def test_unpadded_date_is_formatted_into_user_friendly_format(self): | ||
question = self.question().summary({'example': '2003-2-1'}) | ||
assert question.value == 'Saturday 1 February 2003' | ||
|
||
def test_not_a_date_format_falls_back_to_raw_string(self): | ||
non_date_string = 'not-a-date-formatted-string' | ||
question = self.question().summary({'example': non_date_string}) | ||
|
||
assert question.value == non_date_string | ||
|
||
def test_answer_not_required_if_non_empty_string_exists(self): | ||
question = self.question().summary({'example': 'a string'}) | ||
assert not question.answer_required | ||
|
||
question = self.question().summary({'example': '2016-02-18'}) | ||
assert not question.answer_required | ||
|
||
def test_is_not_empty_if_not_empty_string_exists(self): | ||
question = self.question().summary({'example': 'a string'}) | ||
assert not question.is_empty | ||
|
||
question = self.question().summary({'example': '2016-02-18'}) | ||
assert not question.is_empty | ||
|
||
|
||
class TestTextSummary(QuestionSummaryTest): | ||
def question(self, **kwargs): | ||
data = { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
Date
potentially an unused import?