Add support for currency switcher plugin by albingeorge · Pull Request #46 · razorpay/razorpay-woocommerce

albingeorge · 2017-06-23T13:38:47Z

If the currency is not INR:
  If the currency swicth plugin is installed
    If both INR and current currency is configured in the currency switch plugin:
      Convert to INR and process further
    Else
      Throw an error asking to configure the CS plugin with both INR and the current currency
Else
  Throw an error saying currency is not supported

captn3m0

Please test against both WC 2.4 and 3.x

captn3m0 · 2017-06-26T04:52:57Z

-    const BAD_REQUEST_ERROR                 = 'BAD_REQUEST_ERROR';
-    const SERVER_ERROR                      = 'SERVER_ERROR';
-    const GATEWAY_ERROR                     = 'GATEWAY_ERROR';
+    const BAD_REQUEST_ERROR                    = 'BAD_REQUEST_ERROR';


Create a new class for this. Try not to make any changes to sdk that we can't push back upstream.

captn3m0 · 2017-06-26T04:53:48Z

+
+namespace Razorpay\Api\Errors;
+
+class InvalidCurrencyError extends Error


Put this in a different directory and namespace?

captn3m0 · 2017-06-26T04:56:52Z

+         *
+         * @return void
+         */
+        protected function validateOrderCreateData($data): Array


The flow here seems to be using exceptions pretty loosely. Instead of:

Setup data

Validate (if, check, throw)

Catch-> Fix

Proceed

Why not just check the conditions upfront:

if(INR) -> else if ($installed and not INR) else if(not INR)

captn3m0

Minor changes
Please add screenshots on the PR for both the happy and sad flows.

captn3m0 · 2017-06-27T12:44:24Z

