WIP: Forms/update responses management #41602

ntsekouras · 2025-02-06T14:57:31Z

Fixes #41122

This is WIP right now and no reviews are needed. I'll update the description soon.

Proposed changes:

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Go to '..'

github-actions · 2025-02-06T14:58:29Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add a "[Type]" label (Bug, Enhancement, Janitorial, Task).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

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 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Choose a review path based on your changes:
- A. Team Review: add the "[Status] Needs Team Review" label
  - For most changes, including minor cross-team impacts.
  - Example: Updating a team-specific component or a small change to a shared library.
- B. Crew Review: add the "[Status] Needs Review" label
  - For significant changes to core functionality.
  - Example: Major updates to a shared library or complex features.
- C. Both: Start with Team, then request Crew
  - For complex changes or when you need extra confidence.
  - Example: Refactor affecting multiple systems.
Get at least one approval before merging.

Still unsure? Reach out in #jetpack-developers for guidance!

github-actions · 2025-02-06T14:58:45Z

Are you an Automattician? The PR will need to be tested on WordPress.com. This comment will be updated with testing instructions as soon the build is complete.

github-actions · 2025-02-06T15:12:14Z

Code Coverage Summary

Coverage changed in 8 files. Only the first 5 are listed here.

File	Coverage	Δ%	Δ Uncovered
projects/packages/forms/src/contact-form/class-contact-form-endpoint.php	0/272 (0.00%)	0.00%	254 💔
projects/packages/forms/src/dashboard/state/actions.js	0/16 (0.00%)	0.00%	6 💔
projects/packages/forms/src/class-wpcom-rest-api-v2-endpoint-forms.php	0/290 (0.00%)	0.00%	1 ❤️‍🩹
projects/packages/forms/src/contact-form/class-contact-form-plugin.php	111/837 (13.26%)	-0.02%	1 ❤️‍🩹
projects/packages/forms/src/dashboard/state/reducer.js	28/51 (54.90%)	1.71%	1 ❤️‍🩹

3 files are newly checked for coverage.

File	Coverage
projects/packages/forms/src/dashboard/inbox/dataviews/actions/index.js	0/26 (0.00%) 💔
projects/packages/forms/src/dashboard/inbox/dataviews/index.js	0/91 (0.00%) 💔
projects/packages/forms/src/dashboard/state/resolvers.js	0/4 (0.00%) 💔

Full summary · PHP report · JS report

ntsekouras · 2025-02-11T14:52:37Z

projects/packages/forms/src/contact-form/class-contact-form-endpoint.php

+	 * @param array $schema Schema array.
+	 * @return array Modified Schema array.
+	 */
+	public function add_additional_fields_schema( $schema ) {


@anton-vlasenko the PHP changes are the first pass, but is there a better way to do this?

We want to add more fields to the response object. It's also related to the prepare_item_for_response below.

I wonder if you could use something along the lines of WP_REST_Attachments_Controller::get_item_schema()?
Since the Contact_Form_Endpoint class extends WP_REST_Posts_Controller, you could call parent::get_item_schema() and then add your own fields to the schema - just as WP_REST_Attachments_Controller::get_item_schema() does.
Personally, I don’t see a reason to override the add_additional_fields_schema here.
What do you think?

anton-vlasenko

Here's a quick review of the PR (I understand it's still in draft). Thank you.

anton-vlasenko · 2025-02-11T17:09:11Z

projects/packages/forms/src/contact-form/class-contact-form-endpoint.php

