singingwolfboy / flask-dance

Doing the OAuth dance with style using Flask, requests, and oauthlib.
https://pypi.python.org/pypi/Flask-Dance/
MIT License
997 stars 156 forks source link

Adds base_url to allow different data center usage #407

Closed ajay-k closed 1 year ago

ajay-k commented 1 year ago

Gives the ability to change base_url, currently, it defaults to the US data center (https://api.nylas.com/) but customers in EU are unable to make requests since the EU data center is at (https://ireland.api.nylas.com)

daenney commented 1 year ago

This looks mostly good, though we'd need a test in https://github.com/singingwolfboy/flask-dance/blob/main/tests/contrib/test_nylas.py too to ensure the base_url passes through correctly.

You can add something like def test_blueprint_factory_base_url(make_app):, call the constructor with a value in base_url and assert that the right URLs are found in the base_url, token_url etc.

ajay-k commented 1 year ago

This looks mostly good, though we'd need a test in https://github.com/singingwolfboy/flask-dance/blob/main/tests/contrib/test_nylas.py too to ensure the base_url passes through correctly.

You can add something like def test_blueprint_factory_base_url(make_app):, call the constructor with a value in base_url and assert that the right URLs are found in the base_url, token_url etc.

@daenney Made these on this PR

daenney commented 1 year ago

This looks mostly good, though we'd need a test in https://github.com/singingwolfboy/flask-dance/blob/main/tests/contrib/test_nylas.py too to ensure the base_url passes through correctly. You can add something like def test_blueprint_factory_base_url(make_app):, call the constructor with a value in base_url and assert that the right URLs are found in the base_url, token_url etc.

@daenney Made these on this PR

Thanks, but we need the tests in this PR, not a separate one. Apologies for not being clearer on that. Otherwise they won't run here and validate the changes.

ajay-k commented 1 year ago

This looks mostly good, though we'd need a test in https://github.com/singingwolfboy/flask-dance/blob/main/tests/contrib/test_nylas.py too to ensure the base_url passes through correctly. You can add something like def test_blueprint_factory_base_url(make_app):, call the constructor with a value in base_url and assert that the right URLs are found in the base_url, token_url etc.

@daenney Made these on this PR

Thanks, but we need the tests in this PR, not a separate one. Apologies for not being clearer on that. Otherwise they won't run here and validate the changes.

Thanks for the help, I made the tests in this PR now

codecov[bot] commented 1 year ago

Codecov Report

Merging #407 (be0f45a) into main (847d81a) will decrease coverage by 0.70%. The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main     #407      +/-   ##
==========================================
- Coverage   93.53%   92.82%   -0.71%     
==========================================
  Files          36       37       +1     
  Lines        1067     1073       +6     
==========================================
- Hits          998      996       -2     
- Misses         69       77       +8     
Impacted Files Coverage Δ
flask_dance/contrib/nylas.py 52.94% <50.00%> (-47.06%) :arrow_down:
flask_dance/__init__.py 100.00% <0.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

ajay-k commented 1 year ago

@daenney When you get a chance, could you review the changes I made?

singingwolfboy commented 1 year ago

@ajay-k are you still working on this pull request?

singingwolfboy commented 1 year ago

@ajay-k are you still working on this pull request?

It's been two weeks, and @ajay-k has not responded. I will assume that the answer is no, and close this pull request.