When creating an SQL report, if you use an undefined authorised value category (for example if you have a typo or copied an SQL from the wiki without creating the corresponding category), you will be provided an empty dropdown menu when trying to run it. As spoken with jcamins, we should provide a warning when trying to run such report.
Created attachment 15560 [details] [review] Bug 9659 - Better handling of non-existent authorised value categories used in SQL reports A user might create a SQL report that relies on non-existent authorised value categories. Because of a typo, or just because they copy&pasted the report from the Wiki. I added comments on the code, refactored some really tiny bits, and fixed an annoying warning of uninitialised variable also. Sponsored-by: Universidad Nacional de Córdoba
I must say that handling this kind of error on the 'Save report' stage might be ok too. I'd like to hear what do u think about it before working on a follow-up.
Created attachment 15596 [details] [review] Bug 9659 - Better handling of non-existent authorised value categories used in SQL reports A user might create a SQL report that relies on non-existent authorised value categories. Because of a typo, or just because they copy&pasted the report from the Wiki. Use cases are: - The user creates a report from SQL a) Uses bad authorised values b) Clicks 'Save Report' c) Koha lists the problematic authorised values d) The user decides to e-1) Save it anyway, it gets saved e-2) Edit the report, it gets back to where it chose 'Save Report' - The user edits an already saved report (Update SQL) a) Uses bad authorised values b) Clicks 'Update SQL' c) Koha lists the problematic authorised values d) The user decides to e-1) Save it anyway, it gets saved e-2) Edit the report, it gets back to where it chose 'Update SQL' - The user tries to run a saved report that contains bad authorised values, Koha advertises the problem and provides the user with a button 'Edit SQL' to fix things. Notes: - I added several comments on the code. - Fixed an annoying warning of uninitialised variable also (refactored some tiny bits to do it). - Added the following methods - C4::Reports::Guided::GetReservedAuthorisedValues - C4::Reports::Guided::GetParametersFromSQL - C4::Reports::Guided::IsAuthorisedValueValid - C4::Reports::Guided::ValidateSQLParameters - C4::Koha::IsAuthorisedValueCategory - Those methods could have been used to refactor this guided reports code as its *a bit messy*. I chose to do it in a new bug of course :-D. - Thanks to Owen and Jared for their patience heh. Sponsored-by: Universidad Nacional de Córdoba
Created attachment 15603 [details] [review] Bug 9659 - Better handling of non-existent authorised value categories used in SQL reports A user might create a SQL report that relies on non-existent authorised value categories. Because of a typo, or just because they copy&pasted the report from the Wiki. Use cases are: - The user creates a report from SQL a) Uses bad authorised values b) Clicks 'Save Report' c) Koha lists the problematic authorised values d) The user decides to e-1) Save it anyway, it gets saved e-2) Edit the report, it gets back to where it chose 'Save Report' - The user edits an already saved report (Update SQL) a) Uses bad authorised values b) Clicks 'Update SQL' c) Koha lists the problematic authorised values d) The user decides to e-1) Save it anyway, it gets saved e-2) Edit the report, it gets back to where it chose 'Update SQL' - The user tries to run a saved report that contains bad authorised values, Koha advertises the problem and provides the user with a button 'Edit SQL' to fix things. To test, just create a report from SQL using invalid authorised values like this (misspelled 'branch'): SELECT * FROM itemtypes WHERE hola=<<Test branch1|branchee>> AND hola2=<<Test branch2|brancha>> Regards To+ Notes: - I added several comments on the code. - Fixed an annoying warning of uninitialised variable also (refactored some tiny bits to do it). - Added the following methods - C4::Reports::Guided::GetReservedAuthorisedValues - C4::Reports::Guided::GetParametersFromSQL - C4::Reports::Guided::IsAuthorisedValueValid - C4::Reports::Guided::ValidateSQLParameters - C4::Koha::IsAuthorisedValueCategory - Those methods could have been used to refactor this guided reports code as its *a bit messy*. I chose to do it in a new bug of course :-D. - Fixed some trivial perlcritic -5 errors - Removed some debugging stuff left by mistake - Thanks to Owen and Jared for their patience heh. Sponsored-by: Universidad Nacional de Córdoba
Tom, there are small pod errors, but the example queries in online help also generates warnings. Is that ok?
Great idea, Tomás! I haven't looked at the code yet, but it appears that there is a problem when saving and running reports though. When I use your test case, I fix the typos so that the run-time parameter is looking for "branches", and I'm still getting the following error message: Test branch1: The authorised value category (branches) you selected does not exist. Test branch2: The authorised value category (branches) you selected does not exist. However, when I save anyway and run the report, it works successfully. I also tried using the legitimate authorised value category "LOC", but I received the same on screen error message both when saving the report and when trying to run the report :/.
Created attachment 15618 [details] [review] Bug 9659 - Better handling of non-existent authorised value categories used in SQL reports A user might create a SQL report that relies on non-existent authorised value categories. Because of a typo, or just because they copy&pasted the report from the Wiki. Use cases are: - The user creates a report from SQL a) Uses bad authorised values b) Clicks 'Save Report' c) Koha lists the problematic authorised values d) The user decides to e-1) Save it anyway, it gets saved e-2) Edit the report, it gets back to where it chose 'Save Report' - The user edits an already saved report (Update SQL) a) Uses bad authorised values b) Clicks 'Update SQL' c) Koha lists the problematic authorised values d) The user decides to e-1) Save it anyway, it gets saved e-2) Edit the report, it gets back to where it chose 'Update SQL' - The user tries to run a saved report that contains bad authorised values, Koha advertises the problem and provides the user with a button 'Edit SQL' to fix things. To test, just create a report from SQL using invalid authorised values like this (misspelled 'branch'): SELECT * FROM itemtypes WHERE hola=<<Test branch1|branchee>> AND hola2=<<Test branch2|brancha>> Regards To+ Notes: - I added several comments on the code. - Fixed an annoying warning of uninitialised variable also (refactored some tiny bits to do it). - Added the following methods - C4::Reports::Guided::GetReservedAuthorisedValues - C4::Reports::Guided::GetParametersFromSQL - C4::Reports::Guided::IsAuthorisedValueValid - C4::Reports::Guided::ValidateSQLParameters - C4::Koha::IsAuthorisedValueCategory - Those methods could have been used to refactor this guided reports code as its *a bit messy*. I chose to do it in a new bug of course :-D. - Fixed some trivial perlcritic -5 errors - Removed some debugging stuff left by mistake - Fixed some POD problems - Thanks to Owen and Jared for their patience heh. Sponsored-by: Universidad Nacional de Córdoba
(In reply to comment #5) > Tom, > there are small pod errors, but the example queries in online help > also generates warnings. Is that ok? Both fixed in the latest patch.
(In reply to comment #6) > Great idea, Tomás! > > I haven't looked at the code yet, but it appears that there is a problem > when saving and running reports though. > > When I use your test case, I fix the typos so that the run-time parameter is > looking for "branches", and I'm still getting the following error message: > > Test branch1: The authorised value category (branches) you selected does not > exist. > Test branch2: The authorised value category (branches) you selected does not > exist. > > However, when I save anyway and run the report, it works successfully. > > I also tried using the legitimate authorised value category "LOC", but I > received the same on screen error message both when saving the report and > when trying to run the report :/. Thanks for testing the patch. I forgot to fix an error I had in two other places regarding array references handling when I found it in one place. It is working correctly right now.
This function is sub-optimal: sub IsAuthorisedValueCategory { my $category = shift; my $categories_arrayref = GetAuthorisedValueCategories(); foreach my $tmp_category (@$categories_arrayref) { if ( $tmp_category eq $category ) { return 1; } } return 0; } You call GetAuthorisedValueCategories which do a SELECT DISTINCT on the whole authorised values table. You may do it directly in SQL: sub IsAuthorisedValueCategory { my $sth = C4::Context->dbh->prepare( "SELECT category FROM authorised_values WHERE category=? LIMIT 1"); $sth->execute(shift); $sth->fetchrow ? 1 : 0; }
Created attachment 15621 [details] [review] Bug 9659 - Better handling of non-existent authorised value categories used in SQL reports A user might create a SQL report that relies on non-existent authorised value categories. Because of a typo, or just because they copy&pasted the report from the Wiki. Use cases are: - The user creates a report from SQL a) Uses bad authorised values b) Clicks 'Save Report' c) Koha lists the problematic authorised values d) The user decides to e-1) Save it anyway, it gets saved e-2) Edit the report, it gets back to where it chose 'Save Report' - The user edits an already saved report (Update SQL) a) Uses bad authorised values b) Clicks 'Update SQL' c) Koha lists the problematic authorised values d) The user decides to e-1) Save it anyway, it gets saved e-2) Edit the report, it gets back to where it chose 'Update SQL' - The user tries to run a saved report that contains bad authorised values, Koha advertises the problem and provides the user with a button 'Edit SQL' to fix things. To test, just create a report from SQL using invalid authorised values like this (misspelled 'branch'): SELECT * FROM itemtypes WHERE hola=<<Test branch1|branchee>> AND hola2=<<Test branch2|brancha>> Regards To+ Notes: - I added several comments on the code. - Fixed an annoying warning of uninitialised variable also (refactored some tiny bits to do it). - Added the following methods - C4::Reports::Guided::GetReservedAuthorisedValues - C4::Reports::Guided::GetParametersFromSQL - C4::Reports::Guided::IsAuthorisedValueValid - C4::Reports::Guided::ValidateSQLParameters - C4::Koha::IsAuthorisedValueCategory - Those methods could have been used to refactor this guided reports code as its *a bit messy*. I chose to do it in a new bug of course :-D. - Fixed some trivial perlcritic -5 errors - Removed some debugging stuff left by mistake - Fixed some POD problems - Optimal SQL-driven IsAuthorisedValueCategory method - Thanks to Owen and Jared for their patience heh. Sponsored-by: Universidad Nacional de Córdoba
Created attachment 15626 [details] [review] [SIGNED-OFF] Bug 9659 - Better handling of non-existent authorised value categories used in SQL reports A user might create a SQL report that relies on non-existent authorised value categories. Because of a typo, or just because they copy&pasted the report from the Wiki. Use cases are: - The user creates a report from SQL a) Uses bad authorised values b) Clicks 'Save Report' c) Koha lists the problematic authorised values d) The user decides to e-1) Save it anyway, it gets saved e-2) Edit the report, it gets back to where it chose 'Save Report' - The user edits an already saved report (Update SQL) a) Uses bad authorised values b) Clicks 'Update SQL' c) Koha lists the problematic authorised values d) The user decides to e-1) Save it anyway, it gets saved e-2) Edit the report, it gets back to where it chose 'Update SQL' - The user tries to run a saved report that contains bad authorised values, Koha advertises the problem and provides the user with a button 'Edit SQL' to fix things. To test, just create a report from SQL using invalid authorised values like this (misspelled 'branch'): SELECT * FROM itemtypes WHERE hola=<<Test branch1|branchee>> AND hola2=<<Test branch2|brancha>> Regards To+ Notes: - I added several comments on the code. - Fixed an annoying warning of uninitialised variable also (refactored some tiny bits to do it). - Added the following methods - C4::Reports::Guided::GetReservedAuthorisedValues - C4::Reports::Guided::GetParametersFromSQL - C4::Reports::Guided::IsAuthorisedValueValid - C4::Reports::Guided::ValidateSQLParameters - C4::Koha::IsAuthorisedValueCategory - Those methods could have been used to refactor this guided reports code as its *a bit messy*. I chose to do it in a new bug of course :-D. - Fixed some trivial perlcritic -5 errors - Removed some debugging stuff left by mistake - Fixed some POD problems - Optimal SQL-driven IsAuthorisedValueCategory method - Thanks to Owen and Jared for their patience heh. Sponsored-by: Universidad Nacional de Córdoba Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Comment: Work as described. No koha-qa errors. Test: Tried with examples (from help and test plan) reports, correctly identifies invalid authorized values, and no problem with authorized ones. NOTE: Online help for this does not states that partial values need to be between '%' in a SQLish way. Perhaps this could be addressed inserting % in values or adding a checkbox (partial|exact). Or changing help.
Hi Tomas - this looks quite good, but could you please provide unit tests for the new subs? You can do it as a QA follow up and switch back to signed off.
Created attachment 17351 [details] [review] Bug 9659 - QA Follow up: Unit tests Added some tests against the methods added by this patch. To test, prove -v - t/Koha.t - t/ReportsGuided.t Sponsored-by: Universidad Nacional de Córdoba
I forgot to run koha-qa.pl before sending. There's a perlcritic error.
There's seems to be no perlcritic error. Was a problem with my setup.
Hmm, this one seems to have strings on it.
Hi Tomas, running out of time this morning to investigate further, could you please check the unit tests? t/Koha.t (Wstat: 65280 Tests: 16 Failed: 5) Failed tests: 12-16 Non-zero exit status: 255 Parse errors: Bad plan. You planned 11 tests but ran 16. t/ReportsGuided.t (Wstat: 65280 Tests: 11 Failed: 0) Non-zero exit status: 255 Parse errors: Bad plan. You planned 12 tests but ran 11. Thx!
Created attachment 17652 [details] [review] Bug 9659 - QA Follow up: Unit tests Added some tests against the methods added by this patch. To test, prove -v - t/Koha.t - t/ReportsGuided.t Edit: fixed the amount of tests in the skip block. Tests fail for people having earlier versions of DBD::Mock. Sponsored-by: Universidad Nacional de Córdoba
Hi Tomas, wondering if you can use git bz with the diacritics in your commit messages? It always gives me trouble so I have to fall back to the old fashioned ways of attaching a patch here.
Created attachment 17660 [details] [review] PASSED-QA-Bug-9659-Better-handling-of-non-existent-authorised-values
Created attachment 17661 [details] [review] PASSED-QA-Bug-9659-QA-Follow-up-Unit-tests
This patch has been pushed to master and 3.12.x.