Only save global settings the user has entered - #17
Draft
roborourke wants to merge 3 commits into
Draft
Conversation
Fixes #15. The "Set as global defaults" button always sent all three settings, using the block attributes as the source of values. A blank field means "inherit the global default shown as the placeholder", but it resolved to null (or 0 for the business unit ID), and the settings REST endpoint deletes an option when it receives null. Saving a business unit ID therefore wiped a previously saved portal ID, and saving a portal ID reset a non-default region back to eu1. Track which of the global fields the user actually edited and build the payload from only those that hold a value, so each global can be saved on its own without disturbing the others. The button is now hidden unless there is something to promote, and only the overrides that were promoted are cleared from the block attributes. Also fall back to the block-level portal ID in the two editor notices, which previously looked at the global value alone and told the user to enter a portal ID even when the block supplied one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DWkBq1aQNvFyvGdXpJUa39
Playwright E2E Test Results (PHP 8.4, WP latest)Details
Flaky testschromium › form-render.spec.js › HubSpot Form — frontend render › should render form container with correct data attributes |
Writing null to a registered setting only deletes the option when it has no stored value; once it holds one, the settings endpoint bails out with rest_invalid_stored_value and a 500. The spec's beforeEach/afterEach hit that and failed all three tests before reaching their assertions. Use 0 for the two ID options instead. It is always a valid value, and both the plugin and the editor already treat it as unset. Correct the comment in edit.js to describe the same behaviour accurately: sending a blank field as null aborted the whole save rather than deleting the option, while the `|| 0` coercion overwrote a stored global. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DWkBq1aQNvFyvGdXpJUa39
The regression test drove both saves through the sidebar, so it depended on the inspector surviving the first one. It does not: the Global Settings panel's initialOpen is `! defaultPortalId`, which flips once a global Portal ID is stored, collapsing the panel and unmounting the Portal ID field. The test then failed with "element(s) not found". Seed the pre-existing Portal ID through the REST API instead and drive only the Business Unit ID through the UI. That needs no post-save DOM interaction, and is a closer reproduction of the report, where the Portal ID was saved in an earlier session. Split the portal-only save into its own test, and cover the two cases that were previously asserted mid-save as stable states: a saved global shows as the field's placeholder, and a block-level Portal ID satisfies the editor notice when no global is set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DWkBq1aQNvFyvGdXpJUa39
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
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.
Fixes #15.
The bug
The Set as global defaults button built its payload from all three block attributes every time:
A blank field in that panel means "inherit the global default shown as the placeholder", but it was still sent — and it broke in two different ways depending on the field:
0by|| 0, overwriting a previously stored value with an empty one. This is the wipe described in the issue.null.WP_REST_Settings_Controlleronly deletes an option onnullwhen it has no stored value; once it holds one, it bails out withrest_invalid_stored_valueand a 500 — and because it returns on the first offending option, nothing in the request is saved, including the value the user was actually trying to set. (CI confirmed this: my first attempt at the tests usednullto reset the options and gotThe hubspot_embed_portal_id property has an invalid stored value, and cannot be updated to null.)Region had a third problem: its block attribute defaults to
eu1, so promoting any global on a site whose region wasna1senteu1over the top of it.The fix
Track which of the global fields the user actually edited, and build the
saveSitepayload from only those that hold a value:Also fixed the two editor notices, which checked the global portal ID alone and told the user to "enter a Portal ID" even when the block supplied its own — they now fall back the same way
render.phpdoes.Testing
tests/global-settings.spec.jscovers the reported sequence and the region case:region = na1leaves the region alone.npm run lint:jsandnpm run buildpass locally. The Playwright suite needs Playground to download WordPress, which is blocked in my sandbox, so these run in CI only.