[wp-meta] [Making WordPress.org] #1074: Gracefully handle unrefundable ticket refund attempts

[Making WordPress.org]

#1074: Gracefully handle unrefundable ticket refund attempts
----------------------+--------------------------------------
  Reporter:  iandunn  |      Owner:  iandunn
      Type:  defect   |     Status:  accepted
  Priority:  normal   |  Component:  wordcamp.org
Resolution:           |   Keywords:  good-first-bug has-patch
----------------------+--------------------------------------
Changes (by iandunn):

 * owner:   => iandunn
 * status:  new => accepted


Comment:

 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 `P`s 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.

--
Ticket URL: <https://meta.trac.wordpress.org/ticket/1074#comment:4>
Making WordPress.org <https://meta.trac.wordpress.org/>
Making WordPress.org