Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1016 +/- ##
===========================================
+ Coverage 75.22% 75.24% +0.02%
===========================================
Files 132 133 +1
Lines 9239 9409 +170
===========================================
+ Hits 6950 7080 +130
- Misses 2289 2329 +40
🚀 New features to boost your workflow:
|
deepeshgarg007
left a comment
deepeshgarg007
left a comment
There was a problem hiding this comment.
Also, the standard GL's need to be skipped for the imported loans. Didn't find code for that anywhere
| }, | ||
| { | ||
| "default": "0", | ||
| "fieldname": "is_imported", |
There was a problem hiding this comment.
This should have a fetch from the loan doctype, right?
| }, | ||
| { | ||
| "default": "0", | ||
| "fieldname": "is_imported", |
There was a problem hiding this comment.
Same here, should be imported from the loan document
| return created_disbursements | ||
|
|
||
|
|
||
| def create_gl_entries( |
There was a problem hiding this comment.
Don't directly create GL Entries. Use the ERPNext make_gl_entry api the way every doctype does
|
|
||
| def get_mandatory_fieldnames(self): | ||
| if self.import_for == "Loan Repayment": | ||
| return ["loan_repayment_id", "against_loan", "loan_disbursement", "repayment_type", "posting_date", "value_date", "amount_paid", |
There was a problem hiding this comment.
get meta fields from meta instead of hardcoding
| def get_numeric_fieldnames(self): | ||
| if self.import_for == "Loan Repayment": | ||
| return { | ||
| "amount_paid", "principal_amount_paid", "total_interest_paid", "total_penalty_paid", "total_charges_paid", |
There was a problem hiding this comment.
Try getting this fields from meta as well
| ] | ||
| return [] | ||
|
|
||
| def prevalidate_and_make_logs(self, di_name: str, payloads, parsed_file) -> bool: |
There was a problem hiding this comment.
No need to do mandatory validation check. That is already enforced by document config and mandatory fields
| def validate_import_mandatory_fields(self): | ||
| if not self.is_imported: | ||
| return | ||
|
|
||
| migration_date = self.get("migration_date") | ||
| if not migration_date: | ||
| return | ||
|
|
||
| if self.get("status") == "Closed": | ||
| frappe.db.set_value("Loan", self.name, "status", "Closed") | ||
| return |
There was a problem hiding this comment.
These 3 can be clubbed in one function or condition
| "opening_principal_outstanding", | ||
| "opening_interest_outstanding", | ||
| "opening_penalty_outstanding", | ||
| "opening_additional_outstanding", | ||
| "opening_charge_outstanding", |
There was a problem hiding this comment.
Mandatory depends on config can be done for these fields based on is_imported check, right?
| if not self.is_imported: | ||
| return | ||
|
|
||
| if not self.get("migration_date"): | ||
| return | ||
|
|
||
| if self.get("status")== "Closed": | ||
| return |
There was a problem hiding this comment.
Repetition from above, commonify in a function and use in both places
| def update_demand_generated_for_repayment_schedule(loan_name, disbursement_name, migration_date): | ||
| frappe.db.sql( | ||
| """ | ||
| UPDATE `tabRepayment Schedule` rs | ||
| INNER JOIN `tabLoan Repayment Schedule` lrs ON lrs.name = rs.parent | ||
| SET rs.demand_generated = 1 | ||
| WHERE lrs.loan = %s | ||
| AND lrs.loan_disbursement = %s | ||
| AND lrs.docstatus = 1 | ||
| AND lrs.status = 'Active' | ||
| AND rs.payment_date < %s | ||
| """, | ||
| (loan_name, disbursement_name, migration_date), | ||
| ) No newline at end of file |
There was a problem hiding this comment.
A similar query is there in loan_repayment_repost_tool.py. I guess that can be used here as well without using any joins. Make a utils field and move it there and try and reuse in both.
| common = { | ||
| "doctype": "Loan Interest Accrual", | ||
| "company": self.company, | ||
| "loan": self.name, | ||
| "loan_disbursement": disbursement_name, | ||
| "posting_date": migration_date, | ||
| "accrual_date": migration_date, | ||
| "accrual_type": "Regular", | ||
| "applicant_type": self.applicant_type, | ||
| "applicant": self.applicant, | ||
| "loan_product": self.loan_product, | ||
| "rate_of_interest": flt(self.get("rate_of_interest") or 0, precision), | ||
| "is_imported": 1, | ||
| } | ||
|
|
||
| if interest_outstanding > 0: | ||
| doc = frappe.get_doc( | ||
| { | ||
| **common, | ||
| "interest_type": "Normal Interest", | ||
| "base_amount": principal_outstanding, | ||
| "interest_amount": interest_outstanding, | ||
| "additional_interest_amount": 0, | ||
| "penalty_amount": 0, | ||
| } | ||
| ) | ||
| doc.insert(ignore_permissions=True) | ||
| doc.submit() |
There was a problem hiding this comment.
why not reuse make_loan_interest_accrual_entry from loan_interest_accrual.py?
| frappe.db.set_value("Loan Demand", demand.name, "is_imported", 1) | ||
|
|
||
|
|
||
| def create_migrated_loan_disbursement( |
There was a problem hiding this comment.
| def create_migrated_loan_disbursement( | |
| def import_loan_disbursement( |
| def get_migration_date_for_import(self): | ||
| if not self.get("is_imported"): | ||
| return None | ||
|
|
||
| migration_date = self.get("migration_date") | ||
| if not migration_date: | ||
| return None | ||
|
|
||
| if self.get("status") == "Closed": | ||
| return None | ||
|
|
||
| return migration_date |
There was a problem hiding this comment.
Why not just make the migration date mandatory for all migrated loans irrespective of closed or active loans?
| def is_line_of_credit_loan(self): | ||
| if self.get("repayment_schedule_type"): | ||
| return self.repayment_schedule_type == "Line of Credit" | ||
|
|
||
| return ( | ||
| frappe.db.get_value("Loan Product", self.loan_product, "repayment_schedule_type") | ||
| == "Line of Credit" | ||
| ) |
There was a problem hiding this comment.
repayment schedule type is anyways fetch from loan product, why check separately in the loan product?
| migration_date = self.get_migration_date_for_import() | ||
| if not migration_date: | ||
| return | ||
|
|
||
| precision = cint(frappe.db.get_default("currency_precision")) or 2 | ||
| migration_date = self.migration_date |
There was a problem hiding this comment.
Migration date is being assigned twice
Also, why not just use self.migration_date instead of assigning in a variable?
| migration_date = self.migration_date | ||
| is_loc = self.is_line_of_credit_loan() | ||
|
|
||
| for d in (self.get("loan_import_details") or []): |
There was a problem hiding this comment.
| for d in (self.get("loan_import_details") or []): | |
| for d in (self.get("loan_import_details")): |
| loan_disbursement_id = d.loan_disbursement_id | ||
| disbursed_amount = flt(d.disbursed_amount, precision) | ||
| disbursement_date = d.disbursement_date |
There was a problem hiding this comment.
Why assign to a variable when d.disbursement_date or d.loan_disbursement_id can be used. This adds 3 unnecessary extra lines
| if existing: | ||
| return existing | ||
|
|
||
| disbursement_date = disbursement_date |
| if self.get("repayment_schedule_type"): | ||
| data["repayment_schedule_type"] = self.repayment_schedule_type | ||
|
|
||
| if repayment_method: | ||
| data["repayment_method"] = repayment_method | ||
| if repayment_frequency: | ||
| data["repayment_frequency"] = repayment_frequency | ||
| if tenure: | ||
| data["tenure"] = tenure | ||
| if repayment_start_date: | ||
| data["repayment_start_date"] = repayment_start_date |
There was a problem hiding this comment.
These values can be directly assigned in the data dict. No need to check for if repayment_method. get_doc is capalbe to handle None values
| principal_outstanding = flt(principal_outstanding or 0, precision) | ||
| interest_outstanding = flt(interest_outstanding or 0, precision) | ||
| penalty_outstanding = flt(penalty_outstanding or 0, precision) | ||
| additional_outstanding = flt(additional_outstanding or 0, precision) | ||
|
|
||
| rate_of_interest = flt(self.get("rate_of_interest") or 0, precision) |
There was a problem hiding this comment.
Why is or 0 needed? flt can handle None values
| if not self.loan_product: | ||
| frappe.throw(_("Loan Product is mandatory to create opening GL entries.")) |
There was a problem hiding this comment.
Isn't the loan product anyway a mandatory column?
| loan_account = acc.get("loan_account") | ||
| payment_account = acc.get("payment_account") |
There was a problem hiding this comment.
Unnecessary variable assignment
3 participants
Frappe Lending Roadmap
For Mid Tenure Loans:
For Closed Loans:
Import Mid Tenure Loans
Mid.Tenure.Loans.mov
Import Closed Loans
Closed.Loans.mov
Create Missing Customers
Create.Missing.Customers.mov
Partial Success
Partial.Success.mov
Import Loan Repayment
Loan.Repayment.Import.mov