-
Notifications
You must be signed in to change notification settings - Fork 835
Forms: Add REST API endpoint for exporting responses #45275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
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?
|
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! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
49302ee
to
0ff05d9
Compare
Code Coverage SummaryCoverage changed in 4 files.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
projects/packages/forms/src/contact-form/class-contact-form-endpoint.php
Outdated
Show resolved
Hide resolved
Isn't it a convention that the REST endpoints always return JSON? What was the problem with AJAX method that we are trying to solve here? We could also make use of the |
Core REST API Handbook mentions
It also says
|
@manzoorwanijk Thanks for the feedback. You are right about REST conventions. In this version, we return JSON for errors and CSV on success. While |
projects/packages/forms/src/contact-form/class-contact-form-endpoint.php
Outdated
Show resolved
Hide resolved
233fb69
to
06c5d76
Compare
I tested this and it works as expected. 🥳 I tested exporting. Exporting Google Sheets ( for regressions ) It would be good to add an endpoint test for this PR. So that we have something that checks for regressions. |
If you wish to laverage the REST authentication for api-fetch, then you can return a URL to admin-post action mentioned above. So, it can pass only the Then update |
projects/packages/forms/src/contact-form/class-contact-form-endpoint.php
Show resolved
Hide resolved
Thanks for the feedback @manzoorwanijk. Updated the implementation to follow your suggestion about using admin-post for file downloads. The REST endpoint now returns JSON with a download_url that points to an admin-post handler. |
projects/packages/forms/src/contact-form/class-contact-form-endpoint.php
Show resolved
Hide resolved
I retested this changes and it works well. It would be good to show some sort of in progress button while the URL is being returned. This will help the user not click the button multiple times. Lets add a test for the new endpoint and then I think this is ready to ship! Nice work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for implementing the suggestions. This looks much better now. I have some further suggestions for improvement, some of which you may ignore if you prefer.
$data = $this->get_feedback_entries_from_post(); | ||
public function admin_post_feedback_export() { | ||
$feedback_ids_str = sanitize_text_field( wp_unslash( $_GET['feedback_ids'] ?? '' ) ); | ||
$post_id = sanitize_text_field( wp_unslash( $_GET['post_id'] ?? '' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't pass the post_id
when generating the URL from the REST endpoint. Do we?
projects/packages/forms/src/contact-form/class-contact-form-plugin.php
Outdated
Show resolved
Hide resolved
window.location.href = response.download_url; | ||
return response; | ||
} | ||
throw new Error( 'Invalid response: missing download URL' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to translate this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say not yet since it would only show up in the dev console for now.
return new WP_Error( 'no_responses', __( 'No responses found', 'jetpack-forms' ), array( 'status' => 404 ) ); | ||
} | ||
|
||
$feedback_ids = wp_list_pluck( $feedback_posts, 'ID' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add 'fields' => 'ids'
to $query_args
above to get rid of this step and make it a bit faster.
|
…ugin.php Co-authored-by: Manzoor Wani <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing the feedback. This looks good to me now.
👉 I would still love to improve admin_post_feedback_export
method in some next iteration like this.
function admin_post_feedback_export() {
// First check for the user permissions before we do anything.
if ( ! current_user_can( 'export' ) ) {
wp_die( esc_html__( 'You do not have permission to export form responses.', 'jetpack-forms' ), 403 );
}
// Next, let us sanitize the input parameters that we need immediately.
$feedback_ids_str = sanitize_text_field( wp_unslash( $_GET['feedback_ids'] ?? '' ) );
$nonce = sanitize_text_field( wp_unslash( $_GET['nonce'] ?? '' ) );
// Ensure we have the required parameters.
if ( empty( $feedback_ids_str ) || empty( $nonce ) ) {
wp_die( esc_html__( 'Invalid request parameters.', 'jetpack-forms' ), 400 );
}
// Verify the nonce.
if ( ! wp_verify_nonce( $nonce, 'feedback_export_' . $feedback_ids_str ) ) {
wp_die( esc_html__( 'Security check failed.', 'jetpack-forms' ), 403 );
}
// Now extract and sanitize feedback IDs.
$feedback_ids = array_values(
array_filter(
array_map(
'intval',
explode( ',', $feedback_ids_str )
)
)
);
// Shortcircuit if we have no feedback IDs.
if ( empty( $feedback_ids ) ) {
wp_die( esc_html__( 'Invalid request parameters.', 'jetpack-forms' ), 400 );
}
// Let us get the export data.
$export_data = $this->get_export_feedback_data( $feedback_ids );
if ( empty( $export_data ) ) {
wp_die( esc_html__( 'No responses found to export.', 'jetpack-forms' ), 404 );
}
// Now let us get the post ID as we need it now.
$post_id = sanitize_text_field( wp_unslash( $_GET['post_id'] ?? '' ) );
$this->download_feedback_as_csv( $export_data, $post_id );
}
Introduces an isExporting state to track export progress and prevent UI flickering. The export button is now disabled and shows a busy indicator while exporting, improving user feedback during CSV downloads. props @simison
} ); | ||
setIsExporting( false ); | ||
} | ||
}, [ currentQuery, selected, closeModal, createErrorNotice ] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've updated the onExport method returned from this hook. It doesn’t accept args and returns an object from the endpoint rather than fetch Response.
This onExport method is passed to both the CSV and Google export components, and the CSV file has been updated to use the new onExport as expected.
But the Google Drive card is still using this same onExport function the old way here.
onExport( 'grunion_export_to_gdrive', 'feedback_export_nonce_gdrive' )
.then( ( response: Response ) => response.json() )
.then( ( { data } ) => {
window.open( data.sheet_link, '_blank' );
} )
.finally( () => {
setIsExporting( false );
} );
}, [ tracks, onExport, isExporting ] );
I'd think we either need to update Google drive to use the new onExport method correctly, or else if there's a reason we don't want to tackle that here, maybe have the useExportResponses() hook still return the old method (for Google) along with the new one (for CSV), and then handle updating Google in a follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm if you try to Export to Google with this PR, you do get an error:
Uncaught (in promise) TypeError: response.json is not a function
at eval (google-drive.tsx:76:100)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. @edanzer. It used to work but I think I broke it :(
Will create a fix for google drive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
Proposed changes
This PR replaces the legacy AJAX-based CSV export with a modern REST API endpoint for Jetpack Forms responses.
Changes:
/wp/v2/feedback/export
REST endpoint for CSV exportswp_ajax_feedback_export
and thedownload_feedback_as_csv()
method@wordpress/api-fetch
Other information
Testing instructions
Product impact
Does this pull request change what data or activity we track or use?
No
Checklist