careerbuilder / ruby-cb-api

Ruby wrapper around the CareerBuilder.com APIs
Apache License 2.0
14 stars 18 forks source link

Adding back error checking and throwing on Job call #265

Closed MajinFro closed 7 years ago

MajinFro commented 7 years ago

So cool story. Not pushing the Job hash through the models skipped some error checking and throwing. Added that back in.

After discussions with Jeff/Colin. We decided to throw the 404 DocumentNotFoundError since the api should be returning 404 instead of 200. Going to put in an AO ticket to have that api changed.

Casao commented 7 years ago

As I said from the beginning, these are a 200 response from the API. That's why no exceptions are being thrown.

HTTP/1.1 200 OK
Cache-Control: private
Content-Encoding: gzip
Content-Type: application/json;charset=UTF-8
Date: Wed, 14 Dec 2016 21:10:27 GMT
P3P: CP="CAO CURa IVAa HISa OUR IND UNI COM NAV INT STA",policyref="https://secure.icbdr.com/images/CBP3P.xml"
Server: Apache-Coyote/1.1
Vary: Accept-encoding
X-AspNet-Version: 4.0.30319
X-NewRelic-App-Data: PxQAUlVQAAMTXVlWBwQGUUYdFGQHBDcQUQxLA1tMXV1dSngyYU5SEg0ZXQ4EEF1WWwETTV1eVUkOXlQdAxUTGhJOCEwIFAQcA1cBUQlZAU5JBxtDJVoLBwFQI1AOdQYgVwBycUBKBQNcEV0/
X-PBY: BEARWEB22
X-Powered-By: ASP.NET
transfer-encoding: chunked
Connection: Close

{"ResponseJob":{"Errors":{"Error":"Job was not found, or could not be retrieved."},"TimeResponseSent":"12/14/2016 4:10:27 PM","TimeElapsed":"0.0156"}}
Casao commented 7 years ago

What, exactly, is this trying to solve?

ghost commented 7 years ago

Ok so the API is returning a 200 when it should 404. I'm fine putting a temporary fix (like if there is an errors node that says Job was not found" then manually throw a document not found exception) and ticketing AO to fix the API so the temp fix wont be needed in the future

MajinFro commented 7 years ago

So before we ran the api response through an object. that object got deleted. it would extract meta data.

[3:52 PM] Brandon Koch: https://github.com/careerbuilder/ruby-cb-api/blob/master/lib/cb/responses/api_response.rb#L25 [3:53 PM] Brandon Koch: that line sets this up https://github.com/careerbuilder/ruby-cb-api/blob/master/lib/cb/responses/metadata.rb#L28 [3:53 PM] Brandon Koch: which causes this to happen https://github.com/careerbuilder/ruby-cb-api/blob/master/lib/cb/responses/errors.rb#L29

this would cause the ApiResponseError we used to receive. If we are fine with checking the errors node in main and not worrying about this error, then we can close this. This was my way to make sure that old functionality was still there. I thought based on the code review discussion, this was something we still wanted to do since it was an accidental removal.

Casao commented 7 years ago

This code still does not throw the exception.

[3] pry(main)> Cb::Clients::Job.get did: 'asdf'
=> {"ResponseJob"=>{"Errors"=>{"Error"=>"Job was not found, or could not be retrieved."}, "TimeResponseSent"=>"12/14/2016 4:25:04 PM", "TimeElapsed"=>"0.0312"}}

Compare to the old code:

[2] pry(main)> Cb::Clients::Job.find_by_did 'Asdf'
Cb::ApiResponseError: Job was not found, or could not be retrieved.
from /Users/cewen/code/ruby-cb-api/lib/cb/responses/errors.rb:29:in `parsed_errors'
MajinFro commented 7 years ago

forgot to drill down a layer.

bowersaa commented 7 years ago

👍

MajinFro commented 7 years ago

k concerns addressed