Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

🚀 Fetch all fields from API instead of hardcoding #67

Open
balasankarc opened this issue Aug 10, 2019 · 3 comments
Open

🚀 Fetch all fields from API instead of hardcoding #67

balasankarc opened this issue Aug 10, 2019 · 3 comments

Comments

@balasankarc
Copy link

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
Copy link
Author

balasankarc commented Aug 10, 2019

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
Copy link
Contributor

splybon commented Aug 12, 2019

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
Copy link
Contributor

@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.

  • pros: doesn't add any new features or change the API
  • cons: doesn't fix the issue

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")
  • pros: a more expressive API
  • cons: requires new feature work. adds complexity. requires research of the custom field alias (why is it sometimes nil?)

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

3 participants