[Master]- Recurring billing is stuck due to an error related to updating the Subscription Line table#7404
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes recurring billing getting stuck by preventing Subscription Line.CalculateNextToDate (with “Align to End of Month”) from returning a NextToDate earlier than FromDate, and adds a regression test for AB#623011.
Changes:
- Add guard logic in
Subscription Linedate calculation to clampNextToDatewhen it would fall beforeFromDate. - Add a new test verifying
CalculateNextToDatedoes not return a date beforeFromDatefor “Align to End of Month”. - Add a small helper to mock/insert a
Subscription Linerecord for the test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
.../RecurringBillingDocsTest.Codeunit.al |
Adds regression test + helper for reproducing and validating AB#623011 scenario |
.../SubscriptionLine.Table.al |
Adjusts CalculateNextToDate logic to avoid returning a date before FromDate |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Apps/W1/Subscription Billing/Test/Billing/RecurringBillingDocsTest.Codeunit.al
Show resolved
Hide resolved
src/Apps/W1/Subscription Billing/Test/Billing/RecurringBillingDocsTest.Codeunit.al
Outdated
Show resolved
Hide resolved
src/Apps/W1/Subscription Billing/Test/Billing/RecurringBillingDocsTest.Codeunit.al
Outdated
Show resolved
Hide resolved
| if NextToDate < FromDate then | ||
| NextToDate := CalcDate(PeriodFormula, FromDate) - 1; |
There was a problem hiding this comment.
This fix can still yield NextToDate < FromDate if CalcDate(PeriodFormula, FromDate) returns FromDate (e.g., a zero/empty period formula, if permitted), since subtracting 1 would move it earlier. To fully enforce the invariant, clamp the result (e.g., set NextToDate to FromDate when the computed value would be earlier) so NextToDate is guaranteed to be >= FromDate for all allowed PeriodFormula values.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Could not find linked issues in the pull request description. Please make sure the pull request description contains a line that contains 'Fixes #' followed by the issue number being fixed. Use that pattern for every issue you want to link. |
…DocsTest.Codeunit.al Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…DocsTest.Codeunit.al Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…DocsTest.Codeunit.al Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| LibraryLowerPermissions.SetO365Full(); | ||
| NextToDate := SubscriptionLine.CalculateNextToDate(PeriodFormula, FromDate); | ||
| LibraryLowerPermissions.RestoreO365Permissions(); |
There was a problem hiding this comment.
If CalculateNextToDate errors, RestoreO365Permissions() will not run, which can leak elevated permissions into subsequent tests and cause cross-test interference. Consider wrapping the call in a [TryFunction] helper (or an existing run-with-permissions utility if available) so permissions are always restored on both success and failure paths.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| LibraryLowerPermissions.SetO365Full(); | ||
| NextToDate := SubscriptionLine.CalculateNextToDate(PeriodFormula, FromDate); | ||
| LibraryLowerPermissions.RestoreO365Permissions(); | ||
|
|
||
| // [THEN] NextToDate is not before FromDate | ||
| Assert.IsTrue(NextToDate >= FromDate, | ||
| StrSubstNo(NextToDateBeforeFromDateErr, NextToDate, FromDate)); | ||
| end; | ||
|
|
There was a problem hiding this comment.
RestoreO365Permissions() won’t run if CalculateNextToDate(...) throws, which can leak elevated permissions into subsequent tests and cause cascading failures. Consider wrapping the calculation in a [TryFunction] helper (or equivalent test utility) so permissions are restored on both success and failure, and then rethrow the captured error after restoring permissions.
| LibraryLowerPermissions.SetO365Full(); | |
| NextToDate := SubscriptionLine.CalculateNextToDate(PeriodFormula, FromDate); | |
| LibraryLowerPermissions.RestoreO365Permissions(); | |
| // [THEN] NextToDate is not before FromDate | |
| Assert.IsTrue(NextToDate >= FromDate, | |
| StrSubstNo(NextToDateBeforeFromDateErr, NextToDate, FromDate)); | |
| end; | |
| NextToDate := CalculateNextToDateWithO365Permissions(SubscriptionLine, PeriodFormula, FromDate); | |
| // [THEN] NextToDate is not before FromDate | |
| Assert.IsTrue(NextToDate >= FromDate, | |
| StrSubstNo(NextToDateBeforeFromDateErr, NextToDate, FromDate)); | |
| Assert.IsTrue(NextToDate >= FromDate, | |
| StrSubstNo(NextToDateBeforeFromDateErr, NextToDate, FromDate)); | |
| end; | |
| local procedure CalculateNextToDateWithO365Permissions(var SubscriptionLine: Record "Subscription Line"; PeriodFormula: DateFormula; FromDate: Date): Date | |
| var | |
| NextToDate: Date; | |
| begin | |
| LibraryLowerPermissions.SetO365Full(); | |
| if not TryCalculateNextToDate(SubscriptionLine, PeriodFormula, FromDate, NextToDate) then begin | |
| LibraryLowerPermissions.RestoreO365Permissions(); | |
| Error(GetLastErrorText()); | |
| end; | |
| LibraryLowerPermissions.RestoreO365Permissions(); | |
| exit(NextToDate); | |
| end; | |
| [TryFunction] | |
| local procedure TryCalculateNextToDate(var SubscriptionLine: Record "Subscription Line"; PeriodFormula: DateFormula; FromDate: Date; var NextToDate: Date) | |
| begin | |
| NextToDate := SubscriptionLine.CalculateNextToDate(PeriodFormula, FromDate); | |
| end; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Apps/W1/Subscription Billing/Test/Billing/RecurringBillingDocsTest.Codeunit.al
Outdated
Show resolved
Hide resolved
src/Apps/W1/Subscription Billing/Test/Billing/RecurringBillingDocsTest.Codeunit.al
Outdated
Show resolved
Hide resolved
| if NextToDate < FromDate then | ||
| NextToDate := CalcDate(PeriodFormula, FromDate) - 1; |
There was a problem hiding this comment.
Coverage gap: the new safeguard is only regression-tested for <1D> with a specific date combination. Add at least one additional test that exercises a formula that could evaluate to FromDate (if such formulas are supported) to ensure the fix truly prevents NextToDate < FromDate across valid inputs.
…DocsTest.Codeunit.al Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…DocsTest.Codeunit.al Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
AB#629263
Work Item Bug 629263: [master] [ALL-E] Recurring billing is stuck due to an error related to updating the Subscription Line table
Fixes AB#629263