Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Calculate monthly repayment precisely to the fraction of a currency unit, turning rounding up into an optional feature. #36

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

bosue
Copy link
Contributor

@bosue bosue commented Sep 18, 2023

Before:
IMG_1553

Now also possible:
IMG_1561

And this is how it works, UI-wise:

RPReplay_Final1695066171.mov

Fixes #31. Includes some minor UI improvements and some minor refactoring of related code.

Full Loan Repayment Schedules for comparison:
Before.pdf
With PR (rounded up).pdf
With PR (to the cent).pdf

@bosue bosue changed the title fix: Calculate monthly repayment precisely to the fraction of a curre ncy, turning rounding up into an optional feature. fix: Calculate monthly repayment precisely to the fraction of a currency unit, turning rounding up into an optional feature. Sep 18, 2023
@bosue bosue force-pushed the increase_currency_precision branch 4 times, most recently from 9897f38 to 3cd35c0 Compare September 18, 2023 20:43
@bosue
Copy link
Contributor Author

bosue commented Sep 18, 2023

A few failing tests, no big deal, but nice we have coverage!
Will set up a local test environment, fix the bug, amend and expand the tests and come back. ;)

@bosue bosue marked this pull request as draft September 18, 2023 21:00
…ncy, turning rounding up into an optional feature.
@bosue
Copy link
Contributor Author

bosue commented Sep 21, 2023

The business logic related test fails need to be tackled in the end, that’s fine, these basically is the feature rather than a bug, though of course both need to be tested to work right:

  File "/home/runner/frappe-bench/apps/lending/lending/loan_management/doctype/loan/test_loan.py", line 130, in test_loan
    self.assertEqual(loan_repayment_schedule.monthly_repayment_amount, 15052)
AssertionError: 15051.72264885 != 15052

But these other update_after_submit related fails are almost driving me crazy:

  File "/home/runner/frappe-bench/apps/lending/lending/loan_management/doctype/loan/test_loan.py", line 415, in test_security_shortfall
    loan_application = create_loan_application(
  File "/home/runner/frappe-bench/apps/lending/lending/loan_management/doctype/loan/test_loan.py", line 1237, in create_loan_application
    loan_application.save()
  File "/home/runner/frappe-bench/apps/frappe/frappe/model/document.py", line 331, in save
    return self._save(*args, **kwargs)
  File "/home/runner/frappe-bench/apps/frappe/frappe/model/document.py", line 372, in _save
    self.validate_update_after_submit()
  File "/home/runner/frappe-bench/apps/frappe/frappe/model/document.py", line 859, in validate_update_after_submit
    self._validate_update_after_submit()
  File "/home/runner/frappe-bench/apps/frappe/frappe/model/base_document.py", line 1016, in _validate_update_after_submit
    frappe.throw(
  File "/home/runner/frappe-bench/apps/frappe/frappe/__init__.py", line 570, in throw
    msgprint(
  File "/home/runner/frappe-bench/apps/frappe/frappe/__init__.py", line 542, in msgprint
    _raise_exception()
  File "/home/runner/frappe-bench/apps/frappe/frappe/__init__.py", line 491, in _raise_exception
    raise raise_exception(msg)
frappe.exceptions.UpdateAfterSubmitError:  Not allowed to change <strong>Monthly Repayment Amount</strong> after submission from <strong>89552.027429802</strong> to <strong>89552.02742980218</strong>

We shouldn‘t be changing anything on update after submit. I tried almost everything without any success. But specifically for updates after submit, in get_repayment_details() I can‘t even evaluate self.docstatus nor can I throw a frappe.msgprint() for debugging.
When skipping the method by an early unconditional return, the problem is gone. So even on upon update after submit, get_repayment_details() is getting called and does stuff.
But how can I conditionally skip get_repayment_details() only in the case we‘re updating a submitted doc, if for some reason or other I can‘t reliably evaluate self.docstatus?

This stubborn problem now cost me more time and nerves than all of the actual feature. I would be really grateful for some advise how to tackle this, @deepeshgarg007 or @anandbaburajan or someone else who happened to hit a similar problem.

Thank you so much! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rounding the monthly repayment amount to the next full currency unit needs to be optional
1 participant