Bug 27007

Summary: GetMarcSubfieldStructure called with "unsafe" in tests
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Test SuiteAssignee: Jonathan Druart <jonathan.druart>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: andrew, joonas.kylmala, martin.renvoize, tomascohen, victor
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.07
Bug Depends on: 23807    
Bug Blocks:    
Attachments: Bug 27007: Don't call GetMarcSubfieldStructure with unsafe in tests
Bug 27007: Don't call GetMarcSubfieldStructure with unsafe in tests

Description Jonathan Druart 2020-11-13 08:32:25 UTC
It does not feel correct to call GetMarcSubfieldStructure with the unsafe flag set in tests:
   my $mss = C4::Biblio::GetMarcSubfieldStructure( '', { unsafe => 1 } );

Some tests are failing is run twice

Use of uninitialized value $code_wanted in string eq at /usr/share/perl5/MARC/Field.pm line 314.
[x~10]
Use of uninitialized value $code_wanted in string eq at /usr/share/perl5/MARC/Field.pm line 314.
                                                                       
    #   Failed test 'Value is mapped correctly for column biblionumber'
    #   at t/db_dependent/Koha/Item.t line 115.          
    #          got: undef                                                                    
    #     expected: '483'                              
[etc.]
Comment 1 Jonathan Druart 2020-11-13 08:33:59 UTC
Created attachment 113585 [details] [review]
Bug 27007: Don't call GetMarcSubfieldStructure with unsafe in tests

It does not feel correct to call GetMarcSubfieldStructure with the unsafe flag set in tests:
   my $mss = C4::Biblio::GetMarcSubfieldStructure( '', { unsafe => 1 } );

Some tests are failing is run twice

Use of uninitialized value $code_wanted in string eq at /usr/share/perl5/MARC/Field.pm line 314.
[x~10]
Use of uninitialized value $code_wanted in string eq at /usr/share/perl5/MARC/Field.pm line 314.

    #   Failed test 'Value is mapped correctly for column biblionumber'
    #   at t/db_dependent/Koha/Item.t line 115.
    #          got: undef
    #     expected: '483'
[etc.]

Test plan:
Run the tests twice, without the patch it fails, with the patch it
passes
Comment 2 Martin Renvoize 2020-11-13 09:40:26 UTC
Created attachment 113587 [details] [review]
Bug 27007: Don't call GetMarcSubfieldStructure with unsafe in tests

It does not feel correct to call GetMarcSubfieldStructure with the unsafe flag set in tests:
   my $mss = C4::Biblio::GetMarcSubfieldStructure( '', { unsafe => 1 } );

Some tests are failing is run twice

Use of uninitialized value $code_wanted in string eq at /usr/share/perl5/MARC/Field.pm line 314.
[x~10]
Use of uninitialized value $code_wanted in string eq at /usr/share/perl5/MARC/Field.pm line 314.

    #   Failed test 'Value is mapped correctly for column biblionumber'
    #   at t/db_dependent/Koha/Item.t line 115.
    #          got: undef
    #     expected: '483'
[etc.]

Test plan:
Run the tests twice, without the patch it fails, with the patch it
passes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2020-11-13 09:41:26 UTC
I dug into the code and agree with this change.. tests well.. Signing off
Comment 4 Tomás Cohen Arazi 2020-11-13 12:31:05 UTC
I never went back to this, sorry. The idea was that eventually the caller would fetch the mss data, accumulate it as needed, and pass the right one to the method as a parameter.

The context is you will iterate on the biblio's items and it doesn't make sense to retrieve the whole thing.

I promise I'll be back to this on this new cycle.

I agree with the change BTW.
Comment 5 Jonathan Druart 2020-11-13 13:30:24 UTC
*** Bug 26972 has been marked as a duplicate of this bug. ***
Comment 6 Jonathan Druart 2020-11-20 09:37:42 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 7 Andrew Fuerste-Henry 2020-12-09 21:56:46 UTC
Pushed to 20.05.x for 20.05.07
Comment 8 Victor Grousset/tuxayo 2020-12-10 17:03:49 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.