Bug 37077 - SQL Reports - Picking only one option for each multiple selection results in wrong query
Summary: SQL Reports - Picking only one option for each multiple selection results in ...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Brendan Lawlor
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 35746
Blocks: 37886
  Show dependency treegraph
 
Reported: 2024-06-12 13:43 UTC by Pedro Amorim
Modified: 2024-11-14 15:01 UTC (History)
7 users (show)

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


Attachments
Bug 37077: Fix report muti select for zero and single selections (2.77 KB, patch)
2024-06-14 19:23 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37077: Fix report muti select for zero and single selections (3.34 KB, patch)
2024-06-14 19:31 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37077: Fix report muti select for zero and single selections (3.32 KB, patch)
2024-06-14 19:39 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37077: Fix report muti select for zero and single selections (3.38 KB, patch)
2024-06-17 09:21 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37077: Fix report muti select for zero and single selections (3.48 KB, patch)
2024-08-02 07:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2024-06-12 13:43:05 UTC
To reproduce:
1. 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 but pick only 1 library and 1 item type
4. Click the "Show SQL code"
5. Notice itype value is wrong. It's using the same value selected for library.
Comment 1 Brendan Lawlor 2024-06-13 17:44:55 UTC
This one is my bad. Thanks Pedro!

It's happening because the url query string is getting sent duplicate values when only one value is selected.

Then on the perl side the duplicate param for the first select gets inserted into the query where the second param should be.

The example query you wrote generates a url like this:

http://localhost:8081/cgi-bin/koha/reports/guided_reports.pl?id=1&param_name=Libraries%7Cbranches%3Ain&sql_params=CPL&sql_params=CPL&param_name=Item+type%7Citemtypes%3Ain&sql_params=BK&sql_params=BK&op=run

when it should be like this:

http://localhost:8081/cgi-bin/koha/reports/guided_reports.pl?id=1&param_name=Libraries%7Cbranches%3Ain&sql_params=CPL&param_name=Item+type%7Citemtypes%3Ain&sql_params=BK&op=run

This should be fixable in the javascript that overrides the form submission.
Comment 2 Brendan Lawlor 2024-06-14 19:23:15 UTC
Created attachment 167762 [details] [review]
Bug 37077: Fix report muti select for zero and single selections

This patch updates the javascript overriding the form submission when reports have multi select parameters.

When there are more than one multi selects, and the user selcts one value from each, it skips updating the value of the select, so it doens't send duplicate parameters anymore.

If there are no selections made it will pass '%' for all values. This allows the multi select to be optional in the report. If no selections are made it is assumed that you are not using that parameter to limit the report.
Comment 3 Brendan Lawlor 2024-06-14 19:31:30 UTC
Created attachment 167763 [details] [review]
Bug 37077: Fix report muti select for zero and single selections

This patch updates the javascript overriding the form submission when reports have multi select parameters.

When there are more than one multi selects, and the user selcts one value from each, it skips updating the value of the select, so it doens't send duplicate parameters anymore.

If there are no selections made it will pass '%' for all values. This allows the multi select to be optional in the report. If no selections are made it is assumed that you are not using that parameter to limit the report.

Test plan:
1. 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 but pick only 1 library and 1 item type
4. Click the "Show SQL code"
5. Notice that the two parameters were filled correctly
6. Run the report again with zero selections
7. Click the "Show SQL code"
8. Notice that the '%' is used for the parameter
9. Run the report again with combinations of single, multiple and zero selections
10. confirm the SQL code is generated as you would expect.
Comment 4 Brendan Lawlor 2024-06-14 19:39:19 UTC
Created attachment 167765 [details] [review]
Bug 37077: Fix report muti select for zero and single selections

This patch updates the javascript overriding the form submission when reports have multi select parameters.

When there are more than one multi selects, and the user selcts one value from each, it skips updating the value of the select, so it doens't send duplicate parameters anymore.

If there are no selections made it will pass '%' for all values. This allows the multi select to be optional in the report. If no selections are made it is assumed that you are not using that parameter to limit the report.

Test plan:
1. 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 but pick only 1 library and 1 item type
4. Click the "Show SQL code"
5. Notice that the two parameters were filled correctly
6. Run the report again with zero selections
7. Click the "Show SQL code"
8. Notice that the '%' is used for the parameter
9. Run the report again with combinations of single, multiple and zero selections
10. confirm the SQL code is generated as you would expect.
Comment 5 Pedro Amorim 2024-06-17 09:21:42 UTC
Created attachment 167785 [details] [review]
Bug 37077: Fix report muti select for zero and single selections

This patch updates the javascript overriding the form submission when reports have multi select parameters.

When there are more than one multi selects, and the user selcts one value from each, it skips updating the value of the select, so it doens't send duplicate parameters anymore.

If there are no selections made it will pass '%' for all values. This allows the multi select to be optional in the report. If no selections are made it is assumed that you are not using that parameter to limit the report.

Test plan:
1. 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 but pick only 1 library and 1 item type
4. Click the "Show SQL code"
5. Notice that the two parameters were filled correctly
6. Run the report again with zero selections
7. Click the "Show SQL code"
8. Notice that the '%' is used for the parameter
9. Run the report again with combinations of single, multiple and zero selections
10. confirm the SQL code is generated as you would expect.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 6 Marcel de Rooy 2024-08-02 07:23:09 UTC
Created attachment 169987 [details] [review]
Bug 37077: Fix report muti select for zero and single selections

This patch updates the javascript overriding the form submission when reports have multi select parameters.

When there are more than one multi selects, and the user selcts one value from each, it skips updating the value of the select, so it doens't send duplicate parameters anymore.

If there are no selections made it will pass '%' for all values. This allows the multi select to be optional in the report. If no selections are made it is assumed that you are not using that parameter to limit the report.

Test plan:
1. 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 but pick only 1 library and 1 item type
4. Click the "Show SQL code"
5. Notice that the two parameters were filled correctly
6. Run the report again with zero selections
7. Click the "Show SQL code"
8. Notice that the '%' is used for the parameter
9. Run the report again with combinations of single, multiple and zero selections
10. confirm the SQL code is generated as you would expect.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2024-08-02 07:24:38 UTC
QA Comment:
It works but feels like hacking. Can understand that this is easier than rewriting the whole module ;) Hard to maintain..
Comment 8 Katrin Fischer 2024-08-02 15:27:59 UTC
(In reply to Marcel de Rooy from comment #7)
> QA Comment:
> It works but feels like hacking. Can understand that this is easier than
> rewriting the whole module ;) Hard to maintain..

Makes me feel like maybe we should have a follow-up bug at least outlining how we could do better?
Comment 9 Katrin Fischer 2024-08-02 15:28:34 UTC
Fixed typo in commit message. (muti)
Comment 10 Katrin Fischer 2024-08-02 17:01:29 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 11 Lucas Gass (lukeg) 2024-09-11 15:49:13 UTC
Backported to 24.05.x for upcoming 24.05.04
Comment 12 Fridolin Somers 2024-09-25 08:32:06 UTC
Depends on Bug 35746 not in 23.11.x

I do not change status because of "additional_work_needed"