Description
Brendan Lawlor
2024-06-06 19:45:52 UTC
Created attachment 167555 [details] [review] Bug 37050: Add select2 to SQL reports multi select This patch adds select 2 to modifies multi-select parameter fields in SQL reports to use the select2 framework. If no selections are made for a multi select parameter all values will be used by default. 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 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. This allows the query to run when no selections are made instead of causing an error. It allows reports to be written where a multi select parameter is optional. Created attachment 167659 [details]
Run report screen
Something seems wrong with the UI. This is what I'm seeing even after yarn css:build.
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. Created attachment 167838 [details] [review] Bug 37050: Add select2 to SQL reports multi select This patch adds select 2 to modifies multi-select parameter fields in SQL reports to use the select2 framework. If no selections are made for a multi select parameter all values will be used by default. 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 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. This allows the query to run when no selections are made instead of causing an error. It allows reports to be written where a multi select parameter is optional. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 167839 [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. (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. Created attachment 167840 [details] [review] Bug 37050: Add select2 to SQL reports multi select This patch adds select 2 to modifies multi-select parameter fields in SQL reports to use the select2 framework. If no selections are made for a multi select parameter all values will be used by default. 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 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. This allows the query to run when no selections are made instead of causing an error. It allows reports to be written where a multi select parameter is optional. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Created attachment 167841 [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. Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Created attachment 169750 [details] [review] Bug 37050: Add select2 to SQL reports multi select This patch adds select 2 to modifies multi-select parameter fields in SQL reports to use the select2 framework. If no selections are made for a multi select parameter all values will be used by default. 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 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. This allows the query to run when no selections are made instead of causing an error. It allows reports to be written where a multi select parameter is optional. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 169751 [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. Rebased. Created attachment 169818 [details] [review] Bug 37050: Add select2 to SQL reports multi select This patch adds select 2 to modifies multi-select parameter fields in SQL reports to use the select2 framework. If no selections are made for a multi select parameter all values will be used by default. 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 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. This allows the query to run when no selections are made instead of causing an error. It allows reports to be written where a multi select parameter is optional. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Created attachment 169819 [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> 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? *** Bug 37886 has been marked as a duplicate of this bug. *** Created attachment 171340 [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> Created attachment 171341 [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> Created attachment 171342 [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. 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> 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> 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> Taking another look here 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? 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! @Pedro and @Brendan: Hi there, reviewing "additional_work_needed" - can you please review and file new bugs if needed? (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. (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. 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 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 rebased for main 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> Ready for Pedro ;-) 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> 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'; 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 Created attachment 181099 [details] [review] Bug 37050: (follow-up) fix invalid tt html syntax Created attachment 181100 [details] [review] Bug 37050: (follow-up) Fix invalid html syntax in tt 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 :) (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 :) ) Pushed for 25.05! Well done everyone, thank you! |