-
Notifications
You must be signed in to change notification settings - Fork 37
Create pending renewal order (improve safety/error handling) #768
Conversation
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.
Really nice work on this PR @barryhughes and thanks for all of the details in the PR description!
Something we often see in support is issues where third-parties hook onto wcs_renewal_order_created
and incorrectly return false, instead of the order object... here's a classic example:
/**
* Attach additional data to stripe subscription renewals.
*/
add_filter( 'wcs_renewal_order_created', function( $renewal_order, $subscription ) {
// if renewal is not stripe return false
if ( 'stripe' !== $renewal_order->get_payment_method() ) {
return false; // NAUGHTY!
}
$renewal_order->update_meta_data( 'custom_meta_key', 'custom_meta_value' );
$renewal_order->save();
return $renewal_order;
}, 10, 2 );
Should we add some additional checks for this case as well?
It's a pretty edge case issue but while we're here, I figured it would be nicer to display an admin notice and add an order note instead of merchants seeing this exception:
We could possibly extend the try/catch to also capture PHP errors and attach the same "A %1$spending renewal order%2$s was successfully created, but there was a problem setting the payment method. Please review the order." notice/note. Thoughts?
To your questions:
As above, are we happy to tackle all three potential failures from
::create_pending_renewal_action_request()
in the same PR?
Yeah, I'm onboard with this :)
Do we feel there is a need to follow-up, and try to proactive remove create pending renewal order from the order actions list (where possible)? This was one of the bug report author's recommendations, and it seems like this may already have happened from within WooCommerce Pre-Orders.
Good question! Including a subscription action that we know is going to fail/error seems a bit strange, but on the other hand, not including it in the list of actions may confuse merchants that go looking for it?
Given we only add this action when the subscription doesn't have an ended status (code ref), my thinking is that it should be fine to add another condition to this and not include this action if the subscription can't be updated to on-hold (i.e. $subscription->can_be_updated_to( 'on-hold' )
).
Out of curiosity, what do you think makes the most sense from a merchants perspective?
Would you rather see an action that will fail and then be able to see why it failed in the order notes, or would it be better to not see the action to begin with?
It seems like essentially this same set of problems is also true of the create pending parent action: should we follow-up with similar fixes for that, too?
That would be awesome ✋
'success', | ||
sprintf( | ||
/* Translators: %1$s opening link tag, %2$s closing link tag. */ | ||
esc_html__( 'A pending %1$srenewal order%2$s was successfully created!', 'woocommerce-subscriptions' ), |
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've been pondering whether to send this feedback as it feels very minor... 😅
While testing these changes, I noticed that the wcs_create_renewal_order()
function already adds a similar order note to this one, and so on success, you have two very similar order notes added which feels unnecessary:
The Create pending renewal order requested by admin action.
order note was purposefully added to the start of the function in https://github.com/woocommerce/woocommerce-subscriptions/pull/3462 and I believe it served two purposes:
- For debugging purposes, having it at the very start allows us to know whether an issue was coming from an admin requested the action before any uncaught exceptions or php shutdowns happened (worst case I'm talking someone calling
exit;
🙈 ) - It separates the last renewal order notes from the start of the new renewal order notes, for example:
trunk |
this branch |
---|---|
![]() |
![]() |
Here's what I'm thinking
- Add back the order note at the start signalling the start of an admin requested action (consistent with our other subscription actions like "Process renewal"
- Keep this admin notice with the link to the renewal order, but don't include the order note.
Curious to hear your thoughts on this!
The other idea I had was to remove the initial order note at the start and instead pass "Create pending renewal order requested by admin action." as the $note param when calling $subscription->update_status( 'on-hold', $note );
🤔 but this also introduces inconsistencies with the similar "Process Renewal" action.
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.
- Add back the order note at the start signalling the start of an admin requested action (consistent with our other subscription actions like "Process renewal"
- Keep this admin notice with the link to the renewal order, but don't include the order note.
Agreed, except (and here I'm probably missing a trick) I don't think the original Order #XYZ created to record renewal
ever disappeared. At least, I can't seem to replicate what I see in your screenshot from this branch. Perhaps we're each approaching this in a slightly different way? 🤔
Nonetheless, I agree with the concept, and committed f9114d0, which should give us the admin notice (top of screen) alongside the existing order note:
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've been pondering whether to send this feedback as it feels very minor...
Btw, very glad to get notes like this. I think this is how we remove some of the papercuts from the UX (or, in this case, it's how we avoid adding new papercuts, so to speak).
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.
At least, I can't seem to replicate what I see in your screenshot from this branch. Perhaps we're each approaching this in a slightly different way?
Oh sorry I probably should've shared how I was testing this 😅
- Purchase a subscription product
- Process a regular renewal by either running the pending scheduled action for this subscription or using the "Process Renewal" subscription action.
- Process the "Create pending renewal order" action.
I'm not sure if this helps but here's a longer screenshot:
trunk |
this branch |
---|---|
![]() |
![]() |
With the "requested by admin action" note being added at the start of the process, it acts as a separator but also adds context for why this renewal was created.
For that reason, as well as keeping this action consistent with the "Process Renewal" action, I think we should add back this order note which was removed.
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.
(Edit: ignore this comment, I see what you mean now.)
With the "requested by admin action" note being added at the start of the process, it acts as a separator but also adds context for why this renewal was created.
Hmm. I wasn't missing this before, and on trying to replicate once more I do still get the requested by admin action note:
Or perhaps besides a subtle difference in testing, a further commit addressed this?
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.
☝🏼 Ignore!
try { | ||
$subscription->update_status( 'on-hold' ); | ||
} catch ( Exception $e ) { | ||
self::notify( |
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'll wait to see what we do regarding my previous comment (#768 (comment)), but update_status()
will already add an order note when an exception is thrown so I think just the notice is fine here.
Also what are your thoughts on adding the exception message to the admin notice, too much? Or unhelpful?
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.
but update_status() will already add an order note when an exception is thrown so I think just the notice is fine here
Yep, agreed. I think I was failing to see the trees for the forest in regards to admin notices/order notes. This aspect has been adjusted in 95f7c63: we add an admin notice, and retain the original order note, but don't add a second one.
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.
Also what are your thoughts on adding the exception message to the admin notice, too much? Or unhelpful?
Hmm. Potentially useful ... for instance, suppose an exception is raised over here. Then, if we include the exception message, we'd output a notice like:
Pending renewal order was not created, as it was not possible to update the subscription status. Subscription 12345: The creation date of a subscription can not be deleted, only updated.
(I'm putting the exception message in bold just to make the composition of the notice nice and clear.)
That's pretty good (though it could be finessed a little), as it adds extra information about what exactly went wrong. On the other hand, if an exception is raised from here, then it would be:
Pending renewal order was not created, as it was not possible to update the subscription status. Unable to change subscription status to "on hold".
So, in this case, we get some not-very-helpful duplication. I think we could resolve this by changing the wording of this message ... yet I'm not immediately sure what we'd change it to. Or perhaps I'm overthinking it, and this is actually fine?
If WC_Subscription::can_be_updated_to()
supplied a reason as well as a boolean, this would be more elegant (but I'm not sure we can safely make such a change).
All-in-all, I think there's something in what you're suggesting, but I'm also a little reticent to over-complicate things 🤔
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.
Good points! Considering these exceptions are already added as notes on the subscription (here and here).
Maybe we could just add a notice that says something like:
Failed to create a pending renewal order for subscription #1234. View subscription notes for more details.
🤔
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.
Hmm, after posting this i noticed that when wcs_create_order_from_subscription()
returns a WP_Error
, there's no exception message added to the order notes to saying "view notes for details" isn't exactly true in all cases.
Like you said, it's very possible we're both over thinking/complicating this 😄
This PR is already proving to be a much better improvement to what we had previously so I'm happy to run with it as is.
That said, I was thinking... inside the if ( is_wp_error( $renewal_order ) ) {
(here) we could attach the error message to the notice/order note since nothing gets added currently.
This would look something like:
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.
In 467409c I added a change to append the WP_Error
message to the notice/note text:
Simulated using:
add_filter( 'wcs_new_order_created', fn () =>
new WP_Error(
'simulated-order-creation-error',
'Failed to reticulate splines, warp drive cancelled.'
)
);
I'm not really sure. The problem with not showing the action to begin with is that, in this scenario, the merchant would reasonably expect it to be present. Removing it without an explanation would potentially lead to support requests ("why don't I see an option to create a pending renewal order?" ... "Hmm, we're not sure either" ← because, of course, the reality is we don't know what rationale the extension removing this capability might be applying). So, I think the onus is probably on the extension that prohibits the status update to take care of this, in a way that makes sense for our mutual users. With that said, I'm also not sure why the subscription status needs to be set to on-hold (to be clear, I can see why that generally makes sense, I'm more asking if it's vital). So perhaps the gold standard would be something like:
However, I think that would require a much larger effort (and may not actually be worth the effort), so a smaller incremental improvement is probably a better starting point. Does that make sense—what are your own thoughts? |
Right. If I'm understanding correctly, it's essentially that classic problem we have across most of our plugins, and WordPress itself, in which we have code like this: /**
* @return SomeType
*/
function get_some_type() {
/**
* You can modify the instance of SomeType here!
*
* @param SomeType $some_type
*/
return apply_filters( 'create_some_type', new SomeType() );
} The declared return type is potentially undermined by the filter: we say we'll return an instance of /**
* @return SomeType
*/
function get_some_type() {
$filterable_object = new SomeType();
/**
* You can modify the instance of SomeType here!
*
* @param SomeType $some_type
*/
$return_val = apply_filters( 'create_some_type', $filterable_object );
// If $return_val has been converted into something other than an instance
// of SomeType, we return a refence to the original $filterable_object,
// satisfying the declared return type.
Types::ensure_instance_of( $return_val, SomeType::class, fn () => $filterable_object );
} We don't really need the helper for simpler situations like this one, of course (and can't use it from Subscriptions in any case, because we still support Woo Core versions earlier than 9.1, which is when the Types helper was introduced). Mapping this back to an approximation of function wcs_create_renewal_order( $subscription ) {
$renewal_order = wcs_create_order_from_subscription( $subscription, 'renewal_order' );
// ...other mechanics...
$return_val = apply_filters( 'wcs_renewal_order_created', $renewal_order, $subscription );
return $return_val instanceof WC_Subscription ? $return_val : $renewal_order;
} ☝🏼 This is a pretty long blurb (thanks for reading!) but my core questions are: A. Should we fix this at function level (should we ensure functions like this one satisfy their declared signature, instead of looking for unexpected results at the call site)? Edit/update: if preferred, and if we want to keep things a bit more contained, I believe we could also just update this line (so that we guard against the order being anything other than an order, instead of only guarding against the possibility of it being a - if ( is_wp_error( $renewal_order ) ) {
+ if ( ! $renewal_order instanceof WC_Order ) { |
This makes sense to me and I agree with your reasoning! I also liked the example you gave with potential support requests 👍
This action mimics the standard renewal flow, but instead of processing the payment it creates a pending renewal order that customers can pay for. For reference here's the standard renewal flow is:
For "need" to put the subscription on-hold during the automatic payment flow is because if something goes wrong, the subscription doesn't remain active (more for digital/virtual subscriptions to make sure access is removed until payment succeeds). The "create pending renewal order" is there for merchants to trigger a manual renewal so it makes sense to put the subscription on-hold like we do for other renewal processes. We could definitely improve this like allowing merchants to choose if they want to proceed without placing it on-hold. But there might be unforeseen consequences like the next payment date not being properly updated to reflect the most recent payment. |
You raise all good points and I'm happy to run with plan B :) We haven't seen any reports yet of this error popping up for merchants using this action and there's probably a better way we could approach this issue more globally rather than within individual functions. |
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.
Thanks again Barry for all the discussion on this PR.
My feedback is a little all over the place (apologies!), but here's what I believe is left/remaining before we can merge this:
- Look at bringing back the "requested by admin action." order note added at the start of the request (discussed here)
- An alternative option to including the exception message in the admin notice (discussed here)
…ing renewal order.
…not be created), just add an admin notice.
… order note with an admin notice. ...but do not add a duplicate order note.
f83f61a
to
5b78cbc
Compare
…wcs_create_renewal_order() to something unexpected.
In 36ce48f I have added some defensive code to ensure we return either an instance of |
It started off as such a simple problem 😅 ... anyway, thanks as ever for the great feedback, @mattallan. I believe I've addressed both concerns itemized in this comment, and have also updated the testing instructions, if only for posterity (note: some of the snippets were also updated to match other adjustments).
Minor thing, and here seems as good a place as any to ask this (as we all get acclimatized to working together) ... I like these checklists as a way of summarizing what further changes are still required, especially when there are a range of mini-discussions in progress. In this case, I checked each off after making commits to address them. Is that how you normally do things, or do you prefer that they are left unchecked for you as the reviewer to revisit? |
Since KSES is used to perform escaping, we can eliminate the pre-emptive use of `esc_html()`. Use of `esc_html()` was also dropped in 72c4a08, for the same reason (and because it had the effect of 'exposing' the HTML for things like links to the merchant).
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.
Hey @barryhughes thanks for your latest changes! I just left a fun comment regarding removing the esc_html from wcs_display_admin_notices()
😭 (sorry)
In this case, I checked each off after making commits to address them. Is that how you normally do things, or do you prefer that they are left unchecked for you as the reviewer to revisit?
Yes perfect! I'm happy for either you or I to check them off, I'm not fussed :)
@@ -109,7 +109,7 @@ function wcs_display_admin_notices( $clear = true ) { | |||
continue; | |||
} | |||
|
|||
$notice_output[] = esc_html( $notice['message'] ); | |||
$notice_output[] = $notice['message']; |
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.
Does removing this esc_html
mean we should be escaping every other use of wcs_add_admin_notice()
within Subscriptions and the other extensions that use it (WooPayments, Gifting Subscriptions).
I could be wrong by my understanding is that just having wp_kses_post
is only okay if we trust the content, but given these messages are typically passed through __()
, wp_kses_post()
won't be enough as it can still allow inline javascript through :(
I'm sorry for putting you on this rollercoaster! I didn't even consider this escaping links issue when suggesting putting links in the admin notices 😭
What are your thoughts given the above?
I'm thinking we roll back the idea of including a link in the notice, but then that means we don't have a need for the new notify()
function you added because I'm assuming we'll still want to add links to the order notes? Oh man 🙈
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.
Who doesn't enjoy rollercoasters? 🎢 ...
I think the links are useful—the user experience feels better—so it would be great to keep them if we can do so safely. Drawing some amount of inspiration from Woo Core's HtmlSanitizer, what if continue to use Kses, but with a tighter set of rules than we get from wp_kses_post()
?
$allow_links = array(
'a' => array(
'href' => true,
),
);
wp_kses( $html, $allow_links );
We could expand this over time if, for instance, we wished to allow <em>
, <strong>
, etc. However...
I'm thinking we roll back the idea of including a link in the notice, but then that means we don't have a need for the new notify() function you added because I'm assuming we'll still want to add links to the order notes?
I'm not tied to the new notify()
method, we can drop it if it turns out to be problematic and we can also drop the link from the admin notice if that feels better/safer.
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.
That would still allow bad URLs like this wouldn't it?
<a href="javascript:alert('XSS')">Click here</a>
Maybe we can go a step further and manually escape all hrefs for example?
$allow_links = [
'a' => [
'href' => true,
],
];
// Escape href values
$sanitized_message = preg_replace_callback(
'/href="([^"]*)"/i',
function ( $matches ) {
return 'href="' . esc_url( $matches[1] ) . '"';
},
$notice_message
);
wp_kses( $sanitized_message, $allow_links );
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.
That would still allow bad URLs like this wouldn't it?
<a href="javascript:alert('XSS')">Click here</a>
As with so many things in WordPress, this isn't fixed in stone, but assuming defaults that wouldn't be possible (because javascript
is not one of the allowed protocols). We'd get back some HTML looking like this:
<a href="alert('XSS')">Click here</a>
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.
TIL! Thanks @barryhughes for sharing! I guess if some malicious actor has access to filter 'kses_allowed_protocols' then we're in deep trouble, unless another plugin adds it unknowing that is poses security risks.
In any case, given the above, I think I'm fine to stick with either wp_kses( $sanitized_message, $allow_links );
or leave it with wp_kses_post()
since my concerns were mainly injecting JS/some other script.
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.
Massively overthinking this no-doubt, but wcs_add_admin_notice()
could be updated to support an array of links. Using modern syntax only to keep the example concise:
wcs_add_admin_notice(
'A new order was created.',
links: [
__( 'Review new order', 'txtdomain' ) => get_admin_url( 'foo bar baz' ),
__( 'Cancel new order', 'txtdomain' ) => get_admin_url( 'bar baz foo' ),
__( 'Give us 5 stars', 'txtdomain' ) => get_admin_url( 'baz foo bar' ),
]
);
The output might then look a little like this:
Escaping being done as late as possible, from within wcs_display_admin_notices()
.
To be clear, though, I'm just jamming on possibilities rather than pushing this as my preferred choice. I think it's fairly reasonable, and especially secure, but I also think using Kses is fine, or we could do as you suggest with a regex, or we could just drop the links from these messages (if anything, that's my least prefered option, because I think we lose something if we do, but it's definitely not a deal breaker).
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.
Thanks a tonne @barryhughes for engaging in all of the discussions on this PR!
Following-up on the kses discussion and including links, I'm happy to leave it for now and we can look at any admin notice UI improvements at a later time 😅
- Catch exceptions thrown by
$subscription->update_status()
and display admin notice.
- WP Errors returned by
wcs_create_renewal_order()
are logged in order notes and displayed in an admin notice.
- New notice after an admin successfully creates a pending order.
- New admin notice when a pending renewal order was created but failed to set the payment method.
- Improve handling when a 3rd party incorrectly returns
false
on the'wcs_renewal_order_created'
hook.
Thanks again! Kept with |
$filtered_renewal_order = apply_filters( 'wcs_renewal_order_created', $renewal_order, $subscription ); | ||
|
||
// It is possible that a filter function will replace the renewal order with something else entirely. | ||
return $filtered_renewal_order instanceof WC_Order ? $filtered_renewal_order : $renewal_order; |
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.
Just wanted to chime in to say I ❤️ this change. I was looking at #757 and I think that error wouldn't have occurred with this change.
I wonder if it would be worth to report these kind of failures somewhere. I'm thinking that if there are 2 or 3+ hooks which use this filter correctly, the changes they make would be overridden by 1 poorly written piece of code that returns a bool or null etc.
We don't have a good mechanism for doing this kind of thing but I'm thinking something like a WC log entry or something.
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'd definitely be up for developing this concept a little further (and agree there is lots of value in logging when things go wrong).
For most active subscriptions, it's possible for the merchant to create a pending renewal order. This is generally done via the Order Actions dropdown:
However, as described in the linked issue, it is possible for an uncaught exception to be raised (this might happen if it is not possible to change the subscription status), which breaks the process and leads the user to a blank screen or error message:
Though the original report cites a problem when the subscription status cannot be changed, on further examination it looked to me like there are 3 points of failure within
WCS_Admin_Meta_Boxes::create_pending_renewal_action_request()
, so I've tried to address all three. However, if we prefer to stay tightly focused only on the potential for an uncaught exception (when the subscription status is changed), I'm more than happy to revise and simplify the PR.The three areas I felt could benefit from additional safety:
WP_Error
will have been raised (if so, we'd hit problems when we subsequently try to call order methods).Therefore, this change mostly involves adding try/catch blocks or conditionals so that we handle things gracefully should the unexpected happen.
Lastly, the linked issue proposes we remove the create pending renewal order item from the Order Actions dropdown if it looks like creating it will lead to an error. As described above, there are lots of conditions in which this might happen, not just one, so I'm not too sure about the merits of this (and, it seems a fix has already been made to the extension with which this was being experienced). However, more than happy to follow-up and put this in place.
Updates https://github.com/woocommerce/woocommerce-subscriptions/issues/4735
How to test this PR
We can contrive error conditions for each of the three points of failure by applying the following snippets, one at a time (you could, for instance, place these lines in a mu-plugin file as needed):
Let's do some testing! Create or find a suitable existing subscription (one that is active, has a valid payment method already, etc):
Questions/notes
::create_pending_renewal_action_request()
in the same PR? Yes, we agreed to tackle them all here.Product impact