Skip to content
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

Fix client fetch_token throwing NoMethodError when raise_errors is set to false #460

Conversation

joaolrpaulo
Copy link
Contributor

The problem occurs when the identifier service is unreachable and we are passing the :raise_errors option set to false.

The fetch_token method throws NoMethodError while trying to perform the following code:

response.parsed.merge(access_token_opts)

This happens because the response.parsed is nil.

…s set to false

The problem occurs when the identifier service is unreachable and we are passing the :raise_errors option set to false.

The `fetch_token` method throws `NoMethodError` while trying to perform the following code:

`response.parsed.merge(access_token_opts)`

This happens because the `response.parsed` is `nil`.
@joaolrpaulo
Copy link
Contributor Author

Related to opened issue: #459

@coveralls
Copy link

coveralls commented Jul 9, 2019

Pull Request Test Coverage Report for Build 937

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at ?%

Totals Coverage Status
Change from base Build 847: 0.0%
Covered Lines:
Relevant Lines: 0

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 848

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at ?%

Totals Coverage Status
Change from base Build 847: 0.0%
Covered Lines:
Relevant Lines: 0

💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 848

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at ?%

Totals Coverage Status
Change from base Build 847: 0.0%
Covered Lines:
Relevant Lines: 0

💛 - Coveralls

Copy link
Member

@pboling pboling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is excellent. If you have time to think about a solution, please do! Otherwise one of the maintainers may look at this eventually. We're all short on time, and long on commitments, so this project is primarily running on community effort.

@pboling pboling added this to the 2.0.0 milestone Jul 20, 2019
@pboling pboling self-assigned this Jul 20, 2019
@pboling pboling added the bug label Jul 20, 2019
@joaolrpaulo
Copy link
Contributor Author

Hello @pboling, I was really short on time to contribute to this PR. But here you go.

My suggestion to this is to return nil when there is now way we can build the access token, which is the saying the same that !response_contains_token.

Cheers mate!

@pboling pboling merged commit 3bef9fe into oauth-xx:master Nov 19, 2019
@pboling
Copy link
Member

pboling commented Nov 19, 2019

Thanks @joaolrpaulo !!

@joaolrpaulo
Copy link
Contributor Author

Hello @pboling, is there a possibility to add this to a new patch release in order for we to use it ?

@pboling
Copy link
Member

pboling commented Dec 5, 2019

@joaolrpaulo it would have to be backported to the 1-4-stable branch, as a second PR.

@joaolrpaulo
Copy link
Contributor Author

I will do it in the next few days 🙏

@pboling pboling added the in Changelog Has been added to Changelog label Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug in Changelog Has been added to Changelog
Projects
None yet
3 participants