Conversation
…method, adding this as an option
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for retrieving raw response data from the Work Histories API by introducing an optional include_raw parameter to the all method. When enabled, it returns both parsed and raw data in a hash structure.
- Added
include_rawparameter to theallmethod with default value of false - Modified return value to optionally include both parsed data and raw response
- Refactored method formatting for consistency
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@lienvdsteen - i expect there might be a better way to do this, which is why I have left it in PR 🙇 |
lienvdsteen
left a comment
There was a problem hiding this comment.
If you need both the parsed and the raw data, feel free to merge, otherwise I would move the return up and only return the raw data. But then maybe also change the variable to something like no_parsing or whatever.
I'll make an issue to think about how we can solve this for all endpoints vs just this one.
lib/bob/api/employee/work_history.rb
Outdated
| response = get("people/#{employee_id}/work") | ||
| WorkHistoryParser.new(response).work_histories | ||
| parsed = WorkHistoryParser.new(response).work_histories | ||
| return { data: parsed, raw: response.values.first } if include_raw |
There was a problem hiding this comment.
I think it could be cool to solve this for all endpoints (so you can always choose if you want it parsed or not). That said, for the sake of moving you forward, I wouldn't do that. I'll thinker about it a bit. Do you still need the parsed data as well when you need the raw? If no, then I would move the return up:
response = get("people/#{employee_id}/work")
return { raw: respone.values.first } if include_raw
parsed = ...Main reason to do that is that the parsing does create ruby objects and technically slows calling the endpoint down + it also takes up memory.
There was a problem hiding this comment.
FYI Bob did also release a new endpoint to access work history for multiple employees at once https://apidocs.hibob.com/reference/get_bulk-people-work
(Not sure if that's useful for you here, just wanted to share)
There was a problem hiding this comment.
Awesome; I'll move the return up and get this in for now while we think about a wider fix; the JSON schema for updates is very opinionated it seems. In this case I am updating a line in the Work table in Bob, but they have make the PATCH ability legacy, so the new method is 1) read the line 2) pass the whole line with your changes back to it... it works, but it's not as easy to work with imo.
Thanks for the note on the the bulk! For my usecase not required right now, but might be work implementing regardless
Why?
What?
include_rawwhich is true (DEFAULT: false), returns a hash of the parsed and raw data