Bug 29458 - Show login button consistently in relation to login instructions, reset and register links
Summary: Show login button consistently in relation to login instructions, reset and r...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks: 31064
  Show dependency treegraph
 
Reported: 2021-11-11 11:02 UTC by Marcel de Rooy
Modified: 2023-03-15 01:25 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00


Attachments
Bug 29458: Reorder login elements (5.11 KB, patch)
2021-11-11 13:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29458: Reorder login elements (5.13 KB, patch)
2021-11-19 07:57 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 29458: Reorder login elements (5.20 KB, patch)
2022-01-10 09:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29458: (follow-up) Remove FIXME (1.76 KB, patch)
2022-01-19 13:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29458: (follow-up) Remove FIXME (1.82 KB, patch)
2022-01-19 14:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29458: Reorder login elements (5.41 KB, patch)
2022-01-19 15:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29458: Fix selenium test (2.13 KB, patch)
2022-01-31 16:02 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2021-11-11 11:02:27 UTC
Modal form from masthead:
Usercode Password Instructions Reset Register Login

opac-auth (click Login from home page):
Usercode Password Login Reset Instructions Register

opac-main:
Usercode Password Login Instructions Reset Register

I would suggest to follow the pattern from the home page. After entering usercode and password, next step is normally click Login. Below that offer Instructions, Reset and Register.
Do the same on modal and opac-auth.
Comment 1 Marcel de Rooy 2021-11-11 13:53:53 UTC
Created attachment 127539 [details] [review]
Bug 29458: Reorder login elements

Make masthead.inc modal and opac-auth follow the order of the login
elements on the homepage.

Test plan:
Enable prefs for reset password and self registration.
Fill additional contents for login instructions.
Logout on OPAC.
Click Login from your account on top bar.
Click Login button on the home page (right side, in the middle).
Verify that these two forms correspond with the home page login.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Fridolin Somers 2021-11-19 07:57:43 UTC
Created attachment 127828 [details] [review]
Bug 29458: Reorder login elements

Make masthead.inc modal and opac-auth follow the order of the login
elements on the homepage.

Test plan:
Enable prefs for reset password and self registration.
Fill additional contents for login instructions.
Logout on OPAC.
Click Login from your account on top bar.
Click Login button on the home page (right side, in the middle).
Verify that these two forms correspond with the home page login.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 3 Fridolin Somers 2021-11-19 07:59:43 UTC
Looks way better :D

In opac-auth, with should also fix :
  You may register here.
instead of
  Don't have an account? Register here.

