Skip to content

Feature: Update V3 DonationController to support trashed donations #8018

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

Open
wants to merge 16 commits into
base: epic/donation-details-admin-page
Choose a base branch
from

Conversation

JoshuaHungDinh
Copy link
Contributor

@JoshuaHungDinh JoshuaHungDinh commented Jul 11, 2025

Resolves GIVE-2686

Description

Enhance DonationController to Support Soft and Hard Deletes with force Parameter

This PR improves the Donation REST API by introducing support for soft deletes (trashing) and hard deletes (permanent removal) of donations via a new optional force parameter on delete endpoints.

Key Changes:

  • Added force boolean parameter to both single and batch donation delete routes.
  • When force is false or omitted (default), donations are soft deleted by moving them to trash (wp_trash_post).
  • When force is true, donations are permanently deleted from the database.
  • Updated the REST route argument definitions to include the force parameter for single and multiple deletion endpoints.
  • Updated the delete_item and delete_items methods to handle the force parameter accordingly.

Affects

  • Deleteing donation/'s
  • v3 API Donations Controller

Visuals

Testing Instructions

  • Create several donations.
  • While on the Donation Overview page - useing the 3 dot menu on the top right delete a donation.
  • View the donation in the Post table of the database to verify the its status is now Trashed

Pre-review Checklist

  • Acceptance criteria satisfied and marked in related issue
  • Relevant @unreleased tags included in DocBlocks
  • Includes unit tests
  • Reviewed by the designer (if follows a design)
  • Self Review of code and UX completed

@JoshuaHungDinh JoshuaHungDinh changed the base branch from develop to epic/donation-details-admin-page July 11, 2025 19:45
@JoshuaHungDinh JoshuaHungDinh marked this pull request as ready for review July 15, 2025 14:58
Comment on lines 248 to 249
$query->where('post_type', 'give_payment');
$query->where('post_status', 'trash', '<>');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a way for folks to choose whether trashed donations are included in the results.

Copy link
Contributor Author

@JoshuaHungDinh JoshuaHungDinh Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I’ve was thinking we might need a way to surface trashed donations in the list table and also provide users with a way to 'empty the trash'.

@@ -126,6 +127,16 @@ public function registerRoute()
'columns',
],
],
'postStatus' => [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The post status is something we use under the hood for storage - however, the donation should just refer to this as status

Suggested change
'postStatus' => [
'status' => [

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved: 4b24aa6

'active',
'trash',
],
'description' => 'Filter donations by WordPress post status: active (all except trash), or trash.'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again here let's keep the context of the donation - not WordPress posts (which is the storage we use under the hood and will change in the future)

Suggested change
'description' => 'Filter donations by WordPress post status: active (all except trash), or trash.'
'description' => 'Filter donations by Donation status: active (all except trash), or trash.'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved: 4b24aa6

$errors[] = ['id' => $id, 'message' => __('Failed to delete donation', 'give')];
}
} else {
$trashed = wp_trash_post($donation->id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hesitant to use wp_trash_post directly here as we want this endpoint to interact with the donation model & abstract awa the data layer as much as possible. In the future when the storage of a donation changes - this would be something we would have to update.

wp_trash_post does a couple things:

  1. Changes the status to trash
  2. Stores the previous status & time in meta _wp_trash_meta_status, _wp_trash_meta_time (as a way to restore to previous status)

All that is something we can support directly in the donation repository with a new trash() method and/or build it into the existing delete method.

At the very least if we simply changed the status of the donation to trash and saved - then that would be better for maintainability here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the context! That makes sense in terms of long-term maintainability and keeping the data layer abstracted.

I pushed up a commit here to reflect that approach: 59a1f8b. Let me know if this looks more in line with what you were thinking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants