Closes #8700 Story 2 — Toggle-driven Content Delivery UI - #8732
hellofromahmed merged 29 commits into
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Coverage variation | ✅ +0.10% coverage variation (-0.10%) |
| Diff coverage | ✅ 86.47% diff coverage (50.00%) |
Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (00f42ce) 47766 22536 47.18% Head commit (96a1496) 47853 (+87) 22625 (+89) 47.28% (+0.10%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>
Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#8732) 133 115 86.47% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Screenshot
|
…at/8700-toggle-ui-content
|
@Khadreal Thanks for the PR, Here are the possible risks raised by Claude, please feel free to validate if we need further change(s) or GH or we are good 🙏
|
|
Note Generated by the AI delivery pipeline (lead-reviewer · Claude Sonnet 5). Re-review after fix commits (9fdde98, 74761ec merge, 3043fdb). Review: ❌ CHANGES REQUESTED Resolved since last review:
Remaining blockers:
Nice-to-haves:
|
|
Note Generated by the AI delivery pipeline (orchestrator · Claude Sonnet 5). Full consolidated findings from Lead Review + QA, including the re-review after commits Status: ❌ Not yet mergeable — CHANGES REQUESTED (lead review) + FAIL (QA)✅ Resolved since the first review pass
❌ Still blocking1. [MUST_HAVE — new, QA] RocketCDN Free/Paid can never be activated from "nothing active"
2. [MEDIUM] AC5 — Free-inactive activation prompt / auto-activate on first page — not implemented
3. [MEDIUM] AC3 — active-mode visual cue is on the section header, not the tab
4. [MEDIUM]
|
…e.com toggle compatibility - Split should_disable_element_for_rocketcdn() into a dedicated should_reject_rocketcdn_activation() that excludes the pause-state check: a request to activate RocketCDN is by definition made while CDN is currently paused, so including that check rejected every first activation. Use the new method for both the /mode REST gate and the mode toggle's disabled state (the checkbox was still HTML-disabled even after the REST endpoint was fixed to accept the request). - Add the Free activation-prompt / auto-activate flow to add_page() and add_homepage(): silently activates Free when no mode is active at all, asks for confirmation when another mode is active, applies the same entitlement gate as /mode, and only switches the mode once the page is confirmed persisted (never on a request that fails validation). - Add the active-mode visual cue to the CDN driver tabs themselves (previously only shown on the section header). - Restore One.com's CDN-toggle-disable compatibility hook, adapted from the removed pause button to the new toggle model (disable_cdn_mode_toggle(), targeting is_forced_off instead of the deleted pause button's status flag) so One.com hosting can still prevent the mode toggle from being switched while its own CDN handles delivery. - Remove the now fully dead pause-button leftovers (OneCom's original hook + its 3 tests, and unused status-indicator template variables).
|
Note Generated by the AI delivery pipeline (orchestrator · Claude Sonnet 5). Summary of the fix round following the earlier consolidated review comment, now pushed in commit Status: ✅ All findings from this review round fixed and verifiedWhat changed in this commit
Verification this round
No outstanding blockers from either gate as of this push. |
…est's Options_Data instance The new AC5 integration tests set cdn_state directly through the shared 'options' service before making the request, assuming it was the same Options_Data instance Rest::add_page() reads from. In CI it isn't (or isn't reliably): the pre-set was invisible to add_page(), so cdn_state always read back as whatever the DB row already held, and the three new scenarios exercised the wrong branch. Switched to the same pre_get_rocket_option_cdn_state live filter override technique tests/Integration/.../SaveCdnMode.php already used for the analogous 'cdn' key — this is read on every Options_Data::get() call regardless of which instance holds it, so it reliably reaches add_page()'s own read. Removed the now-meaningless cdn_state assertion on the "already active, no-op" case, since nothing ever persists that value to the DB in that scenario (only the live read is faked). Also simplified add_page()'s three-way if/elseif/else down to a single guard clause (same behavior, no more redundant else), per a Codacy notice on the pushed commit.
…at/8700-toggle-ui-content
…o feat/8700-toggle-ui-content
A recent commit ("return cdn_state when it's saved into DB") changed
CdnStateBridge to only recompute cdn_state from the legacy cdn/cdn_type
fields when no value was already stored, and renamed its filter hook
from pre_get_rocket_option_cdn_state to get_rocket_option_cdn_state.
Both changes broke the class's actual, tested contract: cdn_state must
always be resolved live from cdn/cdn_type (and live subscription state)
on every read, never trusted from whatever was last written — this is
what lets the bridge correctly reflect cdn/cdn_type changes made outside
apply_cdn_mode() (other settings paths, forced-pause, hosting compat
filters like OneCom's) and what tests/Integration/.../CdnStateBridge/
resolveLive.php already asserts. The hook rename also meant the bridge
silently stopped running at all for any site where cdn_state was never
explicitly stored (legacy users upgrading, the bridge's primary use
case), since get_rocket_option_* only fires when the key already exists.
Reverted both changes back to the original, tested design.
91a0742
into
enhancement/8693-rocketcdn-free-tiers-refactor
4 participants
Description
Fixes #8700
Replaces the old "select a tab = activate" model for Content Delivery (CDN) with explicit on/off toggles per mode (RocketCDN Free, RocketCDN Paid, Your own CDN/BYOCDN), decoupled from tab navigation. A user can now browse any CDN tab without activating or deactivating anything, and switching a mode on directly turns the previously active mode off. Along the way this also fixes a real bug uncovered during review: RocketCDN Free/Paid could never be turned back on once switched off, because the REST endpoint and the toggle checkbox both used a "should this look disabled" check that always evaluated true right before any activation attempt.
Type of change
Detailed scenario
What was tested
All scenarios below were validated both via live browser interaction (Chrome, real toggle clicks) and via direct REST calls (WP-CLI
rest_do_request()against the real REST controller, run in isolated processes to avoid stale in-memory option caching), across two full review-and-fix rounds plus a final CI run:cdn_state/cdn/cdn_typeoptions). Verified live and via REST.cdn_state = nothingwith no mode forced back on. Verified live and via REST.409with the current mode named, page not written to the DB; another mode active with confirmation → activates Free and adds the page. Also confirmed the mode switch never happens before the page is actually validated and persisted (no dangling mode change on a failed add-page request).disable_cdn_mode_toggle()correctly disables all three mode toggles when One.com's own CDN handling is active, and that this doesn't regress anything else in the One.com compatibility layer (CNAME, zone, varnish, tab visibility).tests/Unit(CDN + OneCom groups) andtests/Integration(RocketCDN group) — all green in CI across PHP 7.4–8.5. PHPCS and PHPStan lint clean.How to test
vcachingconstant +oc_cdn_enabledoption), confirm all three mode toggles render disabled.Affected Features & Quality Assurance Scope
/wp-rocket/v1/rocketcdn/mode,/wp-rocket/v1/rocketcdn/pages,/wp-rocket/v1/rocketcdn/pages/homepage.inc/ThirdParty/Hostings/OneCom.php).Technical description
Documentation
Controller::should_disable_element_for_rocketcdn()(existing, unchanged) stays the "should this look disabled" check used for rendering (purge button, exclusions, etc.) — it legitimately includes the current pause state.Controller::should_reject_rocketcdn_activation()is the same check without the pause-state condition, since a request to activate is by definition made while paused. It's now the single source of truth for both the/modeREST gate and the mode toggle'sdisabledattribute, so the two can never disagree again.Rest::add_page()/add_homepage()gained an activation-prompt flow: the mode-switch decision is made early (fail-fast on bad entitlement or on "needs confirmation"), but the actual mode change is deferred until after the page is validated and successfully persisted, via a sharedapply_cdn_mode()helper (also used bysave_cdn_mode()).Context::get_applied_cdn_state()(the real persisted mode), kept distinct from the "currently viewed" tab state (cdn_type) — these were previously conflated under the same CSS class.disable_cdn_mode_toggle()(renamed fromdisable_cdn_pause_option(), which disabled a pause button this PR removes) hooks the samerocket_cdn_driver_sectionsfilter at the same priority, now targetingis_forced_offon all three mode sections instead of a status-indicator flag that no longer has a consumer.New dependencies
None.
Risks
add_page()'s activation path uses the identical entitlement gatesave_cdn_mode()enforces, so a site without a valid license/active subscription can't get RocketCDN Free force-activated by adding a page.confirm_activation, boolean, sanitized viarest_sanitize_boolean).Mandatory Checklist
Code validation
Code style
Unticked items justification
N/A — all mandatory items above were completed.
Additional Checks