Bug 26516

Summary: Importing records with unexpected format of copyrightdate fails
Product: Koha Reporter: Nick Clemens <nick>
Component: ToolsAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: andrewfh, david, heather_hernandez, lisettepalouse+koha, martin.renvoize, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.07, 19.11.13
Bug Depends on: 11046    
Bug Blocks:    
Attachments: Bug 26516: Check if int before save
Bug 26516: [alternate] Don't accept incorrect values for copyrightdate/publicationyear
Bug 26516: [alternate] Don't accept incorrect values for copyrightdate/publicationyear
Bug 26516: [alternate] Don't accept incorrect values for copyrightdate/publicationyear

Description Nick Clemens 2020-09-23 14:40:18 UTC
Bug 11046 attempted to improve the parsing of publicationyear/copyrightdate from marc records.

Unfortunately copyrightdate is strictly an integer field, so now adds/imports fail if invalid data is passed

To recreate:
1 - Add a new record to Koha making sure data is valid except 260$c:
    198-
2 - Save the record
3 - It fails

DBD::mysql::st execute failed: Data truncated for column 'copyrightdate' at row 1 [for Statement "INSERT INTO biblio
        SET frameworkcode = ?,
            author = ?,
            title = ?,
            subtitle = ?,
            medium = ?,
            part_number = ?,
            part_name = ?,
            unititle =?,
            notes = ?,
            serial = ?,
            seriestitle = ?,
            copyrightdate = ?,
            datecreated=NOW(),
            abstract = ?
        " with ParamValues: 0='', 1=undef, 2="This record", 3=undef, 4=undef, 5=undef, 6=undef, 7=undef, 8=undef, 9=0, 10=undef, 11="198-", 12=undef] at /kohadevbox/koha/C4/Biblio.pm line 2745.
ERROR in _koha_add_biblio INSERT INTO biblio
Comment 1 Nick Clemens 2020-09-23 14:42:17 UTC
Created attachment 110602 [details] [review]
Bug 26516: Check if int before save

This is how we handle it when modifying a biblio

To test:
To recreate:
1 - Add a new record to Koha making sure data is valid except 260$c:
    198-
2 - Save the record
3 - It fails
4 - Apply patch
5 - restart all the things
6 - Repeat
7 - Success!
Comment 2 Nick Clemens 2020-09-23 14:45:55 UTC
Created attachment 110605 [details] [review]
Bug 26516: [alternate] Don't accept incorrect values for copyrightdate/publicationyear

The previous patch rejects incorrect values when saving to the db specifically for copyrightdate
An error is thrown for the int conversion when it fails

Rather than catching things when saving we can fix the value when generated, simply returning null when parsing
the record

To test:
1 - Add a new record to Koha making sure data is valid except 260$c:
    198-
2 - Save the record
3 - It fails
4 - Apply this patch
5 - Restart all the thigns
6 - Repeat
7 - Success!
Comment 3 Lisette Scheer 2020-09-24 16:29:11 UTC
Before applying the patch, when adding a record with 198- in the 260$c: field, we got the following error on the sandbox:

Broken FK constraint at /usr/share/perl5/Exception/Class/Base.pm line 88
in Exception::Class::Base::throw at /usr/share/perl5/Exception/Class/Base.pm line 88

We were not able to import a record with 198- in the 260$c: field before applying the patch. 

After the patch was applied we still got the same error when trying to add a new record with 198- in the 260$c: field. We were able to export the record, edit it to have 198- and import the record successfully. 


Owen wasn't able to recreate the error below on his test server. 
 
Broken FK constraint at /usr/share/perl5/Exception/Class/Base.pm line 88
in Exception::Class::Base::throw at /usr/share/perl5/Exception/Class/Base.pm line 88

Lisette
Comment 4 David Nind 2020-11-03 10:27:26 UTC
Created attachment 112905 [details] [review]
Bug 26516: [alternate] Don't accept incorrect values for copyrightdate/publicationyear

The previous patch rejects incorrect values when saving to the db specifically for copyrightdate
An error is thrown for the int conversion when it fails

Rather than catching things when saving we can fix the value when generated, simply returning null when parsing
the record

To test:
1 - Add a new record to Koha making sure data is valid except 260$c:
    198-
2 - Save the record
3 - It fails
4 - Apply this patch
5 - Restart all the thigns
6 - Repeat
7 - Success!

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2020-11-03 10:35:40 UTC
(In reply to Lisette Scheer from comment #3)
> Before applying the patch, when adding a record with 198- in the 260$c:
> field, we got the following error on the sandbox:
> 
> Broken FK constraint at /usr/share/perl5/Exception/Class/Base.pm line 88
> in Exception::Class::Base::throw at /usr/share/perl5/Exception/Class/Base.pm
> line 88

I got this error as well locally using koha-testing-docker. For me the error mentioned in the description appeared in /var/log/koha/kohadev/plack-intranet-error.log (not sure whether this will be the same when using kohadevbox).

I imported a record using Z39.50 from the Library of Congress (title = IBM Style Guide), and changed the 260$c to 198- 

After applying the patch I went flush_memcached and restart_all, redid the import and everything worked as per the test plan - successfully able to add a record with 198- for $260$c.
Comment 6 Martin Renvoize 2020-11-16 09:57:52 UTC
Created attachment 113655 [details] [review]
Bug 26516: [alternate] Don't accept incorrect values for copyrightdate/publicationyear

The previous patch rejects incorrect values when saving to the db specifically for copyrightdate
An error is thrown for the int conversion when it fails

Rather than catching things when saving we can fix the value when generated, simply returning null when parsing
the record

To test:
1 - Add a new record to Koha making sure data is valid except 260$c:
    198-
2 - Save the record
3 - It fails
4 - Apply this patch
5 - Restart all the thigns
6 - Repeat
7 - Success!

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2020-11-16 10:01:53 UTC
A simple elegant fix.. I did wonder if we aught to use the same form as we do for the ISBD rules and simply replace the '-' with '0'.. but I'm now really sure that would benefit us in any meaningful way here.


Passing QA
Comment 8 Jonathan Druart 2020-11-17 11:26:07 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 9 Andrew Fuerste-Henry 2020-12-09 20:13:10 UTC
Pushed to 20.05.x for 20.05.07
Comment 10 Victor Grousset/tuxayo 2020-12-10 13:14:07 UTC
Backported to 19.11.x branch for 19.11.13