Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#689 closed defect (cant-reproduce)

forgot password is broken

Reported by: bukosabino Owned by:
Priority: minor Milestone:
Component: programming Keywords: js patch, test
Cc: Parent Tickets:

Description

If you enable javascript in your browser the link is broken.

You can work if you write the url http://127.0.0.1:6543/auth/forgot_password/

Subtickets

Change History (23)

comment:1 Changed 7 years ago by bukosabino

it's resolved.

event "click" was associated to tag <a id="forgot_password">, which did not work good.

I removed, because it really is not necessary.

comment:2 Changed 7 years ago by bukosabino

Resolution: fixed
Status: newclosed

comment:3 Changed 7 years ago by bukosabino

sorry, the address:
/mediagoblin/static/js/autofilledin_password.js

We can to notify to live instances. They have this problem.
http://wiki.mediagoblin.org/Live_instances

comment:4 Changed 7 years ago by Elrond

Keywords: js added
Milestone: 0.4.0
Resolution: fixed
Status: closedreopened

It's not fixed, at least not for me. So re-opening.

Go to /, then cklick "Log in", then click "Forget Password".

The following works for me. But I'm not sure on js things. And we don't have a unit test for this.

--- a/mediagoblin/static/js/autofilledin_password.js
+++ b/mediagoblin/static/js/autofilledin_password.js
@@ -19,7 +19,7 @@
 $(document).ready(function(){
   $('#forgot_password').click(function(event){
     event.preventDefault();
-    window.location.pathname = $(this).attr('href') + '?username=' +
+    window.location = $(this).attr('href') + '?username=' +
                                $('#username').val();
   });
 });

comment:5 Changed 7 years ago by Elrond

Keywords: patch added
Owner: bukosabino deleted
Status: reopenedaccepted

comment:6 Changed 7 years ago by Jakob Kramer

Your patch makes sense! Please commit this. :) pathname is wrong when you put the whole URL into it. Firefox just did what I expected, so I thought it was correct, but other browsers behave correctly.

comment:7 Changed 7 years ago by Elrond

Okay. So.

  1. Good that my patch is the right thing. So we have way to fix the problem at hand.
  2. I would like to not apply this patch NOW, so that we can have a failing unit/functional test written first (that's the target of this bug now).
  3. If (badly enough likely) we don't get the unit test before the next release, we'll apply this patch a week before the next release and keep the bug open to create the unit test.

Does that make sense?

comment:8 Changed 7 years ago by Gabriela Thumé

In ticket #458 we are talking about using Selenium to functional tests. Because of that I tried to create one test based on this broken link of forgot password at https://gist.github.com/GabiThume/5623233
I am not sure about it because I am not getting the same error of broken link, so I made a comparison between the current link and "http://127.0.0.1:6543/auth/forgot_password/?username". Is that right?

comment:9 Changed 7 years ago by rodney757

So the reason it is passing is because this isn't a bug in firefox. It is a bug in chrome and safari, maybe others.

With your test:

  • I think it would be best to go directly to /auth/login/.
  • There are at least 2 ways to go about this:
  1. Click on the link and test that the status code is 200. I'm not sure how easy it is to do this with selenium.
  1. Fill in the username in the login form, and then click the forgot password link. Then: assert driver.current_url == u'http://127.0.0.1:6543/auth/forgot_password/%3Fusername=user' where user is the username you entered into the login form.
Last edited 7 years ago by rodney757 (previous) (diff)

comment:10 Changed 7 years ago by Jakob Kramer

No, that you can put a HREF into window.location.pathname and it puts you to that HREF is a bug in Firefox. The other browsers behave correctly.

comment:11 Changed 7 years ago by Gabriela Thumé

I updated the test in https://gist.github.com/GabiThume/5623233. Now Firefox passes and Chrome not, as expected.

Unfortunately Selenium does not support http request status code. There is a workaround (https://coderwall.com/p/aob6ha) if we want to get this status.

Should I also test having the login form filled?

BTW I am thinking about creating a function test for each browser, so we can select which one we want to test. What this sounds like?

comment:12 Changed 7 years ago by rodney757

That workaround, won't catch the bug because the problem is with the js not the /auth/forgot_password page.

After looking at it again, I don't think it is necessary to actually fill in the login form, but with your test you need to include an '=' at the end of the assert url.

I'm not sure about the try, except thing with the assert statement. Will the test still fail w/o being run with the -rx option?

That's all I can help you with as I'm not really sure where we are going with selenium testing. Either ask someone on irc or change the status of this bug to review if you think that it's ready for code review.

comment:13 Changed 7 years ago by Christopher Allan Webber

Milestone: 0.4.00.4.1

Moving to 0.4.1

comment:14 Changed 7 years ago by rodney757

Owner: set to Emily O'Leary
Status: acceptedin_progress

So LotusEcho is working on functional testing as part of her summer internship, and will create a test for this :)

comment:15 Changed 7 years ago by Emily O'Leary

I've made a test for this and modified it to work under our current testing framework. I just need to set something up so that the server can be started temporarily to run these tests if Selenium is installed.

I use pytests skip function to skip the Selenium tests if it is either not installed or it's version is deprecated. I'll create a new branch once I have that portion working well.

comment:16 Changed 7 years ago by Emily O'Leary

Owner: Emily O'Leary deleted
Status: in_progressreview

Here's a feature branch to add selenium testing to GMG as well as a test case for this particular issue:

https://gitorious.org/~lotusecho/mediagoblin/lotusechos-mediagoblin/commits/selenium_integration

comment:17 Changed 7 years ago by rodney757

Owner: set to Emily O'Leary
Status: reviewin_progress

So i didn't really review much of this, I just ran the tests. So the forgot password test should fail right now, since the we haven't implemented the fix. It passes in your test, because you use the firefox driver (see comment:9 and comment:10). Personally, I think it would be good to use both chrome and firefox drivers, but I'm not sure how cwebb feels about this.

Anyways, thanks for doing this.

comment:18 Changed 7 years ago by Emily O'Leary

We moved to a new repo for selenium tests (my version: https://gitorious.org/mediagoblin-automation/mediagoblin-selenium). I'm going to be writing a quick script so that it runs with all available drivers.

comment:19 Changed 7 years ago by Christopher Allan Webber

Milestone: 0.5.00.6.0

comment:20 Changed 7 years ago by Christopher Allan Webber

Milestone: 0.6.00.7.0

comment:21 Changed 7 years ago by ShawnRisk

Keywords: test added

comment:22 Changed 6 years ago by Christopher Allan Webber

Owner: Emily O'Leary deleted
Resolution: worksforme
Status: in_progressclosed

I just tested the forgot password link and it works fine. I'm not sure what's going on with this bug. I suspect that it's no longer true... feel free to reopen if true.

The functional testing thing is entirely something different. We never set up a permanent functional testing system, but the scaffolding is there, but that's a separate issue.

comment:23 Changed 6 years ago by Christopher Allan Webber

Milestone: 0.7.0
Note: See TracTickets for help on using tickets.