#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/
Change History (23)
comment:1 by , 11 years ago
comment:2 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:3 by , 11 years ago
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 by , 11 years ago
Keywords: | js added |
---|---|
Milestone: | → 0.4.0 |
Resolution: | fixed |
Status: | closed → reopened |
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 by , 11 years ago
Keywords: | patch added |
---|---|
Owner: | removed |
Status: | reopened → accepted |
comment:6 by , 11 years ago
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 by , 11 years ago
Okay. So.
- Good that my patch is the right thing. So we have way to fix the problem at hand.
- 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).
- 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 by , 11 years ago
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 by , 11 years ago
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:
- 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.
- 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.
comment:10 by , 11 years ago
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 by , 11 years ago
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 by , 11 years ago
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:14 by , 11 years ago
Owner: | set to |
---|---|
Status: | accepted → in_progress |
So LotusEcho is working on functional testing as part of her summer internship, and will create a test for this :)
comment:15 by , 11 years ago
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 by , 11 years ago
Owner: | removed |
---|---|
Status: | in_progress → review |
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 by , 11 years ago
Owner: | set to |
---|---|
Status: | review → in_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 by , 11 years ago
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 by , 11 years ago
Milestone: | 0.5.0 → 0.6.0 |
---|
comment:20 by , 11 years ago
Milestone: | 0.6.0 → 0.7.0 |
---|
comment:21 by , 11 years ago
Keywords: | test added |
---|
comment:22 by , 10 years ago
Owner: | removed |
---|---|
Resolution: | → worksforme |
Status: | in_progress → closed |
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 by , 10 years ago
Milestone: | 0.7.0 |
---|
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.