+		$post_ids = $request->get_param( 'post_ids' );
+
+		if ( $action && ! is_array( $post_ids ) ) {
+			return new $this->error_response( __( 'Bad request', 'jetpack-forms' ), 400 );


Suggested change

return new $this->error_response( __( 'Bad request', 'jetpack-forms' ), 400 );

return $this->error_response( __( 'Bad request', 'jetpack-forms' ), 400 );

This code calls a constructor on an object that has already been instantiated by error_response. This should result in a fatal error: https://3v4l.org/kKJYT.

anton-vlasenko · 2025-02-11T17:14:28Z

projects/packages/forms/src/contact-form/class-contact-form-endpoint.php

+							'month' => intval( $row->month ),
+							'year'  => intval( $row->year ),


Nitpick: (int) is faster than intval().

Suggested change

'month' => intval( $row->month ),

'year' => intval( $row->year ),

'month' => (int) $row->month,

'year' => (int) $row->year,

anton-vlasenko · 2025-02-11T17:15:04Z

projects/packages/forms/src/contact-form/class-contact-form-endpoint.php

+					$months
+				),
+				'source' => array_map(
+					function ( $post_id ) {


Suggested change

function ( $post_id ) {

static function ( $post_id ) {

anton-vlasenko · 2025-02-11T17:15:32Z

projects/packages/forms/src/contact-form/class-contact-form-endpoint.php

+		return rest_ensure_response(
+			array(
+				'date'   => array_map(
+					function ( $row ) {


Suggested change

function ( $row ) {

static function ( $row ) {

anton-vlasenko · 2025-02-11T17:49:50Z

projects/packages/forms/src/contact-form/class-contact-form-endpoint.php

+	 * @param array $schema Schema array.
+	 * @return array Modified Schema array.
+	 */
+	public function add_additional_fields_schema( $schema ) {


I wonder if you could use something along the lines of WP_REST_Attachments_Controller::get_item_schema()?
Since the Contact_Form_Endpoint class extends WP_REST_Posts_Controller, you could call parent::get_item_schema() and then add your own fields to the schema - just as WP_REST_Attachments_Controller::get_item_schema() does.
Personally, I don’t see a reason to override the add_additional_fields_schema here.
What do you think?

ntsekouras added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Feb 6, 2025

ntsekouras self-assigned this Feb 6, 2025

github-actions bot added [Feature] Contact Form [Package] Forms [Status] In Progress labels Feb 6, 2025

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Feb 6, 2025

ntsekouras force-pushed the forms/update-responses-management branch 2 times, most recently from 36bb193 to 56200a2 Compare February 10, 2025 08:46

ntsekouras added 2 commits February 11, 2025 12:52

Forms: Update responses management with DataViews

82e18e4

updates

fcd67ca

ntsekouras force-pushed the forms/update-responses-management branch from b02f7a8 to fcd67ca Compare February 11, 2025 10:52

ntsekouras commented Feb 11, 2025

View reviewed changes

anton-vlasenko reviewed Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Forms/update responses management #41602

WIP: Forms/update responses management #41602

ntsekouras commented Feb 6, 2025

github-actions bot commented Feb 6, 2025 •

edited

Loading

github-actions bot commented Feb 6, 2025

github-actions bot commented Feb 6, 2025 •

edited

Loading

ntsekouras Feb 11, 2025

anton-vlasenko Feb 11, 2025 •

edited

Loading

anton-vlasenko left a comment

anton-vlasenko Feb 11, 2025

anton-vlasenko Feb 11, 2025

anton-vlasenko Feb 11, 2025

anton-vlasenko Feb 11, 2025

anton-vlasenko Feb 11, 2025 •

edited

Loading

	return new $this->error_response( __( 'Bad request', 'jetpack-forms' ), 400 );
	return $this->error_response( __( 'Bad request', 'jetpack-forms' ), 400 );

		'month' => intval( $row->month ),
		'year' => intval( $row->year ),

WIP: Forms/update responses management #41602

Are you sure you want to change the base?

WIP: Forms/update responses management #41602

Conversation

ntsekouras commented Feb 6, 2025

Proposed changes:

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Feb 6, 2025 • edited Loading

github-actions bot commented Feb 6, 2025

github-actions bot commented Feb 6, 2025 • edited Loading

Code Coverage Summary

ntsekouras Feb 11, 2025

Choose a reason for hiding this comment

anton-vlasenko Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

anton-vlasenko left a comment

Choose a reason for hiding this comment

anton-vlasenko Feb 11, 2025

Choose a reason for hiding this comment

anton-vlasenko Feb 11, 2025

Choose a reason for hiding this comment

anton-vlasenko Feb 11, 2025

Choose a reason for hiding this comment

anton-vlasenko Feb 11, 2025

Choose a reason for hiding this comment

anton-vlasenko Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 6, 2025 •

edited

Loading

github-actions bot commented Feb 6, 2025 •

edited

Loading

anton-vlasenko Feb 11, 2025 •

edited

Loading

anton-vlasenko Feb 11, 2025 •

edited

Loading