Fatal Error when setting donation date to future

[flack]

User Story

As an admin, I want to set offline donations to Completed in the backend. If I mis-click in the date selector for some reason, I get a Fatal Error and an email is sent to the site administrator

Details

When you set the date of a donation to the future (most likely by accident), the follwing Fatal Error occurs:

 Fatal error: Uncaught UnexpectedValueException: Value 'future' is not part of the enum Give\Donations\ValueObjects\DonationStatus in wp-content/plugins/give/vendor/myclabs/php-enum/src/Enum.php on line 50

Expected Behavior

• If you set an invalid (future) date, there should be a validation that prevents that.
• also, the datepicker should just disable future dates so that the user is less likely to make that mistake
• At the very least, no PHP-level error should be thrown.

Steps to Reproduce

1. Go to the list of donations in the backend, click on one
2. Set donation date to tomorrow
3. Click "Save"
4. View error

Additional Context

The easiest fix would probably be to add

const FUTURE = 'future';

to the constants here:

givewp/src/Donations/ValueObjects/DonationStatus.php

Lines 33 to 41 in 41a1b43

	const PENDING = 'pending';
	const PROCESSING = 'processing';
	const COMPLETE = 'publish';
	const REFUNDED = 'refunded';
	const FAILED = 'failed';
	const CANCELLED = 'cancelled';
	const ABANDONED = 'abandoned';
	const PREAPPROVAL = 'preapproval';
	const REVOKED = 'revoked';

It does fix the Fatal Error, but I didn't test if it has any side effects

System Information

Details

Acceptance Criteria

• Something happens when an action is taken.
• Something does not happen when an action is taken.
• Fixing behavior in Component A does not affect existing behavior in Component B.

[kjohnson]

Hey @flack - thanks for the bug report.

I'm surprised that I wasn't able to find a duplicate issue for this 😅. Seems to be one of those bugs that goes unreported. Somewhat related is #6595 (mentioning here for cross-reference). Perhaps @ravinderk has some related historical knowledge as to why future was not included or how that was previously handled.

We are generally protective of that status list, but whether we add future to the list or enforce a particular status we'll address the Fatal Error.

I'll bring this to the team for prioritization.

[ravinderk]

@kjohnson Current issue and #6595 are separate.

#6595 has a logical error in the donation importer and we should fix it.

The author mentioned that this issue is caused by a future donation date. I did not see any reason to adjust the donation date in the future, so we should not allow a future status on the donation date.
We should add a validator and disable future dates on the donation edit screen.

@Benunc Do you think the donation date can be set to the future?

[kjohnson]

Good point, @ravinderk - thank you.

[canny]

This issue has been linked to a Canny post: Fatal Error when setting donation date to future 🎉

[kjohnson]

Summary from Slack converstaion:

"Donations" are a historical record of what happened, not a promise for the future.
Shouldn’t fatal error the site when you make it future-dated.
Disable the future date selection or, when the page is saved, use a hook to hijack the status and switch it to pending.

[Benunc]

I do not think donations should be able to be set in the future. Sorry I missed this ping. @ravinderk

[flack]

Any news on solving this? It's still happening on production for us, so I have to hack the deployed Give code to make the backend useable again

[JasonTheAdams]

Hi @flack!

This is a UI issue that will be resolved in a future version of GiveWP. In the meantime, I threw together this snippet that you can add somewhere, which will prevent donations from being saved in the future and thus changing the post status:

add_filter('wp_insert_post_data', static function(array $data, array $postarr) {
    if ($data['post_type'] !== 'give_payment') {
        return $data;
    }

    // Check if $data date is in the future, and set it to now if so.
    if ( strtotime($data['post_date']) >= strtotime('+1 second') ) {
        $oldPost = get_post($postarr['ID']);
        $data['post_date'] = current_time('mysql');
        $data['post_date_gmt'] = current_time('mysql', true);
        $data['post_status'] = $oldPost->post_status;
    }

    return $data;
}, 10, 2);

We may consider adding a guard rail like this in GiveWP to help as well, but I don't want that decision to prevent you from having something in place. This should be future-safe from any changes made on our end.

[flack]

@JasonTheAdams thanks, I added this to our code. Fingers crossed that we can delete it soon! :-)

[flack]

@JasonTheAdams did you test this code? Because I've added it on our site, and now I can't create any donations anymore. The strtotime($data['post_date']) >= strtotime('+1 second') is always true, then $data['post_status'] is set to null, because $oldPost doesn't exist (it's a new donation), and then the SQL INSERT fails because post_status cannot be null

[flack]

P.S.: Just a guess: We're in GMT+1 timezone, probably the check is always true because of that? but even if I change to strtotime($data['post_date_gmt']) >= strtotime('+1 second'), $oldPost may be null on create

[JasonTheAdams]

Hi @flack!

Yeah, I did test it. Apparently I dropped the $update check on accident, so good catch there. I'm guess my local WP time settings and server settings are the same, so it's probably mis-aligning the time zones for you. If you add the WP timezone to both strtotime calls, I bet that will work. The current_time() function already uses the wp_timezone() function under the hood.

Friendly note, I would always make sure to test a snippet (or any code change) locally before applying it to production — no matter who gives it to you. Obviously, we can't test it on your exact environment, so whatever we send you may not be appropriately fine-tuned.

[flack]

@JasonTheAdams yeah, no worries, we didn't apply to production right away, it was caught on the staging server, so no actual users were affected :-)

I'll try your suggestions tomorrow, thanks for following up!

[JasonTheAdams]

Hahah! Oh good! I'm glad! If you wouldn't mind dropping the snippet that ends up working here for others to glean from that would be awesome!

[flack]

@JasonTheAdams so the version that works in principle for me looks like this:

add_filter('wp_insert_post_data', static function(array $data) {
    if ($data['post_type'] === 'give_payment') {
        // Check if $data date is in the future, and set it to now if so.
        if ( strtotime($data['post_date_gmt']) >= strtotime('+1 second') ) {
            $data['post_date'] = current_time('mysql');
            $data['post_date_gmt'] = current_time('mysql', true);
        }
    }
    return $data;
}, 10);

I've removed the post_status stuff altogether, because it doesn't really do anything. post_status is always shown as pending, I can see give-payment-status with the actual value in the $_POST data, but that seems to be set in some other place. Also, I'm not sure it's necessary to play with that. As long as the date doesn't change to the future, the fatal error is circumvented, and if we're changing the date after a user action, we may as well set the payment status requested