Forms: save submissions toggle #45072
Conversation
github-actions
bot
added
[Block] Contact Form
|
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. |
vianasw
added
[Status] Needs Review
github-actions
bot
added
[Status] Needs Author Reply
|
I think the same happens to emails: 
|
|
Just listing aspects to take into account:
Overall, I recommend feature-gating and working through issues in separate PRs. Would it be too much if we stored the submission temporarily for all things to work, and then removed the submission? Seems a bit wrong. 😅 |
vianasw
force-pushed
the
add/forms-store-submissions-toggle
branch
from
4f8c0fc to
399bd6d
Compare
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryCoverage changed in 3 files.
|
Sorry, the PR wasn't ready for testing yet. I've addressed the email and post-submission page issues now. |
I've addressed the issues with the email and post-submission page in a way that I think won't be necessary to store the submission temporarily. |
|
I tested and overall works well; the
|
I've added a toggle in the form's block panel to allow users to not save form submissions in wp-admin. The toggle is on by default.
Due to shortcode bacwkard render compability, we need to store the saveResponses attribute as a 'yes'/'no' string attribute.
This commit addresses issues in the contact form email functionality when the "Save responses" toggle is disabled. The changes ensure that: 1. Method signature enhancement: Added an optional $response parameter to get_compiled_form_for_email() to allow passing response data directly instead of always fetching from database 2. Conditional response fetching: Modified the method to only fetch response from database when not provided as parameter, preventing errors when no feedback ID exists 3. Defensive URL building: Added checks before building dashboard URLs and spam marking URLs to handle cases where $post_id is null (when responses aren't saved) 4. Conditional metadata storage: Added guard clause to prevent storing feedback email metadata when no post ID exists 5. Safe nonce generation: Added null check when generating nonces to avoid issues with missing post IDs 6. Action button handling: Made the "View in dashboard" action button conditional on having a valid dashboard URL These changes ensure the email notification system works properly regardless of whether form responses are being saved to the database or not.
* Added $expected_attributes['saveResponses'] = 'yes'; to the expected attributes in the test.
I deleted too much during the rebase conflict resolution.
- Add test_process_submission_stores_feedback_when_save_responses_yes() Tests that form submissions are stored when saveResponses='yes' - Add test_process_submission_does_not_store_feedback_when_save_responses_no() Tests that form submissions are NOT stored when saveResponses='no' - Add test_process_submission_stores_feedback_when_save_responses_default() Tests that form submissions are stored by default (saveResponses defaults to 'yes') These tests ensure the saveResponses attribute properly controls whether form submissions are stored in the WordPress database while maintaining email functionality regardless of the setting.
vianasw
force-pushed
the
add/forms-store-submissions-toggle
branch
from
dda00a3 to
1c5cd16
Compare
- Move status determination logic from Feedback::save() to Contact_Form::process_submission() - Centralize all status logic (trash, spam, temp, publish) in one location - Remove form reference from Feedback class as it's no longer needed - Improve code organization by keeping status logic with form processing - Maintain existing functionality while improving maintainability The temp status is now determined early in the form processing flow and passed to the Feedback object, creating a cleaner separation of concerns between form processing and data persistence.
- Remove redundant $post_id check when updating unread feedback count - Update dashboard URL condition to check feedback_status instead of $post_id - Ensures proper handling of temporary feedback posts when form submissions are not stored # Please enter the commit message for your changes. Lines starting
In the backend we have to live with the 'yes'/'no' string because of the shortcode legacy reason but we don't have to in the block attribute. This way we'd have to just refactor the backend if we move away from the shortcode-mangled way of doing things.
| @@ -279,6 +279,21 @@ protected function __construct() { | |||
| ) | |||
| ); | |||
|
|
|||
| // Add "temp" as a post status for temporary storage when saveResponses is 'no' | |||
| register_post_status( | |||
| 'temp', | |||
There was a problem hiding this comment.
This worked fine, I just wonder if we should purposely use a less common word. After all, this will live both on dotcom and on any self hosted, we need to make sure the post status registration doesn't fail because there was already one there with the same name. I'd use some more complex, like jetpack-temp-feedback or something
There was a problem hiding this comment.
hum, good point. I'll give it a try.
| // translators: The temporary count. | ||
| 'label_count' => _n_noop( 'Temporary <span class="count">(%s)</span>', 'Temporary <span class="count">(%s)</span>', 'jetpack-forms' ), | ||
| 'protected' => true, | ||
| '_builtin' => false, |
There was a problem hiding this comment.
I don't think we need to do this. Since this is the default for '_builtin' is false;
Can we also set internal to true. So that the status work more as expected.
It then future setting will be more align with what we expected.
Such as show_in_admin_status_list - see https://developer.wordpress.org/reference/functions/register_post_status/
| 'exclude_from_search' => true, | ||
| 'show_in_admin_all_list' => false, | ||
| // translators: The temporary count. | ||
| 'label_count' => _n_noop( 'Temporary <span class="count">(%s)</span>', 'Temporary <span class="count">(%s)</span>', 'jetpack-forms' ), |
There was a problem hiding this comment.
I don't think we need this. Not sure where it would show up. Since we should be hiding it.
| @@ -2076,15 +2099,14 @@ public function process_submission() { | |||
| 'contact-form-id' => $id, | |||
| 'contact-form-sent' => $post_id, | |||
| 'contact-form-hash' => $this->hash, | |||
| '_wpnonce' => wp_create_nonce( "contact-form-sent-{$post_id}" ), // wp_nonce_url HTMLencodes :( . | |||
| '_wpnonce' => $post_id ? wp_create_nonce( "contact-form-sent-{$post_id}" ) : '', // wp_nonce_url HTMLencodes :( . | |||
There was a problem hiding this comment.
Do we still need this. Since we should be always setting the $post_id now.
| @@ -1806,6 +1815,8 @@ public function process_submission() { | |||
| $feedback_status = 'trash'; | |||
| } elseif ( $is_spam ) { | |||
| $feedback_status = 'spam'; | |||
| } elseif ( 'no' === $this->get_attribute( 'saveResponses' ) ) { | |||
| $feedback_status = 'temp'; | |||
There was a problem hiding this comment.
Should we always be setting to 'temp' status?
My assumtion would be that this would always happen if saveResponses is set to "no"
So even if we mark the post as "spam" we. Not sure if we need another status here. Such as "spam-temp" or something like that 🤔. I would assume no.
| @@ -1861,15 +1872,17 @@ public function process_submission() { | |||
| // once insert has finished we don't need this filter any more | |||
| remove_filter( 'wp_insert_post_data', array( $plugin, 'insert_feedback_filter' ), 10 ); | |||
|
|
|||
| update_post_meta( $post_id, '_feedback_extra_fields', $this->addslashes_deep( $extra_values ) ); | |||
| if ( $post_id ) { | |||
|
|
||
| if ( 'publish' === $feedback_status ) { | ||
| // Increase count of unread feedback. | ||
| $unread = (int) get_option( 'feedback_unread_count', 0 ) + 1; | ||
| update_option( 'feedback_unread_count', $unread ); | ||
| } | ||
|
|
||
| if ( defined( 'AKISMET_VERSION' ) ) { | ||
| if ( defined( 'AKISMET_VERSION' ) && $post_id ) { |
Fixes FORMS-247
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
Manage responsespanel and set theSave responsetoggle off.