Display Installment terms #196

jacstn · 2019-05-15T06:49:13Z

1. Objective

This PR added functionality to display installment terms.

2. Description of change

Hardcoded interest rates and monthly minimum payments have been added on a per-provider basis. The installment options presented to the customer at the checkout page have been modified to include monthly payment amounts and information/disclaimers about the interest rates. Payment options resulting in monthly payments below the defined minimums are automatically removed from those available for selection

3. Quality assurance

🔧 Environments:

Platform version: Magento CE 2.2.3.
Omise plugin version: Omise-Magento 2.7.
PHP version: 7.0.16.

✏️ Details:

To test this PR make payment with installment, observe if there are amounts properly displayed.
Check if minimum amounts are respected.

ie. If you choose order value for 3k THB than 12 months option should not be displayed, due to the minimum installment amount is not met in any provider.
More info about minimum installment amounts here

Check both options for absorbing rates by Merchant and Customer.

Check if TH translated texts regarding this functionality are displayed.

4. Impact of the change

N/A

5. Priority of change

High

6. Additional Notes

N/A

…nly correct terms

…ons, adjusting rates

view/frontend/web/js/view/payment/method-renderer/omise-offsite-installment-method.js

Model/Api/Capabilities.php

Model/Capabilities.php

i18n/th_TH.csv

jonrandy · 2019-05-16T10:22:19Z

view/frontend/web/js/view/payment/method-renderer/omise-offsite-installment-method.js

+                    case 'bay':          return 300;
+                    case 'first_choice': return 300;
+                    case 'ktc':          return 300;
+                    default:             return NaN;


Not sure why defaulting to NaN? Won't this break stuff if a new instalment provider appears? Or are they hardcoded?

no new installment provider until we hardcode one.

And if they are hardcoded, this default is redundant?

default is good practice.. that is how they taught me during studies..
it is that you have covered everything.

but might delete that.. if omise has different rules.. @guzzilar

getInstallmentMinimum(id) { return {kbank:500, bbl:500}[id] || 300; }

Would be another way to go

mater of taste.. IMHO code is more readable if I show all installment terms of all brands.
they also might be changed and must come from capabilities API.

jonrandy · 2019-05-16T10:22:46Z

view/frontend/web/js/view/payment/method-renderer/omise-offsite-installment-method.js

+                    case 'bay':          return 0.008;
+                    case 'first_choice': return 0.013;
+                    case 'ktc':          return 0.008;
+                    default:             return NaN;


Same as above

it is fixed, this default is to check when testing if everything works as expected, and data is displayed, otherwise would be NaN.
But actually that default will never affect anything as we hardcode stuff in .html
so if new provider will show up it will return NaN, but it will not be displayed anywhere in UI

So the default can be removed

Missed this?

you are looking at outdated code @jonrandy

view/frontend/web/js/view/payment/method-renderer/omise-offsite-installment-method.js

jonrandy · 2019-05-28T09:36:05Z

In the screenshot, the monthly amounts appear to be rounded to the nearest baht? This isn't right, surely?

view/frontend/web/js/view/payment/method-renderer/omise-offsite-installment-method.js

jonrandy

Quite a few magic numbers around that really belong in config files

jonrandy

See notes

jonrandy · 2019-05-28T11:25:37Z

Doesn't alter the fact it's nothing to do with the capabilities of the merchant account

…

On Tue, 28 May 2019, 18:20 jacstn, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In view/frontend/web/js/view/payment/method-renderer/omise-offsite-installment-method.js <#196 (comment)>: > @@ -137,7 +262,7 @@ define( * @return {boolean} */ orderValueTooLow: function () { - return window.checkoutConfig.totalsData.base_grand_total < 3000; depends how you look at it. vat in thailand is for particular merchant his capability, he has different VAT then in Japan. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#196?email_source=notifications&email_token=AALQWMV4BQURTKJIU2ETBYTPXUIP3A5CNFSM4HNAF5PKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOBZ25EEA#discussion_r288052038>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AALQWMT6SVXDL3SL6RAYZK3PXUIP3ANCNFSM4HNAF5PA> .

jonrandy · 2019-05-28T14:20:35Z

