Bug 37050 - Add select2 to SQL report multi select
Summary: Add select2 to SQL report multi select
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Brendan Lawlor
QA Contact: Pedro Amorim
URL:
Keywords:
: 37886 (view as bug list)
Depends on:
Blocks:
 
Reported: 2024-06-06 19:45 UTC by Brendan Lawlor
Modified: 2025-05-26 15:16 UTC (History)
3 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
When writing SQL reports with SQL runtime parameters for selecting multiple items from a list, this is now easier with a Select2-style pull-down that allows for searching and displays choices as a list with 'x' for easy removal of previous selections.
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 37050: Add select2 to SQL reports multi select (4.56 KB, patch)
2024-06-06 20:07 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Run report screen (52.75 KB, image/png)
2024-06-12 13:45 UTC, Pedro Amorim
Details
Bug 37050: Add select2 to SQL reports multi select (4.62 KB, patch)
2024-06-18 12:57 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 37050: (follow-up) CSS and configuration tweak (1.93 KB, patch)
2024-06-18 12:57 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 37050: Add select2 to SQL reports multi select (4.67 KB, patch)
2024-06-18 13:06 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37050: (follow-up) CSS and configuration tweak (1.99 KB, patch)
2024-06-18 13:06 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37050: Add select2 to SQL reports multi select (4.60 KB, patch)
2024-07-26 15:28 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 37050: (follow-up) CSS and configuration tweak (1.93 KB, patch)
2024-07-26 15:28 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 37050: Add select2 to SQL reports multi select (4.66 KB, patch)
2024-07-29 10:01 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37050: (follow-up) CSS and configuration tweak (2.07 KB, patch)
2024-07-29 10:01 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37050: Add select2 to SQL reports multi select (3.80 KB, patch)
2024-09-11 15:03 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37050: (follow-up) CSS and configuration tweak (2.07 KB, patch)
2024-09-11 15:03 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37050: (follow-up) Add select2 to SQL reports multiselect (3.47 KB, patch)
2024-09-11 15:03 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37050: Add select2 to SQL reports multi select (3.86 KB, patch)
2024-09-25 21:58 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 37050: (follow-up) CSS and configuration tweak (2.13 KB, patch)
2024-09-25 21:58 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 37050: (follow-up) Add select2 to SQL reports multiselect (3.52 KB, patch)
2024-09-25 21:58 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 37050: Add select2 to SQL reports multiselect (5.43 KB, patch)
2025-01-29 20:32 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37050: Add select2 to SQL reports multiselect (6.11 KB, patch)
2025-03-24 18:16 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37050: Add select2 to SQL reports multiselect (6.17 KB, patch)
2025-03-26 16:20 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 37050: Add select2 to SQL reports multiselect (6.23 KB, patch)
2025-04-04 09:19 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37050: (follow-up) fix invalid tt html syntax (3.67 KB, patch)
2025-04-17 13:53 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37050: (follow-up) Fix invalid html syntax in tt (2.20 KB, patch)
2025-04-17 14:14 UTC, Brendan Lawlor
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Brendan Lawlor 2024-06-06 19:45:52 UTC
Bug 36545 adds select2 to improve the user experience when making multiple selections in item search.

Bug 35746 adds the ability to reports to make multiple selections for SQL parameters.

It would be nice for consistency and ux for the report multiselect to use select2.
Comment 1 Brendan Lawlor 2024-06-06 20:07:29 UTC Comment hidden (obsolete)
Comment 2 Pedro Amorim 2024-06-12 13:45:10 UTC
Created attachment 167659 [details]
Run report screen

Something seems wrong with the UI. This is what I'm seeing even after yarn css:build.
Comment 3 Brendan Lawlor 2024-06-12 13:55:43 UTC
Thanks for looking at this Pedro!