+
+class ErrorCode extends ApiErrors\ErrorCode
+{
+


nit: Extra newline

captn3m0 · 2017-06-27T12:44:46Z

@@ -0,0 +1,21 @@
+<?php
+
+namespace RazorpayWoo\Errors;


If we are creating a namespace, let us keep it as Razorpay\Woocommerce\

captn3m0 · 2017-06-27T12:46:52Z

+
+            if (array_key_exists('INR', $currencies) and array_key_exists($data['currency'], $currencies))
+            {
+                // Convert the currency to INR using the rates fetched from the Currency Switcher plugin


Are there any specific instructions someone would need to follow to add INR in this list?

Can you write a helper guide at https://github.com/razorpay/razorpay-woocommerce/wiki/Multi-Currency (screenshots will be very helpful)

Added the instructions to get this working on the above wiki page.

captn3m0 · 2017-06-27T12:47:05Z

+            {
+                // Convert the currency to INR using the rates fetched from the Currency Switcher plugin
+                $data['amount'] = round(
+                    (($data['amount'] * $currencies['INR']['rate']) / ($currencies[$data['currency']]['rate'] == 0 ? 1 : $currencies[$data['currency']]['rate'])),


too long, use if instead?

captn3m0 · 2017-06-27T13:12:03Z

+            }
+            else
+            {
+                throw new Errors\BadRequestError(


How does this show up during a payment attempt?

I've added screenshots below.

If the currency is not INR: If the currency swicth plugin is installed If both INR and current currency is configured in the currency switch plugin: Convert to INR and process further Else Throw an error asking to configure the CS plugin with both INR and the current currency Else Throw an error saying currency is not supported

It's not required. All we need is the error codes which we add via ErrorCode.php

The currency switcher plugin sets the currency rate to 0, if the selected currency is the same as default currency set from WooCommerce plugin

…rrenct currency

albingeorge · 2017-06-28T09:32:43Z

Screenshots

Currency set from WooCommerce: GBP
Currency Switcher plugin: Not installed

Currency set from WooCommerce: USD
Currency Switcher plugin: Installed, INR not added

Currency set from WooCommerce: USD
Currency Switcher plugin: Installed and configured

captn3m0 · 2017-06-28T13:16:16Z

Can we improve this wording? Saying pay button where none exists doesn't look good.

Same for the first case as well.

albingeorge · 2017-06-28T13:36:04Z

@captn3m0 Can we hide the message 'Thank you for your order, please click the button below to pay with Razorpay.' based on whether there is an error shown?

Or should we change the verbiage of the error message to something simpler like 'RAZORPAY ERROR: Payment failed'.

Previously, we used to show 'RAZORPAY ERROR: Api could not be reached' for every payment failures(even if the API was reachable).

captn3m0 · 2017-06-28T13:45:39Z

Can we hide the message

Yup, sounds like a good idea.

Previously, we used to show 'RAZORPAY ERROR: Api could not be reached' for every payment failures(even if the API was reachable).

The idea was not to disclose anything sensitive by accident. If we are certain that nothing sensitive will be revealed by the exception message (ie, we are only catching known exceptions, and any other generic exception gets a generic message), I'm happy with switching this to detailed errors.

albingeorge · 2017-06-28T14:17:13Z

Regarding the catching of exception, as of now, we're catching Exception and the message from the same is pushed to the front-end.

Instead, we can catch only the BadRequestError exception and for the rest of the exceptions, we can show some generic message like 'RAZORPAY ERROR: Payment failed'. Let me know if that works.

captn3m0 · 2017-06-28T14:25:38Z

Yup, that is what I was suggesting as well 👍

captn3m0

Minor changes.

captn3m0 · 2017-06-29T17:52:38Z

+              'description'      => $productinfo,
+              'notes'            => array(
+                                        'woocommerce_order_id' => $orderId
+                                    ),


closing bracket should align on left

captn3m0 · 2017-06-29T17:54:14Z

+                // If the currenct currency is the same as the default currency set in WooCommerce,
+                // Currency Switcher plugin sets the rate of currenct currency as 0, because of which
+                // we need to set this to 1 here if it's value is 0
+                $current_currency_rate = ($currencies[$data['currency']]['rate'] == 0 ? 1 : $currencies[$data['currency']]['rate']);


camelCase everywhere, unless wordpress hook-related things

captn3m0

LGTM. Squash merge please.

albingeorge requested a review from captn3m0 June 23, 2017 14:05

captn3m0 reviewed Jun 26, 2017

View reviewed changes

captn3m0 suggested changes Jun 27, 2017

View reviewed changes

albingeorge added 5 commits June 28, 2017 12:42

Refractor code

e52f7eb

Remove InvalidCurrency class

408dce6

It's not required. All we need is the error codes which we add via ErrorCode.php

Resolve division by zero error

ebcbb24

The currency switcher plugin sets the currency rate to 0, if the selected currency is the same as default currency set from WooCommerce plugin

refactor(Errors): Changed the namespace of ErrorCode

0159f33

albingeorge force-pushed the support-currency-switcher branch from 39f956c to 0159f33 Compare June 28, 2017 07:13

albingeorge added 2 commits June 28, 2017 13:00

refactor(currency_converter): Separately calculate the rate of the cu…

be83eac

…rrenct currency

style(ErrorCode): Removed extra line

9ffa51b

fix: Show custom message for all generic exceptions

dfc30f0

captn3m0 reviewed Jun 29, 2017

View reviewed changes

captn3m0 mentioned this pull request Jun 29, 2017

Subscriptions #40

Merged

4 tasks

style: Correct camelcase + alignment

0e3f766

captn3m0 approved these changes Jun 30, 2017

View reviewed changes

albingeorge merged commit 9a401c6 into master Jun 30, 2017

albingeorge deleted the support-currency-switcher branch June 30, 2017 10:43


		namespace Razorpay\Api\Errors;

		class InvalidCurrencyError extends Error

Conversation

albingeorge commented Jun 23, 2017 • edited by harman28 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

captn3m0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

captn3m0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albingeorge commented Jun 28, 2017

Uh oh!

captn3m0 commented Jun 28, 2017

Uh oh!

albingeorge commented Jun 28, 2017

Uh oh!

captn3m0 commented Jun 28, 2017

Uh oh!

albingeorge commented Jun 28, 2017

Uh oh!

captn3m0 commented Jun 28, 2017

Uh oh!

captn3m0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

captn3m0 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

albingeorge commented Jun 23, 2017 •

edited by harman28

Loading