Bug 8175 - items.materials check logs error or displays incorrectly in details.pl
Summary: items.materials check logs error or displays incorrectly in details.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low normal (vote)
Assignee: Mark Tompsett
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-01 03:16 UTC by Mark Tompsett
Modified: 2013-05-23 06:23 UTC (History)
6 users (show)

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


Attachments
Undefined $items->{'materials'} fails a string check, so drop it. (897 bytes, patch)
2012-06-29 15:18 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 8175 - Missing defined check in catalogue/details.pl Actually, just dropping the "ne ''" was better. In some cases $item->{'materials'} isn't defined. Any value is true, so no need for comparison. (908 bytes, patch)
2012-07-01 00:56 UTC, Chris Cormack
Details | Diff | Splinter Review
Fixed the undefined issue and the intent of the ne '' (1.06 KB, patch)
2012-07-03 09:30 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 8175 - Check for something in materials field fails in catalogue/details.pl (1.07 KB, patch)
2012-08-09 13:27 UTC, Julian Maurice
Details | Diff | Splinter Review
items.materials=NULL triggers error, =' ' displays empty column (1.45 KB, patch)
2012-09-02 13:21 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 8175 - check logs error or displays incorrectly in details.pl (1.47 KB, patch)
2012-09-04 09:07 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2012-06-01 03:16:29 UTC
SchemaSpy says items.materials can be null
C4::Items seems to just pull the value from items.*
Error log was generating:
[Fri Jun 01 07:14:16 2012] [error] [client 192.168.100.2] [Fri Jun  1 07:14:16 2
012] detail.pl: Use of uninitialized value in string ne at /usr/share/koha/intra
net/cgi-bin/catalogue/detail.pl line 255.

I believe 255 should read:
 if (defined($item->{'materials'}) && $item->{'materials'} ne ''){
It currently reads:
 if ($item->{'materials'} ne ''){

I was looking at 3.6.3 source
I didn't check my 3.8.1 version
Comment 1 Owen Leonard 2012-06-01 14:39:53 UTC
Confirmed in master.
Comment 2 Mark Tompsett 2012-06-12 10:02:41 UTC
Under 3.8.1 is just below the analytics section that was added with tabs, instead of spaces. Line 260.
Comment 3 Mark Tompsett 2012-06-29 15:18:40 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2012-07-01 00:56:53 UTC Comment hidden (obsolete)
Comment 5 Ian Walls 2012-07-02 13:22:27 UTC
what about the case where $items->materials = 0?  Wouldn't that assess to false, and thus not display?  Zero could be a valid entry, especially if the items.materials field is linked to an authorised value list in the framework.
Comment 6 Mark Tompsett 2012-07-02 14:53:25 UTC
I see the validity of your point, Ian.

What is desired is:
NULL produce false
trim()='' produce false
everything else true (0,"0", and everything non-zero in string or number nature).

I'll do a patch up later, if someone else doesn't beat me to it first.
Comment 7 Mark Tompsett 2012-07-03 09:30:07 UTC Comment hidden (obsolete)
Comment 8 Julian Maurice 2012-08-09 13:27:04 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2012-08-09 14:17:41 UTC
Hi Mark,

Could you resubmit your patch after updating your commit message please ? (clarify "As such, a defined check handles the NULL, 0, and '0' cases.")
Comment 10 Mason James 2012-08-10 07:16:32 UTC
(In reply to comment #9)
> Hi Mark,
> 
> Could you resubmit your patch after updating your commit message please ?
> (clarify "As such, a defined check handles the NULL, 0, and '0' cases.")

status modded to 'in discussion'...
Comment 11 Mark Tompsett 2012-09-02 09:40:16 UTC
Okay, I finally have time to get back to this.

I first found this because my materials fields were all NULL.
mysql -u {kohauser} -p
> use {koha database};
> select distinct materials from items;
+-----------+
| materials |
+-----------+
| NULL      |
+-----------+
1 row in set (0.17 sec)

So, I fixed the bug, and then Ian piped up about 0. So, I decided to break it back to normal and test BEFORE the patch, and AFTER the patch.

TESTING
======
1) Log into Staff Client
2) Do a search to find an item
3) Click an item, and we should be at the detail.pl page for a biblionumber (eg. 19158).

