Open ghost opened 9 years ago
Comment by slackbot:
This ticket was mentioned in Slack in #meta by netweb. View the logs.
Comment by @actual-saurabh:
Comment by @actual-saurabh:
Added a patch. A summary of changes:
However, I need advice
// Attempt to process the refund transaction
$result = $payment_method_obj->payment_refund( $transaction['payment_token'] );
$this->log( 'Individual refund request result.', $attendee->ID, $result, 'refund' );
if ( CampTix_Plugin::PAYMENT_STATUS_REFUNDED == $result ) {
foreach ( $attendees as $attendee ) {
update_post_meta( $attendee->ID, 'tix_refund_reason', $reason );
$this->log( 'Refund reason attached with data.', $attendee->ID, $reason, 'refund' );
}
$this->info( __( 'Your tickets have been successfully refunded.', 'camptix' ) );
return $this->form_refund_success();
} else {
$this->error( __( 'Can not refund the transaction at this time. Please try again later.', 'camptix' ) );
}
also adds another error because $result happens to be false.
So, it seems rather unusable because the second error is confusing: https://www.dropbox.com/s/tu3oipp14hyrsn8/Screen%20Shot%202015-10-16%20at%208.47.30%20pm.png?dl=0
Should we convert this if statement into a switch statement?
And, should I just continue return false and check for that or create a new status constant in the Camptix class and return that instead of false?
Comment by @iandunn:
accepted
This looks really good, thanks Saurabh :)
Instead of returning false
, what do you think about returning a WP_Error
with the message that the refund has expired? Then, form_refund_request()
can check if the returned value is an WP_Error
or not. If it is, then it'd use that error message, and if not, it would use the generic error.
As far as the current path goes, the only big thing I would change would be to modularize the logic that determines if a transaction is refundable, and then just call that method, instead of building it all into the refund method.
Modularity helps keep things simple, reusable, de-coupled, and unit-testable.
Other than that, I'd just recommend a few minor changes:
( $txn_details['raw']['TIMESTAMP'] + $refund_expiry * DAYS_IN_SECONDS ) < time()
, since that'd be simpler and cheaper (in terms of performance) than calling date()
and strtotime()
multiple times. strtotime()
accepts a lot of different inputs and does a lot of processing to generate its output, so it's (relatively) expensive compared to lower-level operations. That won't make a noticable impact in this situation, but it's a good habit to pay attention to those things.transaction_is_refundable()
method, it can just be a local variable there, rather than an option that the entire class has access to. That'll make it adhere to the "information hiding" principle.P
s in PayPal should be capitalized.log()
that the refund was rejected because the refund period has expired, so that someone viewing the logs would know the specific reason.Comment by @actual-saurabh:
Comment by @actual-saurabh:
@iandunn I've made the changes that you suggested; agree with every single one.
Comment by @iandunn:
Good First Bug
Needs Patch
Doh, 1074.2.patch
is great, but I just realized that if we take a different approach, we can provide a better UX.
Right now, this is what the user will experience:
Change of plans? Made a mistake? Don't worry, you can request a refund.
It'd be better if instead we told them up front that they can't get the refund. So instead of saying, Change of plans? Made a mistake? Don't worry, you can request a refund
, that message should say something like This transaction is passed PayPal's refund period and cannot be refunded
.
There is some existing logic that we can leverage. camptix.php
has an is_refundable()
function that checks if the payment method support refunds.
So, I'm thinking we can add to that to check if the transaction itself is refundable, and also show an alternate message to the user if it's not.
CampTix_Payment_Method_PayPal
should probably have a transaction_is_refundable()
method that each child class can override. In order to maintain backwards compatibility, it'll have to default to returning true
. If the transaction isn't refundable, it could return a WP_Error
with the alternative message to show to the user.
Comment by slackbot:
This ticket was mentioned in Slack in #meta-wordcamp by iandunn. View the logs.
Comment by slackbot:
This ticket was mentioned in Slack in #meta-wordcamp by iandunn. View the logs.
Imported from https://meta.trac.wordpress.org/ticket/1074 Created by @iandunn:
CampTix allows users to refund their ticket purchases, but PayPal only allows refunds within 60 days of the ticket purchase.
We should add some logic to the PayPal addon to short-circuit the refund process if the ticket was purchased more than 60 days ago, and show the user a friendly error.