It should ideally be on the Omise servers for sure, but not in capabilities API (I'm talking about VAT). It certainly doesn't belong there. Until it's decided where it will come from, the best place for now is somewhere inside the plugin, with some function that retrieves it. Then when/if it moves into the API we just need change one function. Hardcoding it right in the JS seems very bad practice. I understand that a lot of stuff is done client side in Magento 2, but surely there's a simple way that a bunch of bars you may want access to can be passed to the page when it loads client side? As for the minimum amounts etc. we've already been told these won't be in the capability API. They aren't in the new capability API and Robin specifically told me that they won't be

…

On Tue, 28 May 2019, 21:06 jacstn, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In view/frontend/web/js/view/payment/method-renderer/omise-offsite-installment-method.js <#196 (comment)>: > @@ -137,7 +262,7 @@ define( * @return {boolean} */ orderValueTooLow: function () { - return window.checkoutConfig.totalsData.base_grand_total < 3000; the same as above.. since even we were developing this functionality minimum has been changed from 5k to 3k.. there is chance it will be changed again. It needs to be in capabilities. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#196?email_source=notifications&email_token=AALQWMU7VBLBMTOO5E5F7PDPXU35PA5CNFSM4HNAF5PKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOBZ3TKAA#discussion_r288123065>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AALQWMQNTAODDYV6EZ25WBLPXU35PANCNFSM4HNAF5PA> .

jonrandy · 2019-05-28T14:21:40Z

*vars not bars

…

On Tue, 28 May 2019, 21:20 Jon Randy, ***@***.***> wrote: It should ideally be on the Omise servers for sure, but not in capabilities API (I'm talking about VAT). It certainly doesn't belong there. Until it's decided where it will come from, the best place for now is somewhere inside the plugin, with some function that retrieves it. Then when/if it moves into the API we just need change one function. Hardcoding it right in the JS seems very bad practice. I understand that a lot of stuff is done client side in Magento 2, but surely there's a simple way that a bunch of bars you may want access to can be passed to the page when it loads client side? As for the minimum amounts etc. we've already been told these won't be in the capability API. They aren't in the new capability API and Robin specifically told me that they won't be On Tue, 28 May 2019, 21:06 jacstn, ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In > view/frontend/web/js/view/payment/method-renderer/omise-offsite-installment-method.js > <#196 (comment)>: > > > @@ -137,7 +262,7 @@ define( > * @return {boolean} > */ > orderValueTooLow: function () { > - return window.checkoutConfig.totalsData.base_grand_total < 3000; > > the same as above.. since even we were developing this functionality > minimum has been changed from 5k to 3k.. there is chance it will be changed > again. > It needs to be in capabilities. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#196?email_source=notifications&email_token=AALQWMU7VBLBMTOO5E5F7PDPXU35PA5CNFSM4HNAF5PKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOBZ3TKAA#discussion_r288123065>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AALQWMQNTAODDYV6EZ25WBLPXU35PANCNFSM4HNAF5PA> > . >

jacstn · 2019-05-28T14:29:17Z

so you say that hardcoding in php is better practice than hardcoding in js. why?

jonrandy

There is one currency formatting issue remaining

i18n/th_TH.csv

jonrandy

I think it's good to go now

guzzilar

💯 go go go

Better format of installment minimum amount message (Merge after #196)

jacstn added 10 commits May 9, 2019 15:17

Capabilites api - add zero interests checking function

3d72574

retreive zero interests info in javascript

8f72a96

display monthly installments

924b06d

installments / get zero interest info in JS

47a2045

installment - extract installment number from string

921eb7b

merge branch with fix for unabling to select installment terms

3f5496e

merge master, resolve conflicts

efeea92

installments terms - full text with estimated monthly installments

1a35a3c

installment texts translation functionality

bfbe688

added th translation to displaying installment rates ui

d2bff5b

jacstn added the project: 2.x label May 15, 2019

jacstn added 6 commits May 15, 2019 14:39

check if minimum installment amount condition is met, display those o…

cae266e

…nly correct terms

adjust amount depends on if user pays interests or merchant

5218add

installment - removed unused function

14dcd66

fix interests rates calculation, small refactoring, renamed JS functi…

4e20e5a

…ons, adjusting rates

refactoring - added VAT tax to function return instead of fixed const

778960a

correct false function description text

7e3dedd

jacstn force-pushed the 2-installment-amounts branch from c1cea3c to 7e3dedd Compare May 16, 2019 05:59

jacstn requested review from jonrandy and guzzilar and removed request for jonrandy May 16, 2019 06:02

jacstn added the status: Review needed label May 16, 2019

guzzilar reviewed May 16, 2019

View reviewed changes

view/frontend/web/js/view/payment/method-renderer/omise-offsite-installment-method.js Show resolved Hide resolved

jonrandy reviewed May 16, 2019

View reviewed changes

Model/Api/Capabilities.php Outdated Show resolved Hide resolved

jonrandy reviewed May 16, 2019

View reviewed changes

Model/Capabilities.php Outdated Show resolved Hide resolved

jonrandy reviewed May 16, 2019

View reviewed changes

i18n/th_TH.csv Outdated Show resolved Hide resolved

jonrandy reviewed May 16, 2019

View reviewed changes

view/frontend/web/js/view/payment/method-renderer/omise-offsite-installment-method.js Outdated Show resolved Hide resolved

jonrandy self-requested a review May 28, 2019 09:34

jonrandy reviewed May 28, 2019

View reviewed changes

view/frontend/web/js/view/payment/method-renderer/omise-offsite-installment-method.js Show resolved Hide resolved

jonrandy reviewed May 28, 2019

View reviewed changes

view/frontend/web/js/view/payment/method-renderer/omise-offsite-installment-method.js Outdated Show resolved Hide resolved

jonrandy reviewed May 28, 2019

View reviewed changes

view/frontend/web/js/view/payment/method-renderer/omise-offsite-installment-method.js Show resolved Hide resolved

jonrandy self-requested a review May 28, 2019 10:08

jonrandy reviewed May 28, 2019

View reviewed changes

jonrandy suggested changes May 28, 2019

View reviewed changes

jacstn requested a review from jonrandy May 28, 2019 11:10

round numbers of installments to 2 digits after coma

30fedac

jacstn requested a review from guzzilar May 29, 2019 06:53

removed VAT tax from calculations

664c578

jonrandy suggested changes Jun 5, 2019

View reviewed changes

i18n/th_TH.csv Show resolved Hide resolved

removed unnecessary comments

16e87ec

jonrandy self-requested a review June 7, 2019 04:40

jonrandy approved these changes Jun 7, 2019

View reviewed changes

guzzilar approved these changes Jun 7, 2019

View reviewed changes

jacstn merged commit fe3496d into master Jun 7, 2019

jacstn added a commit that referenced this pull request Jun 7, 2019

Merge pull request #202 from omise/2-format-currency

b65101a

Better format of installment minimum amount message (Merge after #196)

jacstn deleted the 2-installment-amounts branch June 7, 2019 12:13

jacstn restored the 2-installment-amounts branch June 7, 2019 13:19

jacstn deleted the 2-installment-amounts branch June 7, 2019 13:25

jacstn added status: Ready and removed status: Review needed labels Jun 13, 2019

jacstn added status: Released and removed status: Ready labels Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display Installment terms #196

Display Installment terms #196

jacstn commented May 15, 2019 •

edited

Loading

jonrandy May 16, 2019

jacstn May 16, 2019

jonrandy May 16, 2019

jacstn May 16, 2019

jacstn May 16, 2019

jonrandy May 16, 2019

jacstn Jun 6, 2019

jonrandy May 16, 2019

jacstn May 16, 2019

jonrandy May 16, 2019

jonrandy Jun 5, 2019

jacstn Jun 5, 2019

jonrandy commented May 28, 2019

jonrandy left a comment

jonrandy left a comment

jonrandy commented May 28, 2019 via email

jonrandy commented May 28, 2019 via email

jonrandy commented May 28, 2019 via email

jacstn commented May 28, 2019

jonrandy left a comment

jonrandy left a comment

guzzilar left a comment •

edited

Loading

Display Installment terms #196

Display Installment terms #196

Conversation

jacstn commented May 15, 2019 • edited Loading

1. Objective

2. Description of change

3. Quality assurance

4. Impact of the change

5. Priority of change

6. Additional Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonrandy commented May 28, 2019

jonrandy left a comment

Choose a reason for hiding this comment

jonrandy left a comment

Choose a reason for hiding this comment

jonrandy commented May 28, 2019 via email

jonrandy commented May 28, 2019 via email

jonrandy commented May 28, 2019 via email

jacstn commented May 28, 2019

jonrandy left a comment

Choose a reason for hiding this comment

jonrandy left a comment

Choose a reason for hiding this comment

guzzilar left a comment • edited Loading

Choose a reason for hiding this comment

jacstn commented May 15, 2019 •

edited

Loading

guzzilar left a comment •

edited

Loading