Skip to content

MWPW-203042: Plans - English-US language grouped variations updates - #1184

Merged
afmicka merged 15 commits into
mainfrom
mwpw-203042
Sep 7, 2026
Merged

MWPW-203042: Plans - English-US language grouped variations updates#1184
afmicka merged 15 commits into
mainfrom
mwpw-203042

Conversation

@mirafedas

Copy link
Copy Markdown
Contributor

Resolves https://jira.corp.adobe.com/browse/MWPW-203042
QA Checklist: https://wiki.corp.adobe.com/display/adobedotcom/M@S+Engineering+QA+Use+Cases

Please do the steps below before submitting your PR for a code review or QA

  • C1. Cover code with Unit Tests
  • C2. Add a Nala test (double check with #fishbags if nala test is needed)
  • C3. Verify all Checks are green (unit tests, nala tests)
  • C4. PR description contains working Test Page link where the feature can be tested
  • C5: you are ready to do a demo from Test Page in PR (bonus: write a working demo script that you'll use on Thursday, you can eventually put in your PR)
  • C.6 read your Jira one more time to validate that you've addressed all AC's and nothing is missing

🧪 Nala E2E Tests

Nala tests run automatically when you open this PR.

To run Nala tests again:

  1. Add the run nala label to this PR (in the right sidebar)
  2. Tests will run automatically on the current commit
  3. Any future commits will also trigger tests as long as the label remains

To stop automatic Nala tests:

  • Remove the run nala label

Note: Tests only run on commits if the run nala label is present. Add the label whenever you need tests to run on new changes.

Test URLs:

@aem-code-sync

aem-code-sync Bot commented Aug 27, 2026

Copy link
Copy Markdown

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch.
In case there are problems, just click the checkbox below to rerun the respective action.

  • Re-sync branch
Commits

@github-actions

Copy link
Copy Markdown

SNOW Change Request Transaction ID [IO]: 0000019c1eec85a3-1fba758e

@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.74%. Comparing base (2f2e131) to head (a310a18).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1184      +/-   ##
==========================================
+ Coverage   90.71%   90.74%   +0.02%     
==========================================
  Files         318      318              
  Lines      101712   101712              
==========================================
+ Hits        92271    92301      +30     
+ Misses       9441     9411      -30     

see 7 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f2e131...a310a18. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Axelcureno Axelcureno left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Found one real bug: in applyRow, the drift check runs before the already-applied check (pzn-tag-applier.mjs:166 vs 174). Since selectRows already excludes no-op rows, expected and next are never equal for a row that reaches here — so once a row is actually migrated, liveTags == next implies liveTags != expected, and it hits the drift branch first. already-applied can never fire. Re-running the same apply (or resuming after a partial-batch failure) reports those rows as DRIFTED and exits 2 instead of reporting ALREADY-APPLIED and exiting 0. Swap the order — check sameTags(liveTags, next) first.

Same root cause bites --revert too. Also: revert doesn't auto-carry --allow-flags from the original forced apply, so a force-applied COLLISION row silently no-ops on revert unless you pass the same flag again — easy to miss since it just prints a SKIP line.

Nice guardrails otherwise — If-Match on every write, version-before-write, per-market batching, dry-run default. Once the check order's fixed this is in good shape. Small optional nits: no tests on the applier's own selection/apply logic (would've caught the ordering bug), no --author-host allowlist, no formula-injection escaping in the xlsx writer, and a leftover //@TODO host in the README.

…formula injection in xlsx; added host allowlist
@Axelcureno

Copy link
Copy Markdown
Member

Re-checked on f3ced54. The applyRow order fix is right, and it covers revert too since both share the same path. Two leftovers, neither blocking: the new tests only hit sameTags/selectRows, so the already-applied path that caused this is still untested (applyRow isn't exported); and Version snapshot failed: ${response} at line 122 prints [object Response] into the failures file. Dismissing my changes-requested.

@Axelcureno
Axelcureno dismissed their stale review September 3, 2026 16:23

Blocker fixed on f3ced54; remaining items are non-blocking nits.

@Axelcureno Axelcureno left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving. Blocker fixed, the two leftovers I noted are follow-up nits. Heads up that Nala Gate still needs the run nala label before this can merge.

@yesil yesil left a comment

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.

Could you add a dryRun mode with logging and attach the dry-run logs?

The script appears to have a significant impact, so we should make sure the dry run provides an accurate projection of exactly what would happen during a real run, without making any actual changes.

@afmicka
afmicka merged commit 26e2f94 into main Sep 7, 2026
15 of 16 checks passed
@afmicka
afmicka deleted the mwpw-203042 branch September 7, 2026 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants