method-inc / bamboozled

Bamboozled wraps the BambooHR API without the use of Rails dependencies.
MIT License
44 stars 50 forks source link

🚀 Fetch all fields from API instead of hardcoding #67

Open balasankarc opened 5 years ago

balasankarc commented 5 years ago

What problem does this solve?

The list of fields that is returned by custom report is hardcoded in the FieldCollection class.

Because of this, a custom report when passed :all will not include custom fields, but only the hardcoded ones.

Describe the solution you'd like

BambooHR returns the list of all fields via api, which is accessible using client.meta.fields method. Can we populate that while initializing a client and generate the list of fields from that information instead of hardcoding it?

Possible alternatives

Support something like client.report.custom([:all, customField1, customField2], 'JSON') which will let users specify the additional fields they like (in addition to the ones returned by :all).

balasankarc commented 5 years ago

The alternative mentioned above seems easy to implement. For example, I think this implementation supports the following scenarios

  1. client.report.custom(:all, 'JSON')
  2. client.report.custom([:all, 'customField1', 'customField2'], 'JSON')
  3. client.report.custom(':all, customField1, customField2', 'JSON')
--- a/lib/bamboozled/api/field_collection.rb
+++ b/lib/bamboozled/api/field_collection.rb
@@ -2,8 +2,17 @@ module Bamboozled
   module API
     class FieldCollection
       def self.wrap(fields)
-        fields = all_names if fields == :all
-        fields = fields.split(",") if fields.is_a?(String)
+        if fields == :all
+          fields = all_names
+        elsif fields.is_a?(String)
+          fields = fields.split(",").map { |item| item.strip }
+        end
+
+        if fields.include?(:all) || fields.include?(":all")
+          fields -= [:all, ":all"]
+          fields += all_names
+        end
+
         new(fields)
       end

@splybon Does this make sense? :)

splybon commented 5 years ago

Hi @balasankarc thanks for the issue! Yes that makes total sense. I will play around with this myself as well to try and investigate some other methods, but I think I like your alternative solution.

My only hesitation with the client.meta.fields would be that it would make another network call to initialize the client. I like the flexibility of your alternative solution

In the mean time a workaround for you could be something like

fields = (Bamboozled::API::FieldCollection.all_names + client.meta.fields.map {|f| f['alias']}).compact.uniq
client.report.custom(fields, "JSON")
chrisman commented 5 years ago

@splybon and I discussed a documentation based and an API based approach to this issue, but haven't decided which is our preferred method of moving forward.

Possible solutions:

Documentation Don't add any code to the project to address this, but instead just update the documentation with this example to show how users can include custom fields in addition to the default hardcoded fields.

API This is a code based solution that should abstract away some of those calls so that you can write something like the following:

fields = (client.meta.defaultFields + client.meta.fields)
client.report.custom(fields, "JSON")