-
Notifications
You must be signed in to change notification settings - Fork 818
Forms: restore not spam action #43559
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
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. |
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.
Pull Request Overview
This PR reinstates sending the contact form submission email when an entry is moved from spam back to the inbox.
- Overrides
update_item
to detect a status change from spam to publish and triggerresend_email
. - Adds
resend_email
method to reconstruct and send the original notification email. - Updates changelog to document the new behavior.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
projects/packages/forms/src/contact-form/class-contact-form-endpoint.php | Adds update_item override and resend_email to resend emails on spam restore |
projects/packages/forms/changelog/fix-jetpack-forms-restore-not-spam-action | Documents the behavior in the changelog |
Comments suppressed due to low confidence (1)
projects/packages/forms/src/contact-form/class-contact-form-endpoint.php:391
- [nitpick] This new
update_item
behavior (restoring from spam and resending email) isn’t covered by existing tests; consider adding a unit or integration test to verify the status transition and email resend invocation.
public function update_item( $request ) {
$post_id = $request['id']; | ||
$previous_status = get_post_status( $post_id ); | ||
$updated_item = parent::update_item( $request ); | ||
if ( ! is_wp_error( $updated_item ) && ! empty( $updated_item->data && ! empty( $updated_item->data['status'] ) ) ) { |
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.
The conditional combines empty
checks incorrectly. Consider simplifying to if ( ! is_wp_error( $updated_item ) && ! empty( $updated_item->data['status'] ) ) {
or use isset( $updated_item->data['status'] )
for clarity and correctness.
if ( ! is_wp_error( $updated_item ) && ! empty( $updated_item->data && ! empty( $updated_item->data['status'] ) ) ) { | |
if ( ! is_wp_error( $updated_item ) && isset( $updated_item->data['status'] ) && ! empty( $updated_item->data['status'] ) ) { |
Copilot uses AI. Check for mistakes.
@@ -381,6 +381,93 @@ public function get_item_schema() { | |||
return $this->add_additional_fields_schema( $this->schema ); | |||
} | |||
|
|||
/** |
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.
[nitpick] New methods update_item
and resend_email
should include @since
annotations in their docblocks to track when this behavior was introduced.
Copilot uses AI. Check for mistakes.
Code Coverage SummaryCoverage changed in 1 file.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
* @param WP_REST_Request $request Request object. | ||
* @return WP_REST_Response Response object. | ||
*/ | ||
public function update_item( $request ) { |
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 didn't look if we check earlier things like id
isn't empty/invalid, post with that ID exists etc but feels like something like that is missing here? Not a blocker for merging.
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're right on this one. I'm mostly relying on the parent's process to validate, but I'm also taking the id
first.
Ref: fbhepr%2Skers%2Sjcpbz%2Sjc%2Qvapyhqrf%2Serfg%2Qncv%2Sraqcbvagf%2Spynff%2Qjc%2Qerfg%2Qcbfgf%2Qpbagebyyre.cuc%3Se%3Qq13qsq59%26zb%3Q235%26sv%3Q17%23932-og
I think I'll repeat the validity check on the parent process (I need the previous status) and then allow the process to continue. Something like:
$valid_check = parent::get_post( $request['id'] );
if ( is_wp_error( $valid_check ) ) {
return $valid_check;
}
$post_id = $request['id'];
$previous_status = get_post_status( $post_id );
$updated_item = parent::update_item( $request );
if ( ! is_wp_error( $updated_item ) && ! empty( $updated_item->data && ! empty( $updated_item->data['status'] ) ) ) {
...
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 haven't tested, but approving based on reviewing code:
- Email sending is there,
- Akismet marking post as "HAM" is there, which was important too
No tracking that I can see so I wonder if we should add that in a follow-up? Either bump start or Tracks.
In my understanding we'll be able to remove the other, old chunk for similar code together with Classic view, so there's no point abstracting these to use shared method?
9571ceb
to
a6c3d36
Compare
a6c3d36
to
1789bf7
Compare
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.
Tested locally and simple sites, worked well.
Rebased as well.
Related to FORMS-89
Proposed changes:
This PR adds the old Admin ajax process to send the submission email when the user moves a response from spam to inbox.
Other information:
Jetpack product discussion
p1747734409666879-slack-C086RGTJT1D
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Test this on your sandbox, local envs usually don't send emails. Unsure about JN sites.
Move a response from Spam to Inbox, you should receive a submission email on the associated email address as with a regular submission.