Fixed Wrong Tab Selected When Loggin Fails And Multiple Login Tabs Exists
What does this MR do?
Fixes issue: 15081 Wrong Tab Selected When Loggin Fails And Multiple Login Tabs Exists
Are there points in the code the reviewer needs to double check?
No
Why was this MR needed?
Screenshots (if relevant)
Does this MR meet the acceptance criteria?
-
CHANGELOG entry added -
Documentation created/updated -
API support added - Tests
-
Added for this feature/bug -
All builds are passing
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Branch has no merge conflicts with master(if it does - rebase it please) -
Squashed related commits together
What are the relevant issue numbers?
Closes #15081 (closed)
Merge request reports
Activity
Marked the task Conform by the merge request performance guides as completed
Marked the task Conform by the merge request performance guides as incomplete
/cc @ClemMakesApps
@brycepj can you take a look at this since you are more familiar with the login page?
Reassigned to @brycepj
@ClemMakesApps At first glance this seems like a needed fix, but I think a backend person would have more to say about it. @rspeicher ?
Reassigned to @rspeicher
Added 1 commit:
- 8fb0df6b - Fixed Wrong Tab Selected When Loggin Fails And Multiple Login Tabs Exists
-
@jacopo-beschi did you test the various possible configurations (crowd/ldap/signup enabled and disabled) of this manually? I know from recent experience that it can be easy to miss one.
In fact, it would probably be worth adding a few assertions to configuration-specific specs here: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/features/login_spec.rb#L219
Edited by Bryce Johnson 1 1 - if form_based_providers.any? 2 2 - if crowd_enabled? 3 .login-box.tab-pane.active{id: "crowd", role: 'tabpanel', class: 'tab-pane'} 3 .login-box.tab-pane{id: "crowd", role: 'tabpanel', class: (:active if ldap_active_tab == 'crowd')} -
@jacopo-beschi I know you didn't cause it, but since we're here, can we fix this to just use single-quotes?
(same thing for other instances/files in this diff)
Edited by Bryce Johnson @brycepj I've noticed that when you enable Ldap or Crowd (or Both) the signup tab is not present but the tabpanel is still present. My question is: you still want to show the signup tab and their panel in that case or I should just hide them both?
-
@jacopo-beschi You're right -- I hadn't noticed that. Thanks for pointing it out. Someone else noticed it as well and just submitted a fix: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7274#diff-0. Should be merged shortly.
Edited by Bryce Johnson
-
@jacopo-beschi it also looks like there are conflicts between this branch and master -- can you update yours?
Also @jacopo-beschi, we recently made some changes to how our changelog works (to minimize changelog conflicts). Can you revert CHANGELOG.md and follow the instructions in https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/changelog.md for our new changelog process. Apologies for the inconvenience
37 37 - Refactor email, use setter method instead AR callbacks for email attribute (Semyon Pupkov) 38 38 - Shortened merge request modal to let clipboard button not overlap 39 39 - In all filterable drop downs, put input field in focus only after load is complete (Ido @leibo) 40 - Fix wrong tab selected when loggin fails and multiple login tabs exists (@jacopo-beschi) We've moved away from editing
CHANGELOG.mddirectly in order to solve our merge conflict nightmare. Please see the new documentation for instructions on generating your own entry file.
I'm not sure I like handling this in a controller, the failure app, and a helper. Can we store their selected tab in a cookie whenever it changes and always select that tab when we render the form? That way it's all frontend, and if they sign out their previously-used tab will be selected then too.
Added 1 commit:
- c8adfae5 - Fixed Wrong Tab Selected When Loggin Fails And Multiple Login Tabs Exists
Added 1 commit:
- cf86bce7 - Fixed Wrong Tab Selected When Loggin Fails And Multiple Login Tabs Exists
@rspeicher Yes that's the other option I was thinking about. I think the trade-off is that you may have a flashing effect when changing tab on page load. If you prefer this way i can do another PR with that proposal aswell.
Edited by 🙈 jacopo beschi 🙉Added 298 commits:
-
cf86bce7...86b8fb4e - 297 commits from branch
gitlab-org:master - 98e991b9 - Fixed Wrong Tab Selected When Loggin Fails And Multiple Login Tabs Exists
-
cf86bce7...86b8fb4e - 297 commits from branch
@rspeicher Got a working solution with just JS/Cookies. I'll post it in a new PR
@rspeicher I've posted the working solution with just JS/Cookie (as you suggested) in the following PR: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7314 (if you prefer that one I'll close this one)
Mentioned in merge request !7314 (merged)
@rspeicher shall we close this MR then?
@ClemMakesApps Let's wait 'til one is merged, IMO.
Sounds good @rspeicher
Going to close this one since !7314 (merged) is progressing nicely. Thanks @jacopo-beschi!
