Bug 35402 - Update the OPAC and staff interface to Bootstrap 5
Summary: Update the OPAC and staff interface to Bootstrap 5
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Owen Leonard
QA Contact: Martin Renvoize
URL:
Keywords: additional_work_needed
Depends on: 33526 37030
Blocks: 27063 28453 37732 37733 37734 37739 37740 37745 37748 37752 37753 37755 37759 37795 37807 37812 37833 37841 37859 37746
  Show dependency treegraph
 
Reported: 2023-11-27 11:38 UTC by Martin Renvoize
Modified: 2024-09-06 19:55 UTC (History)
20 users (show)

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


Attachments
Bug 35402: (follow-up) Change a checkin form from POST to GET (5.96 KB, patch)
2024-08-23 16:12 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35402: (follow-up) Use ~ to resolve to nearest node_modules when building css (15.50 KB, patch)
2024-08-26 11:38 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 35402: (follow-up) Reintroduce bsBanner includes (1.39 KB, patch)
2024-08-26 12:03 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 35402: (follow-up) Remove bootstrap version 4 and revert import paths to 'bootstrap' (11.36 KB, patch)
2024-08-26 12:03 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 35402: (follow-up) Remove duplicate of import in staff-global.scss (801 bytes, patch)
2024-08-26 12:09 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 35402: (follow-up) Use ~ to resolve to nearest node_modules when building css (15.59 KB, patch)
2024-08-26 12:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35402: (follow-up) Reintroduce bsBanner includes (1.46 KB, patch)
2024-08-26 12:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35402: (follow-up) Remove bootstrap version 4 and revert import paths to 'bootstrap' (11.44 KB, patch)
2024-08-26 12:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35402: (follow-up) Remove duplicate of import in staff-global.scss (876 bytes, patch)
2024-08-26 12:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35402: Fix some selenium tests (6.38 KB, patch)
2024-08-27 07:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35402: Fix some selenium tests (6.43 KB, patch)
2024-08-27 12:06 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35402: (follow-up) Fix regressions.t (1.15 KB, patch)
2024-08-29 17:53 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35402: (follow-up) Fix regressions.t (1.21 KB, patch)
2024-08-30 12:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35402: Fix regressions.t (1.70 KB, patch)
2024-09-03 09:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35402: Fix regressions.t (1.76 KB, patch)
2024-09-03 09:18 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2023-11-27 11:38:18 UTC
We're still using Bootstrap 4 which has been EOL for nearly 10 months (I hadn't spotted until someone drew my attention to it).

We'll need to try to update as soon as possible before an exploit is reported against it.
Comment 1 Owen Leonard 2024-01-09 13:08:18 UTC
I've looked at the scope of the changes required in the OPAC and staff interface and it looks like the OPAC--being on 4.5.2--should be much easier than the staff interface which is still 3.x.
Comment 2 Owen Leonard 2024-06-03 20:19:35 UTC
I have a work in progress branch here: https://gitlab.com/koha-dev/koha-dev/-/commits/Bootstrap5

It includes changes to both the OPAC and staff interface.

Does this bug really need to be in Security?
Comment 3 Martin Renvoize 2024-06-21 13:30:42 UTC
Lets drop the security flag..
Comment 4 Martin Renvoize 2024-07-10 09:41:43 UTC
How would you like feedback Owen?  In a gitlab merge request to get context or in here as comments?

It's generally looking good.. we have some slight oddities in the about page I notice around background colors and striping of tables though..

I think we should take the opportunity to convert those tables to `class="table table-striped"` and for the highlighted rows use `tr class="table-success"` and `tr class="table-warning"` etc and remove the `bg-success` on the td, spans.. I feel like those look too strong now in bootstrap 5.
Comment 5 Martin Renvoize 2024-07-10 09:46:59 UTC
My general feeling it that there's a number of little subtle things.. but an early push is the best way to spot them and have them fixed for release.

It's looking great really so far in my testing though.
Comment 6 Jake Deery 2024-07-17 17:00:47 UTC
Hello!

After the call earlier, here's my notes on what I've found (so far focused on the staff client)...

https://koha-hedgedoc.servers.llownd.net/s/g4KPjfLjF 

I'll have a look in anger at the OPAC tomorrow. Enjoy your evening, all!
Comment 7 Owen Leonard 2024-08-12 17:44:49 UTC
I'll go ahead and mark this "Needs signoff" since it's gone through one set of revisions. Now testing seems to have stalled!
Comment 8 Jake Deery 2024-08-20 13:51:07 UTC
Hello! I've just had another cursive look through this. I can only really see three issues:

