Add capability api #99

jonrandy · 2018-12-03T06:22:32Z

Adds initial functionality for using the Capability API. Quite basic right now, probably needs to be expanded upon

Example usage:

<?php

require_once 'lib/Omise.php';

define('OMISE_PUBLIC_KEY', 'pkey_xxxxxxxxxxxxx');
define('OMISE_SECRET_KEY', 'skey_xxxxxxxxxxxxx');



$capabilities = OmiseCapabilities::retrieve();

print_r($capabilities);


$instalmentBackends = $capabilities->getBackends(
	$capabilities->makeBackendFilterType('installment'),
	$capabilities->makeBackendFilterCurrency('THB'),
	$capabilities->makeBackendFilterChargeAmount(749999)
);


foreach ($instalmentBackends as $backend) {
	print_r($backend);
}

jonrandy · 2018-12-03T06:24:48Z

Just occurred to me that although this is mergeable, it may not work, as master moved on quite a lot with your changes @guzzilar ? Can you confirm? If so, I'll bring the changes from master in and get it working again

guzzilar · 2018-12-04T01:47:41Z

@jonrandy you can try merge or rebase the code and test it on your local machine as well.
Also, can you provide some example code in the pull request's description? Like, how to execute, test, and also for the output result.

guzzilar · 2018-12-04T02:44:32Z

@jonrandy ah sorry, I think I misunderstood. Anyway, checked the code. So far it should be ok.
The changes in v2.10.0 won't effect to any current implementation. All code should still look & execute in the same way.

jacstn · 2018-12-04T03:19:52Z

lib/omise/OmiseCapabilities.php