I think there is some css needed to make the little chips appear fit to the value and in line, like they do in the new item search, but I haven't been able to look much into it yet.
Comment 4 Owen Leonard 2024-06-18 12:57:26 UTC Comment hidden (obsolete)
Comment 5 Owen Leonard 2024-06-18 12:57:28 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2024-06-18 13:01:40 UTC
(In reply to Brendan Lawlor from comment #3)
> I think there is some css needed to make the little chips appear fit

It was necessary to override some of the CSS inherited from fieldset.rows li.
Comment 7 Pedro Amorim 2024-06-18 13:06:23 UTC Comment hidden (obsolete)
Comment 8 Pedro Amorim 2024-06-18 13:06:26 UTC Comment hidden (obsolete)
Comment 9 Owen Leonard 2024-07-26 15:28:06 UTC Comment hidden (obsolete)
Comment 10 Owen Leonard 2024-07-26 15:28:08 UTC Comment hidden (obsolete)
Comment 11 Owen Leonard 2024-07-26 15:28:23 UTC
Rebased.
Comment 12 Pedro Amorim 2024-07-29 10:01:30 UTC Comment hidden (obsolete)
Comment 13 Pedro Amorim 2024-07-29 10:01:33 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2024-08-23 17:00:12 UTC
I am unsure about the small conflict here. It's about this line:

<option value="null" hidden></option>

Previously changed by bug 37077. 

Can you please have a look and rebase yourself since you wrote the other patch too?
Comment 15 Brendan Lawlor 2024-09-10 20:38:34 UTC
*** Bug 37886 has been marked as a duplicate of this bug. ***
Comment 16 Brendan Lawlor 2024-09-11 15:03:52 UTC Comment hidden (obsolete)
Comment 17 Brendan Lawlor 2024-09-11 15:03:54 UTC Comment hidden (obsolete)
Comment 18 Brendan Lawlor 2024-09-11 15:03:56 UTC Comment hidden (obsolete)
Comment 19 Biblibre Sandboxes 2024-09-25 21:58:04 UTC
Created attachment 171957 [details] [review]
Bug 37050: Add select2 to SQL reports multi select

This patch adds select 2 to multi-select parameter fields in
SQL reports. If no selections are made for a multi-select
all authorised values will be used by default. This allows reports
to be written where a multi select parameter is optional.

To test:
1. Apply patch
2. Go to Reports and create a report from SQL
   select *
   from items
   where
   homebranch in <<Libraries|branches:in>>
   and
   itype in <<Item type|itemtypes:in>>
3. Run the report making multiple selections for each parameter
    and making single selections for each parameter
4. Run the report making multiple selections for one parameter and no
   selections in another parameter
5. Check that the results are as you would expect
6. On the results page click 'Show SQL code'
7. Confirm that the codes are included according to your selections
8. Note that when no selections are made for a parameter all codes are
   used by defaut

Sponsored-by: CLAMS
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 20 Biblibre Sandboxes 2024-09-25 21:58:07 UTC
Created attachment 171958 [details] [review]
Bug 37050: (follow-up) CSS and configuration tweak

This follow-up tweaks the Select2 CSS so that the selected choices don't
display with 100% width.

The patch also removes the "closeOnSelect: false" configuration from
Select2. In discussing this option for Bug 36545 more people preferred
the default behavior and I think we should be consistent.

PA amended commit message: `yarn css:build` is required to test these
CSS changes

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 21 Biblibre Sandboxes 2024-09-25 21:58:09 UTC
Created attachment 171959 [details] [review]
Bug 37050: (follow-up) Add select2 to SQL reports multiselect

Rebase for changes made on bug 37077
This patch removes some javascript in favor of an update to the template.
The default value for multi select parameter's is a newline separated list
so that report results are not limited when no selections are made.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 22 Pedro Amorim 2024-10-29 12:07:10 UTC
Taking another look here
Comment 23 Pedro Amorim 2024-10-29 12:26:28 UTC
Observations:
It feels a bit weird to allow for an empty selection (causing an empty box with an x), and have that empty option behave the same as if no option was selected at all. Should we just not show an empty option in this case? The user can deselect any selections and submit it empty anyway.

Found the following error:
A) Follow steps 1 and 2 of the test plan, submit the empty form -> all good
B) Follow steps 1 and 2 of the test plan, pick a single library, then remove, submit the empty form -> BOOM

This may have been caused by bug 35746 and not the patches here.

For the reasons above I'm not comfortable putting this back to PQA, but I don't think they're strong enough reasons to FQA either (1 is an observation, the other a bug that I don't relates to patches here).

@Brendan, what do you think of the comments above?
Comment 24 Brendan Lawlor 2024-10-30 15:54:27 UTC
Hi Pedro, thanks for finding these issues!

