Payment Accrual Code Improvements #529
Conversation
There was a problem hiding this comment.
Left a few questions, generally otherwise concerned with how well tested this is, but if we are confident in our tests of this logic, i think it looks fine. Ideally we would be able to use modification dates to make sure we recalculate for anything that changed since the last calculation, but not sure how much more work that is.
| @@ -186,14 +187,19 @@ def get_missing_justification_message(visits_ids): | |||
| return f"Justification is required for flagged visits: {id_list}" | |||
|
|
|||
|
|
|||
| def update_payment_accrued(opportunity: Opportunity, users): | |||
| """Updates payment accrued for completed and approved CompletedWork instances.""" | |||
| def update_payment_accrued(opportunity: Opportunity, users, incremental=False): | |||
There was a problem hiding this comment.
nit: the other args have type hints, the last should too
| access_objects = OpportunityAccess.objects.filter(user__in=users, opportunity=opportunity, suspended=False) | ||
| completed_work_filter = ~Q(status=CompletedWorkStatus.rejected) |
There was a problem hiding this comment.
nit: this could probably be written with simple exclude and filter statements rather than fancy Q ones, which are a bit harder to interpret
| completed_work_filter = ~Q(status=CompletedWorkStatus.rejected) | ||
| if incremental: | ||
| completed_work_filter |= ~Q(status=CompletedWorkStatus.approved) | ||
| completed_work_filter &= Q(saved_approved_count=0) |
There was a problem hiding this comment.
if one with duplicates has the second approved, will this not account for that?
There was a problem hiding this comment.
The visit import code still runs non-incremental payment accrual updates. So if the status is updated for a duplicate visit, the payment accrual will update, taking the duplicate visit in the approved count.
| counts = defaultdict(lambda: {"completed": 0, "approved": 0}) | ||
|
|
||
| completed_works = completed_works.prefetch_related( | ||
| Prefetch("uservisit_set", queryset=UserVisit.objects.all()), "payment_unit" |
There was a problem hiding this comment.
why is the queryset necessary?
There was a problem hiding this comment.
This is redundant. I have removed the queryset since doing a preftech for uservisit set already does the same. Also removed the payment_unit prefetch here as that is also not needed.
f950ece
Product Description
update_statusto process bulk completed worksget_completed_work_countsto process bulk completed works, and get approved and completed countsapproved_count,completed_countandcompletedproperties withsaved_properties.update_payment_accruedto only update unprocessed completed works.Technical Summary
Ticket Link
Safety Assurance
Safety story
Automated test coverage
Labels & Review