Fixes #2912 - Implement login functionality#2942
Conversation
There was a problem hiding this comment.
Sorry for the delay in review @munteanu210. This PR seems to have more changes than just login functionality -- it would be easier to understand if related changes were committed together.
Can you please investigate why we have a failing test?
firefox on linux 4.15.0-1043-aws - Reporting (non-auth) - Submit buttons are disabled (0.16s)
AssertionError: expected null to not equal null
@ tests/functional/reporting-non-auth.js:34:20
at Array.forEach @ anonymous
at Command.<anonymous> @ tests/functional/reporting-non-auth.js:33:18
at Test.Submit buttons are disabled [as test] @ tests/functional/reporting-non-auth.js:32:10
@ src/lib/Test.ts:263:51
We can't merge this in until it's passing.
|
Just thinking about the failing test here... we probably need a mechanism to run tests in both "a" and "b" modes. Right now, I believe it's serving "b" 100% of the time, so as markup changes, tests assuming an "a" DOM being tested against a "b" DOM will diverge. This is where it gets painful... possibly #2944 can help us solve this problem, if we export the right env var before running the test suite. |
|
I filed #2947 to tackle the test thing. |
|
@miketaylr I will address your initial comment below: This is a test that checks the submit buttons on the old form. Initially it must be disabled, which is verified by this test. Here is the code of the test. I don't know why this test failed, so as both buttons are disabled. In addition, I did not reach this part of the form. I don't know if it was a test execution error or something was broken there, but it's something that I did not touch for sure. |
| </head> | ||
| <body id="body-webcompat" data-username="{{ session.username }}"> | ||
| <body id="body-webcompat" | ||
| class="{%- if request.url_rule.endpoint == 'create_issue' and ab_active('exp') == 'form-v2' %}no-top-padding{% endif %}" |
There was a problem hiding this comment.
Keep in mind that this doesn't only change the new form. I would prefer if this wasn't fixed here but be contained in the codebase of the experiment.
| <script src="{{ url_for('static', filename='js/lib/navbar.js') }}"></script> | ||
| <script src="{{ url_for('static', filename='js/lib/autogrow-textfield.js') }}"></script> | ||
| <script src="{{ url_for('static', filename='js/lib/bugform-validation.js') }}"></script> | ||
| {%- if ab_active('exp') == 'form-v1' %} |
There was a problem hiding this comment.
This was omitted here on purpose. I would rather override only things for form-v2 version
There was a problem hiding this comment.
Also by reverting this the current website tests are not broken.
There was a problem hiding this comment.
Also keep in mind that not all the population would end up in an experiment branch (eg. private browsing etc)
There was a problem hiding this comment.
Also people that work on the project are exempt from the experiments.
There was a problem hiding this comment.
This might work better: johngian@975be04
Also keeps the tests happy
There was a problem hiding this comment.
@johngian thank you for the proposed commit. I committed your update and all tests passed successfully.
miketaylr
left a comment
miketaylr
left a comment
There was a problem hiding this comment.
I think everything has been addressed except for #2955, which will be taken care of shortly. Let's merge, thanks @munteanu210.
5 participants
This PR fixes issue #2912
Proposed PR background
This pull request contains the redesign for the login with GitHub and report anonymously on the bug form.