Not really showstoppers:- 
*   Overflow upon editing notice content, made worse when expanding all accordions
    * http://localhost:8081/cgi-bin/koha/tools/letter.pl?op=add_form&branchcode=&module=members&code=ACCOUNTS_SUMMARY
*   Colour of pagination buttons doesn't match Koha
    * http://localhost:8080/cgi-bin/koha/opac-search.pl?idx=kw&q=history&offset=0&sort_by=relevance&count=20

Possible showstoppers (will need fixing before 24.11):- 
*   ERM Module is stylistically broken (chevrons everywhere, buttons wrong colour...)


* * * 


Overall, I think the branch is looking amazing! The sooner we merge it, the sooner we can fix the rest of the minor issues (and find more as people get eyes on).

Great work, Owen! Signing off.
Comment 9 Martin Renvoize 2024-08-22 11:09:42 UTC
I'm going to be bold and mark this PQA.

I've done a fair bit of testing and we've iterated a fair bit.

I'm never going to spot everything and I don't think we will until it's out there.

But I do think it's ready for the masses, the developer masses that is.

So lets go for it (Also agreed on chat.koha-community.org with Owen and Katrin that this is the best way forward at this stage.)

Great work guys..

PQA
Comment 10 David Cook 2024-08-22 23:10:43 UTC
(In reply to Martin Renvoize from comment #9)
> I'm never going to spot everything and I don't think we will until it's out
> there.
> 
> But I do think it's ready for the masses, the developer masses that is.

Makes sense to me! Exciting!
Comment 11 Katrin Fischer 2024-08-23 14:00:27 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 12 Owen Leonard 2024-08-23 16:12:27 UTC
Created attachment 170651 [details] [review]
Bug 35402: (follow-up) Change a checkin form from POST to GET

If you have AllowReturnToBranch enabled and you check in an item which
is disallowed by the preference (returned at the wrong branch, for
instance) You get a "Connot check in" modal.

Previously clicking the "OK" button in this form triggered JS which is
supposed to submit a form. However, all the button needs to do is
dismiss the modal. The form and the "approve" class on the button are
removed by this patch.

NOTE: This patch contains indentation changes.

To test, apply the patch and set the AllowReturnToBranch system
preference to "only at the library that the item is from."

- Locate an item belonging to another library and check it out to a
  patron.
- Check in the item.
- You should see a modal confirmation, "Cannot check in".
- Clicking the "OK" button should dismiss the modal window without
  triggering a reload of the page.
Comment 13 David Nind 2024-08-23 20:34:32 UTC
Should we create an omnibus bug for post Bootstrap 5 push issues?
Comment 14 Martin Renvoize 2024-08-24 06:48:07 UTC
I think I'd just mark bugs as depending on this one.  Then I this one acts to collect them all.  I've also been investigating different ways of grouping bugs in the dashboard views but I'm still thinking about how to use that more widely.  Right now it's only available for circulation component bugs via the kanban view option.
Comment 15 Katrin Fischer 2024-08-24 09:33:49 UTC
(In reply to Martin Renvoize from comment #14)
> I think I'd just mark bugs as depending on this one.  Then I this one acts
> to collect them all.  I've also been investigating different ways of
> grouping bugs in the dashboard views but I'm still thinking about how to use
> that more widely.  Right now it's only available for circulation component
> bugs via the kanban view option.

+1
Comment 16 Jonathan Druart 2024-08-26 09:49:40 UTC
There is something wrong for ktd users:

Error in plugin "sass"
Message:
    koha-tmpl/intranet-tmpl/prog/css/src/installer.scss
Error: Can't find stylesheet to import.
  ╷
3 │ @import "../../../../../node_modules/bootstrap5/scss/functions";
  │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  koha-tmpl/intranet-tmpl/prog/css/src/installer.scss 3:9  root stylesheet

You are importing only what is in /kohadevbox/koha/node_modules. But we are expecting ktd to have /kohadevbox/node_modules part of the image.

At the moment jenkins is failing because of this, `yarn css:build` does not succeed.
Comment 17 Paul Derscheid 2024-08-26 11:38:42 UTC
Created attachment 170709 [details] [review]
Bug 35402: (follow-up) Use ~ to resolve to nearest node_modules when building css

- This patch adds a new dev dependency node-sass-tilde-importer.
- We use the tilde to resolve to the nearest node_moduldes, meaning
	- /kohadevbox/koha/node_modules
	or
	- /kohadevbox/node_modules
- We explicitly prefix the last path component with an underscore to prevent the
  resolution from failing (this doesn't seem necessary for all cases, though)
Comment 18 Paul Derscheid 2024-08-26 12:03:53 UTC
Created attachment 170710 [details] [review]
Bug 35402: (follow-up) Reintroduce bsBanner includes

I initially removed this because it failed to build, but this issue seems to be resolved.
Comment 19 Paul Derscheid 2024-08-26 12:03:57 UTC
Created attachment 170711 [details] [review]
Bug 35402: (follow-up) Remove bootstrap version 4 and revert import paths to 'bootstrap'

This patch
- removes bootstrap 4 from the project dependencies.
- reverts the import paths for sass back to 'bootstrap' from 'bootstrap5'.
- updates yarn's lockfile.
Comment 20 Paul Derscheid 2024-08-26 12:09:42 UTC
Created attachment 170712 [details] [review]
Bug 35402: (follow-up) Remove duplicate of import in staff-global.scss
Comment 21 Jonathan Druart 2024-08-26 12:21:33 UTC
Created attachment 170714 [details] [review]
Bug 35402: (follow-up) Use ~ to resolve to nearest node_modules when building css

- This patch adds a new dev dependency node-sass-tilde-importer.
- We use the tilde to resolve to the nearest node_moduldes, meaning
	- /kohadevbox/koha/node_modules
	or
	- /kohadevbox/node_modules
- We explicitly prefix the last path component with an underscore to prevent the
  resolution from failing (this doesn't seem necessary for all cases, though)

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Jonathan Druart 2024-08-26 12:21:37 UTC
Created attachment 170715 [details] [review]
Bug 35402: (follow-up) Reintroduce bsBanner includes

I initially removed this because it failed to build, but this issue seems to be resolved.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Jonathan Druart 2024-08-26 12:21:41 UTC
Created attachment 170716 [details] [review]
Bug 35402: (follow-up) Remove bootstrap version 4 and revert import paths to 'bootstrap'

This patch
- removes bootstrap 4 from the project dependencies.
- reverts the import paths for sass back to 'bootstrap' from 'bootstrap5'.
- updates yarn's lockfile.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2024-08-26 12:21:45 UTC
Created attachment 170717 [details] [review]
Bug 35402: (follow-up) Remove duplicate of import in staff-global.scss

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Katrin Fischer 2024-08-26 15:44:42 UTC
Follow-ups pushed to main.
Comment 26 Katrin Fischer 2024-08-26 17:02:01 UTC
It looks like we got past the first hurdle, but now the Selenium tests highlight some changes in mark-up that make the tests fail:
	
Error while executing command: no such element: Unable to locate element: //div[@class="alert alert-info"] 
Error while executing command: no such element: Unable to locate element: //div[@id="registration-status-disabled"]/div[@class="two-factor-status"] 
  I think in this case it doesn't like the additional classes on the element, switch to use id?
  <div class="alert alert-info two-factor-status">Status: Disabled</div>
Error while executing command: no such element: Unable to locate element: //span[@id="audio-alert"]/audio[@src="/intranet-tmpl/prog/sound/beep.ogg"]
Comment 27 Jonathan Druart 2024-08-27 07:34:50 UTC
Created attachment 170745 [details] [review]
Bug 35402: Fix some selenium tests
Comment 28 Jonathan Druart 2024-08-27 07:38:47 UTC
(In reply to Jonathan Druart from comment #27)
> Created attachment 170745 [details] [review] [review]
> Bug 35402: Fix some selenium tests

This patch fixes the following tests:

t/db_dependent/selenium/administration_tasks.t 
t/db_dependent/selenium/authentication_2fa.t
t/db_dependent/selenium/patrons_search.t 

2 others are not fixed:

1. t/db_dependent/selenium/regressions.t .. 5/9 Error while executing command: no such element: Unable to locate element: //span[@id="audio-alert"]/audio[@src="/intranet-tmpl/prog/sound/beep.ogg"] at /usr/share/perl5/Selenium/Remote/Driver.pm line 411.

opening.ogg is played instead of beep.ogg (change in behaviour). Is that expected?
In my understanding it's happening because we know have a modal in the DOM with the class "audio-alert-action":
    <div id="circ-warnwaitingholds-modal" class="modal audio-alert-action block">

2. 
#   Failed test 'Update child to patron'
#   at t/db_dependent/selenium/update_child_to_adult.t line 153.
Error while executing command: unexpected alert open: Dismissed user prompt dialog: Are you sure you want to update this child to an Adult category? This cannot be undone. at /usr/share/perl5/Selenium/Remote/Driver.pm line 411.

This is happening because the "Update child to adult" item in the "More" menu (members module) is clickable. There is a tooltip and prior to this change the item was not clickable and we didn't get the alert.
Comment 29 Jonathan Druart 2024-08-27 07:44:37 UTC
(In reply to Jonathan Druart from comment #28)
> 2. 
> #   Failed test 'Update child to patron'
> #   at t/db_dependent/selenium/update_child_to_adult.t line 153.
> Error while executing command: unexpected alert open: Dismissed user prompt
> dialog: Are you sure you want to update this child to an Adult category?
> This cannot be undone. at /usr/share/perl5/Selenium/Remote/Driver.pm line
> 411.
> 
> This is happening because the "Update child to adult" item in the "More"
> menu (members module) is clickable. There is a tooltip and prior to this
> change the item was not clickable and we didn't get the alert.

Related to this, it seems that the next entry "Export today's checked in barcodes" is not shown and disabled when needed. We certainly want this change:

                 [% IF Koha.Preference('intranetreadinghistory') %]
-                    [%IF ( privacy == 2 ) %]
+                    [%IF ( patron.privacy == 2 ) %]
Comment 30 Katrin Fischer 2024-08-27 10:16:34 UTC
Pushed follow-up for selenium tests.

Is anyone working on the remaining fails?
Comment 31 Jake Deery 2024-08-27 10:19:47 UTC
Would just like to raise that the rows per page dropdown on guided_reports.pl?op=run seems to be broken. Not sure if it is related to this bug, but I've only noticed it since today.

Jake
Comment 32 Jonathan Druart 2024-08-27 10:53:46 UTC
(In reply to Jake Deery from comment #31)
> Would just like to raise that the rows per page dropdown on
> guided_reports.pl?op=run seems to be broken. Not sure if it is related to
> this bug, but I've only noticed it since today.
> 
> Jake

Reported on bug 37745.
Comment 33 Owen Leonard 2024-08-27 12:06:41 UTC
Created attachment 170768 [details] [review]
Bug 35402: Fix some selenium tests

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 34 Katrin Fischer 2024-08-27 12:07:23 UTC
(In reply to Owen Leonard from comment #33)
> Created attachment 170768 [details] [review] [review]
> Bug 35402: Fix some selenium tests
> 
> Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Sorry, already pushed :(
Comment 35 Jake Deery 2024-08-28 11:18:21 UTC
Just an FYI – 

Follow-up bug 37755 needs looking at. It addresses being unable to submit modborrowers and possible issues with batch_modify_records / batch_delete_records

Jake.
Comment 36 Katrin Fischer 2024-08-29 15:28:05 UTC
Waiting for Jenkins - I think I pushed everything ready to be pushed now. Are we still missing a patch for one Selenium test?
Comment 37 Jonathan Druart 2024-08-29 15:38:22 UTC
(In reply to Katrin Fischer from comment #36)
> Waiting for Jenkins - I think I pushed everything ready to be pushed now.
> Are we still missing a patch for one Selenium test?

Yes I don't think we have fixed regressions.t
Comment 38 Katrin Fischer 2024-08-29 15:48:44 UTC
(In reply to Jonathan Druart from comment #37)
> (In reply to Katrin Fischer from comment #36)
> > Waiting for Jenkins - I think I pushed everything ready to be pushed now.
> > Are we still missing a patch for one Selenium test?
> 
> Yes I don't think we have fixed regressions.t

Should we add a separate bug?
Comment 39 Owen Leonard 2024-08-29 17:53:08 UTC
Created attachment 170877 [details] [review]
Bug 35402: (follow-up) Fix regressions.t

regressions.t looks for "beep.ogg" on the checkout page, but by default
when a patron's account loads for checkout the "opening.ogg" sound is
played.
Comment 40 Owen Leonard 2024-08-29 17:55:16 UTC
I can't figure out what changed to break regressions.t, but this patch seems to fix it. What am I missing?
Comment 41 Tomás Cohen Arazi 2024-08-30 12:06:55 UTC
Created attachment 170905 [details] [review]
Bug 35402: (follow-up) Fix regressions.t

regressions.t looks for "beep.ogg" on the checkout page, but by default
when a patron's account loads for checkout the "opening.ogg" sound is
played.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 42 Tomás Cohen Arazi 2024-08-30 12:10:19 UTC
I tested the last follow-up and it fixes regressions.t.

Best regards
Comment 43 Tomás Cohen Arazi 2024-08-30 13:04:19 UTC
(In reply to Tomás Cohen Arazi from comment #42)
> I tested the last follow-up and it fixes regressions.t.

This fixes the tests, but it feels we should instead track the class name changes on this patchset so we make sure the right sound is wired instead.
Comment 44 Katrin Fischer 2024-08-30 13:07:12 UTC
(In reply to Owen Leonard from comment #40)
> I can't figure out what changed to break regressions.t, but this patch seems
> to fix it. What am I missing?

I think Owen was a bit suspicious of it being the right fix as well. Could we get someone to dig a bit deeper here? I think we need a fresh set of eyes.
Comment 45 Jonathan Druart 2024-09-02 12:13:18 UTC
It fixes the test but it is a change in the behaviour. Prior to this patchset, beep was played. Now it's opening.
It happens because we have the modal in the DOM, and it has the 'audio-alert-action' class.
Comment 46 Jonathan Druart 2024-09-02 12:27:04 UTC
(In reply to Jonathan Druart from comment #45)
> It fixes the test but it is a change in the behaviour. Prior to this
> patchset, beep was played. Now it's opening.
> It happens because we have the modal in the DOM, and it has the
> 'audio-alert-action' class.

And opening is not played when it should:
Turn on WaitingNotifyAtCheckout
Place a hold and checkin barcode_1 (waiting hold)
checkout barcode_2
=> modal is displayed ("This patron has waiting holds"), but nothing played.
Comment 47 Jonathan Druart 2024-09-02 12:41:31 UTC
(In reply to Jonathan Druart from comment #29)
> (In reply to Jonathan Druart from comment #28)
> > 2. 
> > #   Failed test 'Update child to patron'
> > #   at t/db_dependent/selenium/update_child_to_adult.t line 153.
> > Error while executing command: unexpected alert open: Dismissed user prompt
> > dialog: Are you sure you want to update this child to an Adult category?
> > This cannot be undone. at /usr/share/perl5/Selenium/Remote/Driver.pm line
> > 411.
> > 
> > This is happening because the "Update child to adult" item in the "More"
> > menu (members module) is clickable. There is a tooltip and prior to this
> > change the item was not clickable and we didn't get the alert.
> 
> Related to this, it seems that the next entry "Export today's checked in
> barcodes" is not shown and disabled when needed. We certainly want this
> change:
> 
>                  [% IF Koha.Preference('intranetreadinghistory') %]
> -                    [%IF ( privacy == 2 ) %]
> +                    [%IF ( patron.privacy == 2 ) %]

Opened bug 37807.
Comment 48 Jonathan Druart 2024-09-03 09:06:21 UTC
Created attachment 170962 [details] [review]
Bug 35402: Fix regressions.t

The audio alert feature is not correctly designed and is problematic.
It looks at the class name in the DOM, not when the element with this class name
will be displayed to the end user

So it's not possible to make it work for modals for instance, or if an element
is built in JS then displayed

This patch remove the modal code from the code, if there are no waiting
holds. It is the behaviour that existed prior to the Bootstrap update.
However it is kind of random behaviour, the song that is played when the
librarian open the circ page is different if the user has waiting holds or not.
Comment 49 Jonathan Druart 2024-09-03 09:07:03 UTC
This patch fixes the failing test and restore the behaviour, but it is not working as intended...
Comment 50 Martin Renvoize 2024-09-03 09:18:25 UTC
Created attachment 170963 [details] [review]
Bug 35402: Fix regressions.t

The audio alert feature is not correctly designed and is problematic.
It looks at the class name in the DOM, not when the element with this class name
will be displayed to the end user

So it's not possible to make it work for modals for instance, or if an element
is built in JS then displayed

This patch remove the modal code from the code, if there are no waiting
holds. It is the behaviour that existed prior to the Bootstrap update.
However it is kind of random behaviour, the song that is played when the
librarian open the circ page is different if the user has waiting holds or not.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 Martin Renvoize 2024-09-03 09:19:21 UTC
I concur, this is the fix we need to make the tests happy again and restore the functionality...

However, I also agree that the code is functionally wrong and needs a rethink to work nicely with modals and other triggers..  we should open a bug about that.
Comment 52 Katrin Fischer 2024-09-05 07:54:35 UTC
Pushed "Bug 35402: Fix regressions.t" to main. Go Jenkins!