Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@

namespace Microsoft.Integration.Shopify;

using Microsoft.Finance.GeneralLedger.Journal;
using Microsoft.Sales.Document;
using Microsoft.Sales.History;
using Microsoft.Sales.Posting;
using Microsoft.Warehouse.Activity;

codeunit 30262 "Shpfy Document Link Mgt."
{
Expand Down Expand Up @@ -63,18 +65,64 @@ codeunit 30262 "Shpfy Document Link Mgt."
end;

[EventSubscriber(ObjectType::Codeunit, Codeunit::"Sales-Post", 'OnAfterPostSalesDoc', '', true, false)]
local procedure OnAfterSalesPosting(var SalesHeader: Record "Sales Header"; PreviewMode: Boolean; SalesShptHdrNo: Code[20]; SalesInvHdrNo: Code[20]; RetRcpHdrNo: Code[20]; SalesCrMemoHdrNo: Code[20])
local procedure OnAfterSalesPosting(var SalesHeader: Record "Sales Header"; PreviewMode: Boolean; CommitIsSuppressed: Boolean; InvtPickPutaway: Boolean; SalesShptHdrNo: Code[20]; SalesInvHdrNo: Code[20]; RetRcpHdrNo: Code[20]; SalesCrMemoHdrNo: Code[20])
var
SalesShipmentHeader: Record "Sales Shipment Header";
SalesInvoiceLine: Record "Sales Invoice Line";
SalesShipments: List of [Code[20]];
ShpfyAutoPostTransactions: Codeunit "Shpfy Auto Post Transactions";
begin
if SalesHeader.IsTemporary() then
exit;

if PreviewMode then
exit;

CreateDocLinksToBCDocs(SalesHeader, SalesShptHdrNo, SalesInvHdrNo, RetRcpHdrNo, SalesCrMemoHdrNo);

if CommitIsSuppressed or InvtPickPutaway then
exit;

// CreateDocLinksToBCDocs can open a write transaction after Sales-Post's final commit.
// Flush those links before invoking the isolated Codeunit.Run posting operations.
Commit();
ShpfyAutoPostTransactions.AutoPostTransactions(SalesInvHdrNo, SalesCrMemoHdrNo, HasJournalPermissions());
end;

[EventSubscriber(ObjectType::Codeunit, Codeunit::"Whse.-Activity-Post", 'OnAfterPostWhseActivityCompleted', '', false, 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\ —\ Testing}$

The PR adds a separate warehouse-posting entry point for Invt. Pick/Invt. Put-away completion, but the new test codeunit only covers direct sales posting through LibrarySales.PostSalesDocument(...). Add an integration test that posts through the warehouse flow and asserts auto-posting still runs afterward, otherwise this new event path can regress without any failing test.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

local procedure OnAfterPostWhseActivityCompleted(WhseActivHeader: Record "Warehouse Activity Header"; var SalesHeader: Record "Sales Header"; SuppressCommit: Boolean; IsPreview: Boolean)
var
ShpfyAutoPostTransactions: Codeunit "Shpfy Auto Post Transactions";
begin
if not (WhseActivHeader.Type in [WhseActivHeader.Type::"Invt. Pick", WhseActivHeader.Type::"Invt. Put-away"]) then
exit;
if SuppressCommit or IsPreview or (SalesHeader."Last Posting No." = '') then
exit;

Commit();
case SalesHeader."Document Type" of
SalesHeader."Document Type"::Order,
SalesHeader."Document Type"::Invoice:
ShpfyAutoPostTransactions.AutoPostTransactions(SalesHeader."Last Posting No.", '', HasJournalPermissions());
SalesHeader."Document Type"::"Return Order",
SalesHeader."Document Type"::"Credit Memo":
ShpfyAutoPostTransactions.AutoPostTransactions('', SalesHeader."Last Posting No.", HasJournalPermissions());

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\ —\ Agent}$

HasJournalPermissions() in ShpfyDocumentLinkMgt.Codeunit.al only checks Read/Write TablePermission on 'Gen. Journal Batch' and 'Gen. Journal Line' before deciding automatic posting is authorized, but the actual posting path (Gen. Jnl.-Post Batch, invoked from ShpfyAutoPostTransactions.PostTransaction) also writes to G/L Entry, Cust. Ledger Entry, VAT Entry and other posting-related tables depending on setup. A user who passes this check can still hit an unhandled permission error deep inside GenJnlPostBatch.Run, which would be misreported as a generic 'Posting' stage failure rather than the real 'Authorization' failure the eligibility gate is meant to produce. Broaden the permission probe (or catch permission-specific errors from the post call) so authorization failures are consistently attributed and surfaced through the existing InsufficientPermissionsReasonLbl path.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

end;
end;

local procedure HasJournalPermissions(): Boolean
var
GenJournalBatch: Record "Gen. Journal Batch";
GenJournalLine: Record "Gen. Journal Line";
begin
exit(
GenJournalBatch.ReadPermission() and GenJournalBatch.WritePermission() and
GenJournalLine.ReadPermission() and GenJournalLine.WritePermission());
end;

local procedure CreateDocLinksToBCDocs(var SalesHeader: Record "Sales Header"; SalesShptHdrNo: Code[20]; SalesInvHdrNo: Code[20]; RetRcpHdrNo: Code[20]; SalesCrMemoHdrNo: Code[20])
Comment thread
onbuyuka marked this conversation as resolved.
var
SalesShipmentHeader: Record "Sales Shipment Header";
SalesInvoiceLine: Record "Sales Invoice Line";
SalesShipments: List of [Code[20]];
begin
DocLinkToBCDoc.SetRange("Document Type", ShpfyBCDocumentTypeConvert.Convert(SalesHeader."Document Type"));
DocLinkToBCDoc.SetRange("Document No.", SalesHeader."No.");
DocLinkToBCDoc.SetCurrentKey("Document Type", "Document No.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ permissionset 30104 "Shpfy - Objects"
report "Shpfy Translator" = X,
codeunit "Company Details Checklist Item" = X,
codeunit "Shpfy Authentication Mgt." = X,
codeunit "Shpfy Auto Gen. Jnl.-Post" = X,
codeunit "Shpfy Auto Post Eligibility" = X,
codeunit "Shpfy Auto Post Finalize" = X,
codeunit "Shpfy Auto Post Transactions" = X,
codeunit "Shpfy Background Syncs" = X,
codeunit "Shpfy Balance Today" = X,
codeunit "Shpfy Base64" = X,
Expand Down Expand Up @@ -301,6 +305,7 @@ permissionset 30104 "Shpfy - Objects"
page "Shpfy Customers" = X,
page "Shpfy Data Capture List" = X,
page "Shpfy Disputes" = X,
page "Shpfy Filter Transactions" = X,
page "Shpfy Fulfillment Order Card" = X,
page "Shpfy Fulfillment Order Lines" = X,
page "Shpfy Fulfillment Orders" = X,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
// ------------------------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.
// ------------------------------------------------------------------------------------------------

namespace Microsoft.Integration.Shopify;

using Microsoft.Finance.GeneralLedger.Journal;
using Microsoft.Finance.GeneralLedger.Posting;

/// <summary>
/// Codeunit Shpfy Auto Gen. Jnl.-Post (ID 30422).
/// Creates a dedicated, single-use journal batch (cloned from the configured one) and builds the general
/// journal line(s) for a single Shopify order/refund payment transaction into it. It is invoked through
/// Codeunit.Run so that any failure while creating the batch or building lines is trapped and rolled back
/// without leaving a batch or line behind. While bound, it also pre-confirms the "posting after working
/// date" prompt so the automatic posting stays non-interactive.
/// </summary>
codeunit 30422 "Shpfy Auto Gen. Jnl.-Post"
{
Access = Internal;
EventSubscriberInstance = Manual;
TableNo = "Shpfy Order Transaction";

trigger OnRun()
begin
CreateBatchAndBuildLines(Rec);
end;

var
PaymentMethodMapping: Record "Shpfy Payment Method Mapping";
PostingDate: Date;
IsolatedTemplateName: Code[10];
IsolatedBatchName: Code[10];

internal procedure SetParameters(NewPaymentMethodMapping: Record "Shpfy Payment Method Mapping"; NewPostingDate: Date)
begin
PaymentMethodMapping := NewPaymentMethodMapping;
PostingDate := NewPostingDate;
Clear(IsolatedTemplateName);
Clear(IsolatedBatchName);
end;

internal procedure GetIsolatedBatch(var NewTemplateName: Code[10]; var NewBatchName: Code[10])
begin
NewTemplateName := IsolatedTemplateName;
NewBatchName := IsolatedBatchName;
end;

local procedure CreateBatchAndBuildLines(var OrderTransaction: Record "Shpfy Order Transaction")
var
SuggestPayments: Report "Shpfy Suggest Payments";
begin
CreateIsolatedBatch();
SuggestPayments.SetJournalParameters(IsolatedTemplateName, IsolatedBatchName, PostingDate);
SuggestPayments.GetOrderTransactions(OrderTransaction);
SuggestPayments.CreateGeneralJournalLines();
end;

local procedure CreateIsolatedBatch()
var
ConfiguredBatch: Record "Gen. Journal Batch";
IsolatedBatch: Record "Gen. Journal Batch";
begin
ConfiguredBatch.Get(PaymentMethodMapping."Auto-Post Jnl. Template", PaymentMethodMapping."Auto-Post Jnl. Batch");
IsolatedBatch := ConfiguredBatch;
IsolatedBatch.Name := GetUniqueBatchName(ConfiguredBatch."Journal Template Name");
IsolatedBatch.Insert(true);
IsolatedTemplateName := IsolatedBatch."Journal Template Name";
IsolatedBatchName := IsolatedBatch.Name;
end;

local procedure GetUniqueBatchName(TemplateName: Code[10]): Code[10]
var
ExistingBatch: Record "Gen. Journal Batch";
CandidateName: Code[10];
begin
repeat
CandidateName := CopyStr('SHPFY' + CopyStr(DelChr(Format(CreateGuid()), '=', '{}-'), 1, 5), 1, 10);
until not ExistingBatch.Get(TemplateName, CandidateName);
exit(CandidateName);
end;

[EventSubscriber(ObjectType::Codeunit, Codeunit::"Gen. Jnl.-Post Batch", 'OnBeforeCheckLine', '', false, false)]
local procedure PreconfirmWorkingDateOnBeforeCheckLine(var PostingAfterWorkingDateConfirmed: Boolean)
begin
PostingAfterWorkingDateConfirmed := true;
end;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// ------------------------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.
// ------------------------------------------------------------------------------------------------

namespace Microsoft.Integration.Shopify;

using Microsoft.Sales.Document;
using Microsoft.Sales.History;

/// <summary>
/// Codeunit Shpfy Auto Post Eligibility (ID 30424).
/// Keeps automatic-posting and transaction-list filtering predicates aligned.
/// </summary>
codeunit 30424 "Shpfy Auto Post Eligibility"
{
Access = Internal;

internal procedure IsReadyToPost(OrderTransaction: Record "Shpfy Order Transaction"; var PaymentMethodMapping: Record "Shpfy Payment Method Mapping"): Boolean
begin
exit(
GetPaymentMethodMapping(OrderTransaction, PaymentMethodMapping) and
IsMappingConfigured(PaymentMethodMapping) and
IsTransactionPostable(OrderTransaction));
end;

internal procedure GetPaymentMethodMapping(OrderTransaction: Record "Shpfy Order Transaction"; var PaymentMethodMapping: Record "Shpfy Payment Method Mapping"): Boolean
begin
exit(PaymentMethodMapping.Get(OrderTransaction.Shop, OrderTransaction.Gateway, OrderTransaction."Credit Card Company"));
end;

internal procedure IsMappingConfigured(PaymentMethodMapping: Record "Shpfy Payment Method Mapping"): Boolean
begin
exit(
PaymentMethodMapping."Post Automatically" and
(PaymentMethodMapping."Auto-Post Jnl. Template" <> '') and
(PaymentMethodMapping."Auto-Post Jnl. Batch" <> ''));
end;

internal procedure IsTransactionPostable(OrderTransaction: Record "Shpfy Order Transaction"): 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\ —\ Testing}$

The new shared eligibility predicate in Shpfy Auto Post Eligibility now drives both automatic posting and the new postable-transactions filter, but the added tests only exercise the open-sales-document branch. Add direct tests for the other gating branches (Status <> Success, unsupported Type, Used = true, and missing posted invoice/credit memo) so the page filter and posting routine cannot silently drift apart.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

begin
if OrderTransaction.Status <> OrderTransaction.Status::Success then
exit(false);
if not (OrderTransaction.Type in [OrderTransaction.Type::Capture, OrderTransaction.Type::Sale, OrderTransaction.Type::Refund]) then
exit(false);

OrderTransaction.CalcFields(Used);

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\ —\ Performance}$

"Shpfy Auto Post Eligibility" calls OrderTransaction.CalcFields(Used) inside a helper that the new posting loop and the new "Filter Postable Transactions" page loop invoke once per row. That is the exact per-row FlowField pattern this rule warns about: each iteration can trigger another FlowField query. Calculate Used with SetAutoCalcFields(Used) before FindSet, or remove the redundant reread where the caller already filtered Used = false.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

if OrderTransaction.Used then
exit(false);
if OpenSalesDocumentExists(OrderTransaction) then
exit(false);
exit(PostedDocumentExists(OrderTransaction));
end;

local procedure OpenSalesDocumentExists(OrderTransaction: Record "Shpfy Order Transaction"): Boolean
var
SalesHeader: Record "Sales Header";
begin
if OrderTransaction.Type = OrderTransaction.Type::Refund then begin
if OrderTransaction."Refund Id" = 0 then
exit(true);
SalesHeader.SetRange("Shpfy Refund Id", OrderTransaction."Refund Id");
exit(not SalesHeader.IsEmpty());
end;

if OrderTransaction."Shopify Order Id" = 0 then
exit(true);
SalesHeader.SetRange("Shpfy Order Id", OrderTransaction."Shopify Order Id");
exit(not SalesHeader.IsEmpty());
end;

local procedure PostedDocumentExists(OrderTransaction: Record "Shpfy Order Transaction"): Boolean
var
SalesCrMemoHeader: Record "Sales Cr.Memo Header";
SalesInvoiceHeader: Record "Sales Invoice Header";
begin
if OrderTransaction.Type = OrderTransaction.Type::Refund then begin
SalesCrMemoHeader.SetRange("Shpfy Refund Id", OrderTransaction."Refund Id");
exit(not SalesCrMemoHeader.IsEmpty());
end;

SalesInvoiceHeader.SetRange("Shpfy Order Id", OrderTransaction."Shopify Order Id");
exit(not SalesInvoiceHeader.IsEmpty());
end;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// ------------------------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.
// ------------------------------------------------------------------------------------------------

namespace Microsoft.Integration.Shopify;

using Microsoft.Finance.GeneralLedger.Journal;

/// <summary>
/// Codeunit Shpfy Auto Post Finalize (ID 30423).
/// Runs cleanup and skipped-record persistence in isolated, trappable transactions.
/// </summary>
codeunit 30423 "Shpfy Auto Post Finalize"
{
Access = Internal;

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}$

Codeunit 30423 "Shpfy Auto Post Finalize" places privileged general-journal cleanup behind Access = Internal only: it holds direct delete rights on Gen. Journal Batch and Gen. Journal Line and exposes internal setters that accept arbitrary batch identifiers before OnRun deletes the batch. Internal is not an authorization boundary because companion apps listed in internalsVisibleTo can call those setters, so the destructive operation needs an explicit ownership/authorization check instead of relying on internal visibility.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

TableNo = "Shpfy Order Transaction";
Permissions = tabledata "Gen. Journal Batch" = rimd,
tabledata "Gen. Journal Line" = rimd;

trigger OnRun()
begin
if CleanupBatch then
RemoveIsolatedBatch();
if FailureReason <> '' then
LogFailure(Rec);
end;

var
TemplateName: Code[10];
BatchName: Code[10];
FailureReason: Text;
CleanupBatch: Boolean;

internal procedure SetCleanupParameters(NewTemplateName: Code[10]; NewBatchName: Code[10])
begin
TemplateName := NewTemplateName;
BatchName := NewBatchName;
CleanupBatch := true;
Clear(FailureReason);
end;

internal procedure SetFailureParameters(NewFailureReason: Text)
begin
Clear(TemplateName);
Clear(BatchName);
CleanupBatch := false;
FailureReason := NewFailureReason;
end;

local procedure RemoveIsolatedBatch()
var
IsolatedBatch: Record "Gen. Journal Batch";
begin
if IsolatedBatch.Get(TemplateName, BatchName) then
IsolatedBatch.Delete(true);
end;

local procedure LogFailure(OrderTransaction: Record "Shpfy Order Transaction")
var
Shop: Record "Shpfy Shop";
SkippedRecord: Codeunit "Shpfy Skipped Record";
begin
if Shop.Get(OrderTransaction.Shop) then
SkippedRecord.LogSkippedRecord(OrderTransaction."Shopify Transaction Id", OrderTransaction.RecordId, CopyStr(FailureReason, 1, 250), Shop);
end;
}
Loading
Loading