Add `has_many` method to Ruby Unit #23

mcccoleman · 2023-01-17T19:28:33Z

This PR adds a has_many method to the Ruby gem in order to return all customers that belong to a deposit_account.

One question - Unit returns customer as a key on resources when there is only one customer & customers as a key when there is more than one customer. In this PR I made it so that customers is always returned, regardless of whether there is one or more customers. Do you think this is the appropriate solution?

Please let me know if walking through this together would be helpful!

ianyamey · 2023-01-17T20:08:54Z

lib/unit-ruby/deposit_account.rb

@@ -19,6 +19,7 @@ class DepositAccount < APIResource
    attribute :close_reason, Types::String, readonly: true # Optional. The reason the account was closed, either ByCustomer or Fraud.

    belongs_to :customer, class_name: 'Unit::IndividualCustomer'
+    has_many :customers, class_name: 'Unit::IndividualCustomer'


Might be worth calling out in a comment that this is for joint accounts

@ianyamey thanks for commenting on this - I'd actually appreciate your thoughts on this specific instance. I thought it would be strange for the user of ruby gem to have to know whether the particular resource belongs to customer or to customers, so in my implementation of adding customers enabled it to also work when there was only one particular customer added to the account.

ie if .customers is called on a deposit account with only one customer, we'll return an array with one customer. Do you agree that that's the correct approach to use?

ianyamey · 2023-01-17T20:14:23Z

lib/unit-ruby/util/resource_operations.rb

@@ -11,6 +11,21 @@ def find(id)

          build_resource_from_json_api(located_resource)
        end
+
+        def find_all(ids)


Could we use the List operation here instead?

sorry, do you mean update the existing def list so that it's able to determine whether the resource is a single resource or a plural resource?

Also I realized that my original implementation could be simplified, what's currently reflected is the more streamlined version.

ianyamey · 2023-01-17T20:16:13Z

lib/unit-ruby/util/api_resource.rb

+        end
+      end
+
+      define_method("#{resource_name}=") do |resource|


Does this need to accept an array of resources?

I would have expected usage to look like:

deposit_account = DepositAccount.find(...) deposit_account.customers = [customer_1, customer_2] deposit_account.save

Possibly? In taking a look at this comment & playing around with this a little, I'm not a little unsure what this is doing. Would you be up for pairing on this a little tomorrow AM?

ianyamey · 2023-01-17T20:19:39Z

lib/unit-ruby/util/api_resource.rb

+
+          relationship_id = relationships.dig(singular_resource_name, :data, :id)
+
+          return nil unless relationship_id


Since this is for the plural resource, should we return [] instead of nil?

(same on line L130)

Good suggestion!

ianyamey · 2023-01-18T21:08:15Z

lib/unit-ruby/util/api_resource.rb

+        if relationships.keys.include?(singular_resource_name)
+
+          relationship_id = relationships.dig(singular_resource_name, :data, :id)
+
+          return [] unless relationship_id
+
+          return [relationship_id].map { |id| Kernel.const_get(class_name).find(id) }
+        else
+          relationship_ids = relationships.dig(resource_name.to_sym, :data).map { |x| x[:id] }
+
+          return [] if relationship_ids.empty?
+
+          return [relationship_ids].map { |id| Kernel.const_get(class_name).find(id) }
+        end
+      end


I think this adds a lot of complexity trying to get around Unit's design decision to allow a singular and plural relationship.

My suggestion would just be to keep this narrow, and allow apps that use this SDK to handle those cases when there are mix of singular/plural entities themselves.

Suggested change

if relationships.keys.include?(singular_resource_name)

relationship_id = relationships.dig(singular_resource_name, :data, :id)

return [] unless relationship_id

return [relationship_id].map { |id| Kernel.const_get(class_name).find(id) }

else

relationship_ids = relationships.dig(resource_name.to_sym, :data).map { |x| x[:id] }

return [] if relationship_ids.empty?

return [relationship_ids].map { |id| Kernel.const_get(class_name).find(id) }

end

end

resource_class = Kernel.const_get(class_name)

relationships.dig(resource_name.to_sym, :data).map do |data|

resource_class.build_resource_from_json_api(id)

