[Subscription Billing] Remove internal modifiers and make enums extensible for partner extension support - #8387
Conversation
|
Could not find a linked ADO work item. Please link one by using the pattern 'AB#' followed by the relevant work item number. You may use the 'Fixes' keyword to automatically resolve the work item when the pull request is merged. E.g. 'Fixes AB#1234' |
| end; | ||
|
|
||
| internal procedure GetDataPagePersonalization(ObjectType: Option ,,,Report,,,XMLport,,Page; ObjectID: Text; ValueName: Code[40]; var Value: Text): Boolean | ||
| procedure GetDataPagePersonalization(ObjectType: Option ,,,Report,,,XMLport,,Page; ObjectID: Text; ValueName: Code[40]; var Value: Text): Boolean |
There was a problem hiding this comment.
User personalization read API exposed to partners
Making GetDataPagePersonalization public allows partner extensions to read arbitrary personalization values stored for the current user (UserSecurityId()). Values stored via SetDataPagePersonalization are free-form text blobs with no DataClassification tag and no audit trail, so partner code could silently read or infer user preferences, saved state, or other user-associated data.
Recommendation:
- If this API must remain public, add a
DataClassificationannotation to thePage Data Personalizationvalue field (or document that callers must only writeDataClassification::SystemMetadatadata), and expose read access through a narrower, purpose-specific method rather than a generic key-value getter.
| procedure GetDataPagePersonalization(ObjectType: Option ,,,Report,,,XMLport,,Page; ObjectID: Text; ValueName: Code[40]; var Value: Text): Boolean | |
| [InherentPermissions(PermissionObjectType::TableData, Database::"Page Data Personalization", 'R')] | |
| procedure GetDataPagePersonalization(ObjectType: Option ,,,Report,,,XMLport,,Page; ObjectID: Text; ValueName: Code[40]; var Value: Text): Boolean |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| OnAfterInsertFromItemNoAndCustomerContract(ServiceObject, CustomerContract); | ||
| end; | ||
|
|
||
| internal procedure SetUnitPriceAndUnitCostFromExtendContract(NewUnitPrice: Decimal; NewUnitCost: Decimal) |
There was a problem hiding this comment.
Public pricing override bypasses price calculation
Making SetUnitPriceAndUnitCostFromExtendContract public allows any partner extension to set CalledFromExtendContract := true with arbitrary prices. Once set, all subscription lines created from this table instance use the caller-supplied prices instead of the normal price calculation logic (lines 1947–1958 in the same file), with no guard ensuring ResetCalledFromExtendContract is called afterward.
Recommendation:
- Add an
[InherentPermissions]attribute or expose only through a dedicated integration event. If the public API is intentional, document that callers must always callResetCalledFromExtendContract()in a try-finally block to avoid pricing state leakage.
| internal procedure SetUnitPriceAndUnitCostFromExtendContract(NewUnitPrice: Decimal; NewUnitCost: Decimal) | |
| [InherentPermissions(PermissionObjectType::TableData, Database::"Subscription Header", 'M')] | |
| procedure SetUnitPriceAndUnitCostFromExtendContract(NewUnitPrice: Decimal; NewUnitCost: Decimal) | |
| begin | |
| CalledFromExtendContract := true; | |
| UnitPrice := NewUnitPrice; | |
| UnitCost := NewUnitCost; | |
| end; |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| Access = Internal; | ||
|
|
||
| internal procedure SetDataPagePersonalization(ObjectType: Option ,,,Report,,,XMLport,,Page; ObjectID: Text; ValueName: Code[40]; Value: Text) | ||
| procedure SetDataPagePersonalization(ObjectType: Option ,,,Report,,,XMLport,,Page; ObjectID: Text; ValueName: Code[40]; Value: Text) |
There was a problem hiding this comment.
Personalization write API exposed without InherentPermissions
Removing Access = Internal makes SetDataPagePersonalization callable by any partner extension. The method directly inserts or modifies rows in the Page Data Personalization system table. Without an [InherentPermissions] annotation, there is no explicit permission contract for callers, and the ObjectID text is parsed via CopyStr+Evaluate without error handling—a non-numeric suffix causes a runtime error.
Recommendation:
- Add
[InherentPermissions(PermissionObjectType::TableData, Database::"Page Data Personalization", 'IMD')]and wrap theEvaluate(ObjectNo, ObjectID)call in anif not Evaluate(...)guard to prevent unhandled runtime errors on malformed input.
| procedure SetDataPagePersonalization(ObjectType: Option ,,,Report,,,XMLport,,Page; ObjectID: Text; ValueName: Code[40]; Value: Text) | |
| [InherentPermissions(PermissionObjectType::TableData, Database::"Page Data Personalization", 'IMD')] | |
| procedure SetDataPagePersonalization(ObjectType: Option ,,,Report,,,XMLport,,Page; ObjectID: Text; ValueName: Code[40]; Value: Text) | |
| var | |
| ... | |
| ObjectNo: Integer; | |
| begin | |
| ObjectID := CopyStr(ObjectID, StrPos(ObjectID, ' ') + 1); | |
| if not Evaluate(ObjectNo, ObjectID) then | |
| exit; | |
| ... |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| AddSalesServiceCommitmentsForSalesLine(Rec, false); | ||
| end; | ||
|
|
||
| internal procedure AddSalesServiceCommitmentsForSalesLine(var SalesLine: Record "Sales Line"; SkipAddAdditionalSalesServComm: Boolean) |
There was a problem hiding this comment.
Subscription line attachment bypasses sales-line triggers
Making AddSalesServiceCommitmentsForSalesLine public allows partner extensions to attach subscription package commitments to any Sales Line record outside the normal OnAfterInsert/OnAfterModify trigger flow. This can attach commitments to lines that should not carry subscriptions, or double-add commitments if called together with the event-driven path.
Recommendation:
- Add an
[InherentPermissions]attribute and consider exposing anOnBeforeAddSalesServiceCommitmentsForSalesLineintegration event so that partners can influence the existing trigger-driven call rather than bypassing it entirely.
| internal procedure AddSalesServiceCommitmentsForSalesLine(var SalesLine: Record "Sales Line"; SkipAddAdditionalSalesServComm: Boolean) | |
| [InherentPermissions(PermissionObjectType::TableData, Database::"Sales Subscription Line", 'IM')] | |
| procedure AddSalesServiceCommitmentsForSalesLine(var SalesLine: Record "Sales Line"; SkipAddAdditionalSalesServComm: Boolean) |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| UnitCost := NewUnitCost; | ||
| end; | ||
|
|
||
| internal procedure ResetCalledFromExtendContract() |
There was a problem hiding this comment.
ResetCalledFromExtendContract lacks pairing contract
Exposing ResetCalledFromExtendContract publicly alongside SetUnitPriceAndUnitCostFromExtendContract without documenting a call-pair contract means partners could call Reset at arbitrary points, unexpectedly zeroing UnitPrice/UnitCost mid-flow when another caller has legitimately set them. There is no re-entrancy guard or usage counter protecting the state.
Recommendation:
- Document or enforce that these methods must be used as a matched set. Consider returning a disposable context object or using a try-finally guard pattern to ensure the reset is always paired with the set.
| internal procedure ResetCalledFromExtendContract() | |
| /// <summary>Must always be called in a try-finally block after SetUnitPriceAndUnitCostFromExtendContract.</summary> | |
| procedure ResetCalledFromExtendContract() | |
| begin | |
| CalledFromExtendContract := false; | |
| UnitPrice := 0; | |
| UnitCost := 0; | |
| end; |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| end; | ||
|
|
||
| internal procedure CreateContractInvoicesFromUsageDataImport(ServicePartner: Enum "Service Partner"; ContractNoFilter: Text; ContractLineFilter: Text; BillingRhytmFilter: Text) | ||
| procedure CreateContractInvoicesFromUsageDataImport(ServicePartner: Enum "Service Partner"; ContractNoFilter: Text; ContractLineFilter: Text; BillingRhytmFilter: Text) |
There was a problem hiding this comment.
Financial billing API exposed without permission guard
Removing Access = Internal and the internal modifier from CreateContractInvoicesFromUsageDataImport allows any partner extension to invoke invoice creation for arbitrary contract/line filters. The method has no [InherentPermissions] attribute, so there is no explicit declaration of what permissions callers require; a misconfigured extension could trigger billing runs it should not be allowed to start.
Recommendation:
- Add an
[InherentPermissions]attribute that declares the minimum tabledata permissions needed, or keep the codeunitAccess = Internaland instead expose the functionality via a dedicated integration event.
| procedure CreateContractInvoicesFromUsageDataImport(ServicePartner: Enum "Service Partner"; ContractNoFilter: Text; ContractLineFilter: Text; BillingRhytmFilter: Text) | |
| [InherentPermissions(PermissionObjectType::TableData, Database::"Billing Line", 'R')] | |
| procedure CreateContractInvoicesFromUsageDataImport(ServicePartner: Enum "Service Partner"; ContractNoFilter: Text; ContractLineFilter: Text; BillingRhytmFilter: Text) |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| CreateCustomerInvoices(CustomerContractFilter, CustomerContractLineFilter); | ||
| end; | ||
|
|
||
| internal procedure CollectVendorContractsAndCreateInvoices(var UsageDataImport: Record "Usage Data Import") |
There was a problem hiding this comment.
Vendor invoice creation publicly callable without guards
Making CollectVendorContractsAndCreateInvoices public allows partner code to initiate vendor contract invoice creation for any UsageDataImport record set. No [InherentPermissions] attribute documents the required access level, and there are no pre-conditions validating the caller's intent or the state of the import records.
Recommendation:
- Annotate with
[InherentPermissions]declaring the minimum permissions needed (e.g., indirect Modify on billing tables), or restrict to an integration event pattern so that partner logic can hook in without taking full control of the invoicing flow.
| internal procedure CollectVendorContractsAndCreateInvoices(var UsageDataImport: Record "Usage Data Import") | |
| [InherentPermissions(PermissionObjectType::TableData, Database::"Usage Data Import", 'RM')] | |
| procedure CollectVendorContractsAndCreateInvoices(var UsageDataImport: Record "Usage Data Import") |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
|
|
||
| enum 8057 "Customer Rec. Billing Grouping" | ||
| { | ||
| Extensible = false; |
There was a problem hiding this comment.
Extensible enum breaks billing document creation
CreateBillingDocuments.Codeunit.al contains two case CustomerRecurringBillingGrouping of … end; blocks (lines 58–64 and 737–742) with no else clause. When a partner adds a new grouping value, the outer case at line 58 silently creates no billing documents, and the inner case at line 737 leaves PartnerNo empty, which would create billing documents with a blank customer.
Recommendation:
- Before merging, add
else Error(...)or anOnBeforeProcessBillingLinesintegration event handler in both case blocks inCreateBillingDocuments.Codeunit.also that unrecognised grouping values fail fast rather than silently misbehaving.
| Extensible = false; | |
| case CustomerRecurringBillingGrouping of | |
| CustomerRecurringBillingGrouping::Contract: | |
| CreateSalesDocumentsPerContract(); | |
| CustomerRecurringBillingGrouping::"Sell-to Customer No.", | |
| CustomerRecurringBillingGrouping::"Bill-to Customer No.": | |
| CreateSalesDocumentsPerCustomer(); | |
| else | |
| Error(UnhandledBillingGroupingErr, CustomerRecurringBillingGrouping); | |
| end; |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
|
|
||
| enum 8058 "Vendor Rec. Billing Grouping" | ||
| { | ||
| Extensible = false; |
There was a problem hiding this comment.
Extensible enum breaks vendor billing creation
CreateBillingDocuments.Codeunit.al has two case VendorRecurringBillingGrouping of … end; blocks (lines 66–72 and 748–752) with no else clause. A partner-added grouping value will cause the first block to create no purchase documents and the second block to leave PartnerNo empty, silently producing billing documents with a blank vendor.
Recommendation:
- Add
else Error(...)to both case blocks inCreateBillingDocuments.Codeunit.alforVendorRecurringBillingGrouping, mirroring the existingOnGetAdditionalLineTextElseCasepattern used forContractInvoiceTextType.
| Extensible = false; | |
| case VendorRecurringBillingGrouping of | |
| VendorRecurringBillingGrouping::Contract: | |
| CreatePurchaseDocumentsPerContract(); | |
| VendorRecurringBillingGrouping::"Pay-to Vendor No.", | |
| VendorRecurringBillingGrouping::"Buy-from Vendor No.": | |
| CreatePurchaseDocumentsPerVendor(); | |
| else | |
| Error(UnhandledBillingGroupingErr, VendorRecurringBillingGrouping); | |
| end; |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
|
|
||
| enum 8008 "Usage Based Billing Doc. Type" | ||
| { | ||
| Extensible = false; |
There was a problem hiding this comment.
Extensible enum leaves cleanup logic with silent gaps
Document cleanup logic in UsageBasedContrSubscribers.Codeunit.al (lines 158–164 and 186–192) uses if … = ::Invoice … else if … = ::"Credit Memo" chains. When a partner adds a new UsageBasedBillingDocType value, deleted sales/purchase header records will not trigger any cleanup of UsageDataBilling rows, leaving stale billing references that corrupt usage reconciliation.
Recommendation:
- Expose an
OnAfterCleanupUsageDataBillingForDocumentTypeintegration event in the document-cleanup local procedures so that partners extending the enum can implement the corresponding cleanup logic.
| Extensible = false; | |
| [IntegrationEvent(false, false)] | |
| local procedure OnAfterCleanupUsageDataBillingForDocumentType(ServicePartner: Enum "Service Partner"; UsageBasedBillingDocType: Enum "Usage Based Billing Doc. Type"; DocumentNo: Code[20]; var IsHandled: Boolean) | |
| begin | |
| end; |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
|
Thank you for addressing the However, we noticed that in both codeunits, while Codeunit 8020 "Personalization Data Mgmt."The two procedures inside remain internal procedure SetDataPagePersonalization(ObjectType: Option ,,,Report,,,XMLport,,Page; ObjectID: Text; ValueName: Code[40]; Value: Text)
internal procedure GetDataPagePersonalization(ObjectType: Option ,,,Report,,,XMLport,,Page; ObjectID: Text; ValueName: Code[40]; var Value: Text): BooleanThese are the only procedures in this codeunit and are exactly the ones we need to call Expected result: codeunit 8020 "Personalization Data Mgmt."
{
procedure SetDataPagePersonalization(...)
procedure GetDataPagePersonalization(...)Codeunit 8028 "Usage Based Contr. Subscribers"The procedure inside remains internal procedure CreateContractInvoicesFromUsageDataImport(ServicePartner: Enum "Service Partner"; ContractNoFilter: Text; ContractLineFilter: Text; BillingRhytmFilter: Text)This is the core procedure of the codeunit and the one we need to call from our extension Expected result: codeunit 8028 "Usage Based Contr. Subscribers"
{
procedure CreateContractInvoicesFromUsageDataImport(...)Could the |
|
Patrik Müller (@pamura1977) push the changes you need. Then lets merge it. |
Agentic PR Review - Round 1Recommendation: Request ChangesWhat this PR doesThis Slice removes The visibility-only procedure changes mostly keep the existing behavior, but the diff also turns closed enum domains into permanent extension contracts. Some of those domains are still processed by case statements and conversions that only understand the built-in values. That means custom enum values can compile but are not actually supported at runtime. SuggestionsS1 - Keep document type enums closed S2 - Handle custom billing grouping values S3 - Do not expose temporary internal state S4 - Link the approved Slice work item Risk assessment and necessityRisk: This PR creates a permanent public contract. Once partners compile against these procedures or enum values, removing or tightening them becomes a breaking change. The highest risk is the newly extensible document and grouping enums: several billing, posting, deferral, and usage-billing paths still assume only the built-in values. Necessity: The partner extension scenario is valid, and pure visibility changes do not always need behavioral tests. The scope is not yet safe because some exposed members look like internal coordination helpers, and some extensible enums do not have complete custom-value handling. The missing
|
|
Magnus Hartvig Grønbech (@Groenbech96) alexei-dobriansky Thank you both, and thank you for the detailed review. We have gone through the suggestions. Here is our position on S1 to S3. S1 - Keep document type enums closedYou are right. Please revert to
We do not have an alternative solution for our order-based scenario yet, but we will S2 - Handle custom billing grouping valuesConfirmed on the technical point. You offered two ways forward: add a deliberate extension point with The page already publishes [IntegrationEvent(false, false)]
local procedure OnBeforeProcessBillingLines(var BillingLine: Record "Billing Line"; var DocumentDate: Date; var PostingDate: Date; var CustomerRecBillingGrouping: Enum "Customer Rec. Billing Grouping"; var VendorRecBillingGrouping: Enum "Vendor Rec. Billing Grouping"; var PostDocuments: Boolean; var IsHandled: Boolean)With that safeguard in place, a partner value can no longer fall through silently: either S3 - Do not expose temporary internal stateOn closer inspection we probably do not need these two procedures at all. Page 8002 already calls Please keep as
Summary of our requestPutting it together, our request for this pull request is:
With the We hope this helps to move the pull request forward. Please let us know if you need |
I re-checked the current state of the branch: the three procedures in question ( |
|
Magnus Hartvig Grønbech (@Groenbech96) alexei-dobriansky Apologies for the confusion in the last few comments on this thread. Between the review response and a follow-up correction, our position may have gotten a bit muddled. To make sure we're aligned, here is a clean summary of where we stand on S1–S3: S1 - Document type enums (Enum 8054 "Rec. Billing Document Type", Enum 8008 "Usage Based Billing Doc. Type") S2 - Billing grouping enums (Enum 8057 "Customer Rec. Billing Grouping", Enum 8058 "Vendor Rec. Billing Grouping") [IntegrationEvent(false, false)]
local procedure OnBeforeProcessBillingLines(var BillingLine: Record "Billing Line"; var DocumentDate: Date; var PostingDate: Date; var CustomerRecBillingGrouping: Enum "Customer Rec. Billing Grouping"; var VendorRecBillingGrouping: Enum "Vendor Rec. Billing Grouping"; var PostDocuments: Boolean; var IsHandled: Boolean)With that flag in place, a custom grouping value can no longer fall through silently: either a subscriber handles it, or the standard case runs exactly as it does today. S3 - Internal state procedures on Table 8057 ( Summary of our request for this PR:
Since these code changes were originally proposed by you, could you let us know whether you'd like to push them directly, or whether you'd prefer we do it on our side? And more generally, is there anything further you need from us to move this forward? Thanks again for the detailed review, and sorry again for the back-and-forth. |
Agentic PR Review - Round 1Recommendation: Accept with SuggestionsWhat this PR doesThis PR opens Subscription Billing extensibility by removing internal/Access = Internal from selected procedures/codeunits and by setting five enums to Extensible = true. The diff is consistent with that goal and does not change business logic, posting flow, or data writes, so it addresses the AB#641732 partner-compilation scenario directly. SuggestionsS1 - Add a small extensibility regression check Risk assessment and necessityRisk: Runtime regression risk is low because only access modifiers and enum extensibility flags changed. The main risk is API-surface growth (more callable procedures and extensible enums), which is expected for this request but should be treated as a long-term contract. Necessity: The change is necessary to remove AL0161-style partner extension blockers and enable customization points in Subscription Billing. Scope is targeted to the listed procedures/codeunits/enums and is appropriate for this work item.
|
Multiple procedures in Subscription Billing are declared
internaland several enums haveExtensible = false, blocking partner extensions with AL0161 compiler errors or preventing custom enum values.Part 1 ΓÇö Procedures: removed
internalmodifierExtendContract(Page 8002):SetParameters(),SetUsageBasedParameters()Subscription Header(Table 8057):SetUnitPriceAndUnitCostFromExtendContract(),ResetCalledFromExtendContract()Subscription Package(Table 8055):FilterCodeOnPackageFilter(),ServCommPackageLineExists()Item Subscription Package(Table 8058):GetAllStandardPackageFilterForItem(), three overloads ofGetPackageFilterForItem()Usage Data Supplier Reference(Table 8015):FindSupplierReference()Usage Data Import(Table 8013):CollectVendorContractsAndCreateInvoices()Service Comm. Package Lines(Page 8058):SetItemNo(),SetShowAllPackageLines(),SetPackageCode()Sales Line(Tableextension 8054):IsContractRenewal()Sales Subscription Line Mgmt.(Codeunit 8069):AddSalesServiceCommitmentsForSalesLine()Part 2 ΓÇö Codeunits: removed
Access = InternalUsage Based Contr. Subscribers(Codeunit 8028): removed codeunit-level access restriction; promotedCreateContractInvoicesFromUsageDataImport()to publicPersonalization Data Mgmt.(Codeunit 8020): removed codeunit-level access restriction; promotedSetDataPagePersonalization()andGetDataPagePersonalization()to publicPart 3 ΓÇö Enums: set
Extensible = trueCustomer Rec. Billing GroupingVendor Rec. Billing GroupingContract Invoice Text TypeRec. Billing Document TypeUsage Based Billing Doc. TypeFixes AB#641732