feat: patient registration with expiry period#1043
Open
akurungadam wants to merge 6 commits into
Open
Conversation
WalkthroughA new 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
healthcare/healthcare/doctype/patient/patient.js (1)
25-34: 🧹 Nitpick | 🔵 TrivialConsider coercing checkbox defaults for defensive programming.
frappe.defaults.get_default('collect_registration_fee')may return string values like"0"depending on the Frappe version and serialization behavior. String"0"is truthy in JavaScript, which could cause the wrong button to display. Usecint()to safely normalize the value before branching.Note: Similar patterns exist elsewhere in the codebase (
sample_collection.js:56,lab_test.js:56) without coercion, suggesting this may be version-dependent. However, defensive coercion here is a safe improvement.Suggested coercion
if (frm.doc.status == 'Disabled') { - if (frappe.defaults.get_default('collect_registration_fee')) { + if (cint(frappe.defaults.get_default('collect_registration_fee'))) { frm.add_custom_button(__('Invoice Patient Registration'), function () { invoice_registration(frm); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@healthcare/healthcare/doctype/patient/patient.js` around lines 25 - 34, The condition checking frappe.defaults.get_default('collect_registration_fee') is vulnerable to string-to-boolean coercion issues, where the string "0" would evaluate as truthy and cause the wrong button to display. Wrap the frappe.defaults.get_default('collect_registration_fee') call with cint() to safely convert the value to a proper integer before the conditional evaluation, ensuring reliable branching logic regardless of the Frappe version's serialization behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.json`:
- Around line 108-116: The registration_validity field in
healthcare_settings.json is gated behind the collect_registration_fee setting
using the depends_on condition with "eval:doc.collect_registration_fee == 1".
Remove the depends_on line entirely from the registration_validity field
definition so that administrators can configure registration expiry
independently of whether fee collection is enabled, allowing registration
validity to be configured for all registration flows regardless of fee
collection status.
In `@healthcare/healthcare/doctype/patient_registration/patient_registration.py`:
- Around line 64-69: The function disable_patient_without_active_registration is
checking for the existence of any Active registration status without validating
whether that registration is currently valid (checking if valid_till date hasn't
passed). Modify the frappe.db.exists query to include an additional condition
that verifies the valid_till field is greater than or equal to today's date,
ensuring only registrations with valid dates prevent the patient from being
disabled. This prevents stale Active registrations from keeping a patient
enabled when their registration has actually expired.
In
`@healthcare/healthcare/doctype/patient_registration/test_patient_registration.py`:
- Around line 19-20: The get_patient() method directly indexes [0] on the
frappe.get_list result without checking if the list is empty, which causes an
IndexError when no Patient records exist in the test database. Add a guard check
before indexing into the result list to either return None, raise a meaningful
error, or use a safer access pattern (like next(iter(...), None)) to handle the
empty case gracefully.
In `@healthcare/healthcare/doctype/patient/patient.py`:
- Around line 25-27: The `renew_registration` method in the Patient doctype
needs server-side validation guards to prevent bypassing fee/no-fee split logic
and to enforce renewal eligibility. Add checks within the `renew_registration`
whitelisted method to: verify that when `collect_registration_fee` is enabled,
the appropriate fee collection flow is enforced (preventing free registrations),
and validate that a disabled patient does not get re-enabled if they already
have a current active registration. These guards should run before any patient
reactivation logic to ensure renewal eligibility is properly enforced at the
server level.
---
Nitpick comments:
In `@healthcare/healthcare/doctype/patient/patient.js`:
- Around line 25-34: The condition checking
frappe.defaults.get_default('collect_registration_fee') is vulnerable to
string-to-boolean coercion issues, where the string "0" would evaluate as truthy
and cause the wrong button to display. Wrap the
frappe.defaults.get_default('collect_registration_fee') call with cint() to
safely convert the value to a proper integer before the conditional evaluation,
ensuring reliable branching logic regardless of the Frappe version's
serialization behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a992b9fa-e440-46a3-b113-f511c423ca19
📒 Files selected for processing (12)
healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.jsonhealthcare/healthcare/doctype/patient/patient.jshealthcare/healthcare/doctype/patient/patient.pyhealthcare/healthcare/doctype/patient_registration/__init__.pyhealthcare/healthcare/doctype/patient_registration/patient_registration.jshealthcare/healthcare/doctype/patient_registration/patient_registration.jsonhealthcare/healthcare/doctype/patient_registration/patient_registration.pyhealthcare/healthcare/doctype/patient_registration/test_patient_registration.pyhealthcare/healthcare/utils.pyhealthcare/hooks.pyhealthcare/patches.txthealthcare/patches/v16_0/create_registration_for_existing_patients.py
Comment on lines
+25
to
+27
| from healthcare.healthcare.doctype.patient_registration.patient_registration import ( | ||
| create_patient_registration, | ||
| ) |
There was a problem hiding this comment.
Enforce renewal eligibility in the whitelisted method.
renew_registration is callable directly, so the client-only fee/no-fee split can be bypassed. When collect_registration_fee is enabled, this can create a free registration and reactivate the patient; also avoid re-enabling a disabled patient that already has a current active registration.
Suggested server-side guard
from healthcare.healthcare.doctype.patient_registration.patient_registration import (
create_patient_registration,
+ has_active_patient_registration,
) `@frappe.whitelist`()
def renew_registration(self):
"""Create a fresh registration and re-enable the patient (when no fee is collected)."""
- if not frappe.db.exists("Patient Registration", {"patient": self.name, "status": "Active"}):
- create_patient_registration(self.name)
+ if frappe.db.get_single_value("Healthcare Settings", "collect_registration_fee"):
+ frappe.throw(_("Invoice the patient registration fee before renewing this registration."))
+ if has_active_patient_registration(self.name):
+ frappe.throw(_("Patient already has an active registration."))
+ create_patient_registration(self.name)
self.db_set("status", "Active")Also applies to: 186-191
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@healthcare/healthcare/doctype/patient/patient.py` around lines 25 - 27, The
`renew_registration` method in the Patient doctype needs server-side validation
guards to prevent bypassing fee/no-fee split logic and to enforce renewal
eligibility. Add checks within the `renew_registration` whitelisted method to:
verify that when `collect_registration_fee` is enabled, the appropriate fee
collection flow is enforced (preventing free registrations), and validate that a
disabled patient does not get re-enabled if they already have a current active
registration. These guards should run before any patient reactivation logic to
ensure renewal eligibility is properly enforced at the server level.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
docs