search

mozilla / django-browserid

Django application for adding BrowserID support.
Mozilla Public License 2.0
179 stars 80 forks source link

Add browserid_extra to BrowserIDBackend.authenticate #111

Closed prasoon2211 closed 11 years ago

prasoon2211 commented 11 years ago

Added an optional keyword argument to BrowserIDBackend.authenticate, called browserid_extra that is passed to verify as extra_params instead of passing all the kwargs to verify directly.

Concerned code : Line 65, 77 of auth.py

prasoon2211 commented 11 years ago

The earlier commit is useless. See the second commit for changes.

Osmose commented 11 years ago

The patch looks good, and we can compress the commits into a single one later.

There's only one thing missing here: tests. I can handle those if you want, or I can help you handle them.

First, we'd need to fix the test failure seen at https://travis-ci.org/mozilla/django-browserid/jobs/4017949 by changing test_backend_verify to expect None instead of {} for extra_params.

Then, we should edit https://github.com/mozilla/django-browserid/blob/master/django_browserid/tests/test_auth.py with a few changes:

  1. Update BrowserIDBackendTests.auth to accept a browserid_extra argument and pass it along to authenticate.
  2. Add a new test to BrowserIDBackendTests that passes some value for browserid_extra and checks the verify function to see if it received that value when called (this will require mock, which is used elsewhere in that file if you need an example).

Normally I'd ask for documentation as well, but this feature isn't really official yet, as it was only used by addons.mozilla.org for some special functionality. It'll be mentioned in the changelog, though.

Let me know if you have more questions or just want me to handle the tests. Thanks!

prasoon2211 commented 11 years ago

I have not added the test. Please help me with that part.

Osmose commented 11 years ago

Oh man, sorry for the late comment! Github seems to have failed to send me notifications for this thread. :(

pdb doesn't work when running tests because the test runner captures all output (so if your test has some print statements, for example, they aren't shown when running the test, unless there's an error, in which case the test runner displays the exception and all output from the test). Since pdb requires user input and output, it just freezes.

An easy way around this is to install ipdb, a variant of pdb that uses IPython as the interpreter. Not only is this more useful in general, ipdb is able to be run in the middle of a test!

I'll also look at your code and see if I can spot the issue. :)

Osmose commented 11 years ago

Beyond the issues I mentioned, this looks good! Fix them up and I'll squash and merge. Nice work!

prasoon2211 commented 11 years ago

Thank you for all you help. I was beginning to wonder why hadn't you replied but then I thought that you might just be busy. Anyway, tests seem to be working now. Thanks again!

Osmose commented 11 years ago

Merged in bc284f7502fcd91cd9b102deb2b0c7407997b8f4, nice work! Thanks! :cake: