Bug 25293 - Don't call escapeHtml on null
Summary: Don't call escapeHtml on null
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Assignee: didier
QA Contact: Testopia
URL:
Keywords:
: 25678 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-04-27 14:55 UTC by didier
Modified: 2020-09-18 20:02 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.03, 19.11.09


Attachments
Bug 25293: Don't call escapeHtml on null (3.71 KB, patch)
2020-04-28 14:18 UTC, didier
Details | Diff | Splinter Review
Bug 25293: Don't call escapeHtml on null (3.78 KB, patch)
2020-07-07 14:02 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 25293: Don't call escapeHtml on null (3.84 KB, patch)
2020-07-13 21:31 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 didier 2020-04-27 14:55:14 UTC
EscapeHtml is still called on many fields without testing if they exist:
Uncaught TypeError: Cannot read property 'escapeHtml' of null

Fields like title, branchcode and so on aren't guarantee to be not null.

If they are, for whatever reason, broken items are often next to unfixable within Koha.
Comment 1 didier 2020-04-28 14:18:12 UTC
Created attachment 103862 [details] [review]
Bug 25293: Don't call escapeHtml on null

 Test Plan:

    1) Create two Checkouts
    2) view patron's checkout list cgi-bin/koha/members/moremember.pl?borrowernumber=pp <show checkout>
    3) in mysql shell delete a checkout home branch: update items set homebranch = null where itemnumber = xx;
    4) view patron's checkout list, again

    Without this patch the list is empty.

    Same if biblio title is null (update biblio set title = null where biblionumber = yy;).
    Javascript error:
    TypeError: oObj.title is null
Comment 2 Frédéric Demians 2020-04-30 09:11:54 UTC
How an item can have a 'homebranch' field empty or how can a biblio record have no title?
Comment 3 didier 2020-04-30 10:36:28 UTC
(In reply to Frédéric Demians from comment #2)
> How an item can have a 'homebranch' field empty or how can a biblio record
> have no title?

Hi,

No idea, likely a bug somewhere, but they do.
It seems that some imported MARC data can produce biblio records without title though.

Anyway, both 
update items set homebranch = null where itemnumber = ...;
and
update biblio set title = null where biblionumber = ...;
don't return an error, thus from the DB DDL POV they are valid, if they are not then something at the application level has to deal with it.

checkouts.js is likely not the right place, only one with the fewer unexpected side effects.

Regards
Didier
Comment 4 George Veranis 2020-06-01 07:58:45 UTC
(In reply to Frédéric Demians from comment #2)
> How an item can have a 'homebranch' field empty or how can a biblio record
> have no title?

A biblio can not have a title if you not entered in biblio framework , some cataloguers do that for some special records.
Comment 5 David Cook 2020-06-02 01:38:00 UTC
I usually encounter this issue because of the Keyword To MARC mapping removal, which caused some really bad Koha to MARC mappings in the biblio. 

It caused a lot of problems for us.
Comment 6 Jonathan Druart 2020-06-05 08:41:54 UTC
*** Bug 25678 has been marked as a duplicate of this bug. ***
Comment 7 Jonathan Druart 2020-06-05 08:43:29 UTC
(In reply to David Cook from comment #5)
> I usually encounter this issue because of the Keyword To MARC mapping
> removal, which caused some really bad Koha to MARC mappings in the biblio. 
> 
> It caused a lot of problems for us.

So, sign it off?
Comment 8 David Cook 2020-06-09 01:14:20 UTC
(In reply to Jonathan Druart from comment #7)
> (In reply to David Cook from comment #5)
> > I usually encounter this issue because of the Keyword To MARC mapping
> > removal, which caused some really bad Koha to MARC mappings in the biblio. 
> > 
> > It caused a lot of problems for us.
> 
> So, sign it off?

That takes time and effort, and it isn't a local priority at the moment. It's on my TODO list, but it's a long list with other higher priorities.

This issue almost always indicates a misconfiguration or bad data, so it's not really a critical issue. 

Since it's a non-critical issue, I could just sign it off, but I rather put the time in to test it fully. But that means it's not going to happen today.
Comment 9 Owen Leonard 2020-07-07 14:02:47 UTC
Created attachment 106634 [details] [review]
Bug 25293: Don't call escapeHtml on null

Test Plan:

    1) Create two Checkouts
    2) view patron's checkout list
       cgi-bin/koha/members/moremember.pl?borrowernumber=pp <show
       checkout>
    3) in mysql shell delete a checkout home branch: update items set
       homebranch = null where itemnumber = xx;
    4) view patron's checkout list, again

    Without this patch the list is empty.

    Same if biblio title is null (update biblio set title = null where
    biblionumber = yy;).
    Javascript error:
    TypeError: oObj.title is null

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 10 Katrin Fischer 2020-07-13 21:31:03 UTC
Created attachment 106854 [details] [review]
Bug 25293: Don't call escapeHtml on null

Test Plan:

    1) Create two Checkouts
    2) view patron's checkout list
       cgi-bin/koha/members/moremember.pl?borrowernumber=pp <show
       checkout>
    3) in mysql shell delete a checkout home branch: update items set
       homebranch = null where itemnumber = xx;
    4) view patron's checkout list, again

    Without this patch the list is empty.

    Same if biblio title is null (update biblio set title = null where
    biblionumber = yy;).
    Javascript error:
    TypeError: oObj.title is null

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Jonathan Druart 2020-07-20 12:05:03 UTC
There are also those two, but apparently they cannot be undefined

 724                     {
 725                         "mDataProp": function ( oObj ) {
 726                             return oObj.recordtype_description.escapeHtml();
 727                         }
 728                     },
 729                     {
 730                         "mDataProp": function ( oObj ) {
 731                             return oObj.itemtype_description.escapeHtml();
 732                         }
 733                     },
Comment 12 Jonathan Druart 2020-07-20 12:08:01 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 13 Lucas Gass 2020-07-25 17:51:48 UTC
backported to 20.05.x for 20.05.03
Comment 14 Aleisha Amohia 2020-08-03 22:43:19 UTC
backported to 19.11.x for 19.11.09
Comment 15 Victor Grousset/tuxayo 2020-09-18 20:02:49 UTC
Not backported to oldoldstable (19.05.x). Feel free to ask if it's needed.