Skip to content

[Subscription Billing] Remove internal modifiers and make enums extensible for partner extension support - #8387

Open
Magnus Hartvig Grønbech (Groenbech96) with Copilot wants to merge 2 commits into
mainfrom
copilot/remove-internal-from-setparameters
Open

[Subscription Billing] Remove internal modifiers and make enums extensible for partner extension support#8387
Magnus Hartvig Grønbech (Groenbech96) with Copilot wants to merge 2 commits into
mainfrom
copilot/remove-internal-from-setparameters

Conversation

Copilot AI commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Multiple procedures in Subscription Billing are declared internal and several enums have Extensible = false, blocking partner extensions with AL0161 compiler errors or preventing custom enum values.

Part 1 ΓÇö Procedures: removed internal modifier

  • ExtendContract (Page 8002): SetParameters(), SetUsageBasedParameters()
  • Subscription Header (Table 8057): SetUnitPriceAndUnitCostFromExtendContract(), ResetCalledFromExtendContract()
  • Subscription Package (Table 8055): FilterCodeOnPackageFilter(), ServCommPackageLineExists()
  • Item Subscription Package (Table 8058): GetAllStandardPackageFilterForItem(), three overloads of GetPackageFilterForItem()
  • 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 = Internal

  • Usage Based Contr. Subscribers (Codeunit 8028): removed codeunit-level access restriction; promoted CreateContractInvoicesFromUsageDataImport() to public
  • Personalization Data Mgmt. (Codeunit 8020): removed codeunit-level access restriction; promoted SetDataPagePersonalization() and GetDataPagePersonalization() to public

Part 3 ΓÇö Enums: set Extensible = true

Enum ID
Customer Rec. Billing Grouping 8057
Vendor Rec. Billing Grouping 8058
Contract Invoice Text Type 8001
Rec. Billing Document Type 8054
Usage Based Billing Doc. Type 8008

Fixes AB#641732

Copilot AI changed the title [WIP] Remove internal from SetParameters procedure [Subscription Billing] Remove internal modifiers and make enums extensible for partner extension support Jun 1, 2026
@Groenbech96
Magnus Hartvig Grønbech (Groenbech96) marked this pull request as ready for review June 1, 2026 11:58
@github-actions github-actions Bot added the AL: Apps (W1) Add-on apps for W1 label Jun 1, 2026
@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

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'

@github-actions github-actions Bot added the needs-approval Workflow runs require maintainer approval to start label Jun 1, 2026
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Privacy} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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 DataClassification annotation to the Page Data Personalization value field (or document that callers must only write DataClassification::SystemMetadata data), and expose read access through a narrower, purpose-specific method rather than a generic key-value getter.
Suggested change
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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 call ResetCalledFromExtendContract() in a try-finally block to avoid pricing state leakage.
Suggested change
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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 the Evaluate(ObjectNo, ObjectID) call in an if not Evaluate(...) guard to prevent unhandled runtime errors on malformed input.
Suggested change
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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 an OnBeforeAddSalesServiceCommitmentsForSalesLine integration event so that partners can influence the existing trigger-driven call rather than bypassing it entirely.
Suggested change
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()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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.
Suggested change
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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 codeunit Access = Internal and instead expose the functionality via a dedicated integration event.
Suggested change
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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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.
Suggested change
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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 an OnBeforeProcessBillingLines integration event handler in both case blocks in CreateBillingDocuments.Codeunit.al so that unrecognised grouping values fail fast rather than silently misbehaving.
Suggested change
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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 in CreateBillingDocuments.Codeunit.al for VendorRecurringBillingGrouping, mirroring the existing OnGetAdditionalLineTextElseCase pattern used for ContractInvoiceTextType.
Suggested change
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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 OnAfterCleanupUsageDataBillingForDocumentType integration event in the document-cleanup local procedures so that partners extending the enum can implement the corresponding cleanup logic.
Suggested change
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

@pamura1977

Copy link
Copy Markdown

Thank you for addressing the Access = Internal on Codeunit 8020 "Personalization Data Mgmt."
and Codeunit 8028 "Usage Based Contr. Subscribers".