CASE NULL
=========
4) Go back to mysql and:
    update items SET materials=NULL where biblionumber=19158;
5) Go back to browser and refresh:
    There is no "Materials Specified" column displayed.
6) Check koha-error_log
[Sun Sep 02 16:08:38 2012] [error] [client 192.168.100.2] [Sun Sep  2 16:08:38 2012] detail.pl: Use of uninitialized value in string ne at /usr/share/koha/intranet/cgi-bin/catalogue/detail.pl line 255., referer: https://.../cgi-bin/koha/catalogue/search.pl?q=the

CASE 0
======
7) Go back to mysql and:
    update items SET materials='0' where biblionumber=19158;
8) Go back to browser and refresh:
    There is a "Materials Specified" column with 0 displayed in it.
9) Check error log again. Nothing new.

CASE ''
=======
10) Go back to mysql and:
    update items SET materials='' where biblionumber=19158;
11) Go back to browser and refresh:
    There is no "Materials Specified" column displayed.
12) Check error log again. Nothing new.

CASE ' '
========
13) Go back to mysql and:
    update items SET materials='0' where biblionumber=19158;
14) Go back to browser and refresh:
    There is a "Materials Specified" column with nothing visible in it.
15) Check error log again. Nothing new.

CASE 'blah'
======
7) Go back to mysql and:
    update items SET materials='blah' where biblionumber=19158;
8) Go back to browser and refresh:
    There is a "Materials Specified" column with blah displayed in it.
9) Check error log again. Nothing new.

There are two things wrong: 1) the generated error, which caused me to find this bug, and 2) showing a column for no apparent reason.

| CASE   | Defined | =~ /\S/ | $var | ne '' | Should Display? | Display? |
+========+=========+=========+======+=======+=================+==========+
| NULL   | 0       | Error   | 0    | Error | 0               | 0        |
| ''     | 1       | 0       | 0    | 0     | 0               | 0        |
| '0'    | 1       | 1       | 0    | 1     | 1               | 1        |
| ' '    | 1       | 0       | 1    | 1     | 0               | 1        |
| 'blah' | 1       | 1       | 1    | 1     | 1               | 1        |
+========+=========+=========+======+=======+=================+==========+

The above table shows 1/0 values for potential replacement code or existing code options, and whether or not it displays correctly.

Patch and repeat the test cases. Once you are finished, you may wish to set your materials value back to what it was.

Seeing as materials is a 'text' type, the 0 vs. '0' difference is irrelevant. But I hope this table and test plan explains what I was thinking when I wrote those comments. I'll have patches up shortly.
Comment 12 Mark Tompsett 2012-09-02 13:21:33 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2012-09-04 09:07:42 UTC
Created attachment 11956 [details] [review]
Bug 8175 - check logs error or displays incorrectly in details.pl

Changed "$item->{'materials'} ne ''" to
"defined($item->{'materials'}) && $item->{'materials'} =~ /\S/"
in if condition to prevent error when it is NULL, and to
properly capture the intent of printing if there is something
visible.

Cases tested include NULL, '0', '', ' ', and 'blah' by using
 UPDATE items SET materials=NULL where biblionumber=19158;
where the biblionumber was chosen randomly, because only NULL
was in the items.materials field. The NULL case triggers an
error, but it does display correctly.

The ' ' case displays an apparently empty column, which does not
seem to be the intent of the flag that is being set. This is why
a simple $var check is not sufficient.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 14 Mark Tompsett 2012-10-02 14:04:12 UTC
This bug still exists in 3.6.x, 3.8.x, and master. That's why I changed the version to unspecified.
Comment 15 Paul Poulain 2012-10-12 14:54:19 UTC
QA comment
 * tiny patch, widely and wisely explained

passed QA
Comment 16 Paul Poulain 2012-10-12 14:57:09 UTC
Patch pushed to master
Comment 17 Chris Cormack 2012-10-12 18:35:19 UTC
I cant seem to find this in master. Has it not been pushed yet?
Comment 18 Paul Poulain 2012-10-12 19:26:55 UTC
(In reply to comment #17)
> I cant seem to find this in master. Has it not been pushed yet?

forgot to push to kc.org repo before leaving. pushed now
Comment 19 Chris Cormack 2012-10-14 07:33:14 UTC
Pushed to 3.8.x, will be in 3.8.6