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.

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?

Lets drop the security flag..

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.

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.

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!

I'll go ahead and mark this "Needs signoff" since it's gone through one set of revisions. Now testing seems to have stalled!

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.

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

(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!

Pushed for 24.11!

Well done everyone, thank you!

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.

Should we create an omnibus bug for post Bootstrap 5 push issues?

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.

(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

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.

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)

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.

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.

Created attachment 170712 [details] [review]
Bug 35402: (follow-up) Remove duplicate of import in staff-global.scss

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>

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>

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>

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>

Follow-ups pushed to main.

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"]

Created attachment 170745 [details] [review]
Bug 35402: Fix some selenium tests

(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.

(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 ) %]

Pushed follow-up for selenium tests.

Is anyone working on the remaining fails?

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

(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.

Created attachment 170768 [details] [review]
Bug 35402: Fix some selenium tests

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

(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 :(

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.

Waiting for Jenkins - I think I pushed everything ready to be pushed now. Are we still missing a patch for one Selenium test?

(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

(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?

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.

I can't figure out what changed to break regressions.t, but this patch seems to fix it. What am I missing?

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>

I tested the last follow-up and it fixes regressions.t.

Best regards

(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.

(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.

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.

(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.

(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.

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.

This patch fixes the failing test and restore the behaviour, but it is not working as intended...

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>

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.

Pushed "Bug 35402: Fix regressions.t" to main. Go Jenkins!

Created attachment 171176 [details] [review]
Bug 35402: Fix cypress tests

(In reply to Jonathan Druart from comment #53)
> Created attachment 171176 [details] [review] [review]
> Bug 35402: Fix cypress tests

One more failure after this patch, see bug 37513 comment 19.

Pushed "Fix cypress tests" and pinged Tomas.

Great work here, Owen. Not going to backport it to 24.05.x though! :)