Create an Omise charge

[guzzilar]

Note1, original PR was made by @nimid at PR #14, #17, #16. This PR just did (and aimed to) rearrange those 3 PRs to make it easy to review by merging all those 3 PRs to only one, which is, this branch.

Note2, this branch contains the same code at PR #16 (except, this branch already rebased with develop.

1. Objective

Create an Omise charge by using the Omise card token that submitted from the client side.

Related information: Related issue: - Related ticket: - Required pull request: #13

2. Description of change

• Add a helper class and a function, OmisePluginHelperCharge.amount() to format the charge amount for the Thai Baht currency.
• Complete the checkout form function to submit the Omise card token from client side to server side.
• Implement a class to process charge at server side.
• Add a dummy page for display the checkout result.

3. Quality assurance

Environments:

• Platform: PrestaShop 1.6.1.6
• Omise plugin: Omise-PrestaShop 1.6.0.0
• PHP: 5.6.28
• Mozilla Firefox: 50.1.0

Details:

• Make sure that the Omise-PrestaShop has been installed and enabled.
• At the front-end, add a product to cart.
• Proceed to checkout.
• At the step, 05. Payment, fill the valid card information and click button, Submit Payment. The system will process the payment and display the result page.

The screenshot below shows the result page display the successful payment.

The screenshot below shows charge detail at Omise dashboard. This charge was created from the above checkout.

4. Impact of the change

-

5. Priority of change

Normal

6. Additional notes

For the next steps of charge process, such as record the order information to PrestaShop or display the proper result page, it will be developed for next pull request.

[guzzilar]

@nimid I checked at PR #21. I think that design is a bit complex,

• OmiseBasePaymentModuleFrontController shouldn't contain logic in postProcess If this base class contains logic to create charge itself, then it shouldn't named as base cuz it can't reuse for other purpose except for create charge

• To make OmiseBasePaymentModuleFrontController more flexible. I think OmiseBasePaymentModuleFrontController shouldn't handle postProcess itself, instead, controllers/front/payment.php or controllers/front/threedomainsecurepayment.php should decide by themselves which action they're going to perform.

• Continue from above, then we can move try { } catch() { } into classes/charge.php instead. Since classes/charge.php performs create by itself, I think it's better to let that class handle catch exception.

• Slightly, about OmiseThreeDomainSecure name. Do you have any reason behind that you chose OmiseThreeDomainSecure instead of OmiseThreeDSecure? Asking because usually in Omise plugin, we keep exposing term of ThreeDSecure instead of ThreeDomainSecure. If there is no special reason behind, I prefer to use the same name as other plugins instead.

[nimid]

@guzzilar

• Can you provide other use cases at the front-end side (by the payer) except create charge?

How about an alternative design below?

• A function Charge.createInternetBanking() is for example only.

class OmisePaymentModuleFrontController extends OmiseBasePaymentModuleFrontController
{
    public function postProcess()
    {
        try {
            $this->charge = $this->omise_charge->create(Tools::getValue('omise_card_token'));
        } catch (Exception $e) {
            $this->error_message = $e->getMessage();
            return;
        }

        if ($this->charge->isFailed()) {
            $this->error_message = $this->charge->getErrorMessage();
            return;
        }

        $this->payment_order->save();

        $this->setRedirectAfter('index.php?controller=order-confirmation' .
            '&id_cart=' . $this->context->cart->id .
            '&id_module=' . $this->module->id .
            '&id_order=' . $this->module->currentOrder .
            '&key=' . $this->context->customer->secure_key
        );
    }
}

class OmiseThreeDomainSecurePaymentModuleFrontController extends OmiseBasePaymentModuleFrontController
{
    public function postProcess()
    {
        $this->payment_order->saveAsProcessing();

        try {
            $this->charge = $this->omise_charge->createThreeDomainSecure(Tools::getValue('omise_card_token'));
        } catch (Exception $e) {
            $this->error_message = $e->getMessage();
            return;
        }

        if ($this->charge->isFailed()) {
            $this->error_message = $this->charge->getErrorMessage();
            return;
        }

        $this->setRedirectAfter($this->charge->getAuthorizeUri());
    }
}

abstract class OmiseBasePaymentModuleFrontController extends ModuleFrontController
{
    protected $charge;
    public $display_column_left = false;
    protected $error_message;
    protected $omise_charge;
    protected $payment_order;
    protected $setting;

    public function __construct()
    {
        parent::__construct();

        $this->omise_charge = new Charge();
        $this->payment_order = new PaymentOrder();
        $this->setting = new Setting();
    }

    public function initContent()
    {
        parent::initContent();

        $this->context->smarty->assign('error_message', $this->error_message);
        $this->setTemplate('payment-error.tpl');
    }

