Feat: Combine Registration Fee with Consultation Fee for New Patient Appointment Billing#947
Open
md-umair-21 wants to merge 9 commits into
Open
Feat: Combine Registration Fee with Consultation Fee for New Patient Appointment Billing#947md-umair-21 wants to merge 9 commits into
md-umair-21 wants to merge 9 commits into
Conversation
…appointment billing
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds registration-fee support across the healthcare app. Three new Healthcare Settings fields were added: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
healthcare/healthcare/doctype/patient_appointment/patient_appointment.js (1)
1386-1541:⚠️ Potential issue | 🟡 MinorPipeline failure: Prettier formatting check failed.
The linter reports that files were modified by the Prettier hook. Run
pre-commit run --all-filesorprettier --writeto fix code style issues and re-commit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.js` around lines 1386 - 1541, Prettier formatting changed this file; run the formatter and re-commit to satisfy the pre-commit hook: open the patient_appointment.js (function show_payment_dialog and its nested validate_discount) and run the project formatter (e.g., prettier --write or pre-commit run --all-files) to fix styling (spacing, indentation, trailing commas/quotes) so the diff only includes intended logic changes, then re-stage and push the formatted file.healthcare/healthcare/doctype/patient_appointment/patient_appointment.py (1)
1-1:⚠️ Potential issue | 🟡 MinorPipeline failure: Ruff-format reformatted the file.
Run
pre-commit run --all-filesto apply formatting fixes and re-commit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.py` at line 1, Ruff reformatted patient_appointment.py; to fix the pipeline failure run the project's formatter/pre-commit hooks (e.g., run `pre-commit run --all-files` or `ruff --fix` against the patient_appointment.py module) and re-commit the changes so the file matches the repository's formatting rules.
🧹 Nitpick comments (2)
healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.json (2)
516-538: Consider disambiguating field labels to avoid user confusion.The new fields
reg_itemandreg_feehave identical labels ("Registration Item" and "Registration Fee") to the existing fieldsregistration_itemandregistration_fee. This could confuse administrators configuring settings since both sets appear in the same form.Consider using more distinctive labels like:
- "Consultation Registration Item" and "Consultation Registration Fee", or
- "New Patient Registration Item" and "New Patient Registration Fee"
💡 Suggested label changes
{ "depends_on": "eval:doc.include_registration_fee==1", "fieldname": "reg_item", "fieldtype": "Link", "in_list_view": 1, - "label": "Registration Item", + "label": "New Patient Registration Item", "mandatory_depends_on": "eval:doc.include_registration_fee==1", "options": "Item" }, { "depends_on": "eval:doc.include_registration_fee==1", "fieldname": "reg_fee", "fieldtype": "Currency", - "label": "Registration Fee", + "label": "New Patient Registration Fee", "mandatory_depends_on": "eval:doc.include_registration_fee==1" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.json` around lines 516 - 538, The labels for the new fields reg_item and reg_fee in healthcare_settings.json collide with existing registration_item and registration_fee labels and can confuse admins; update the "label" values for reg_item and reg_fee to be more distinctive (e.g., "New Patient Registration Item" / "New Patient Registration Fee" or "Consultation Registration Item" / "Consultation Registration Fee") so they are clearly differentiated from registration_item and registration_fee throughout the settings form.
523-531: Remove unnecessaryin_list_viewproperty.The
in_list_view: 1property at line 527 is typically used for child table fields to control visibility in list views. For fields in a Single DocType like Healthcare Settings, this property has no effect and should be removed.🧹 Proposed fix
{ "depends_on": "eval:doc.include_registration_fee==1", "fieldname": "reg_item", "fieldtype": "Link", - "in_list_view": 1, "label": "Registration Item", "mandatory_depends_on": "eval:doc.include_registration_fee==1", "options": "Item" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.json` around lines 523 - 531, The field definition for reg_item in the Healthcare Settings JSON includes an unnecessary "in_list_view": 1 property; remove that property from the reg_item field object (the block with "fieldname": "reg_item", "fieldtype": "Link", "label": "Registration Item", "options": "Item", and the depends/mandatory_depends expressions referencing doc.include_registration_fee) so the field retains the same behavior without the redundant in_list_view key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.py`:
- Around line 696-716: The current check_is_new_patient_by_invoice function
matches invoices by patient ID and additionally by patient_name AND mobile which
can cause false negatives (different patients with same name+mobile). Remove the
secondary matching condition "(p.patient_name = %(patient_name)s and
ifnull(p.mobile, '') = %(mobile)s)" from match_conditions so the query only
checks si.patient, or if cross-patient matching was intentional, add a clear
docstring and inline comment on check_is_new_patient_by_invoice explaining this
behavior and its implications; optionally refactor the query to use frappe.qb
(Sales Invoice and Patient DocTypes) for clarity and to avoid manual SQL string
assembly.
---
Outside diff comments:
In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.js`:
- Around line 1386-1541: Prettier formatting changed this file; run the
formatter and re-commit to satisfy the pre-commit hook: open the
patient_appointment.js (function show_payment_dialog and its nested
validate_discount) and run the project formatter (e.g., prettier --write or
pre-commit run --all-files) to fix styling (spacing, indentation, trailing
commas/quotes) so the diff only includes intended logic changes, then re-stage
and push the formatted file.
In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.py`:
- Line 1: Ruff reformatted patient_appointment.py; to fix the pipeline failure
run the project's formatter/pre-commit hooks (e.g., run `pre-commit run
--all-files` or `ruff --fix` against the patient_appointment.py module) and
re-commit the changes so the file matches the repository's formatting rules.
---
Nitpick comments:
In `@healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.json`:
- Around line 516-538: The labels for the new fields reg_item and reg_fee in
healthcare_settings.json collide with existing registration_item and
registration_fee labels and can confuse admins; update the "label" values for
reg_item and reg_fee to be more distinctive (e.g., "New Patient Registration
Item" / "New Patient Registration Fee" or "Consultation Registration Item" /
"Consultation Registration Fee") so they are clearly differentiated from
registration_item and registration_fee throughout the settings form.
- Around line 523-531: The field definition for reg_item in the Healthcare
Settings JSON includes an unnecessary "in_list_view": 1 property; remove that
property from the reg_item field object (the block with "fieldname": "reg_item",
"fieldtype": "Link", "label": "Registration Item", "options": "Item", and the
depends/mandatory_depends expressions referencing doc.include_registration_fee)
so the field retains the same behavior without the redundant in_list_view key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: be3a0567-038c-415d-821e-a7c1dd67651b
📒 Files selected for processing (3)
healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.jsonhealthcare/healthcare/doctype/patient_appointment/patient_appointment.jshealthcare/healthcare/doctype/patient_appointment/patient_appointment.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.py`:
- Around line 650-660: The registration item currently sets
reference_dt/reference_dn to ("Patient", patient), which makes patient-scoped
rows indistinguishable from other patient charges when auto-canceling invoices;
change get_registration_item to set reference_dt = "Appointment" and
reference_dn = appointment_doc.name so the registration row is scoped to the
appointment. Also update the invoice cancellation logic (the code that uses
has_other_references) to treat rows with no appointment-scoped reference as
billable (i.e., do not ignore unreferenced rows) and ensure has_other_references
checks for any non-appointment reference_dt/reference_dn or empty references
before allowing auto-cancel; apply the same change for the other registration
helper at the 749-763 region.
- Around line 682-693: The helper get_registration_fee_settings currently
swallows all exceptions; instead remove the broad except and explicitly handle
missing custom fields by checking their existence (e.g., using
frappe.db.has_column or frappe.get_meta("Healthcare Settings") to see if
"include_registration_fee", "reg_item", and "reg_fee" exist) before calling
frappe.db.get_single_value; only fall back to {"include_registration_fee": 0,
"reg_item": None, "reg_fee": 0} when those fields are absent, and allow other
unexpected errors from get_registration_fee_settings (or from
frappe.db.get_single_value) to propagate so they surface in logs/CI.
- Around line 668-679: The current check uses "not flt(settings.get('reg_fee'))"
which only excludes zero but allows negative registration fees; update the
condition in the registration-fee branch to require a strictly positive fee
(e.g., replace "not flt(settings.get('reg_fee'))" with
"flt(settings.get('reg_fee')) <= 0" or check "flt(settings.get('reg_fee')) > 0"
and gate the True branch accordingly) so that negative reg_fee is rejected; keep
the existing checks for settings.get('reg_item') and the new-patient check
(check_is_new_patient_by_invoice) and ensure the returned "registration_fee"
uses the positive flt(...) value only when > 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c3f67bc0-ebb3-40eb-b74a-4460393a6867
📒 Files selected for processing (1)
healthcare/healthcare/doctype/patient_appointment/patient_appointment.py
Comment on lines
+650
to
+660
| def get_registration_item(appointment_doc, item, registration_context): | ||
| item.item_code = registration_context.get("registration_item") | ||
| item.description = _("Registration Charges") | ||
| item.income_account = get_income_account(appointment_doc.practitioner, appointment_doc.company) | ||
| item.cost_center = frappe.get_cached_value("Company", appointment_doc.company, "cost_center") | ||
| item.rate = flt(registration_context.get("registration_fee")) | ||
| item.amount = flt(registration_context.get("registration_fee")) | ||
| item.qty = 1 | ||
| item.reference_dt = "Patient" | ||
| item.reference_dn = appointment_doc.patient | ||
| return item |
There was a problem hiding this comment.
Use an appointment-scoped reference before auto-canceling the invoice.
The registration row is linked as ("Patient", patient), and the cancellation path treats those rows as safe to discard. That makes unrelated patient-scoped charges indistinguishable from the registration fee, and the current has_other_references check also ignores extra unreferenced rows entirely. This can cancel invoices that still contain other billable items.
Suggested fix
def get_registration_item(appointment_doc, item, registration_context):
item.item_code = registration_context.get("registration_item")
item.description = _("Registration Charges")
item.income_account = get_income_account(appointment_doc.practitioner, appointment_doc.company)
item.cost_center = frappe.get_cached_value("Company", appointment_doc.company, "cost_center")
item.rate = flt(registration_context.get("registration_fee"))
item.amount = flt(registration_context.get("registration_fee"))
item.qty = 1
- item.reference_dt = "Patient"
- item.reference_dn = appointment_doc.patient
+ item.reference_dt = "Patient Appointment"
+ item.reference_dn = appointment_doc.name
return item def cancel_sales_invoice(sales_invoice):
if frappe.db.get_single_value("Healthcare Settings", "show_payment_popup"):
has_appointment_reference_item = any(
item.reference_dt == "Patient Appointment" and item.reference_dn == sales_invoice.appointment
for item in sales_invoice.items
)
- has_other_references = any(
- (item.reference_dt or item.reference_dn)
- and not (
- (item.reference_dt == "Patient Appointment" and item.reference_dn == sales_invoice.appointment)
- or (item.reference_dt == "Patient" and item.reference_dn == sales_invoice.patient)
- )
- for item in sales_invoice.items
- )
+ has_other_items = any(
+ not (
+ item.reference_dt == "Patient Appointment"
+ and item.reference_dn == sales_invoice.appointment
+ )
+ for item in sales_invoice.items
+ )
- if has_appointment_reference_item and not has_other_references:
+ if has_appointment_reference_item and not has_other_items:
if sales_invoice.docstatus.is_submitted():
sales_invoice.cancel()
return True
return FalseAlso applies to: 749-763
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.py`
around lines 650 - 660, The registration item currently sets
reference_dt/reference_dn to ("Patient", patient), which makes patient-scoped
rows indistinguishable from other patient charges when auto-canceling invoices;
change get_registration_item to set reference_dt = "Appointment" and
reference_dn = appointment_doc.name so the registration row is scoped to the
appointment. Also update the invoice cancellation logic (the code that uses
has_other_references) to treat rows with no appointment-scoped reference as
billable (i.e., do not ignore unreferenced rows) and ensure has_other_references
checks for any non-appointment reference_dt/reference_dn or empty references
before allowing auto-cancel; apply the same change for the other registration
helper at the 749-763 region.
Contributor
Author
There was a problem hiding this comment.
The registration fee item usually has reference to Patient Doctype and patient name instead of patient appointment in Sales Invoice. This is also in the case of Collect Registration Fee Option
…and improve appointment cancellation logic
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
healthcare/healthcare/doctype/patient_appointment/patient_appointment.py (1)
650-660:⚠️ Potential issue | 🟠 MajorKeep the registration row scoped to the appointment.
cancel_sales_invoice()now only auto-cancels invoices whose items all point at the same appointment. This registration row still points to("Patient", patient), so any invoice that includes the new fee will always fall back to manual cancellation.Proposed fix
def get_registration_item(appointment_doc, item, registration_context): item.item_code = registration_context.get("registration_item") item.description = _("Registration Charges") item.income_account = get_income_account(appointment_doc.practitioner, appointment_doc.company) item.cost_center = frappe.get_cached_value("Company", appointment_doc.company, "cost_center") item.rate = flt(registration_context.get("registration_fee")) item.amount = flt(registration_context.get("registration_fee")) item.qty = 1 - item.reference_dt = "Patient" - item.reference_dn = appointment_doc.patient + item.reference_dt = "Patient Appointment" + item.reference_dn = appointment_doc.name return item🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.py` around lines 650 - 660, The registration line is currently linked to the patient ("reference_dt"="Patient", "reference_dn"=appointment_doc.patient) which prevents cancel_sales_invoice() from grouping rows by appointment; in get_registration_item change the reference to the appointment by setting reference_dt to the appointment doctype (e.g. "Patient Appointment") and reference_dn to appointment_doc.name so the row is scoped to the appointment (update the reference_dt/reference_dn assignments inside get_registration_item).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.py`:
- Around line 34-36: In the validate path that checks include_registration_fee
and reg_fee (inside Healthcare Settings, e.g., the validate method in
healthcare_settings.py), also ensure reg_item is provided and refers to a
service (non-stock) item: if include_registration_fee is true then assert
self.get("reg_item") is present, load the Item record for that code and raise
frappe.throw if the item is missing or if the item is stock-managed (e.g.,
item.is_stock_item / item.has_stock == True) so registration charges behave like
service items instead of failing later during invoice creation.
In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.py`:
- Around line 578-583: The discount application blocks (where paid_amount is
reduced using sales_invoice.additional_discount_percentage and
sales_invoice.discount_amount) must guard so paid_amount never becomes negative;
after applying each discount (in the code that sets
sales_invoice.additional_discount_percentage and sales_invoice.discount_amount
and updates paid_amount) clamp paid_amount = max(0, paid_amount) (or equivalent)
before it is used to create the POS payment row or written back to the
appointment; apply this same clamp in both the percentage and fixed-amount
branches so payment.amount and the appointment's paid_amount cannot be negative.
---
Duplicate comments:
In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.py`:
- Around line 650-660: The registration line is currently linked to the patient
("reference_dt"="Patient", "reference_dn"=appointment_doc.patient) which
prevents cancel_sales_invoice() from grouping rows by appointment; in
get_registration_item change the reference to the appointment by setting
reference_dt to the appointment doctype (e.g. "Patient Appointment") and
reference_dn to appointment_doc.name so the row is scoped to the appointment
(update the reference_dt/reference_dn assignments inside get_registration_item).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0e925fa5-79f3-4d90-ab14-01371fa51b2d
📒 Files selected for processing (2)
healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.pyhealthcare/healthcare/doctype/patient_appointment/patient_appointment.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
healthcare/healthcare/doctype/patient_appointment/patient_appointment.py (1)
773-776:⚠️ Potential issue | 🟠 MajorCancellation logic has a logic error with registration items.
The
has_other_referencescheck treats ANY item that doesn't match(Patient Appointment, appointment_name)as blocking auto-cancel. Since registration items currently use(Patient, patient)as reference (lines 659-660), an invoice with only appointment + registration items will havehas_other_references=True, preventing auto-cancellation.If the registration item fix is applied (using
Patient Appointmentreference), this logic will work correctly. Otherwise, the cancellation logic needs to explicitly handle registration items.Alternative fix if registration item reference cannot change
has_other_references = any( not (item.reference_dt == "Patient Appointment" and item.reference_dn == appointment_name) + and not (item.reference_dt == "Patient" and item.reference_dn == sales_invoice.patient) for item in sales_invoice.items )However, the cleaner solution is to fix the registration item reference_dt/reference_dn at the source (lines 659-660).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.py` around lines 773 - 776, The auto-cancel is blocked because has_other_references treats registration invoice lines (which currently reference Patient/ patient) as "other"—fix by setting the registration invoice item's reference_dt/reference_dn to "Patient Appointment" and appointment_name where the registration item is added (the block that creates/appends the registration item), or if you cannot change that source, modify the has_other_references check to explicitly ignore registration items (e.g., by checking item.item_code or a registration flag/description) when iterating sales_invoice.items so only truly unrelated references block auto-cancel; update the symbol references has_other_references and sales_invoice.items accordingly.
🧹 Nitpick comments (2)
healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.py (1)
51-52: Confusing error message when item is a stock item.The error message says "Configure a service Item for {0}" where
{0}isself.get("reg_item")- the item that was configured. This reads awkwardly since it's telling users to configure a service item "for" the invalid item they selected.Suggested improvement
- if item_values.get("is_stock_item") or item_values.get("has_stock"): - frappe.throw(_("Configure a service Item for {0}").format(self.get("reg_item"))) + if item_values.get("is_stock_item") or item_values.get("has_stock"): + frappe.throw(_("Registration Item {0} must be a service item, not a stock item").format(self.get("reg_item")))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.py` around lines 51 - 52, The thrown error message when a stock item is selected is confusing; in the conditional that checks item_values.get("is_stock_item") or item_values.get("has_stock") (in healthcare_settings.py, around the code using self.get("reg_item")), replace the awkward "Configure a service Item for {0}" message with a clear, direct message that includes the problematic item (e.g., "The selected item {0} is a stock item; please configure a service item instead."). Update the frappe.throw call to format with self.get("reg_item") and ensure capitalization/spacing is consistent.healthcare/healthcare/doctype/patient_appointment/patient_appointment.js (1)
1499-1501: Variableregistration_feeshadows the outer scope variable.Line 1500 declares a new
registration_feevariable insidevalidate_discountthat shadows the outerregistration_feefrom line 1387. While this works because the dialog field value is retrieved, it creates confusing code where the same name refers to different sources.Suggested clarity improvement
function validate_discount(field) { let message = ""; let discount_percentage = d.get_value("discount_percentage"); let discount_amount = d.get_value("discount_amount"); let consultation_charge = flt(d.get_value("consultation_charge")); - let registration_fee = flt(d.get_value("registration_fee")) || 0; + let dialog_registration_fee = flt(d.get_value("registration_fee")) || 0; - let total_charge = consultation_charge + registration_fee; + let total_charge = consultation_charge + dialog_registration_fee;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.js` around lines 1499 - 1501, The inner declaration of registration_fee inside validate_discount shadows the outer registration_fee variable—rename the local variable (e.g., dialog_registration_fee or reg_fee_dialog) used to store flt(d.get_value("registration_fee")) and update its uses in validate_discount so the outer registration_fee (the module-level/appointment value) is not shadowed; ensure total_charge is computed using consultation_charge + dialog_registration_fee (or the chosen local name) and leave the outer registration_fee identifier untouched elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.py`:
- Around line 773-776: The auto-cancel is blocked because has_other_references
treats registration invoice lines (which currently reference Patient/ patient)
as "other"—fix by setting the registration invoice item's
reference_dt/reference_dn to "Patient Appointment" and appointment_name where
the registration item is added (the block that creates/appends the registration
item), or if you cannot change that source, modify the has_other_references
check to explicitly ignore registration items (e.g., by checking item.item_code
or a registration flag/description) when iterating sales_invoice.items so only
truly unrelated references block auto-cancel; update the symbol references
has_other_references and sales_invoice.items accordingly.
---
Nitpick comments:
In `@healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.py`:
- Around line 51-52: The thrown error message when a stock item is selected is
confusing; in the conditional that checks item_values.get("is_stock_item") or
item_values.get("has_stock") (in healthcare_settings.py, around the code using
self.get("reg_item")), replace the awkward "Configure a service Item for {0}"
message with a clear, direct message that includes the problematic item (e.g.,
"The selected item {0} is a stock item; please configure a service item
instead."). Update the frappe.throw call to format with self.get("reg_item") and
ensure capitalization/spacing is consistent.
In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.js`:
- Around line 1499-1501: The inner declaration of registration_fee inside
validate_discount shadows the outer registration_fee variable—rename the local
variable (e.g., dialog_registration_fee or reg_fee_dialog) used to store
flt(d.get_value("registration_fee")) and update its uses in validate_discount so
the outer registration_fee (the module-level/appointment value) is not shadowed;
ensure total_charge is computed using consultation_charge +
dialog_registration_fee (or the chosen local name) and leave the outer
registration_fee identifier untouched elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 816692e7-de9c-4646-9c47-c3f5857668c2
📒 Files selected for processing (3)
healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.pyhealthcare/healthcare/doctype/patient_appointment/patient_appointment.jshealthcare/healthcare/doctype/patient_appointment/patient_appointment.py
…ation_fee_details function
…on fee' setting for clarity
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
healthcare/healthcare/doctype/patient_appointment/patient_appointment.py (1)
570-607:⚠️ Potential issue | 🟡 MinorThe
paid_amountfield semantics have changed and should be tracked separately.The code writes a modified
paid_amountback to the appointment (line 605) that includes registration fees and subtracts discounts. Initially,paid_amountrepresents the practitioner charge; after invoice creation, it represents the net payable amount.This semantic shift could confuse code that expects
paid_amountto consistently represent the consultation charge. The field is read-only afterinvoiced=1is set, which prevents accidental reprocessing, but the data integrity concern remains.Consider maintaining the original practitioner charge separately or document that
paid_amountchanges meaning after invoice creation to avoid misuse in reports and downstream logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.py` around lines 570 - 607, The code mutates appointment_doc.paid_amount (used as the practitioner charge) to a net payable amount before persisting, which changes its semantics; preserve the original practitioner charge by capturing appointment_doc.paid_amount into a new DB field (e.g., practitioner_charge_before_invoice or original_paid_amount) before you add registration_fee and apply discounts, then write both values in the frappe.db.set_value call (keep paid_amount as the net/paid_amount_net or vice versa depending on desired canonical meaning) and update any consumers to read the new explicit field; reference symbols: Patient Appointment, paid_amount, get_registration_fee_context, registration_fee, sales_invoice, invoiced, ref_sales_invoice.
🧹 Nitpick comments (2)
healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.json (1)
516-531: Consider more descriptive labels to distinguish from existing registration fields.Both
reg_item(line 528) andregistration_item(line 511) have the label "Registration Item", and bothreg_fee(line 536) andregistration_fee(line 105) have the label "Registration Fee". This could confuse users navigating the Healthcare Settings form.Consider using more descriptive labels like "Combined Registration Item" or "Registration Item (Combined Invoice)" to clearly distinguish the two different registration workflows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.json` around lines 516 - 531, Update the ambiguous labels in healthcare_settings.json so users can tell the combined-invoice settings apart from the standalone registration settings: rename the label for the Link field with fieldname "reg_item" to something like "Registration Item (Combined Invoice)" and rename the label for the Currency/Amount field with fieldname "reg_fee" to "Registration Fee (Combined Invoice)" (or similar descriptive text); ensure you only change the "label" values for reg_item and reg_fee while leaving "registration_item" and "registration_fee" untouched so both workflows remain clearly distinguishable in the Healthcare Settings form.healthcare/healthcare/doctype/patient_appointment/patient_appointment.js (1)
1386-1410: Registration fee field insertion may insert at wrong position.The
splice(total_payable_index, 0, ...)inserts the registration fee field before thetotal_payablefield, which is the intended behavior. However, iftotal_payable_indexis-1(field not found), the splice would insert at the second-to-last position due to JavaScript's handling of negative indices, which could cause unexpected dialog layout.💡 Add explicit check for field existence
if (registration_data?.apply_registration_fee) { registration_fee = flt(registration_data.registration_fee); let total_payable_index = fields.findIndex( field => field.fieldname === "total_payable", ); - if (total_payable_index > -1) { + if (total_payable_index !== -1) { fields.splice(total_payable_index, 0, { label: "Registration Fee", fieldname: "registration_fee", fieldtype: "Currency", read_only: true, }); + } else { + // Fallback: append registration fee field if total_payable not found + fields.push({ + label: "Registration Fee", + fieldname: "registration_fee", + fieldtype: "Currency", + read_only: true, + }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.js` around lines 1386 - 1410, The insertion logic for the "registration_fee" field should explicitly handle the case where "total_payable" is not found to avoid relying on splice with negative indices; in show_payment_dialog compute total_payable_index (from fields.findIndex) and if total_payable_index >= 0 use fields.splice(total_payable_index, 0, {...}) to insert before it, otherwise use fields.push({...}) to append the registration_fee field to the end.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.json`:
- Around line 532-538: The new JSON field "reg_fee" is missing the UI-level
non-negative constraint; update the healthcare_settings doctype JSON by adding
"non_negative": 1 to the "reg_fee" field definition (same place where
"depends_on", "fieldname": "reg_fee", "fieldtype": "Currency", etc. are defined)
so it matches the existing "registration_fee" field's non_negative behavior and
prevents negative values in the UI.
In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.py`:
- Around line 769-782: The check in cancel_sales_invoice that computes
has_other_references incorrectly treats registration fee line(s) as "other"
because they use reference_dt == "Patient" and reference_dn ==
appointment_doc.patient; update the logic in cancel_sales_invoice (the
has_appointment_reference_item / has_other_references computation over
sales_invoice.items) to treat registration-related invoice lines as
appointment-related: consider an item appointment-related if it matches
(reference_dt == "Patient Appointment" and reference_dn == appointment_name) OR
if it matches the registration item pattern used in get_registration_item (e.g.,
reference_dt == "Patient" and reference_dn == appointment_doc.patient and/or the
same registration item_code/item identifier returned/used by
get_registration_item); then only treat truly unrelated items as "other
references" so invoices containing consultation + registration lines can be
auto-cancelled.
---
Outside diff comments:
In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.py`:
- Around line 570-607: The code mutates appointment_doc.paid_amount (used as the
practitioner charge) to a net payable amount before persisting, which changes
its semantics; preserve the original practitioner charge by capturing
appointment_doc.paid_amount into a new DB field (e.g.,
practitioner_charge_before_invoice or original_paid_amount) before you add
registration_fee and apply discounts, then write both values in the
frappe.db.set_value call (keep paid_amount as the net/paid_amount_net or vice
versa depending on desired canonical meaning) and update any consumers to read
the new explicit field; reference symbols: Patient Appointment, paid_amount,
get_registration_fee_context, registration_fee, sales_invoice, invoiced,
ref_sales_invoice.
---
Nitpick comments:
In `@healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.json`:
- Around line 516-531: Update the ambiguous labels in healthcare_settings.json
so users can tell the combined-invoice settings apart from the standalone
registration settings: rename the label for the Link field with fieldname
"reg_item" to something like "Registration Item (Combined Invoice)" and rename
the label for the Currency/Amount field with fieldname "reg_fee" to
"Registration Fee (Combined Invoice)" (or similar descriptive text); ensure you
only change the "label" values for reg_item and reg_fee while leaving
"registration_item" and "registration_fee" untouched so both workflows remain
clearly distinguishable in the Healthcare Settings form.
In `@healthcare/healthcare/doctype/patient_appointment/patient_appointment.js`:
- Around line 1386-1410: The insertion logic for the "registration_fee" field
should explicitly handle the case where "total_payable" is not found to avoid
relying on splice with negative indices; in show_payment_dialog compute
total_payable_index (from fields.findIndex) and if total_payable_index >= 0 use
fields.splice(total_payable_index, 0, {...}) to insert before it, otherwise use
fields.push({...}) to append the registration_fee field to the end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 32fb38db-6bbb-41f4-95e7-c4bfa1c58a2c
📒 Files selected for processing (4)
healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.jsonhealthcare/healthcare/doctype/healthcare_settings/healthcare_settings.pyhealthcare/healthcare/doctype/patient_appointment/patient_appointment.jshealthcare/healthcare/doctype/patient_appointment/patient_appointment.py
🚧 Files skipped from review as they are similar to previous changes (1)
- healthcare/healthcare/doctype/healthcare_settings/healthcare_settings.py
Contributor
Author
|
Hi @Sajinsr |
|
This PR has had no activity for 30 days and is now marked stale. It will be closed in 14 days unless there is new activity. |
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.
ERPNext Healthcare/Marley Healthcare already provides a “Collect Registration Fee” setting. When enabled, the system expects the registration fee to be collected immediately for new patients. However, in many hospitals appointments are often booked over phone calls, and patients typically pay only when they arrive at the hospital, which can create issues during appointment creation as disabled patients are not listed in dropdown.
This change improves the billing flow by combining the registration fee with the consultation fee for new patients when the setting "Include Registration Fee with Consultaion Fee" is enabled. If the patient has no submitted Sales Invoice (new patient), the registration fee is included in the payable amount shown in the payment popup and reflected during invoice creation.
Discounts are calculated on the total charge (consultation + registration fee) so that the payment popup totals and invoice totals remain consistent.
This approach supports hospitals that prefer combined billing for consultation and registration fees.
no-docs