Open flack opened 1 year ago
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.
@kjohnson Current issue and https://github.com/impress-org/givewp/issues/6595 are separate.
https://github.com/impress-org/givewp/issues/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?
Good point, @ravinderk - thank you.
This issue has been linked to a Canny post: Fatal Error when setting donation date to future :tada:
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.
I do not think donations should be able to be set in the future. Sorry I missed this ping. @ravinderk
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
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.
@JasonTheAdams thanks, I added this to our code. Fingers crossed that we can delete it soon! :-)
@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
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
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.
@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!
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!
@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
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:
Expected Behavior
Steps to Reproduce
Additional Context
The easiest fix would probably be to add
to the constants here:
https://github.com/impress-org/givewp/blob/41a1b43d922c08211f894a5eb2813bf002265b77/src/Donations/ValueObjects/DonationStatus.php#L33-L41
It does fix the Fatal Error, but I didn't test if it has any side effects
System Information
Details
Acceptance Criteria