1871: Cleaned status code in model #882
Conversation
There was a problem hiding this comment.
The tests are failing because cases/models.py contains conflicts:
As there is no information in the Trello card or PR explaining what this change is trying to achieve, I cannot tell if the code changes have worked.
Also, there need to be new automated tests to cover the new logic.
| if self.no_contact_four_week_chaser_sent_date is not None: | ||
| return self.no_contact_four_week_chaser_sent_date + timedelta(days=ONE_WEEK_IN_DAYS) | ||
|
|
||
| return date(1970, 1, 1) |
There was a problem hiding this comment.
The value returned by this function may be returned by next_action_due_date below which itself can return None. I don't know why this function returns a magic date instead of None.
| return ( | ||
| self.case.enforcement_body_pursuing == Case.EnforcementBodyPursuing.YES_IN_PROGRESS | ||
| or self.case.enforcement_body_closed_case == Case.EnforcementBodyClosedCase.IN_PROGRESS | ||
| ) |
There was a problem hiding this comment.
I don't see the benefit of turning if statement conditions into functions which return booleans. Unless they are being reused, which is not the case here.
There was a problem hiding this comment.
It splits up the code into something easier to understand but there is also the vague idea to use these in other ways
There was a problem hiding this comment.
The problem is that I think it makes the code more difficult to understand. These conditions make no sense in isolation, they only identify a Client status when they occur at specific positions inside the chain of if...elifs.
There was a problem hiding this comment.
I hadn't noticed that you had amended some of the conditions to calculate when statuses had been completed for reuse on the Status overflow page. I am not sure that the needs of that page should be driving how Case status itself is calculated. It feels a bit like the tail wagging the dog!
There was a problem hiding this comment.
The Unit Tests are still failing. You haven't written new unit tests for each of your new methods.
The rule of thumb I have followed when developing the platform is that any bespoke code needs a specific test added to the unit test suite. Also, this provides a degree of self documenting code. The tests should illustrate the set of expected behaviours.
|
|
||
| if ( | ||
| self.status.status == CaseStatus.Status.REPORT_READY_TO_SEND | ||
| and self.contact_details_found == Case.ContactDetailsFound.NOT_FOUND | ||
| ): | ||
| if ( |
There was a problem hiding this comment.
Case.contact_details_found is never updated in the platform. Its default value is ContactDetailsFound.NOT_CHECKED so the above condition is never met.
This field is also checked in notifications/utils.py when looking for overdue Cases. This is the cause of the original issue reported by the user.
If this pull request is not seeking to address the issue then is there another Trello card somewhere to do so?
|
|
||
| for key, status in status_dict.items(): | ||
| if status is False: | ||
| return key |
There was a problem hiding this comment.
I hadn't noticed that the status is returned when for the first _status_complete check which fails.
That being the case, it is not obvious how to make the code more readable.
No description provided.