Maybe in another bug.
Comment 4 Marcel de Rooy 2021-12-17 09:35:50 UTC
(In reply to Fridolin Somers from comment #3)
> Looks way better :D
> 
> In opac-auth, with should also fix :
>   You may register here.
> instead of
>   Don't have an account? Register here.
> 
> Maybe in another bug.

Please open another bug. Can we get this one further please ?
Comment 5 Fridolin Somers 2021-12-17 21:16:38 UTC
I opened Bug 29730
Comment 6 Tomás Cohen Arazi 2022-01-07 12:36:31 UTC
Please rebase. It like it but doesn't apply.
Comment 7 Marcel de Rooy 2022-01-10 09:31:16 UTC
Created attachment 129240 [details] [review]
Bug 29458: Reorder login elements

Make masthead.inc modal and opac-auth follow the order of the login
elements on the homepage.

Test plan:
Enable prefs for reset password and self registration.
Fill additional contents for login instructions.
Logout on OPAC.
Click Login from your account on top bar.
Click Login button on the home page (right side, in the middle).
Verify that these two forms correspond with the home page login.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 8 Marcel de Rooy 2022-01-10 09:31:37 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> Please rebase. It like it but doesn't apply.

Easy fix :)
Comment 9 Jonathan Druart 2022-01-19 12:34:53 UTC
+                                <div class="">[%# FIXME Add right class, remove br %]
+                                    <br>

Easy fixme, should be implemented here.
Comment 10 Marcel de Rooy 2022-01-19 13:16:03 UTC
Created attachment 129606 [details] [review]
Bug 29458: (follow-up) Remove FIXME

Actually, just leaving the br tag is fine too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2022-01-19 13:17:14 UTC
Owen, could you have a look at the second patch? Would you suggest to add a class and additional css or so instead?
Comment 12 Marcel de Rooy 2022-01-19 14:31:24 UTC
Created attachment 129612 [details] [review]
Bug 29458: (follow-up) Remove FIXME

With some help of Owen Leonard:
<oleonard> Regarding the <br/>, you can wrap the submit button in <fieldset class="action"></fieldset> instead.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2022-01-19 14:32:06 UTC
(In reply to Jonathan Druart from comment #9)
> +                                <div class="">[%# FIXME Add right class,
> remove br %]
> +                                    <br>
> 
> Easy fixme, should be implemented here.

Fixed now with help from Owen.
Comment 14 Jonathan Druart 2022-01-19 15:47:00 UTC
Created attachment 129617 [details] [review]
Bug 29458: Reorder login elements

Make masthead.inc modal and opac-auth follow the order of the login
elements on the homepage.

Test plan:
Enable prefs for reset password and self registration.
Fill additional contents for login instructions.
Logout on OPAC.
Click Login from your account on top bar.
Click Login button on the home page (right side, in the middle).
Verify that these two forms correspond with the home page login.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Bug 29458: (follow-up) Remove FIXME

With some help of Owen Leonard:
<oleonard> Regarding the <br/>, you can wrap the submit button in <fieldset class="action"></fieldset> instead.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Fridolin Somers 2022-01-28 21:12:58 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 16 Jonathan Druart 2022-01-31 11:38:43 UTC
Selenium authentication tests are failing because of this change:
Error while executing command: element not interactable: Element <input class="btn btn-primary" type="submit"> could not be scrolled into view at /usr/local/share/perl/5.32.1/Selenium/Remote/Driver.pm line 411.
 at /usr/local/share/perl/5.32.1/Selenium/Remote/Driver.pm line 356.
Comment 17 Marcel de Rooy 2022-01-31 12:30:47 UTC
(In reply to Jonathan Druart from comment #16)
> Selenium authentication tests are failing because of this change:
> Error while executing command: element not interactable: Element <input
> class="btn btn-primary" type="submit"> could not be scrolled into view at
> /usr/local/share/perl/5.32.1/Selenium/Remote/Driver.pm line 411.
>  at /usr/local/share/perl/5.32.1/Selenium/Remote/Driver.pm line 356.

Which test specifically ?
Comment 18 Marcel de Rooy 2022-01-31 13:16:26 UTC
(In reply to Marcel de Rooy from comment #17)
> (In reply to Jonathan Druart from comment #16)
> > Selenium authentication tests are failing because of this change:
> > Error while executing command: element not interactable: Element <input
> > class="btn btn-primary" type="submit"> could not be scrolled into view at
> > /usr/local/share/perl/5.32.1/Selenium/Remote/Driver.pm line 411.
> >  at /usr/local/share/perl/5.32.1/Selenium/Remote/Driver.pm line 356.
> 
> Which test specifically ?

I have spent some time on setting up Selenium, and all its prerequisites within a Docker container. But I dont really have time for getting that working now.
This sounds like a simple change required, but I cant provide or test it.
Comment 19 Jonathan Druart 2022-01-31 16:02:12 UTC
Created attachment 130016 [details] [review]
Bug 29458: Fix selenium test

The authentication.t selenium tests (and a couple of others) were
failing with:
Error while executing command: element not interactable: Element <input class="btn btn-primary" type="submit"> could not be scrolled into view at /usr/local/share/perl/5.32.1/Selenium/Remote/Driver.pm line 411. at /usr/local/share/perl/5.32.1/Selenium/Remote/Driver.pm line 356.

We changed the other of the form, and t::lib::Selenium::submit_form was
not getting the correct (first) form. The one from the auth modal was
retrieved and submit button was clicked. Selenium raised an error as it
is not displayed.
The ->is_displayed selenium method does not work, as per the doc
"""
Note: This does *not* tell you an element's 'visibility' property; as it still takes up space in the DOM and is therefore considered 'displayed'.
"""
https://metacpan.org/pod/Selenium::Remote::WebElement#is_displayed

"The internet" is saying we should be able to use the following in our
xpath expression: not(ancestor::div[contains(@style,'display:none')]
but it actually only works if the display:none rule is defined on the
node (not from .css). Which does not work for us.

The only solution I found is to check for the size of the element, which
is (0,0) if not effectively displayed.
Comment 20 Jonathan Druart 2022-01-31 16:06:09 UTC
I am getting another selenium failure locally, with and without the patch. Not sure it's related to this, Jenkins has it passing.
 
#   at t/db_dependent/selenium/update_child_to_adult.t line 148.
Cannot wait more for element '//input[@type="submit"]' to be visible at /kohadevbox/koha/t/lib/Selenium.pm line 189.
Comment 21 Marcel de Rooy 2022-02-01 12:10:32 UTC
(In reply to Jonathan Druart from comment #19)
> Created attachment 130016 [details] [review] [review]
> Bug 29458: Fix selenium test
> 
> The authentication.t selenium tests (and a couple of others) were
> failing with:
> Error while executing command: element not interactable: Element <input
> class="btn btn-primary" type="submit"> could not be scrolled into view at
> /usr/local/share/perl/5.32.1/Selenium/Remote/Driver.pm line 411. at
> /usr/local/share/perl/5.32.1/Selenium/Remote/Driver.pm line 356.
> 
> We changed the other of the form, and t::lib::Selenium::submit_form was
> not getting the correct (first) form. The one from the auth modal was
> retrieved and submit button was clicked. Selenium raised an error as it
> is not displayed.
> The ->is_displayed selenium method does not work, as per the doc
> """
> Note: This does *not* tell you an element's 'visibility' property; as it
> still takes up space in the DOM and is therefore considered 'displayed'.
> """
> https://metacpan.org/pod/Selenium::Remote::WebElement#is_displayed
> 
> "The internet" is saying we should be able to use the following in our
> xpath expression: not(ancestor::div[contains(@style,'display:none')]
> but it actually only works if the display:none rule is defined on the
> node (not from .css). Which does not work for us.
> 
> The only solution I found is to check for the size of the element, which
> is (0,0) if not effectively displayed.

Thanks. Too bad that we need such workarounds.
Comment 22 Marcel de Rooy 2022-02-01 12:10:55 UTC
Change status to ?
Comment 23 Jonathan Druart 2022-02-01 13:54:16 UTC
(In reply to Marcel de Rooy from comment #22)
> Change status to ?

No need to, there is the additional_work_needed keyword.
Comment 24 Fridolin Somers 2022-02-11 18:44:21 UTC
> Bug 29458: Fix selenium test
Pushed to master
Comment 25 wainuiwitikapark 2023-03-15 01:25:06 UTC
Not backported to 21.05.x