search

mozilla / django-browserid

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

Issue #167 #209

Closed pramodhkp closed 10 years ago

pramodhkp commented 10 years ago

Pull request for Issue #167

Osmose commented 10 years ago

Thanks for the PR! It's really close, just gotta move the code a bit.

Could you also include some tests for success_url when you update it? One for when it is able to be reversed, and one when it isn't (you can use mock.patch to mock out reverse and force it to return a string or raise an exception in your test). Let me know if you want an example to see what I mean.

Once that's done I can merge the change. :)

Osmose commented 10 years ago

The functionality looks great now! All that's left is the tests. Again, let me know if you want help on the tests, I'm happy to provide it.

Osmose commented 10 years ago

@pramodhkp How is it going? Do you need any help on wrapping up this PR?

pramodhkp commented 10 years ago

Hi, I have my finals going on. Will get back to this in about a week or so.

On Tue, Dec 10, 2013 at 3:55 AM, Michael Kelly notifications@github.comwrote:

@pramodhkp https://github.com/pramodhkp How is it going? Do you need any help on wrapping up this PR?

— Reply to this email directly or view it on GitHubhttps://github.com/mozilla/django-browserid/pull/209#issuecomment-30180298 .

pramodhkp commented 10 years ago

Hi, Did you check the commit? Is it correct? Otherwise could you help by providing a snippet of code, please?

On Wed, Dec 11, 2013 at 7:28 PM, pramodh kp prmdh1@gmail.com wrote:

Hi, I have my finals going on. Will get back to this in about a week or so.

On Tue, Dec 10, 2013 at 3:55 AM, Michael Kelly notifications@github.comwrote:

@pramodhkp https://github.com/pramodhkp How is it going? Do you need any help on wrapping up this PR?

— Reply to this email directly or view it on GitHubhttps://github.com/mozilla/django-browserid/pull/209#issuecomment-30180298 .

Osmose commented 10 years ago

I skimmed over it the other day, and I think it's mergable, I might add like one or two tiny tweaks. I'm a bit fuzzy on what I wanted to change, though, and I'm busy with another side project (taking advantage of the holidays!). I'll try to get this merged within the next week once Christmas is over and I pay some more attention to django-browserid.

Thanks for the fixes!

Osmose commented 10 years ago

So far what we've learned is that I am a dirty liar when it comes to dates on when I'll do stuff.

(I blame Final Fantasy 14, personally.)

I was reading through this and making the tweaks I mentioned, when it occurred to me that we don't need to have all of this named URL support anyway, because users can just use django.core.urlresolvers.reverse_lazy directly in their settings files. This removes complexity on our side and makes it easier for users to diagnose issues when they use an incorrect URL name.

Unfortunately, that means that we won't be accepting this PR anymore. :( It's good work though, and I really appreciate the time you put into it, @pramodhkp . Thanks!