Forms: Add source filter to feedback REST API#48027
Conversation
… by source post ID Introduces a `source` query parameter to the feedback REST API and counts endpoint. Uses `_feedback_source_post_id` meta with fallback to `post_parent` for legacy data. Includes caching for source post ID lookups and cache invalidation on save/delete. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
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 7 files. Only the first 5 are listed here.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
…meta - Switch from update_post_meta to add_post_meta (with unique flag) for new feedback since the meta doesn't exist yet on a new post. - Add maybe_backfill_source_meta() that writes the source meta for old feedback using the source ID resolved from the Feedback object's parsed content. - Call the backfill in prepare_item_for_response so old feedback gradually gets the meta as it's served through the REST API. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@copilot review this PR. |
- Fix Phan error: use `= null` instead of `unset()` for declared class property - Rewrite get_all_source_post_ids tests to mock SQL via wordbless_wpdb_query_results filter since WorDBless has no database - Rewrite endpoint source filter tests to verify SQL clause generation and hook cleanup rather than asserting on query results - Add query structure test to verify UNION, meta key, and post_parent fallback Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a dedicated source filter for Jetpack Forms feedback REST endpoints and updates the Forms dashboard to use it, while persisting the source post ID in post meta for reliable querying and backwards compatibility.
Changes:
- Add
sourcequery parameter support to/wp/v2/feedbackand/wp/v2/feedback/counts, including SQL filtering with meta + legacypost_parentfallback. - Persist
_feedback_source_post_idmeta on new feedback saves and add a cachedFeedback::get_all_source_post_ids()for populating the “Source” dropdown. - Update dashboard query plumbing (store, hooks, stage routing) to use
sourceinstead of overloadingparent, and add PHPUnit coverage.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/packages/forms/tests/php/contact-form/Feedback_Creation_Test.php | Adds tests for source meta persistence, source ID lookup query/caching, and cache invalidation on save. |
| projects/packages/forms/tests/php/contact-form/Contact_Form_Endpoint_Test.php | Adds REST schema and SQL-behavior tests for the new source parameter. |
| projects/packages/forms/src/dashboard/store/resolvers.js | Includes source in counts request parameters. |
| projects/packages/forms/src/dashboard/store/reducer.js | Includes source in query cache key generation. |
| projects/packages/forms/src/dashboard/store/actions.js | Triggers query updates when source changes. |
| projects/packages/forms/src/dashboard/inbox/stage/types.tsx | Extends query param typing to include source. |
| projects/packages/forms/src/dashboard/inbox/stage/index.js | Routes “Source” filter UI to source query param (not parent). |
| projects/packages/forms/src/dashboard/inbox/stage/actions.tsx | Includes source when building count query params. |
| projects/packages/forms/src/dashboard/hooks/use-inbox-data.ts | Adds source to feedback fetch params. |
| projects/packages/forms/src/contact-form/class-feedback.php | Introduces source meta key, cached source ID discovery, cache invalidation hook, and meta backfill helper. |
| projects/packages/forms/src/contact-form/class-contact-form-endpoint.php | Registers source param, applies source filtering to collection and counts queries, and triggers lazy meta backfill. |
| projects/packages/forms/src/class-jetpack-forms.php | Hooks cache invalidation on permanent feedback deletion. |
| projects/packages/forms/routes/responses/stage.tsx | Updates router-stage query args to use source for the “Source” filter. |
| projects/packages/forms/changelog/fix-forms-source-filter-responses | Adds changelog entry for the new source filter parameter. |
projects/packages/forms/src/contact-form/class-contact-form-endpoint.php
Outdated
Show resolved
Hide resolved
- Delete test_source_meta_key_constant_exists (coverage chaser) - Delete test_source_filter_returns_200 (dodger, covered by other tests) - Rewrite test_source_filter_modifies_query to test get_source_filter_sql directly via reflection, avoiding WorDBless WP_Query caching issues - Rewrite test_counts_with_source to verify source filter SQL is applied - Soften UNION assertion to only check semantic SQL content - Add test_source_filter_with_zero_value_is_ignored edge case - Use wordbless_wpdb_query_results consistently for SQL capture Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review completed above. Summary of findings (all suggestions, no blockers):
Overall: well-structured, secure, and well-tested. Minor issues — can merge after addressing. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
- Add post_type guard to join_source_meta and filter_by_source_id so they only modify feedback queries, preventing unintended SQL changes to unrelated WP_Query calls during the same request - Invalidate source IDs cache after successful backfill in maybe_backfill_source_meta to keep the filter dropdown fresh Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Cache get_source_filter_sql() result so join_source_meta and filter_by_source_id reuse it instead of recomputing per hook call - Add sanitize_callback and validate_callback to source param in get_collection_params for consistency with other params - Replace hardcoded 'feedback' string with self::POST_TYPE in get_all_source_post_ids SQL query Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ReflectionMethod::setAccessible() is deprecated in PHP 8.5 and has no effect since PHP 8.1. Remove the call to fix the CI deprecation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Only call ReflectionMethod::setAccessible() on PHP < 8.1 where it is required. On 8.1+ it has no effect, and on 8.5+ it triggers a deprecation notice. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fixes FORMS-671
The issue that this PR is trying to fix is the following.
When a new feedback comes in from that is added via a synced form. We end up in the situation where we are not able to filter this feedback by source any more.
The main issue is that source is ID is not stored as the post parent any more. Since the post parent is now the jetpack_form.
This PR fixes this by setting the source ID on a post meta. This makes it possible to filter posts by query the post meta which gets stored when the feedback gets created. For backwards compatibility we also add the post meta when feedback that don't have it set yet. In the future versions we could remove the filtering by post_parent completely since most feedback will contain the source id in the post meta.
Proposed changes
sourceREST API query parameter to the feedback endpoints (/wp/v2/feedbackand/wp/v2/feedback/counts) for filtering responses by the page/post they were submitted from._feedback_source_post_idmeta on new feedback submissions usingadd_post_meta.get_all_source_post_ids()on theFeedbackclass to populate the source filter dropdown, using meta with fallback topost_parentfor legacy data (excludingjetpack_formparents)._feedback_source_post_idmeta for old feedback viamaybe_backfill_source_meta(), called fromprepare_item_for_response()— gradually migrates legacy data as it's served through the REST API.get_source_filter_sql()helper to avoid duplicating JOIN/WHERE SQL betweenget_counts()and WP_Query filter hooks.join_source_meta/filter_by_source_idwithpost_typecheck so they only modify feedback queries, not unrelated queries in the same request.deleted_posthook).sourceparameter instead of overloadingparent.Related product discussion/links
Does this pull request change what data or activity we track or use?
No. This adds a new post meta (
_feedback_source_post_id) that stores the same source post ID that was previously only available viapost_parent. No new tracking or external data collection.Testing instructions
post_parentshould handle legacy data, and it will be lazily backfilled with meta as it's served.