end

end

@ianyamey apologies, I thought I had responded to this. I guess my question would be how is the user of this service supposed to determine whether or not a particular deposit account has one or many users without throwing an error. If we're only providing a mapping of what's sent from them to us, calling customers on a deposit account that only has one will result in an error since the relationship returned from unit will only have a customer property.

mcccoleman · 2023-01-20T14:46:09Z

lib/unit-ruby/util/api_resource.rb

+            Kernel.const_get(class_name).find(resource.id)
+          end
+        elsif defined?(singular_resource_name)
+          [send(singular_resource_name)]


@ianyamey .to_a isn't a method on Unit::IndividualCustomer

add has many method

refactors

…count-owners-endpoint-to Add Ability to Add Customer to Deposit Account

* add has many method add has many method * add ability to add a customer to a deposit account refactors

mcccoleman force-pushed the coleman-add-has_many-method branch 5 times, most recently from 098657a to 85435b6 Compare January 17, 2023 19:34

mcccoleman force-pushed the coleman/add-create-deposit-account-spec branch from a1175ca to d78aa30 Compare January 17, 2023 19:37

mcccoleman force-pushed the coleman-add-has_many-method branch 4 times, most recently from d711154 to 7bec1eb Compare January 17, 2023 19:46

mcccoleman requested a review from cisacke January 17, 2023 19:47

mcccoleman force-pushed the coleman-add-has_many-method branch from 7bec1eb to e420f1d Compare January 17, 2023 19:48

ianyamey reviewed Jan 17, 2023

View reviewed changes

mcccoleman force-pushed the coleman-add-has_many-method branch from e420f1d to d146595 Compare January 17, 2023 21:30

mcccoleman removed the request for review from cisacke January 18, 2023 15:23

mcccoleman force-pushed the coleman-add-has_many-method branch from d146595 to 6449a0f Compare January 18, 2023 16:29

mcccoleman requested review from ianyamey and cisacke January 18, 2023 16:30

ianyamey reviewed Jan 18, 2023

View reviewed changes

mcccoleman force-pushed the coleman-add-has_many-method branch 3 times, most recently from e6ff60f to 8fae143 Compare January 20, 2023 14:43

mcccoleman commented Jan 20, 2023

View reviewed changes

add has many method

a7eed46

add has many method

mcccoleman force-pushed the coleman-add-has_many-method branch from 8fae143 to a7eed46 Compare January 20, 2023 14:46

Michael Coleman and others added 2 commits January 20, 2023 11:32

add ability to add a customer to a deposit account

a23d645

refactors

Merge pull request #18 from retirable/michael/ret-2541-add-the-add-ac…

b4f9c71

…count-owners-endpoint-to Add Ability to Add Customer to Deposit Account

mcccoleman merged commit 36f58b6 into coleman/add-create-deposit-account-spec Jan 20, 2023

mcccoleman added a commit that referenced this pull request Jan 20, 2023

Add has_many method to Ruby Unit (#23)

d511de4

* add has many method add has many method * add ability to add a customer to a deposit account refactors

mcccoleman deleted the coleman-add-has_many-method branch April 5, 2023 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `has_many` method to Ruby Unit #23

Add `has_many` method to Ruby Unit #23

mcccoleman commented Jan 17, 2023 •

edited

Loading

ianyamey Jan 17, 2023

mcccoleman Jan 17, 2023

ianyamey Jan 17, 2023

mcccoleman Jan 17, 2023

mcccoleman Jan 17, 2023

ianyamey Jan 17, 2023

mcccoleman Jan 17, 2023

ianyamey Jan 17, 2023

mcccoleman Jan 17, 2023

ianyamey Jan 18, 2023

mcccoleman Jan 19, 2023

mcccoleman Jan 20, 2023


		relationship_id = relationships.dig(singular_resource_name, :data, :id)

		return nil unless relationship_id

Add has_many method to Ruby Unit #23

Add has_many method to Ruby Unit #23

Conversation

mcccoleman commented Jan 17, 2023 • 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

Add `has_many` method to Ruby Unit #23

Add `has_many` method to Ruby Unit #23

mcccoleman commented Jan 17, 2023 •

edited

Loading