-
-
Notifications
You must be signed in to change notification settings - Fork 559
[16.0][IMP] contract: Improve recurring invoice offset handling and resolve issue 1220 #1221
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
base: 16.0
Are you sure you want to change the base?
[16.0][IMP] contract: Improve recurring invoice offset handling and resolve issue 1220 #1221
Conversation
2946fa4
to
dc7bdd0
Compare
f2b8adc
to
e87422f
Compare
e87422f
to
4bf798e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chnges tested and OK
@mktsrl Thanks for this. Could you improve your commit message by making module appear like |
@api.onchange( | ||
"recurring_invoicing_offset", | ||
) | ||
def onchange_recurring_invoicing_offset(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not putting field in compute method's depends ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onchange
was added primarily to fix the UI issue (Point 3 in the original issue description) where the recurring_next_date wasn't updating immediately when the offset changed. It just calls the compute method to provide that instant feedback.
You're right that adding the offset to depends
is generally best practice for ensuring the compute runs on save. We did explore this, but noticed it caused unexpected failures in several existing core tests related to date calculations.
To keep this PR focused on fixing the reported offset issues without needing to refactor unrelated tests, we decided the onchange
sufficiently addressed the user-facing problem for now.
@@ -517,10 +520,208 @@ def test_check_date_end(self): | |||
self.acct_line.date_end = "2015-12-31" | |||
|
|||
def test_check_recurring_next_date_start_date(self): | |||
with self.assertRaises(ValidationError): | |||
self.acct_line.write( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that normal that somenthing that should fail do not anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the change in test_check_recurring_next_date_start_date is indeed expected.
This test previously failed because the validation was overly strict, blocking valid negative offsets even on new contracts (as noted in the original issue, point 2). The fix relaxes this by checking for posted invoices before raising the error.
Since this specific test runs without posted invoices, it correctly passes now with the new logic. The scenario where the error should occur (with posted invoices) is covered by the 2 new test_constraint_..._with_posted_invoice tests added in this PR.
This PR has the |
4bf798e
to
4e7de27
Compare
- Made recurring_invoicing_offset editable (store=True, readonly=False). - Relaxed date validation constraints (_check_last_date_invoiced, _check_recurring_next_date_start_date) to trigger only when posted invoices exist, allowing negative offsets on new contracts. - Added onchange on recurring_invoicing_offset for dynamic UI update of recurring_next_date. - Removed #INVOICEMONTHNAME# marker processing. - Added/updated unit tests for new validation logic. - Ensured recurring_invoicing_offset is in compute depends. Fixes #numero_tua_issue
4e7de27
to
5c743ae
Compare
Fixes #1220