cosmir / openmic-annotator

Annotation framework for annotating data for OpenMIC
MIT License
56 stars 1 forks source link

Issue 9 -- OAuth login #42

Closed ejhumphrey closed 7 years ago

ejhumphrey commented 7 years ago

This PR implements Google & Spotify OAuth handshaking for properly credentialed applications through an abstraction layer, and attempts to document how others can do the same.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 90.909% when pulling f793110b8026bc2c5c2f3813f223b72a31dcf3d4 on ejh_20161229_iss9_oauth_login into b74a454f37bae9e876ef7750074a97e3e82da607 on master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 90.909% when pulling f793110b8026bc2c5c2f3813f223b72a31dcf3d4 on ejh_20161229_iss9_oauth_login into b74a454f37bae9e876ef7750074a97e3e82da607 on master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.4%) to 90.558% when pulling 42ae35147d266cfcaa7945f98c81d017d9c59010 on ejh_20161229_iss9_oauth_login into b74a454f37bae9e876ef7750074a97e3e82da607 on master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.4%) to 90.558% when pulling 42ae35147d266cfcaa7945f98c81d017d9c59010 on ejh_20161229_iss9_oauth_login into b74a454f37bae9e876ef7750074a97e3e82da607 on master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.4%) to 90.558% when pulling cadee353dd8819aa73771675dac88a5a954480a5 on ejh_20161229_iss9_oauth_login into b74a454f37bae9e876ef7750074a97e3e82da607 on master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.08%) to 90.987% when pulling 01d8dfa0f240ba2c976b2f70b309598953c64e44 on ejh_20161229_iss9_oauth_login into b74a454f37bae9e876ef7750074a97e3e82da607 on master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.08%) to 90.987% when pulling 01d8dfa0f240ba2c976b2f70b309598953c64e44 on ejh_20161229_iss9_oauth_login into b74a454f37bae9e876ef7750074a97e3e82da607 on master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.08%) to 90.987% when pulling 01d8dfa0f240ba2c976b2f70b309598953c64e44 on ejh_20161229_iss9_oauth_login into b74a454f37bae9e876ef7750074a97e3e82da607 on master.

ejhumphrey commented 7 years ago

alrighty, I think this is ready for a final pass:

paging @bmcfee 😉

bmcfee commented 7 years ago

alrighty, I think this is ready for a final pass:

Looks like there are still a bunch of outstanding comments in my previous review?

ejhumphrey commented 7 years ago

sorry, all comments have been addressed on recent commits, updated each with some kind of ack., lemme know how it currently looks, and I'll take a look at the request.args declaration.

ejhumphrey commented 7 years ago

weird, looks stale to me, but I'm on my phone... will investigate further shortly.

On Mon, Jan 16, 2017 at 11:21 AM Brian McFee notifications@github.com wrote:

@bmcfee commented on this pull request.


In backend_server/main.py https://github.com/cosmir/open-mic/pull/42:

CORS(app)

Set the cloud backend

CLOUD_CONFIG = os.path.join(os.path.dirname(file), '.config.json')

app.config['cloud'] = json.load(open(CLOUD_CONFIG))

+OAUTH_CONFIG = os.path.join(os.path.dirname(file), 'configs', 'oauth.yaml')

+app.config['oauth'] = yaml.load(open(OAUTH_CONFIG))

what about context managers on the file opens?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/cosmir/open-mic/pull/42, or mute the thread https://github.com/notifications/unsubscribe-auth/AA4iq_jIblTnY-QUW6-Rosvhhmn7aGXTks5rS5khgaJpZM4LaGUk .

bmcfee commented 7 years ago

weird, looks stale to me, but I'm on my phone... will investigate further shortly.

Annoyingly, it gives the latest version if i click on the "view changes" link instead of the "review" link. I added some comments.

ejhumphrey commented 7 years ago

thx / sorry for the troubles, willfix tonight

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.4%) to 89.54% when pulling 48e5f00bd1e9aeef624e1947fdf59a1cbfa5936f on ejh_20161229_iss9_oauth_login into b74a454f37bae9e876ef7750074a97e3e82da607 on master.

ejhumphrey commented 7 years ago

@bmcfee can haz another look? also, I think we shouldn't worry about the drop in coverage ... OAuth adds a bunch of machinery that can only talk to real web services, and I'm not sure there's a meaningful way to test them (or that it's worth the time right now).

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.4%) to 89.54% when pulling 52dbb46166f33ecb5b4a0f90da522b80a87cac15 on ejh_20161229_iss9_oauth_login into b74a454f37bae9e876ef7750074a97e3e82da607 on master.

bmcfee commented 7 years ago

think we shouldn't worry about the drop in coverage ... OAuth adds a bunch of machinery that can only talk to real web services, and I'm not sure there's a meaningful way to test them (or that it's worth the time right now).

This is what mocking is for. This thread has some notes on how to do it.

Otherwise, I'll try to look at this one tomorrow.

ejhumphrey commented 7 years ago

merging now, adding issues for mocking (#46) and refactoring (#47)