Comment by slackbot:

This ticket was mentioned in Slack in #meta by netweb. View the logs.

Comment by @actual-saurabh:

• #1074.patch added

Comment by @actual-saurabh:

Added a patch. A summary of changes:

1. added new option to Paypal gateway: refund-expiry set to 60
2. added a new error after comparing the payment timestamp (from paypal stored in tix_transaction_details): Paypal only allows refunds within %d days of purchase.

However, I need advice

• I have returned false immediately after adding this error in payment_refund function.
• Because of which in the form_refund_request function:

// 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 @actual-saurabh:

• Keywords set to Good First Bug Has Patch

Comment by @iandunn:

• Status set to accepted
• Owner set to @iandunn

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:

• I'd probably do something more like ( $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.
• Since the expiration time is only used inside the 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.
• The error message string needs CampTix's text domain.
• Both Ps in PayPal should be capitalized.
• It'd also be good to 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:

• #1074.2.patch added

Comment by @actual-saurabh:

@iandunn I've made the changes that you suggested; agree with every single one.

Comment by @iandunn:

• Keywords set to 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:

1. They see a link saying Change of plans? Made a mistake? Don't worry, you can request a refund.
2. They click on that link and are taken to a form they have to fill out.
3. After filling out the form, they're informed that they can't get a refund after all.

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 @iandunn:

• Status set to assigned
• Owner cleared

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.