Add required version parameter when requesting user information from …

[Hirurg103]

…vk provider

[fix #108]

Seems like now vk provider requires v (version) parameter to be present when calling their API:

curl -XGET https://api.vk.com/method/getProfiles\?user_id\=1
=> {"error":{"error_code":8,"error_msg":"Invalid request: v (version) is required","request_params":[{"key":"oauth","value":"1"},{"key":"method","value":"getProfiles"},{"key":"user_id","value":"1"}]}}

curl -XGET https://api.vk.com/method/getProfiles\?user_id\=1\&v\=5.71
{"response":[{"id":1,"first_name":"Павел","last_name":"Дуров"}]}

[ebihara99999]

Thank you very much for he PR! It looks good to me.

[ebihara99999]

It matters to the issue that cuts out to external gems. Even though it’s better if the PR has tests of the change, considering the code base and circumstances, I think I can’t require tests to the PR.

[joshbuker]

Facebook similarly requires an API version, and that's currently set in the initializer. @Hirurg103 can you try setting config.vk.v = '5.71' inside your initializer and see if that fixes the issue?

We should never hardcode api versions as it would require updating the entire Sorcery gem if VK bumps their version requirement. Instead, lets turn this PR into adding the version to the initializer template, and fix any issues that might arise from setting it that way.

[ebihara99999]

setting config.vk.v = '5.71' inside your initializer

I'm sorry I miss that. Thanks for the response.

[Hirurg103]

We should never hardcode api versions as it would require updating the entire Sorcery gem if VK bumps their version requirement. Instead, lets turn this PR into adding the version to the initializer template, and fix any issues that might arise from setting it that way.

Hi @athix , sounds good to me. I'll do that, thank you

[Hirurg103]

Hi @ebihara99999 @athix, I've added possibility to configure vk api version with tests

[ebihara99999]

@Hirurg103
Thank you for adding tests! Before looking into the test suites, I'd like to explain how to set api version again.

Firstly, follow this and add vk configuration block to configure an api version. Facebook api version example is L119. Now It seems you can't receive the api version because of lacking of this configuration.
https://github.com/Sorcery/sorcery/blob/master/lib/generators/sorcery/templates/initializer.rb#L119

Then can you confirm working it properly? If you confirm it's working, please add tests if necessary.

[Hirurg103]

Hi @ebihara99999, done. Thank you for explanation! Could you review this PR if it can be merged?

[ebihara99999]

I will check the PR in a few days:)

[Hirurg103]

Hi @ebihara99999 , thank you!

[ebihara99999]

@Hirurg103
The codebase looks good to me, but as for tests, seems to have to have some modification.
Controller layer tests exist in spec/controllers/controller_oauth2_spec.rb and change there if necessary.
However you change the initilizer to set an api version, only where you need testing in the PR. If need testing, test whether to set and get the api version in the model layer specs in spec/shared_examples/user_oauth_shared_examples.rb.
I can't make up my mind whether to test, looking over similar PRs, and I found this: #50. Following this, no test needed because it's minor change and has little probability to cause regression.

Anyway I think spec/providers/vk_spec.rb isn't needed ( but thanks for adding tests) . Would you remove it?

[Hirurg103]

Hi @ebihara99999

Controller layer tests exist in spec/controllers/controller_oauth2_spec.rb and change there if necessary.

The change relates to the providers/vk model so I added providers/vk_spec to make tests more modular instead of putting test cases for different providers into spec/controllers/controller_oauth2_spec.rb

As I see there is an example which checks that facebook Graph API version is being passed correctly when calling facebook API - see controller_oauth2_spec.rb#L94-L99. In the future we could refactor that controller spec to test each provider individually - I've found another bug with Facebook provider which relates to loading user avatar - will report it later

Either way I can remove that vk provider spec if you insist

[Hirurg103]

Hi @ebihara99999, do I need to remove the tests to get this work merged?

[ebihara99999]

Would you wait for the answer in a few days? I’m somehow busy now...

[Hirurg103]

Hello @ebihara99999 yes, of course

[Hirurg103]

Hi @ebihara99999 maybe athix can look at this PR if you are busy?

[ebihara99999]

@Hirurg103
As for tests It’s the best way to test just the difference where you change or add in one PR. At this viewpoint, you change the version parameter, but the test are not tested only there. I think tests should be refactored too, but it’s done in another PR.
Would you remove specs? And the members will move on to merge.

[Hirurg103]

Hi @ebihara99999

As for tests It’s the best way to test just the difference where you change or add in one PR. At this viewpoint, you change the version parameter, but the test are not tested only there

I do not understand your point. My test case sets the version parameter in the config and checks that vk API gets called with that version when Vk#process_callback is invoked

[ebihara99999]

Then it should be like these examples ( just an example, should not be all the same ). Yours are wider. Focus on what you need to test :)

[Hirurg103]

Hi @ebihara99999

Then it should be like these examples ( just an example, should not be all the same )

I do not see much value in those examples. These tests are like setting a variable and then checking that is set correctly, like:

bar = 1
expect(bar).to eq 1

In my tests I set vk version in the config and then check that VK API is being actually called with that version when we request user info

[joshbuker]

I tested this locally and the spec doesn't fail even without online access, which satisfies my only worry with this spec (if it used real API calls, it could be flaky and fail if offline/rate limited).

I'd have to dig a little deeper to fully understand what it's doing, but I don't see any issues merging it in for now. Any additional testing is a welcome addition.

[joshbuker]

Spec fails if the param is removed, as expected. Still a little rough on what's going on under the hood with the stubbing, but I'm comfortable merging this.

@ebihara99999 if you want to open another PR with how you'd like the specs to look and discuss it with @Hirurg103 and myself, I would appreciate it. There's a lot of ways to do testing, so I'd like to make sure we have the optimal solution here.

[ebihara99999]

Thanks for dealing with that!

[Ch4s3]

@athix and @ebihara99999 Thanks for handling this!