    public function redirect()
    {
        Tools::redirect($this->redirect_after);
    }
}

class Charge
{
    ...

    public function create($card_token)
    {
        $charge_request = array(
            'amount' => $this->getAmount(),
            'card' => $card_token,
            'capture' => $this->getCapture(),
            'currency' => $this->getCurrencyCode(),
            'description' => $this->getChargeDescription(),
        );

        $this->charge_response = OmiseCharge::create($charge_request, '', $this->getSecretKey());

        return $this;
    }

    public function createInternetBanking()
    {
        $charge_request = array(
            'amount' => $this->getAmount(),
            'capture' => $this->getCapture(),
            'currency' => $this->getCurrencyCode(),
            'description' => $this->getChargeDescription(),
            'offsite' => 'internet_banking_scb',
            'return_uri' => $this->getReturnUri(),
        );

        $this->charge_response = OmiseCharge::create($charge_request, '', $this->getSecretKey());

        return $this;
    }

    public function createThreeDomainSecure($card_token)
    {
        $charge_request = array(
            'amount' => $this->getAmount(),
            'card' => $card_token,
            'capture' => $this->getCapture(),
            'currency' => $this->getCurrencyCode(),
            'description' => $this->getChargeDescription(),
            'return_uri' => $this->getReturnUri(),
        );

        $this->charge_response = OmiseCharge::create($charge_request, '', $this->getSecretKey());

        return $this;
    }

...

}

Do you have any reason behind that you chose OmiseThreeDomainSecure instead of OmiseThreeDSecure?

I found the full word for explanation can have more clearly understandable. If a word Domain is long name, the abbreviation is preferred but for more 5 characters (omain), it can help in part of understandable.

In the camel case coding convention, when D beside Secure, it may make the confusion.

[guzzilar]

@nimid Note, I reviewed without checking another PR, so, some of my comments may already been solved in somewhere else in some PRs. (And also, we may have already talked about it in some part of my comments. Feel free to point it out if comments below were mentioned/duplicated in some place even in this PR)

Here we go.

• Docblock(s) are missing. https://github.com/omise/omise-prestashop/pull/19/files#r102140277

• If classes/charge.php is a controller, I think it better to keep it in controllers folder.

• classes/charge.php shall we rename it or make it more unique name? (i.e. OmiseCharge.php) to avoid duplicate class name (that some 3rd-party modules may have the same name, since it's pretty common name). https://github.com/omise/omise-prestashop/pull/19/files#r97012652

• Personally, I try avoid using getter, setter functions without specific case and just use it when I really need to add some complex logic in it. i.e. classes/charge.php, getAmount() looks fine for me but classes/charge.php, getCapture() looks unnecessary and it makes function name looks weird. (think about interface. Charge->getCapture(). My first impression when I saw it. It's totally nothing deal with Payment Action: Authorize only or Authorize and Capture. Because getCapture is meaningless.

• In classes/charge.php if you separate between create() and buildRequest(). I think it's gonna be easier for test. i.e.

  class Charge extends ModuleFrontController
  {
      /**
       * @return OmiseCharge
       */
      public function create()
      {
          return OmiseCharge::create($this->buildRequest(), '', $this->getSecretKey());
      }
  
      /**
       * @return array
       */
      protected function buildRequest()
      {
          return array(
              'amount'      => $this->getAmount(),
              'card'        => $this->getCardToken(),
              'capture'     => $this->getCapture(),
              'currency'    => $this->getCurrencyCode(),
              'description' => $this->getChargeDescription(),
          );
      }
  }

• Vertical alignment would make your code much easier to read.

[nimid]

@guzzilar https://github.com/omise/omise-prestashop/pull/19#issuecomment-281457688

Docblock(s) are missing.

Which DocBlocks tag is required?

If classes/charge.php is a controller, I think it better to keep it in controllers folder.

It has been changed.

classes/charge.php shall we rename it or make it more unique name?

OmiseCharge has already defined.

How about OmiseChargeClass?

but classes/charge.php, getCapture() looks unnecessary and it makes function name looks weird.

Done. 672b59305ef127367c4c5fe7c4673e3ba8a7c56d

I think it's gonna be easier for test.

Can you show the test, compare and clarify how it easier?

Vertical alignment would make your code much easier to read.

It is not my code. It is not your code. It is not our code.

It belongs to Omise.

It is not just change the ownership words. It is the attitude in working. It is a way how to behave with it. It is the attitude when communicate with co-worker and people.

I found you have shared a code review guideline. In the guideline, it clarified Avoid selective ownership of code. ("mine", "not mine", "yours").

Is vertical alignment personal preference?

If it is not, can you show the publicly accepted standard for reference that this project should be consistent?

[nimid]

The source code in this pull request has been changed and combined to merge together with the pull request #21.