However, we noticed that in both codeunits, while Access = Internal is being removed at
the codeunit level, the procedures inside remain declared as internal. A codeunit without
Access = Internal but with exclusively internal procedures is still not callable from a
partner extension — the AL compiler will produce an AL0161 error on any attempt to call
these procedures.


Codeunit 8020 "Personalization Data Mgmt."

The two procedures inside remain internal:

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): Boolean

These are the only procedures in this codeunit and are exactly the ones we need to call
from our extension (cloning Page 8058 "Service Comm. Package Lines" for item-variant-level
subscription package management).

Expected result:

codeunit 8020 "Personalization Data Mgmt."
{
    procedure SetDataPagePersonalization(...)
    procedure GetDataPagePersonalization(...)

Codeunit 8028 "Usage Based Contr. Subscribers"

The procedure inside remains internal:

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
to handle usage-based contract invoice creation programmatically.

Expected result:

codeunit 8028 "Usage Based Contr. Subscribers"
{
    procedure CreateContractInvoicesFromUsageDataImport(...)

Could the internal modifier be removed from all three procedures as part of this PR?

@Groenbech96

Copy link
Copy Markdown
Contributor

Patrik Müller (@pamura1977) push the changes you need. Then lets merge it.

@JesperSchulz Jesper Schulz-Wedde (JesperSchulz) added Finance GitHub request for Finance area From Fork Pull request is coming from a fork labels Jul 7, 2026
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Request Changes

What this PR does

This Slice removes internal / Access = Internal from several Subscription Billing procedures and codeunits, and changes five enums to Extensible = true. The intent is to let partner extensions call selected Subscription Billing helpers and add custom enum values.

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.

Suggestions

S1 - Keep document type enums closed
Rec. Billing Document Type and Usage Based Billing Doc. Type model real invoice and credit memo states. The surrounding posting, deferral, filter, and conversion code only handles the built-in values. Keep these enums non-extensible unless the full document flow supports partner values.

S2 - Handle custom billing grouping values
Customer Rec. Billing Grouping and Vendor Rec. Billing Grouping are used in ProcessBillingLines() case statements with no else path. A partner value would skip sales or purchase document creation. Add a deliberate extension point with IsHandled, or keep these enums closed.

S3 - Do not expose temporary internal state
SetUnitPriceAndUnitCostFromExtendContract() and ResetCalledFromExtendContract() expose the hidden CalledFromExtendContract state on Subscription Header. This is an internal coordination detail for the Extend Contract page. Use a deliberate public API for the partner scenario instead of making this state toggle permanent.

S4 - Link the approved Slice work item
The PR body has no AB# link, and the internal work-item validation check is failing. Add the Slice link so reviewers can verify the approved extensibility scope. This is important because public API changes are hard to remove later.

Risk assessment and necessity

Risk: 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 AB# link also makes the approved contract boundary unclear.


[AI-PR-REVIEW] version=1 system=github pr=8387 round=1 by=alexei-dobriansky at=2026-07-07 lastSha=fcfacacb99702e74ea868dea6500073142193fd7 suggestions=S1,S2,S3,S4

@djukicmilica Milica Đukić (djukicmilica) added the Linked Issue is linked to a Azure Boards work item label Jul 8, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone Jul 8, 2026
@ghost

Copy link
Copy Markdown

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 closed

You are right. Usage Based Doc. Type Conv. (codeunit 8024) converts only None,
Invoice and Credit Memo, and raises ConversionNotAllowedErr for anything else.
The two enums are also referenced across the deferral, posting, correction and display
paths, none of which would understand a partner value.

Please revert to Extensible = false:

  • Enum 8054 Rec. Billing Document Type
  • Enum 8008 Usage Based Billing Doc. Type

We do not have an alternative solution for our order-based scenario yet, but we will
explore whether it can be handled entirely within our own extension. Should that turn
out not to be feasible, we would come back to you with a separate issue describing the
scenario in detail.

S2 - Handle custom billing grouping values

Confirmed on the technical point. ProcessBillingLines() has no else branch, so a
partner value would silently skip document creation. Shipping the enums as extensible
without a safeguard would indeed be unsafe.

You offered two ways forward: add a deliberate extension point with IsHandled, or keep
the enums closed. We need the first option, as custom grouping is a requirement on our
side.

The page already publishes OnBeforeProcessBillingLines() with both grouping enums passed
as var parameters, so the entry point largely exists. What is missing is the
IsHandled flag that would let a subscriber take over document creation for a custom
value:

[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
a subscriber handles it, or the standard case runs as today.

S3 - Do not expose temporary internal state

On closer inspection we probably do not need these two procedures at all.

Page 8002 already calls SetUnitPriceAndUnitCostFromExtendContract() and
ResetCalledFromExtendContract() itself, passing its own UnitPrice and UnitCostLCY
variables. Since those two variables are covered by our additional issue #8856, we can hopefully supply
our own prices from a PageExtension without the state toggle ever being exposed.

Please keep as internal:

  • Table 8057 SetUnitPriceAndUnitCostFromExtendContract()
  • Table 8057 ResetCalledFromExtendContract()

Summary of our request

Putting it together, our request for this pull request is:

  1. Revert the two document type enums to Extensible = false (S1)
  2. Keep the two Subscription Header procedures as internal (S3)
  3. Add the IsHandled parameter to OnBeforeProcessBillingLines() and keep the two
    billing grouping enums as Extensible = true (S2)
  4. Keep the remaining visibility changes unchanged

With the IsHandled safeguard in place, the concern raised in S2 no longer applies: a
custom grouping value can no longer fall through silently, because either a subscriber
handles it or the standard case runs exactly as it does today.

We hope this helps to move the pull request forward. Please let us know if you need
anything further from our side.

@pamura1977

Copy link
Copy Markdown

Patrik Müller (@pamura1977) push the changes you need. Then lets merge it.

I re-checked the current state of the branch: the three procedures in question (SetDataPagePersonalization, GetDataPagePersonalization in codeunit 8020, and CreateContractInvoicesFromUsageDataImport in codeunit 8028) are already public — the internal modifier appears to have already been removed as part of the original commit. My previous comment was therefore incorrect, and I apologize for the confusion.

@pamura1977

Copy link
Copy Markdown

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")
We agree with your assessment. Please keep both enums as Extensible = false. We don't have an alternative solution for our order-based scenario yet, but we're exploring whether it can be handled entirely within our own extension. If that turns out not to be feasible, we'll come back with a separate issue describing the scenario.

S2 - Billing grouping enums (Enum 8057 "Customer Rec. Billing Grouping", Enum 8058 "Vendor Rec. Billing Grouping")
We need these to remain extensible, as custom grouping is a requirement on our side. To make that safe, we'd ask for an IsHandled parameter to be added to the existing OnBeforeProcessBillingLines() event:

[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 (SetUnitPriceAndUnitCostFromExtendContract(), ResetCalledFromExtendContract())
No longer needed on our side. Please keep both as internal. Page 8002 already calls them itself, and our related request in issue #8856 / PR #9715 covers what we actually need here.

Summary of our request for this PR:

  1. Revert Enum 8054 and Enum 8008 to Extensible = false (S1)
  2. Add the IsHandled parameter to OnBeforeProcessBillingLines(), keep Enum 8057 and Enum 8058 as Extensible = true (S2)
  3. Keep the two Table 8057 procedures internal (S3)
  4. Keep the remaining visibility changes as they are

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.

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Accept with Suggestions

What this PR does

This 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.

Suggestions

S1 - Add a small extensibility regression check
Consider adding one test-app scenario that consumes one now-public procedure and one newly extensible enum value. This helps prevent accidental rollback of these access settings in future refactors.

Risk assessment and necessity

Risk: 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.


[AI-PR-REVIEW] version=1 system=github pr=8387 round=1 by=alexei-dobriansky at=2026-08-04T05:48:12Z lastSha=fcfacacb99702e74ea868dea6500073142193fd7 suggestions=S1

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

Labels

AL: Apps (W1) Add-on apps for W1 Finance GitHub request for Finance area From Fork Pull request is coming from a fork Linked Issue is linked to a Azure Boards work item

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: [Subscription Billing] Internal procedures and non-extensible enums prevent partner extensions

6 participants