Over Limit User Visit Alerts#962
Conversation
zandre-eng
requested review from
Charl1996,
ajeety4,
calellowitz,
hemant10yadav,
pxwxnvermx and
sravfeyn
WalkthroughAdds an OVER_LIMIT flag to flags enums, a UserVisit.has_over_limit_flag property, and appends an over_limit validation flag during form processing. Introduces a flag-badges template and updates table rendering to use it, adjusting icon/status logic to include over-limit. Adds an over-limit warning block to the user visit details template. Tests updated to assert the over-limit flag is present on over-limit visits. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Per the discussion on ticket, I see that we want to display this message when a visit is in 'over-limit' status, but not once it's approved/rejected, right? However this PR seems to display the message even after its approved/rejected? I am a bit worried about the status being in multiple places on model resulting in further confusion in future. There's currently |
| models.Index(fields=["opportunity", "status"]), | ||
| ] | ||
|
|
||
| def save(self, *args, **kwargs): |
There was a problem hiding this comment.
Can you clarify the reasoning behind overriding the save method? As far as I know, only the form-processing code is responsible for marking a visit as “over-limit.” If that’s the case, do we really need to override save? Would it be cleaner to keep this logic in the same place where we mark a visit as over-limit?
There was a problem hiding this comment.
My reasoning was simply to have a catch-all for setting the flag. If we ever extend the code in the future to set the status as over-limit somewhere else then we don't have to worry about accidentally forgetting to also mark the over-limit flag.
If the above feels overkill and you think we'll only ever mark a visit as "over-limit" in the form processor then I'm happy to remove this save() override.
There was a problem hiding this comment.
I ended up changing my approach to use flags, and have decided to remove the save() override altogether (cc6480d).
hemant10yadav
left a comment
hemant10yadav
left a comment
There was a problem hiding this comment.
Thank you for addressing this. I have a small question and one change that needs to be made. I believe we need to update the over-limit condition in render_icons, as well as the flags column logic in the User Visit table.
| status = models.CharField( | ||
| max_length=50, choices=VisitValidationStatus.choices, default=VisitValidationStatus.pending | ||
| ) | ||
| is_over_limit = models.BooleanField(default=False) |
There was a problem hiding this comment.
Instead of a new field we can also use the existing flag_reason column and introduce a new over_limit flag. What do you think about this? Are there any possible caveats to this approach?
There was a problem hiding this comment.
The only caveat I can think of is that it could increase complexity a little, since we will no longer be looking at a boolean field but will now need to parse the flag_reason JSON field. But, I can see the benefit in keeping things more organized.
Let me look into this.
There was a problem hiding this comment.
I've pushed a few commits to utilise flags instead of a separate field. Please let me know if there are any concerns with this approach.
There was a problem hiding this comment.
I believe we currently treat over-limit as a status rather than a flag. If you look at the Deliver tab filters, Over-Limit and Flagged are two distinct filters, and the same separation exists in the User Visit table filters. With this change, those two concepts would effectively converge — is that the intended outcome?
At the same time, we display over-limit as a flag in the visit table, which introduces some inconsistency and potential confusion from both a UX and data-model perspective. I think we should first align on how we want to conceptually represent “over-limit” — whether it should be treated as a status or purely as a flag — before moving forward with the change.
That said, the earlier approach you implemented aligns well with our current setup. However, the final decision should depend on how we want to define and handle the over-limit concept going forward.
| {% else %} | ||
| <p class="text-sm"> | ||
| {% blocktranslate %} | ||
| This visit has been flagged as over-limit and should not be approved. |
There was a problem hiding this comment.
Should we disable the approve button in this case as well?
There was a problem hiding this comment.
Not yet. We've added this to the ideas board for further thought, but right now we don't want to block a core workflow.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@commcare_connect/opportunity/tables.py`:
- Around line 950-955: The bug is that code appends
VisitValidationStatus.over_limit.name (a string) to the status list but
get_icons() and status_meta expect enum members as keys; update the branch that
currently appends VisitValidationStatus.over_limit.name to append the enum
member VisitValidationStatus.over_limit instead so the key matches
status_meta/get_icons() (look for the conditional around
record.review_status/record.status/record.has_over_limit_flag and replace the
.name append with the enum member).
In `@commcare_connect/templates/opportunity/partials/user_visit_flag_badges.html`:
- Around line 6-15: The template's comparison flag == "duplicate" is
case-sensitive and won't match values returned by FlagLabels like "Duplicate";
update the conditional in the user_visit_flag_badges.html template to compare in
a case-insensitive way (e.g., use flag|lower == "duplicate") or compare against
the exact "Duplicate" string so the duplicate flag correctly receives the
warning-light badge; adjust the conditional around the span rendering for the
flag variable used in the for-loop (flags, flag).
🧹 Nitpick comments (1)
commcare_connect/form_receiver/processor.py (1)
258-259: Consider usingFlagDescription.OVER_LIMIT.valuefor consistency.The hardcoded message "User visit exceeds allowed limit." differs slightly from
FlagDescription.OVER_LIMITwhich is "Visit exceeds allowed limit." While other flags in this file also use hardcoded messages, using the enum value would ensure consistency across the codebase.That said, the logic correctly appends the over_limit flag when the visit status warrants it.
♻️ Optional: Use enum for consistency
+from commcare_connect.utils.flags import FlagDescription + if user_visit.status == VisitValidationStatus.over_limit: - flags.append(["over_limit", "User visit exceeds allowed limit."]) + flags.append(["over_limit", FlagDescription.OVER_LIMIT.value])
I've tweaked the implementation a bit so that when a user visit is approved by PM or rejected by NM then the message will not be shown.
I have changed my approach so that we don't use an additional model field anymore, but make use of the existing flags column instead. Let me know if there are any concerns/questions regarding this. |
|
Hi @zandre-eng , I was reading through the ticket and the Slack conversations, and I was wondering whether we can now utilise the status audit history to determine if a visit was over-limit at some stage, given that the status history functionality has now been implemented. I do like the idea of having this as a flag, though. However, since it is currently used as a status field, it becomes a bit confusing and duplicates it. |
6 participants
Product Description
If a user visit has ever had an over-limit status then the following message will be shown in the user visit details pane. This message will not be shown if the user visit has either been accepted by the PM or rejected by the NM:
Program Manager:
Network Manager:
Users will now correctly see the over-limit flag name and icon for user visits that were marked as over-limit:
The over-limit flag will now also be shown with the rest of the flags in the user visit description:
It should be noted that the above only applies to new incoming user visits that have an over-limit status. Any pre-existing user visits that have the over-limit status will not receive the flag unless a separate manual migration is done.
Technical Summary
Link to ticket here.
This PR adds a new
over_limitflag which gets saved toUserVisit.flag_reasonif the incoming user visit is marked as over limit. The purpose of this flag is to be able to correctly show the over limit status of a user visit, even after its status has been changed.This PR also addresses a pre-existing bug where duplicate flag names were not showing their correct colour. This is because the check for a duplicate flag name had mismatched casing.
Safety Assurance
Safety story
Automated test coverage
Unit tests exist.
QA Plan
No QA planned.
Labels & Review