Create and Delete Profiles #3

cassidycodes · 2019-10-18T01:08:53Z

This PR adds a class to interface with the /profiles endpoint. It implements both create and delete actions.

See my review below for more details.

cassidycodes · 2019-10-18T12:25:16Z

README.md

+)
+# => {
+#       code: 1,
+#       message: 'Hup...want...buy.',


Poor Hup got motion sickness: https://www.youtube.com/watch?v=Sqj8xY0_Q0s

lib/bambora/client.rb

cassidycodes · 2019-10-18T12:27:13Z

lib/bambora/json_request.rb

+# frozen_string_literal: true
+
+module Bambora
+  class JSONRequest


This is shared code for all JSON APIs. We'll also need an XMLRequest for some of the endpoints.

cassidycodes · 2019-10-18T12:30:18Z

README.md

+
+```ruby
+client.profile.delete(1234)
+# => {


The return from these methods is the same as what the existing beanstream-ruby library gives us.

cassidycodes · 2019-10-18T12:38:43Z

lib/bambora/json_request.rb

+    end
+
+    def request(method:, path:, params: {}, body: {})
+      parse_response(


Do we need any error handling here? If there is a network Error, we'll get an informative error from Excon.

Based on the existing gem, is possible that we'll get an error response back from Bambora, but those errors are in JSON as well, so I think we should handle them in the application.

i guess if server error happens(not application which will form correct json with error as the body) you won't get to JSON response instead you'll get JSON parse exception and this is our responsibility to handle it. Possible solution is to check for response code first and if it signals about error wrap it in valid JSON.

cassidycodes · 2019-10-18T14:36:56Z

lib/bambora/client.rb

    # Summary: Payment profiles store confidential payment information. Transactions can be processed against profiles.
    # Docs: https://dev.na.bambora.com/docs/guides/payment_profiles/
    # Endpoint: https://api.na.bambora.com/v1/profiles
-    def profiles; end
+    def profile
+      @profile ||= Bambora::Profile.new(self)


Passing the Client here so that the same client instance can be used for profiles, payment, etc.

cassidycodes · 2019-10-18T14:38:09Z

Bambora API Wrapper: Profiles API [3]

bilogub · 2019-10-18T15:10:40Z

lib/bambora/json_request.rb

+    end
+
+    def symbolize_keys(obj)
+      return obj unless obj.is_a?(Hash)


what if you get a hash which holds a collection of hashes e.g.

obj = { 'user' => { 'accounts' => [ { 'number' => '123', }, { 'number' => '456', }, ] } } def symbolize_keys(obj) return obj unless obj.is_a?(Hash) obj.each_with_object({}) do |(k, v), hash| hash[k.to_sym] = symbolize_keys(v) hash end end symbolize_keys(obj) => {:user=>{:accounts=>[{"number"=>"123"}, {"number"=>"456"}]}}

took from rails source code:

def _deep_transform_keys_in_object(object, &block) case object when Hash object.each_with_object({}) do |(key, value), result| result[yield(key)] = _deep_transform_keys_in_object(value, &block) end when Array object.map { |e| _deep_transform_keys_in_object(e, &block) } else object end end _deep_transform_keys_in_object(obj){ |key| key.to_sym } => {:USER=>{:ACCOUNTS=>[{:NUMBER=>"123"}, {:NUMBER=>"456"}]}}

You could wrap that method into your method

Nice, thanks @bilogub!

bilogub · 2019-10-18T15:17:06Z

lib/bambora/profile.rb

+    def initialize(client)
+      super
+
+      @path = '/v1/profiles'


would probably start to break into api versions here and namespacing the classes into Bambora::V1:: in case users would want to stay on old version if supported and someone would develop V2 if available.

harrylewis

Love watching this shape up, and I'm learning a lot from it. I had a few comments to consider.

harrylewis · 2019-10-18T15:19:44Z

Gemfile.lock

@@ -7,10 +7,15 @@ PATH
 GEM
  remote: https://rubygems.org/
  specs:
+    addressable (2.7.0)


Were these changes added manually? I don't see any changes made to the Gemfile.

There are different ways of managing dependencies for gems. The "bundler way" is to list all dependencies in the gemspec then call gemspec in the Gemfile to load dependencies listed there.

https://bundler.io/v2.0/guides/creating_gem.html#testing-our-gem

I don't know much about this project, but hoe has another way of managing dependencies.

harrylewis · 2019-10-18T15:28:46Z

README.md

+#### Create a Profile
+
+```ruby
+client.profile.create(


Great example here!

README.md

harrylewis · 2019-10-18T15:32:48Z

README.md

+## Delete a Profile
+
+```ruby
+client.profile.delete(1234)


Bambora remote customer codes contain letters, and are therefore strings. Use a string input instead for the example.

harrylewis · 2019-10-18T15:52:15Z

lib/bambora/profile.rb

+# frozen_string_literal: true
+
+module Bambora
+  class Profile < JSONRequest


More of a semantic thing, I find it a bit confusing that Profile inherits from JSONRequest. I infer from this inheritance chain that Profile is a type of JSONRequest, when in reality it contains multiple methods that can make JSON requests. This seems to me to be more like a concern/module.

From the perspective of the caller though, when you are using this client, you are only ever using one API method at a time, so from the point of view of the caller, it does only represent one JSON request.

Now I've just argued with myself, haha.

I was going to call this Domain since it represents the Bambora domain, but then I realized that the domain also uses XML. I could perhaps name this JSONDomain and XMLDomain.

harrylewis · 2019-10-18T15:52:48Z

lib/bambora/profile.rb

+      request(method: :post, path: @path, body: card_data)
+    end
+
+    def delete(remote_id)


To better align with the Bambora terminology, call this parameter, customer_code. Also, could this be a named parameter so that is clear? When looking at the documentation, I had to go to the implementation to determine what the input was. A named parameter would clear up that confusion immediately.

spec/bambora/client_spec.rb

harrylewis

Multi-threading here we come!

harrylewis · 2019-10-21T15:25:37Z

README.md

-    $ gem install bambora-client
+```bash
+gem install bambora-client
+


Is the extra space needed? Not a blocker for this PR. This could be fixed when some Markdown linting is included 👀.

harrylewis · 2019-10-21T15:26:55Z

README.md

@@ -50,20 +54,89 @@ client = Bambora::Client.new(
 )
 ```

+### Profiles
+
+*Summary*: Payment profiles store confidential payment information. Transactions can be processed against profiles.


Thanks for cutting the line-length here.

harrylewis · 2019-10-21T15:30:19Z

lib/bambora/json_request.rb

+    end
+
+    def deep_transform_keys_in_object(object, &block)
+      case object


spec/bambora/client_spec.rb

bilogub · 2019-10-21T15:39:34Z

lib/bambora/json_request.rb

+
+    def request(method:, path:, params: {}, body: {})
+      resp = client.request(method: method, path: path, params: params, body: body.to_json.to_s)
+      return parse_response(resp) if resp.status <= 300


i see here a problem - what if it is 404. As a json client i want to get a response with 404 and empty collection.
instead i get a generic error which i have to parse(raw text) to interpret it something meaningful in my app.
https://jsonplaceholder.typicode.com/posts/1 - this returns me an object and this returns me an empty object
https://jsonplaceholder.typicode.com/posts/12123123123 with a status in the response header.

@bilogub Do you think we should parse all 4xx level errors and only raise an erro on 5xx and 3xx?

@cassidycodes i think i gave a falsey option in my prev comment regading possible solution here and misled you, sorry.
In nutshell we should relay anything we get from api back to our client app(as long as we don't have our own API doc to rely on - we are only sitting in the middle between API and client app passing bytes there and back) - that said parse response in begin/rescue block and pass it up the chain. In case we end up in rescue block compose 50x server response in json format. What do you think?

Sounds good! I just pushed a change that passes a hash with the error back to the user.

cassidycodes force-pushed the feat/profiles-api branch from 0171400 to c119c1c Compare October 18, 2019 12:15

cassidycodes commented Oct 18, 2019

View reviewed changes

cassidycodes self-assigned this Oct 18, 2019

cassidycodes added enhancement New feature or request documentation Improvements or additions to documentation labels Oct 18, 2019

cassidycodes changed the base branch from chore/add-circle-ci to master October 18, 2019 14:09

cassidycodes added 5 commits October 18, 2019 10:14

Tests for Profild#delete

3a260ae

Testing with real data for Profile#create

2c6d213

Moving shared code to domain object

9eec953

Rename Domain to JSONRequest to allow for future XMLRequest

ca4f3a6

Update readme with responses

452f2cf

cassidycodes force-pushed the feat/profiles-api branch from 19d302e to 452f2cf Compare October 18, 2019 14:16

Adding things back in that I disappeard during rebase

77ce1fa

cassidycodes commented Oct 18, 2019

View reviewed changes

cassidycodes requested review from bilogub and harrylewis October 18, 2019 14:37

cassidycodes marked this pull request as ready for review October 18, 2019 14:37

bilogub reviewed Oct 18, 2019

View reviewed changes

cassidycodes added 2 commits October 18, 2019 11:43

Accounting for arrays when transforming keys

c6ef9c8

Adding sub-merchant id to profile spec

8073a5a

harrylewis reviewed Oct 18, 2019

View reviewed changes

cassidycodes added 4 commits October 18, 2019 17:43

Move Profile to V1 namespace

5081b5f

Using customer_code for delete

4a3e186

Raising an erro if resp is not 2xx

68c3d4b

Merge branch 'master' into feat/profiles-api

97f6dba

bilogub reviewed Oct 21, 2019

View reviewed changes

spec/bambora/client_spec.rb Show resolved Hide resolved

harrylewis approved these changes Oct 21, 2019

View reviewed changes

bilogub reviewed Oct 21, 2019

View reviewed changes

cassidycodes added 3 commits October 21, 2019 11:41

Adding version validation

3a80025

Typo in readme

3ef95d8

Returning error hash with status

2994aa4

bilogub approved these changes Oct 22, 2019

View reviewed changes

cassidycodes merged commit 2a2378d into master Oct 22, 2019

cassidycodes deleted the feat/profiles-api branch October 22, 2019 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create and Delete Profiles #3

Create and Delete Profiles #3

cassidycodes commented Oct 18, 2019 •

edited

Loading

cassidycodes Oct 18, 2019

cassidycodes Oct 18, 2019

cassidycodes Oct 18, 2019

cassidycodes Oct 18, 2019

bilogub Oct 18, 2019

cassidycodes Oct 18, 2019

cassidycodes commented Oct 18, 2019

bilogub Oct 18, 2019

bilogub Oct 18, 2019

cassidycodes Oct 18, 2019

bilogub Oct 18, 2019

harrylewis left a comment

harrylewis Oct 18, 2019

cassidycodes Oct 18, 2019 •

edited

Loading

harrylewis Oct 18, 2019

harrylewis Oct 18, 2019

harrylewis Oct 18, 2019

cassidycodes Oct 18, 2019

harrylewis Oct 18, 2019

harrylewis left a comment

harrylewis Oct 21, 2019

harrylewis Oct 21, 2019

harrylewis Oct 21, 2019

bilogub Oct 21, 2019

cassidycodes Oct 21, 2019

bilogub Oct 21, 2019

cassidycodes Oct 22, 2019

Create and Delete Profiles #3

Create and Delete Profiles #3

Conversation

cassidycodes commented Oct 18, 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

cassidycodes commented Oct 18, 2019

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

harrylewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cassidycodes Oct 18, 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

harrylewis left a 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

cassidycodes commented Oct 18, 2019 •

edited

Loading

cassidycodes Oct 18, 2019 •

edited

Loading