Bug 9659 - Undefined authorised value category yields empty dropdown menu on SQL reports
Summary: Undefined authorised value category yields empty dropdown menu on SQL reports
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-18 15:56 UTC by Tomás Cohen Arazi
Modified: 2014-05-26 21:04 UTC (History)
4 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9659 - Better handling of non-existent authorised value categories used in SQL reports (9.19 KB, patch)
2013-02-19 19:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 9659 - Better handling of non-existent authorised value categories used in SQL reports (22.96 KB, patch)
2013-02-21 18:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 9659 - Better handling of non-existent authorised value categories used in SQL reports (23.69 KB, patch)
2013-02-21 21:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 9659 - Better handling of non-existent authorised value categories used in SQL reports (26.80 KB, patch)
2013-02-22 13:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 9659 - Better handling of non-existent authorised value categories used in SQL reports (26.90 KB, patch)
2013-02-22 14:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9659 - Better handling of non-existent authorised value categories used in SQL reports (27.38 KB, patch)
2013-02-22 16:56 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 9659 - QA Follow up: Unit tests (5.52 KB, patch)
2013-04-10 18:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 9659 - QA Follow up: Unit tests (5.63 KB, patch)
2013-04-24 13:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
PASSED-QA-Bug-9659-Better-handling-of-non-existent-authorised-values (27.47 KB, patch)
2013-04-25 05:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
PASSED-QA-Bug-9659-QA-Follow-up-Unit-tests (5.75 KB, patch)
2013-04-25 05:46 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2013-02-18 15:56:11 UTC
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.
Comment 1 Tomás Cohen Arazi 2013-02-19 19:37:28 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2013-02-19 19:40:19 UTC
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.
Comment 3 Tomás Cohen Arazi 2013-02-21 18:53:04 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2013-02-21 21:23:48 UTC Comment hidden (obsolete)
Comment 5 Bernardo Gonzalez Kriegel 2013-02-21 22:39:09 UTC
Tom,
there are small pod errors, but the example queries in online help
also generates warnings. Is that ok?
Comment 6 David Cook 2013-02-22 00:36:47 UTC
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 :/.
Comment 7 Tomás Cohen Arazi 2013-02-22 13:40:04 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2013-02-22 13:54:10 UTC
(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.
Comment 9 Tomás Cohen Arazi 2013-02-22 13:57:23 UTC
(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.
Comment 10 Frédéric Demians 2013-02-22 14:09:22 UTC
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;
}
Comment 11 Tomás Cohen Arazi 2013-02-22 14:47:00 UTC Comment hidden (obsolete)
Comment 12 Bernardo Gonzalez Kriegel 2013-02-22 16:56:11 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2013-03-21 19:54:39 UTC
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.
Comment 14 Tomás Cohen Arazi 2013-04-10 18:53:31 UTC Comment hidden (obsolete)
Comment 15 Tomás Cohen Arazi 2013-04-10 19:01:36 UTC
I forgot to run koha-qa.pl before sending. There's a perlcritic error.
Comment 16 Tomás Cohen Arazi 2013-04-10 21:35:34 UTC
There's seems to be no perlcritic error. Was a problem with my setup.
Comment 17 Tomás Cohen Arazi 2013-04-19 01:40:34 UTC
Hmm, this one seems to have strings on it.
Comment 18 Katrin Fischer 2013-04-24 06:03:40 UTC
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!
Comment 19 Tomás Cohen Arazi 2013-04-24 13:09:26 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2013-04-25 05:39:33 UTC
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.
Comment 21 Katrin Fischer 2013-04-25 05:46:06 UTC
Created attachment 17660 [details] [review]
PASSED-QA-Bug-9659-Better-handling-of-non-existent-authorised-values
Comment 22 Katrin Fischer 2013-04-25 05:46:31 UTC
Created attachment 17661 [details] [review]
PASSED-QA-Bug-9659-QA-Follow-up-Unit-tests
Comment 23 Jared Camins-Esakov 2013-04-26 12:10:38 UTC
This patch has been pushed to master and 3.12.x.