Skip to content

[Shopify] Persist current B2B Catalog row before disabling Sync Prices on other lines#8211

Open
onbuyuka wants to merge 1 commit into
mainfrom
bugs/630273-sync-prices-current-line-lost
Open

[Shopify] Persist current B2B Catalog row before disabling Sync Prices on other lines#8211
onbuyuka wants to merge 1 commit into
mainfrom
bugs/630273-sync-prices-current-line-lost

Conversation

@onbuyuka
Copy link
Copy Markdown
Contributor

@onbuyuka onbuyuka commented May 19, 2026

Summary

Reactivation of bug 630273. PR #7585 introduced a confirmation dialog for the Sync Prices toggle on B2B Catalog rows that share a catalog ID across multiple companies. PM testing showed that when the user clicked Yes on the dialog, the toggle was lost on the current row too — not just the other line — so nothing ended up with Sync Prices enabled.

Root cause

"Sync Prices" OnValidate called OtherCatalog.ModifyAll("Sync Prices", false) while the user's change Rec."Sync Prices" := true was still only in memory. The client refresh that follows the ModifyAll re-read the current row from the database (still false), clobbering the in-memory true.

The existing SyncPricesConfirmDisablesOtherLine test asserted the in-memory value of the current row after Validate + Modify(true), so it passed even though the persisted state was wrong.

Fix

Persist the current row with Modify() before the ModifyAll on other rows. This way the database already reflects the user's intent if the page refreshes.

Test plan

  • Strengthen SyncPricesConfirmDisablesOtherLine to re-read Catalog2 from the database (mirroring the existing pattern used for Catalog1 in the same test) so the regression would be caught next time.
  • SyncPricesCancelKeepsBothUnchanged — unchanged, still passes.
  • SyncPricesNoConfirmWhenNoDuplicate — unchanged, still passes.
  • All three tests pass locally against the published App with this fix.

Fixes AB#630273

…s on other lines

When the user enables `Sync Prices` on a B2B Catalog row whose catalog ID
is already in use on another company row and clicks `Yes` on the
confirmation dialog, the current row's toggle was being lost. The OnValidate
trigger called `OtherCatalog.ModifyAll("Sync Prices", false)` against the
same table before the page had a chance to persist the user's change on the
current row. The resulting client refresh re-read the current row from the
database (still `Sync Prices = false`), clobbering the in-memory `true`.

Persist the current Rec with `Modify()` before the `ModifyAll` on the
other rows so the database already reflects the user's intent if the page
refreshes.

Also strengthen `SyncPricesConfirmDisablesOtherLine` to re-read Catalog2
from the database (mirroring the existing pattern used for Catalog1) so the
test would catch this regression.

Fixes AB#630273

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@onbuyuka onbuyuka requested a review from a team as a code owner May 19, 2026 17:36
@github-actions github-actions Bot added the AL: Apps (W1) Add-on apps for W1 label May 19, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone May 19, 2026
@github-actions
Copy link
Copy Markdown
Contributor

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

Modify() skips OnModify trigger for current record

Calling Modify() (RunTrigger defaults to false) persists the current record's Sync Prices = true before disabling other catalogs, which is the correct intent. However, any business logic registered in the table's OnModify trigger will not execute for this save, potentially leaving dependent data inconsistent.

Recommendation:

  • If no OnModify trigger exists on this table (or it is safe to skip), add a comment explaining why RunTrigger = false is intentional. If OnModify contains relevant logic, use Modify(true) and guard against recursion with a flag.
// Persist current record first; RunTrigger=false is intentional to avoid
// re-entering CheckDuplicateSyncPrices and causing infinite recursion.
Modify();

Line mapping was unavailable, so this was posted as an issue comment.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant