Merge pull request #25 from Investec-Developer-Community/refactor/quick-wins

refactor: implement quick wins for code modernization

Author: programmable-banking-community

Gemfile.lock

@@ -2,6 +2,7 @@ PATH
   remote: .
   specs:
     investec_open_api (2.1.0)
+      base64
       faraday
       money
 
@@ -10,6 +11,7 @@ GEM
   specs:
     addressable (2.8.6)
       public_suffix (>= 2.0.2, < 6.0)
+    base64 (0.3.0)
     bigdecimal (3.1.7)
     coderay (1.1.3)
     concurrent-ruby (1.2.3)
@@ -58,6 +60,7 @@ GEM
 
 PLATFORMS
   arm64-darwin-23
+  x86_64-linux
 
 DEPENDENCIES
   faker (~> 3.4)

README.md

@@ -16,6 +16,10 @@ A simple client wrapper for the [Investec Open API](https://developer.investec.c
 - Retrieve balances per account
 - Transfer between accounts
 
+## Requirements
+
+- **Ruby 2.7.0 or higher**
+
 ## Installation
 
 Add this line to your application's Gemfile:
@@ -123,6 +127,83 @@ client.transfer_multiple(
 )
 ```
 
+## Error Handling
+
+The client raises specific exceptions for different error scenarios. Always wrap API calls in error handling:
+
+### Custom Exception Classes
+
+- `InvestecOpenApi::AuthenticationError` - Raised when OAuth authentication fails
+- `InvestecOpenApi::ValidationError` - Raised when required parameters are missing or invalid
+- `InvestecOpenApi::NotFoundError` - Raised when a requested resource is not found
+- `InvestecOpenApi::APIError` - Raised for general API errors
+- `InvestecOpenApi::RateLimitError` - Raised when rate limits are exceeded
+
+### Example: Handling Errors
+
+```ruby
+client = InvestecOpenApi::Client.new
+
+begin
+  client.authenticate!
+rescue InvestecOpenApi::AuthenticationError => e
+  puts "Authentication failed: #{e.message}"
+  # Handle authentication error
+end
+
+begin
+  transactions = client.transactions(account_id)
+rescue InvestecOpenApi::ValidationError => e
+  puts "Invalid parameters: #{e.message}"
+  # Handle validation error
+rescue InvestecOpenApi::NotFoundError => e
+  puts "Account not found: #{e.message}"
+  # Handle not found error
+rescue InvestecOpenApi::APIError => e
+  puts "API error occurred: #{e.message}"
+  # Handle general API error
+end
+
+begin
+  transfer = InvestecOpenApi::Models::Transfer.new(
+    beneficiary_id,
+    1000.00,
+    "my ref",
+    "their ref"
+  )
+rescue InvestecOpenApi::ValidationError => e
+  puts "Invalid transfer parameters: #{e.message}"
+  # Handle validation error
+end
+```
+
+## Thread Safety
+
+**Important:** The client instance caches a Faraday connection object. If you're using this client in a multi-threaded environment (such as a Rails application), ensure that each thread has its own instance of `InvestecOpenApi::Client`:
+
+```ruby
+# ✅ Correct: Each thread gets its own client
+threads = 5.times.map do
+  Thread.new do
+    client = InvestecOpenApi::Client.new
+    client.authenticate!
+    # Use the client...
+  end
+end
+threads.each(&:join)
+
+# ❌ Incorrect: Sharing a single client across threads
+client = InvestecOpenApi::Client.new
+client.authenticate!
+threads = 5.times.map do
+  Thread.new do
+    # Don't do this - connection caching is not thread-safe
+    client.accounts
+  end
+end
+threads.each(&:join)
+```
+
 ## Running in Sandbox mode
 
 To run in sandbox mode, use the following configuration:

investec_open_api.gemspec

@@ -10,7 +10,7 @@ Gem::Specification.new do |spec|
   spec.description   = %q{A small wrapper client for accessing Investec's Open API}
   spec.homepage      = "https://github.com/programmable-banking-community/investec_open_api"
   spec.license       = "MIT"
-  spec.required_ruby_version = Gem::Requirement.new(">= 2.3.0")
+  spec.required_ruby_version = Gem::Requirement.new(">= 2.7.0")
 
   spec.metadata["allowed_push_host"] = "https://rubygems.org"
 
@@ -30,6 +30,7 @@ Gem::Specification.new do |spec|
   # add runtime dependencies
   spec.add_runtime_dependency 'faraday'
   spec.add_runtime_dependency 'money'
+  spec.add_runtime_dependency 'base64'
 
   # add development dependencies
   spec.add_development_dependency 'rake'

lib/investec_open_api.rb

@@ -1,10 +1,17 @@
+# frozen_string_literal: true
+
 require "investec_open_api/version"
 require "investec_open_api/models/base"
 require "investec_open_api/camel_case_refinement"
 require "investec_open_api/client"
 
 module InvestecOpenApi
   class Error < StandardError; end
+  class AuthenticationError < Error; end
+  class NotFoundError < Error; end
+  class ValidationError < Error; end
+  class APIError < Error; end
+  class RateLimitError < Error; end
 
   class Configuration
     DEFAULT_BASE_URL = "https://openapi.investec.com/"

lib/investec_open_api/camel_case_refinement.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module InvestecOpenApi
   module CamelCaseRefinement
     refine Hash do

lib/investec_open_api/client.rb

@@ -1,60 +1,108 @@
+# frozen_string_literal: true
+
 require "faraday"
 require "investec_open_api/models/account"
 require "investec_open_api/models/transaction"
 require "investec_open_api/models/balance"
 require "investec_open_api/models/transfer"
 require "investec_open_api/camel_case_refinement"
-require 'base64'
+require "base64"
 
 class InvestecOpenApi::Client
   using InvestecOpenApi::CamelCaseRefinement
 
   def authenticate!
     @token = get_oauth_token["access_token"]
+  rescue StandardError => e
+    raise InvestecOpenApi::AuthenticationError, "Failed to authenticate: #{e.message}"
   end
 
+  # Get all accounts for the authenticated user
+  # @return [Array<InvestecOpenApi::Models::Account>]
+  # @raise [InvestecOpenApi::APIError] if the request fails
   def accounts
     response = connection.get("za/pb/v1/accounts")
     response.body["data"]["accounts"].map do |account_raw|
       InvestecOpenApi::Models::Account.from_api(account_raw)
     end
+  rescue StandardError => e
+    raise InvestecOpenApi::APIError, "Failed to fetch accounts: #{e.message}"
   end
 
-  ## Get cleared transactions for an account
+  # Get cleared transactions for an account
   # @param [String] account_id The id of the account to get transactions for
-  # @param [Hash] options
+  # @param [Hash] options Optional query parameters
   # @option options [String] :fromDate Start date from which to get transactions
   # @option options [String] :toDate End date for transactions
-  # @option options [String] :transactionType Type of transaction to filter by eg: CardPurchases, Deposits
+  # @option options [String] :transactionType Type of transaction to filter by (e.g., CardPurchases, Deposits)
+  # @return [Array<InvestecOpenApi::Models::Transaction>]
+  # @raise [InvestecOpenApi::ValidationError] if account_id is blank
+  # @raise [InvestecOpenApi::APIError] if the request fails
   def transactions(account_id, options = {})
+    raise InvestecOpenApi::ValidationError, "account_id cannot be blank" if account_id.to_s.strip.empty?
+
     endpoint_url = "za/pb/v1/accounts/#{account_id}/transactions"
     perform_transaction_request(endpoint_url, options)
+  rescue InvestecOpenApi::ValidationError
+    raise
+  rescue StandardError => e
+    raise InvestecOpenApi::APIError, "Failed to fetch transactions: #{e.message}"
   end
 
-  ## Get pending transactions for an account
+  # Get pending transactions for an account
   # @param [String] account_id The id of the account to get pending transactions for
-  # @param [Hash] options
+  # @param [Hash] options Optional query parameters
   # @option options [String] :fromDate Start date from which to get pending transactions
   # @option options [String] :toDate End date for pending transactions
+  # @return [Array<InvestecOpenApi::Models::Transaction>]
+  # @raise [InvestecOpenApi::ValidationError] if account_id is blank
+  # @raise [InvestecOpenApi::APIError] if the request fails
   def pending_transactions(account_id, options = {})
+    raise InvestecOpenApi::ValidationError, "account_id cannot be blank" if account_id.to_s.strip.empty?
+
     endpoint_url = "za/pb/v1/accounts/#{account_id}/pending-transactions"
     perform_transaction_request(endpoint_url, options)
+  rescue InvestecOpenApi::ValidationError
+    raise
+  rescue StandardError => e
+    raise InvestecOpenApi::APIError, "Failed to fetch pending transactions: #{e.message}"
   end
 
+  # Get balance for an account
+  # @param [String] account_id The id of the account to get balance for
+  # @return [InvestecOpenApi::Models::Balance]
+  # @raise [InvestecOpenApi::ValidationError] if account_id is blank
+  # @raise [InvestecOpenApi::NotFoundError] if account not found or balance data unavailable
+  # @raise [InvestecOpenApi::APIError] if the request fails
   def balance(account_id)
+    raise InvestecOpenApi::ValidationError, "account_id cannot be blank" if account_id.to_s.strip.empty?
+
     endpoint_url = "za/pb/v1/accounts/#{account_id}/balance"
     response = connection.get(endpoint_url)
-    raise "Error fetching balance" if response.body["data"].nil?
+    raise InvestecOpenApi::NotFoundError, "Balance data not found for account #{account_id}" if response.body["data"].nil?
+
     InvestecOpenApi::Models::Balance.from_api(response.body["data"])
+  rescue InvestecOpenApi::ValidationError, InvestecOpenApi::NotFoundError
+    raise
+  rescue StandardError => e
+    raise InvestecOpenApi::APIError, "Failed to fetch balance: #{e.message}"
   end
 
-  # @param [String] account_id
-  # @param [Array<InvestecOpenApi::Models::Transfer>] transfers
+  # Transfer funds between accounts
+  # @param [String] account_id The id of the account to transfer from
+  # @param [Array<InvestecOpenApi::Models::Transfer>] transfers List of transfers to perform
+  # @param [String, nil] profile_id Optional profile ID for the transfer
+  # @return [Hash] The response body from the API
+  # @raise [InvestecOpenApi::ValidationError] if parameters are invalid
+  # @raise [InvestecOpenApi::APIError] if the request fails
   def transfer_multiple(
     account_id,
     transfers,
     profile_id = nil
   )
+    raise InvestecOpenApi::ValidationError, "account_id cannot be blank" if account_id.to_s.strip.empty?
+    raise InvestecOpenApi::ValidationError, "transfers cannot be empty" if transfers.nil? || transfers.empty?
+
     endpoint_url = "za/pb/v1/accounts/#{account_id}/transfermultiple"
     data = {
       transferList: transfers.map(&:to_h),
@@ -65,6 +113,10 @@ def transfer_multiple(
       JSON.generate(data)
     )
     response.body
+  rescue InvestecOpenApi::ValidationError
+    raise
+  rescue StandardError => e
+    raise InvestecOpenApi::APIError, "Failed to process transfers: #{e.message}"
   end
 
   private
@@ -85,6 +137,10 @@ def get_oauth_token
   end
 
   def connection
+    # NOTE: This connection is cached in an instance variable. If you use this client
+    # in a multi-threaded environment, ensure each thread has its own client instance.
+    # The connection itself is thread-safe (Faraday uses thread-safe adapters),
+    # but the caching mechanism is not.
     @_connection ||= Faraday.new(url: InvestecOpenApi.config.base_url) do |builder|
       if @token
         builder.headers["Authorization"] = "Bearer #{@token}"

lib/investec_open_api/models/account.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module InvestecOpenApi::Models
   class Account < Base
     attr_reader :id,

lib/investec_open_api/models/base.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require_relative "../string_utilities"
 
 module InvestecOpenApi::Models

lib/investec_open_api/models/transaction.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require "money"
 
 module InvestecOpenApi::Models

lib/investec_open_api/models/transfer.rb

@@ -4,16 +4,22 @@ module InvestecOpenApi::Models
   class Transfer
     attr_reader :beneficiary_account_id, :amount, :my_reference, :their_reference
 
-    # @param [String] beneficiary_account_id
-    # @param [Float] amount
-    # @param [String] my_reference
-    # @param [String] their_reference
+    # @param [String] beneficiary_account_id The ID of the beneficiary account
+    # @param [Float] amount The amount to transfer
+    # @param [String] my_reference Reference visible to the sender
+    # @param [String] their_reference Reference visible to the recipient
+    # @raise [InvestecOpenApi::ValidationError] if required parameters are blank
     def initialize(
       beneficiary_account_id,
       amount,
       my_reference,
       their_reference
     )
+      raise InvestecOpenApi::ValidationError, "beneficiary_account_id cannot be blank" if beneficiary_account_id.to_s.strip.empty?
+      raise InvestecOpenApi::ValidationError, "amount cannot be nil or zero" if amount.nil? || amount.to_f.zero?
+      raise InvestecOpenApi::ValidationError, "my_reference cannot be blank" if my_reference.to_s.strip.empty?
+      raise InvestecOpenApi::ValidationError, "their_reference cannot be blank" if their_reference.to_s.strip.empty?
+
       @beneficiary_account_id = beneficiary_account_id
       @amount = amount.to_s
       @my_reference = my_reference