+     *    
+     * @return array
+     */
+    public function getBackends() {


Still not convinced about naming convetion... get Backends..

Don't blame me - it's what they're called in the Capability API response (and probably elsewhere) - I'm just following the naming convention

lib/omise/OmiseCapabilities.php

guzzilar · 2018-12-04T03:31:44Z

lib/omise/OmiseCapabilities.php

+            $res,
+            function($v, $k) use (&$res) {
+                $id = array_keys($v)[0];
+                $res[$k][$id]['_id'] = $id;


@jonrandy Is there any reason to have _id here?

To differentiate it from the 'real' bits of the API - I added it for convenience

@jonrandy ic, not sure but does it necessary or is going to be used at any place in the future-pr(s)?

It's already in use - proved extremely useful in Magento extension

guzzilar · 2018-12-04T03:47:02Z

lib/omise/OmiseCapabilities.php

@@ -0,0 +1,139 @@
+<?php
+
+class OmiseCapabilities extends OmiseApiResource


@jonrandy For the naming, OmiseCapability would be more consistency with the rest classes' name.

I thought about this when I made it. The object returned has details of all the capabilities of the system, not a single capability. I think this (the pluralised name) makes more sense in the current naming scheme. Calling it by the singular would be confusing and inconsistent IMO

there are many different capabilities listed..

@jonrandy @jacstn Isn't it based on the API response body?
In this case, API returns

{ "object": "capability", "location": "/capability", "banks": [], "payment_backends": [] }

The Capability is a single object, but many different items that are listed are payment_backends and others.

I discussed with Shunichi back in mid-December, he agrees with the naming and is going to rename the object in the API, as well as the endpoint

guzzilar · 2018-12-04T03:52:13Z

lib/omise/OmiseCapabilities.php

+     *
+     * @return function
+     */
+    public function backendSupportsChargeAmount($amount) {


@jonrandy I have a feeling that this will cause a confusion in case if someone using this filter with card backend-type (or other types).

$capability->getBackends( $capability->backendTypeIs('card'), $capability->backendSupportsChargeAmount(3000) );

What do you think about this case?

Again, sorry seems I misunderstood.. let me test a bit more..

Every backend has a min/max charge amount. So the filter makes sense for all

also 3000 to const ?

guzzilar · 2018-12-04T04:01:28Z

lib/omise/OmiseCapabilities.php

+     *
+     * @return function
+     */
+    public function backendSupportsChargeAmount($amount) {


Also while backendTypeIs, backendSupportsChargeAmount, backendSupportsCurrency sound tempting but it's hard to differentiate between its filter methods and normal method.
How about we prefix it with filter?

$capability->getBackends( $capability->filterBackendType('card'), $capability->filterBackendSupportsChargeAmount(3000) );

--
Actually not sure which one would be ideal, it could look like 👇 as well.

$capability->getBackends([ 'filters' => [ 'type' => 'card', 'supportsChargeAmount' => 3000 ] ]);

🤔 hmm, what do you think?

Doing it with the functions easily allows people to add their own filters if required without touching the omise-php code or requesting a change (they simply pass in their own filter function). I have no strong opinions on the name change for the filter building methods

makeFilter might be a better prefix since those functions don't actually filter anything, rather they build a function that will do the filtering. Or even makeBackendFilterXxxxx where Xxxxxx is the filter type

I've pushed a change with the above naming change

should we add 3000 to const @jonrandy ?

3000 does not appear in the code - the above is an example

@guzzilar Actually, what about this?

$capabilities->getBackends( $capabilities->backendFilter['type']('card'), $capabilities->backendFilter['currency']('THB') $capabilities->backendFilter['amount'](3000) );

I think it reads a bit nicer and still allows the user to pass in their own filter functions

Not sure what PHP version supports this construction though... checking

Works on 5.4 and above :)

guzzilar · 2018-12-04T06:58:22Z

@jonrandy The implementation looks neat! Just need your help to provide some test cases for this implementation. :)

Thanks!

lib/omise/OmiseCapabilities.php

guzzilar

@jonrandy @jacstn Hi, I finished review.
There are some part that need to apply code styling (those brackets { }, space between string, naming convention - no _ prefix for private method).

You can try merge or rebase master branch.
I've added PHP Code Sniffer already. After merge, can run ./vendor/bin/phpcs to see the result.

guzzilar · 2019-01-03T23:22:48Z

lib/omise/OmiseCapabilities.php

@@ -0,0 +1,139 @@
+<?php
+
+class OmiseCapabilities extends OmiseApiResource


@jonrandy @jacstn Isn't it based on the API response body?
In this case, API returns

{ "object": "capability", "location": "/capability", "banks": [], "payment_backends": [] }

The Capability is a single object, but many different items that are listed are payment_backends and others.

guzzilar · 2019-01-03T23:27:47Z

lib/omise/OmiseCapabilities.php

+{
+    const ENDPOINT = 'capability';
+
+    const FILTERS = [


@jonrandy please use array() instead. (for PHP v5.3...) (let's announce of dropping support on this later :D )

Since some of my other code already needs 5.4 (as mentioned above), which I assumed was ok was we are already recommending 5.6 on the readme and hence almost certainly dropping lower PHP versions soon

Oh, I didn't see that. Then probably we need to reconsider to go with a PHP v5.3-friendly approach instead.

We can say that we drop support on v5.3 and give users some times to migrate but not to break the current code please (by breaking, I mean, user that using Omise-PHP's current version on PHP v5.3 and want to upgrade to this version).

guzzilar · 2019-01-04T00:33:59Z

lib/omise/OmiseCapabilities.php

+     * 
+     * @return boolean
+     */
+    protected function isValidAPIResponse($array)


@jonrandy Capability API is now returned "object": "capability" already, we don't need this alternative way anymore right?

It was certainly necessary before, but maybe things have changed. I will test again

Yup, no longer needed - removing

guzzilar · 2019-01-04T00:52:52Z

tests/omise/CapabilitiesTest.php

@@ -0,0 +1,36 @@
+<?php require_once dirname(__FILE__).'/TestConfig.php';


@jacstn @jonrandy can you add more test to cover those below cases?

$capabilities->backendFilter['currency']('THB')

$capabilities->makeBackendFilterCurrency('THB')

$capabilities->getBackends()

I'm satisfied they work

Yeah, but still need test cases 😄

guzzilar · 2019-01-04T01:01:19Z

lib/omise/OmiseCapabilities.php

+     *
+     * @return function
+     */
+    private static function _fixArgs($argArray) {


@jonrandy can we have a better name for this one? fixArgs sounds a bit ambiguous..

_sanitiseArgs?

Have gone with _argsToVariadic

guzzilar · 2019-01-04T05:38:47Z

tests/omise/CapabilitiesTest.php

+    $this->assertArrayHasKey('payment_backends', $capabilities);
+    $this->assertInternalType('array', $capabilities['payment_backends']);
+  }
+}


There is also this comment but somehow it doesn't show on Slack..
Let me repost here:

@jonrandy @jacstn Hi, I finished review.
There are some part that need to apply code styling (those brackets { }, space between string, naming convention - no _ prefix for private method).

You can try merge or rebase master branch.
I've added PHP Code Sniffer already. After merge, can run ./vendor/bin/phpcs to see the result.

Strongly disagree on no underscore prefixes for private and protected methods. It makes the code way easier to read

I don't find it as an obstacle, guess it depends on individual style for this one.
So I'll just follow PSR-2 and the current code style for now.

URL currently pointing at mock API response

…om capabilities data

…re not present

…ility API

…t limits

…ay of filter functions

…to generic standard

guzzilar · 2019-01-07T08:43:48Z

lib/omise/OmiseCapabilities.php

+                $res[$k][$id]['_id'] = $id;
+            }
+        );
+        $res = array_map(function($a) { return (object)reset($a); }, $res);


@jonrandy I'm combining these array_walk and array_map to

$backends = array_map( function($backend) { $id = array_keys($backend)[0]; $backend[$id]['_id'] = $id; return (object)reset($backend); }, $backends); return !empty($filter) ? array_values(array_filter($backends, $filter)) : $backends;

Do you have any concern from the above code? (I tested, the result seems to be the same)

As long as it gives the same result. I don't see that the array_values call is needed though - who cares about the indexes? Wasteful additional processing

guzzilar · 2019-01-07T09:16:04Z

lib/omise/OmiseCapabilities.php

+            function ($backend) {
+                $id = array_keys($backend)[0];
+                $backend[$id]['_id'] = $id;
+                return (object)reset($backend);


@jonrandy is there any reason you want to cast it to Object?

Just I found it's a bit off that the following code won't give the same result (it suppose to).

$capabbilities = OmiseCapabilities::retrieve(); print_r($capabbilities['payment_backends']);

and

$capabbilities = OmiseCapabilities::retrieve(); print_r($capabbilities->getBackends());

Why is it off? the getBackends returns backend objects created from the arrays in the JSON. Objects are nice to deal with in the code

Just slight of feeling that it should be the same value & structure (especially that _id attribute can be found only when call getBackends())

For casting to Object < ok

jonrandy · 2019-01-07T09:31:03Z

As you are editing now @guzzilar , I guess I need to be added as a reviewer. Struggling to work out how to do that though

guzzilar · 2019-01-07T09:32:43Z

@jonrandy Don't think it's possible for adding yourself as a reviewer in your own PR..

jonrandy · 2019-01-07T09:35:29Z

Hmmmm.... do we need a new PR then? Or how is the review process going to work? Seems like we possibly should have merged this before you started adding to it?

guzzilar · 2019-01-07T09:38:15Z

@jonrandy if you don't mind, can comment in the code (just you technically cannot approved or rejected. I can click approved on behalf of you lol).

Or I can reopen PR if you prefer.
Let me know :)

jonrandy · 2019-01-07T09:39:35Z

I guess a new PR is better

guzzilar · 2019-01-07T09:43:31Z

@jonrandy Ok, I'm closing this PR then

jacstn · 2019-01-07T09:45:51Z

we can verbally agree on changes.. you can alway do comments.. not sure if new pr is necessary than... :) @guzzilar @jonrandy

jonrandy assigned guzzilar and jacstn Dec 3, 2018

jonrandy requested review from guzzilar and jacstn December 3, 2018 06:22

jacstn reviewed Dec 4, 2018

View reviewed changes

guzzilar suggested changes Dec 4, 2018

View reviewed changes

lib/omise/OmiseCapabilities.php Outdated Show resolved Hide resolved

guzzilar reviewed Dec 4, 2018

View reviewed changes

jacstn suggested changes Dec 13, 2018

View reviewed changes

lib/omise/OmiseCapabilities.php Show resolved Hide resolved

jacstn approved these changes Dec 14, 2018

View reviewed changes

jacstn mentioned this pull request Dec 18, 2018

Instalment Payment Option Implementation omise/omise-magento#148

Merged

guzzilar suggested changes Jan 4, 2019

View reviewed changes

guzzilar reviewed Jan 4, 2019

View reviewed changes

jonrandy and others added 11 commits January 7, 2019 12:05

Adds Capability API capability

1b6049f

URL currently pointing at mock API response

Adds methods to facilitate getting information on payment backends fr…

e67e83f

…om capabilities data

Faster combined filters function

6219649

Changes 'supportsAmount' logic to handle cases where amount min/max a…

6023391

…re not present

Fixes small issue with filters. Adds correct authentication for capab…

d977f26

…ility API

Switches to live URL as capabilities API is live

30c1bef

Adds method descriptions, makes amount filter builder aware of defaul…

d478ed0

…t limits

Fixes stupid mistake in 'amount' logic

f55b5ef

Adds a hack to correct 'min' amount values in instalment methods

db0b5cc

Changes name of backend filter building functions

06c1415

added tests

b16bf07

jacstn and others added 9 commits January 7, 2019 12:05

Capabilities testing - added reload function test

60ec64d

added missing json result file from Capabilities tests

970afc8

Removes unnecessary testing file

b199a54

Introduces alternative (friendlier?) syntax to build filters

b639c48

Makes getBackends able to accept multiple parameters, or a single arr…

cac2967

…ay of filter functions

Refactors some code

d43c98d

Renames the method that 'fixes' argument array

4a3c0ba

Removes custom isValidAPIResponse as new API response now conforms …

137d0ac

…to generic standard

Removes uses of array short syntax

4775d36

guzzilar force-pushed the add-capability-api branch from b0ea3c0 to 4775d36 Compare January 7, 2019 05:05

guzzilar added 2 commits January 7, 2019 15:02

Adding OmiseCapabilities test cases.

0e358a2

OmiseCapabilities: renaming, code organizing & styling.

5db6ed7

guzzilar reviewed Jan 7, 2019

View reviewed changes

Apply code styling according to Code Sniffer.

9f4cf0e

guzzilar force-pushed the add-capability-api branch from 86ba196 to 9f4cf0e Compare January 7, 2019 09:10

guzzilar reviewed Jan 7, 2019

View reviewed changes

jonrandy self-assigned this Jan 7, 2019

guzzilar closed this Jan 7, 2019

guzzilar mentioned this pull request Jan 7, 2019

Introducing Capability API #100

Merged

guzzilar added the Status: Invalid label Jan 7, 2019

		@@ -0,0 +1,139 @@
		<?php

		class OmiseCapabilities extends OmiseApiResource

		@@ -0,0 +1,36 @@
		<?php require_once dirname(__FILE__).'/TestConfig.php';

Add capability api #99

Add capability api #99

Conversation

jonrandy commented Dec 3, 2018 • edited Loading

jonrandy commented Dec 3, 2018 • edited Loading

guzzilar commented Dec 4, 2018 • edited Loading

guzzilar commented Dec 4, 2018

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 Dec 4, 2018 • edited Loading

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

guzzilar Dec 4, 2018 • edited Loading

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 Dec 4, 2018 • edited Loading

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 Dec 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guzzilar commented Dec 4, 2018

guzzilar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonrandy Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

guzzilar Jan 4, 2019 • edited Loading

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

guzzilar Jan 7, 2019 • edited Loading

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 Jan 7, 2019

guzzilar commented Jan 7, 2019

jonrandy commented Jan 7, 2019 • edited Loading

guzzilar commented Jan 7, 2019

jonrandy commented Jan 7, 2019

guzzilar commented Jan 7, 2019

jacstn commented Jan 7, 2019

jonrandy commented Dec 3, 2018 •

edited

Loading

jonrandy commented Dec 3, 2018 •

edited

Loading

guzzilar commented Dec 4, 2018 •

edited

Loading

jonrandy Dec 4, 2018 •

edited

Loading

guzzilar Dec 4, 2018 •

edited

Loading

jonrandy Dec 4, 2018 •

edited

Loading

jonrandy Dec 14, 2018 •

edited

Loading

jonrandy Jan 4, 2019 •

edited

Loading

guzzilar Jan 4, 2019 •

edited

Loading

guzzilar Jan 7, 2019 •

edited

Loading

jonrandy commented Jan 7, 2019 •

edited

Loading