-
Notifications
You must be signed in to change notification settings - Fork 28
feat: Added support for Authenticated Datafiles #255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, can you add a couple of simple tests in http_project_config_manager_spec.rb? Make sure the header is there, etc.
Hey @thomaszurkan-optimizely . Thanks for reviewing quickly. I am already writing tests. That's what I mentioned in the PR summary too. |
it 'should add authorization header when auth token is provided' do | ||
allow(Optimizely::Helpers::HttpUtils).to receive(:make_request) | ||
@http_project_config_manager = Optimizely::HTTPProjectConfigManager.new( | ||
sdk_key: 'valid_sdk_key', | ||
datafile_access_token: 'the-token' | ||
) | ||
sleep 0.1 | ||
expect(Optimizely::Helpers::HttpUtils).to have_received(:make_request).with(anything, anything, anything, hash_including('Authorization' => 'Bearer the-token'), anything) | ||
end | ||
|
||
it 'should add authorization header when auth token is provided' do | ||
allow(Optimizely::Helpers::HttpUtils).to receive(:make_request) | ||
@http_project_config_manager = Optimizely::HTTPProjectConfigManager.new( | ||
sdk_key: 'valid_sdk_key', | ||
datafile_access_token: 'the-token' | ||
) | ||
sleep 0.1 | ||
expect(Optimizely::Helpers::HttpUtils).to have_received(:make_request).with(anything, anything, anything, hash_including('Authorization' => 'Bearer the-token'), anything) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity, is there a reason why this part of the spec is duplicated? Seems like the two blocks are exactly the same.
Wondering if this is instead supposed to test the case where an invalid SDK key is provided (not sure if that is handled / error-checked elsewhere)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @pthompson127 ... good eye... its just a plain stupid mistake :). i wil fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zashraf1985 ahh gotcha :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, but I am wondering if this was actually tested with a proper access token and sdk key
The last commit where i changed the url was not tested because that url is not working yet. it was thoroughly tested on the previous test url though. @mjc1283 told me he was changing the url to the one we will eventually use to avoid creating a new PR and re-releasing in case of javascript. I asked him should i change the url for ruby too as it is not merged yet, he agreed, thats why we did this to save us a hassle of another PR just to modify the url. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
Added support for Authenticated Datafiles
Test plan
Added new unit tests