(In reply to Pedro Amorim from comment #23)
> Observations:
> It feels a bit weird to allow for an empty selection (causing an empty box
> with an x), and have that empty option behave the same as if no option was
> selected at all. Should we just not show an empty option in this case? The
> user can deselect any selections and submit it empty anyway.

I hadn't noticed that the hidden select option is selectable in the select2 dropdown. I agree this should be fixed. It seems to be that select2 is not respecting the hidden property on that first option for some reason.

> Found the following error:
> A) Follow steps 1 and 2 of the test plan, submit the empty form -> all good
> B) Follow steps 1 and 2 of the test plan, pick a single library, then
> remove, submit the empty form -> BOOM

The template fills all the codes into the hidden option and then every time the user updates selections the value of the hidden option is updated. This error is happening because of an edge case when everything is deselected the value is becoming an empty string when it should revert to a newline delimited list of all codes.

I will come back to this and submit a new patch as soon as I can. Thanks!
Comment 25 Katrin Fischer 2024-11-21 14:26:21 UTC
@Pedro and @Brendan: Hi there, reviewing "additional_work_needed" - can you please review and file new bugs if needed?
Comment 26 Brendan Lawlor 2024-11-21 14:48:09 UTC
(In reply to Katrin Fischer from comment #25)
> @Pedro and @Brendan: Hi there, reviewing "additional_work_needed" - can you
> please review and file new bugs if needed?


Hi Katrin!

I added "additional_work_needed" to remind me to come back to this one. Should I have just set it to Failed QA instead?

I intend to work on this but haven't had the time recently.
Comment 27 Katrin Fischer 2024-11-21 14:49:45 UTC
(In reply to Brendan Lawlor from comment #26)
> (In reply to Katrin Fischer from comment #25)
> > @Pedro and @Brendan: Hi there, reviewing "additional_work_needed" - can you
> > please review and file new bugs if needed?
> 
> 
> Hi Katrin!
> 
> I added "additional_work_needed" to remind me to come back to this one.
> Should I have just set it to Failed QA instead?
> 
> I intend to work on this but haven't had the time recently.

Hah! That was my mistake, we usually use the additional_work_needed for bugs already pushed that still need something done. Failed QA or back to Assigned is fine if you need to work on it still.
Comment 28 Brendan Lawlor 2025-01-29 20:32:38 UTC
Created attachment 177320 [details] [review]
Bug 37050: Add select2 to SQL reports multiselect

This patch adds select 2 to multi-select parameter fields in
SQL reports. If no selections are made for a multi-select
all authorised values will be used by default. This allows reports
to be written where a multi select parameter is optional.

To test:
1. Apply patch
2. Go to Reports and create a report from SQL
   select *
   from items
   where
   homebranch in <<Libraries|branches:in>>
   and
   itype in <<Item type|itemtypes:in>>
3. Run the report making multiple selections for each parameter
    and making single selections for each parameter
4. Run the report making multiple selections for one parameter and no
   selections in another parameter
5. Check that the results are as you would expect
6. On the results page click 'Show SQL code'
7. Confirm that the codes are included according to your selections
8. Note that when no selections are made for a parameter all codes are
   used by defaut

Sponsored-by: CLAMS
Comment 29 Brendan Lawlor 2025-03-24 18:16:50 UTC
Created attachment 179669 [details] [review]
Bug 37050: Add select2 to SQL reports multiselect

This patch adds select 2 to multi-select parameter fields in
SQL reports. If no selections are made for a multi-select
all authorised values will be used by default. This allows reports
to be written where a multi select parameter is optional.

To test:
1. Apply patch
2. Go to Reports and create a report from SQL
   select *
   from items
   where
   homebranch in <<Libraries|branches:in>>
   and
   itype in <<Item type|itemtypes:in>>
3. Run the report making multiple selections for each parameter
    and making single selections for each parameter
4. Run the report making multiple selections for one parameter and no
   selections in another parameter
5. Check that the results are as you would expect
6. On the results page click 'Show SQL code'
7. Confirm that the codes are included according to your selections
8. Note that when no selections are made for a parameter all codes are
   used by defaut

Sponsored-by: CLAMS
Comment 30 Brendan Lawlor 2025-03-24 18:18:16 UTC
rebased for main
Comment 31 Biblibre Sandboxes 2025-03-26 16:20:30 UTC
Created attachment 179728 [details] [review]
Bug 37050: Add select2 to SQL reports multiselect

This patch adds select 2 to multi-select parameter fields in
SQL reports. If no selections are made for a multi-select
all authorised values will be used by default. This allows reports
to be written where a multi select parameter is optional.

To test:
1. Apply patch
2. Go to Reports and create a report from SQL
   select *
   from items
   where
   homebranch in <<Libraries|branches:in>>
   and
   itype in <<Item type|itemtypes:in>>
3. Run the report making multiple selections for each parameter
    and making single selections for each parameter
4. Run the report making multiple selections for one parameter and no
   selections in another parameter
5. Check that the results are as you would expect
6. On the results page click 'Show SQL code'
7. Confirm that the codes are included according to your selections
8. Note that when no selections are made for a parameter all codes are
   used by defaut

Sponsored-by: CLAMS
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 32 Michaela Sieber 2025-03-26 16:21:37 UTC
Ready for Pedro ;-)
Comment 33 Pedro Amorim 2025-04-04 09:19:17 UTC
Created attachment 180618 [details] [review]
Bug 37050: Add select2 to SQL reports multiselect

This patch adds select 2 to multi-select parameter fields in
SQL reports. If no selections are made for a multi-select
all authorised values will be used by default. This allows reports
to be written where a multi select parameter is optional.

To test:
1. Apply patch
2. Go to Reports and create a report from SQL
   select *
   from items
   where
   homebranch in <<Libraries|branches:in>>
   and
   itype in <<Item type|itemtypes:in>>
3. Run the report making multiple selections for each parameter
    and making single selections for each parameter
4. Run the report making multiple selections for one parameter and no
   selections in another parameter
5. Check that the results are as you would expect
6. On the results page click 'Show SQL code'
7. Confirm that the codes are included according to your selections
8. Note that when no selections are made for a parameter all codes are
   used by defaut

Sponsored-by: CLAMS
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Comment 34 Pedro Amorim 2025-04-04 09:28:01 UTC
Tested and all my previous observations have been answered. Awesome and thank you!
My understanding is the new CSS is required or else it inherits the 100% width from fieldset.rows li. This does not cause any regression on existing select2 in item search that I can tell and likely helps future select2 multiple under these fieldset.rows li.

It'd be great to DRY this pattern at some point (in itemsearch.tt and guided_reports_start.tt) if at all possible:
selectField.style.minWidth = '320px';
Comment 35 Katrin Fischer 2025-04-14 06:44:59 UTC
I have unhappy QA tool here, can you please check?

qa
testing 1 commit(s) (applied to fec3b60 '3df Bug 39392: Fix typos in atomicupd')

Processing files before patches
|========================>| 1 / 1 (100.00%)
Processing files after patches
[error] VefMXrl4Sg.tt: SyntaxError: Unexpected closing tag "select". It may happen when the tag has already been closed by another tag. For more info see https://www.w3.org/TR/html5/syntax.html#closing-elements-that-have-implied-end-tags (7:49)
[error]   5 |                                                     #~569~#
[error]   6 |                                                     #~574~#
[error] > 7 |                                                 </select>
[error]     |                                                 ^^^^^^^^^
[error]   8 |                                             </li>
[error]   9 |                                          at misc/devel/tidy.pl line 112.


 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt
   FAIL	  tidiness
		File is not tidy, please run `perl misc/devel/tidy.pl koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt
Comment 36 Brendan Lawlor 2025-04-17 13:53:03 UTC
Created attachment 181099 [details] [review]
Bug 37050: (follow-up) fix invalid tt html syntax
Comment 37 Brendan Lawlor 2025-04-17 14:14:59 UTC
Created attachment 181100 [details] [review]
Bug 37050: (follow-up) Fix invalid html syntax in tt
Comment 38 Brendan Lawlor 2025-04-17 14:23:14 UTC
Thanks Katrin!

I found this error was caused by using an if else block to open the <select> element and then closing it outside that block.

I came up with two different solutions. I think the second one is cleaner, but I'm not 100% sure if it's ok with regard to the HTML1 coding guideline with the tt IF inside the html. It passes the QA script now though :)
Comment 39 Katrin Fischer 2025-04-17 14:57:47 UTC
(In reply to Brendan Lawlor from comment #38)
> Thanks Katrin!
> 
> I found this error was caused by using an if else block to open the <select>
> element and then closing it outside that block.
> 
> I came up with two different solutions. I think the second one is cleaner,
> but I'm not 100% sure if it's ok with regard to the HTML1 coding guideline
> with the tt IF inside the html. It passes the QA script now though :)

HTML1 was recently updated, so you might be OK there (without seeing the code, but having hte QA script pass :) )
Comment 40 Katrin Fischer 2025-04-22 06:49:27 UTC
Pushed for 25.05!

Well done everyone, thank you!