chore: sync develop → main#105
Open
two-inc[bot] wants to merge 17 commits into
Open
Conversation
Mirrors the abn fork's PR #69. New .github/scripts/release-notes.py is the single source of truth for the bucketing + bump-level decision. release.yml calls it for the actual GitHub Release notes; auto-pr.yml calls it with origin/main..HEAD and uses the output as the rolling sync PR's body. The PR description refreshes on every push to develop with the bucketed list AND the predicted bump level. Why Python: regex grows readable (named groups, real re.match) and the script is testable as it grows. setup-python is already installed so the cost is marginal.
INF-1159/ci: shared release-notes script + auto-PR preview body
Mirror of the abn fork's PR. Currently release.yml fires on every push to main, including broken commits — a sync PR somehow merged with red CI would still produce a tagged Release page. Switching the trigger from `push: [main]` to a workflow_run listener on the CI workflow's completion, with: 1. CI's conclusion must be 'success' on the same SHA. 2. Skip on the bumpver loop commit (same guard as before, sourced from workflow_run.head_commit instead of push event). 3. Checkout pinned to workflow_run.head_sha (covers the case where another commit landed between CI passing and release firing). workflow_run resolves the workflow file from the default branch, so this change has to land on main before gating kicks in. The first sync-PR merge after this lands will use the OLD shape; the next one and all after will go through workflow_run.
Currently release.yml fires on every push to main regardless of CI status — a broken commit landing on main (e.g. via a sync PR somehow merged with red CI) would still produce a tagged Release page. Switching the trigger from `push: [main]` to a workflow_run listener on the CI workflow's completion. Two gates: 1. CI's conclusion must be 'success' on the same SHA. workflow_run fires on every CI completion (success/failure/cancelled); the if-filter narrows to green. 2. Skip on the bumpver loop commit (same guard as before, sourced from workflow_run.head_commit.message). Checkout pinned to workflow_run.head_sha (covers the rare case where another commit landed between CI passing and release firing). Note: workflow_run resolves the workflow file from the default branch, so the gating only kicks in after this lands on main. The first sync-PR merge after this lands will use the OLD shape; the next and all after go through workflow_run. README: new Releases section explains the tag/auto-release flow end-to-end — bumpver level rules, auto-PR rolling sync PR with release-notes preview, and the merge-back loop.
INF-1159/ci: gate release.yml on green CI via workflow_run
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Imports the cleaned Two_Gateway runtime from the consolidated private branch. The module is brand-aware via Two\Gateway\Api\BrandRegistryInterface, with a default DI binding to Two\Gateway\Brand\TwoBrand. Downstream brand-overlay packages may rebind this interface via their own DI preference. Bumps to 2.0.0-rc.1 to signal the architectural change. See internal ticket for the full design rationale. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Magento dev mode has a long-standing requirejs/mixins-plugin race that keeps the admin grid loading spinner visible for ~100s+ after page load, even though grid data renders correctly underneath. The root cause is in vendor (Magento_RequireJs) and has no clean local fix. This adds a make-install-time script that creates a tiny local-only module (Local_AdminLoaderHide) shipping a single CSS rule to hide the overlay. Local dev only — only runs from the Makefile install target, never affects the shipped Two_Gateway plugin. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…present KO subscribables don't replay. On slow stacks the surcharge model can mount after Magento's /totals-information has already fired and called quote.setTotals(), so the subscriber misses the emit and the chip loader hangs forever. Trigger loadFees() once at init when getTotals() is already populated; the snapshotTotals dedup and fetchSeq guard handle the double-emit and out-of-order cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The constructor was updated to take TaxCalculation as arg 5 and BrandRegistryInterface as arg 6, dropping the previous State dependency (developer-mode is now read from app/etc/env.php). The unit tests still passed the old argument list, so every test instantiating Repository errored on setUp. - Drop State usage from both Repository test fixtures. - Add a BrandRegistryInterface mock returning the Two URL template. - For URL tests, introduce a RepositoryUrlTestable subclass that overrides isDeveloperMode() via a public flag, since the real check reads BP/app/etc/env.php which doesn't exist in unit tests. - Make Repository::isDeveloperMode() protected so it can be overridden by the test double. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three more test files were instantiating runtime classes with the
pre-rewrite constructor signatures or against runtime defaults that
have since changed.
- AdapterTest: add BrandRegistryInterface mock as arg 2 of Adapter.
- TwoErrorHandlingTest: inject a BrandRegistry mock via reflection
so error messages that reference the product name don't NPE.
- SurchargeCalculatorTest: replace the CurrencyFactory/Currency
active-record mock with a CurrencyRatesProviderInterface mock;
use getRate() multiplier + null-rate for the conversion-failure
path. Pass BrandRegistry mock to the inner Adapter constructor.
- RepositoryPaymentTermsTest: align the expected default surcharge
line description with the runtime ('Payment terms fee - %1 days').
Also fix the i18n source string for the organization_number loc
mapping in Model/Two.php — it was hardcoded to the Dutch "KVK
number" rather than the brand-neutral source string "Company ID"
that the i18n files translate per locale.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A handful of unit tests asserted behaviour that the current runtime does not implement, or used payload shapes from an earlier draft of the surcharge calculator. - testReturnsZeroWhenApiOmitsBuyerFeeShare → renamed to testThrowsWhenApiOmitsBuyerFeeShare. The runtime is strict and throws when the pricing API response is missing the required field; the prior "returns zero" behaviour was never implemented. - testPayloadBuyerFeeShareForPercentage: the percentage payload does not include a `surcharge` key; assert its absence instead of an explicit zero value. - testDifferentialModeDefaultTermShortCircuits → renamed to testDifferentialModeDefaultTermDelegatesToApi. Differential math is the API's job; the plugin always calls the API and trusts the response (which will be 0 at the default term). - testReturnsTaxRateAndDescription: surcharge line description is a Magento phrase template, so the merchant-supplied string needs to embed `%1` for the term-days substitution to take effect. - AdapterTest: stub BrandRegistry::getProductName() so error messages that include the brand name don't render as empty. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The PHPStan and DI-compile jobs were running `composer require two-inc/magento2:@dev` inside the Magento container, which resolved the package from Packagist rather than the working tree that had just been `docker cp`d into /data/extensions/. That meant CI was exercising the already-published version of the plugin on every run, not the code under review. Configure a composer `path` repository pointing at the uploaded directory before requiring the package. Composer reads the local composer.json directly, so the job tests exactly the bytes that were uploaded. Drops the feature-branch / strip-version dance that was only needed to keep Packagist happy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two open review threads: 1. .github/workflows/release.yml — the plugin-rewrite commit reset release.yml to its pre-INF-1159 shape, which restored the bare `push: [main]` trigger and dropped the workflow_run CI gate. A broken commit landing on main could therefore produce a tagged Release page regardless of CI status. Restore Bharat's workflow_run-gated version (with the brand-neutralised tag prefix comment kept). 2. Model/Config/Source/PaymentTermsDurationDays.php — hardcoded a single `30 days` option and wasn't referenced from any admin XML, DI binding, or source_model attribute. Functionality is already covered by Model/Config/Source/AvailablePaymentTerms which delegates to BrandRegistryInterface::getAvailablePaymentTerms(). Drop the dead class rather than keeping a stub that would silently produce wrong results if someone wired it later. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Gold Squadron flagged five real issues. All addressed in this commit.
- i18n full-sentence template (Yoda HIGH). Magento's translator
exact-matches on the full template string, so `__('Payment terms
fee - %1 days', 30)` was never hitting the `"%1 days"` row in
nl_NL.csv / nb_NO.csv / sv_SE.csv — the surcharge line stayed
English in non-EN carts. Ship the full sentence as a translation
source in each shipped locale.
- Header.php sign-up + documentation URLs (Leia HIGH). Pulled the
hardcoded `https://portal.two.inc/...` and `https://docs.two.inc/...`
constants up onto BrandRegistryInterface so brand-overlay packages
can redirect them without subclassing the admin block.
- etc/csp_whitelist.xml (Leia HIGH). Magento's CSP whitelist file is
static XML and the BrandRegistryInterface seam cannot rewrite it
at runtime. Add an explicit comment for future overlay authors
noting that overlays whose checkout lives on a non-two.inc host
must ship their own etc/csp_whitelist.xml (additive merge).
- ci.yml docker cp leak (Vader HIGH). `docker cp "$(pwd)"` does not
respect .gitignore or .gitattributes, so untracked dev junk,
dotfiles, and the .git/ directory crossed into the CI container.
Switch to `git archive HEAD | docker exec -i ... tar -x` which
honours export-ignore and ships only tracked files.
- Surcharges.php + TermSelection.php asymmetric exception handling
(Han MEDIUM). Per-term pricing failures were silently zeroed and
shown to the buyer as 0.00 fee chips, but when the buyer picked
that term the cart total collector would throw on the same path.
Add `addErrorLog` calls inside the inner catch so the silent zero
doesn't mask a broken pricing path; inject LogRepository into
TermSelection (Surcharges already had it).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`git archive` honours `.gitattributes export-ignore`, which strips the repo-only build/test files (phpstan.neon, phpunit.xml, Test/, .github/) — phpstan then can't find its config inside the container. Switch to `git ls-files | tar`: ships all tracked files (so CI gets phpstan.neon and friends) but still leaves .git/, dotfiles, and any untracked dev state on the host — same supply-chain win. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two adversarial iter-2 follow-ups landed: - Yoda: three PHP fallback labels `'Payment terms fee'` in Model/Total/Surcharge, Service/Order/ComposeOrder, and Service/Order/ComposeRefund weren't wrapped in `__()`, so when the upstream description was missing the buyer saw the English string regardless of locale. Wrap each in `__()` (and `(string)`- cast in the two compose paths where the value is sent to the API as a JSON string field). - Leia: tighten the etc/csp_whitelist.xml overlay-author comment. "Additively" was slightly imprecise — within a single policy, Magento merges by `value id`: new ids are appended, but ids reused by an overlay override this module's value. Document the actual semantics so overlay authors aren't surprised. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
TWO-24485: rewrite plugin as brand-aware Two_Gateway module
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rolling sync PR opened by
.github/workflows/auto-pr.yml. Merging this PR firesrelease.ymlonmain(auto bump + tag + GitHub Release) andmerge-back.yml(fast-forwardsdevelopto match).Predicted bump: patch — based on the commits below.
🧰 Internals
Close this PR manually if you need to skip a sync window.