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.

Sign up for GitHub

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

Jump to bottom

Comments and simplifications. #531

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Comments and simplifications. #531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5c5da7
MeasureCondition: Add docsting and simplify description andcondition_…
stuaxo Apr 7, 2022
9f53877
In common override post() instead of allowing form_valid to call form…
stuaxo Apr 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions common/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,11 +347,19 @@ def form_violates(self, form) -> bool:

return violations

def form_valid(self, form):
if self.form_violates(form):
return self.form_invalid(form)
def post(self, request, *args, **kwargs):
"""
Handle POST requests: instantiate a form instance with the passed
POST variables and then check if it's valid.

return super().form_valid(form)
Override the default form .post() method to check business rules
when the form is otherwise valid.
"""
form = self.get_form()
if form.is_valid() and not self.form_violates(form):
return self.form_valid(form)
else:
return self.form_invalid(form)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So instead of overriding form_valid (which also calls form_invalid) on TrackedModelChangeView should we instead override the post call do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably - at the moment I was getting a load of test failures with this change, I need to get past.

Though, this may be another issue as since I re-setup my env again, I noticed I am getting them in plain master now :(



class TrackedModelChangeView(
Expand Down
37 changes: 24 additions & 13 deletions measures/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,11 +804,14 @@ def is_certificate_required(self):

@property
def description(self) -> str:
out: list[str] = []
"""
Human-readable description built from associated actions and conditions.

out.append(
f"Condition of type {self.condition_code.code} - {self.condition_code.description}",
)
:dependencies: with_reference_price_string must be called in queries using this property.
"""
out: list[str] = [
f"Condition of type {self.condition_code.code} - {self.condition_code.description}"
]

if self.required_certificate:
out.append(
Expand All @@ -829,29 +832,37 @@ def description(self) -> str:

@property
def condition_string(self) -> str:
out: list[str] = []
"""
Returns a string representation of the component conditions.

components = self.components.latest_approved()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current() would be better, to take account of any draft data.

This is the duty_sentence, if there is one measure, measure_type and additional_code or
if there are more than one measure_type and additional_code.

:dependencies: with_duty_sentence must be called in queries using this property.
"""
Comment on lines 834 to +842
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this method even used anywhere? I've never really understood what it is meant to be showing?

measures: set[str] = set()
measure_types: set[str] = set()
additional_codes: set[str] = set()

for mcc in components:
measures.add(mcc.condition.dependent_measure.sid)
measure_types.add(mcc.condition.dependent_measure.measure_type.sid)
if mcc.condition.dependent_measure.additional_code:
for mcc in self.components.latest_approved():
# TODO ^ select_related condition_measure, condition_measure.measure_type ?
condition_measure = mcc.condition.dependent_measure

measures.add(condition_measure.sid)
measure_types.add(condition_measure.measure_type.sid)
if condition_measure.additional_code:
additional_codes.add(
mcc.condition.dependent_measure.additional_code.sid,
condition_measure.additional_code.sid,
)

if (
len(measures) == len(measure_types) == len(additional_codes) == 1
or len(measure_types) > 1
or len(additional_codes) > 1
):
out.append(self.duty_sentence)
return self.duty_sentence

return "".join(out)
return ""


class MeasureConditionComponent(TrackedModel):
Expand Down