Bug 31239

Summary: search_for_data_inconsistencies.pl fails for Koha to MARC mapping using biblio table
Product: Koha Reporter: Fridolin Somers <fridolin.somers>
Component: Command-line UtilitiesAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: arthur.suzuki, jonathan.druart, lucas, m.de.rooy, robin, tomascohen, victor
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31299
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.07, 21.11.14
Bug Depends on: 21466    
Bug Blocks:    
Attachments: Bug 31239: Fix incorrect AV linked to a mapped biblio column
Bug 31239: Fix incorrect AV linked to a mapped biblio column
Bug 31239: Fix incorrect AV linked to a mapped biblio column
Bug 31239: (QA follow-up) Fixing ternary formatting

Description Fridolin Somers 2022-07-26 08:57:26 UTC
The script misc/maintenance/search_for_data_inconsistencies.pl search for invalid values using Koha to MARC mapping fields.
The tables items and biblioitems are OK but mappings can be on biblio table :
  my $value = $table eq 'items' ? $i->$column : $i->biblioitem->$column;

For example in UNIMARC if fails with error :

The method Koha::Biblioitem->medium is not covered by tests!
Trace begun at /home/koha/src/Koha/Object.pm line 880
Koha::Object::AUTOLOAD('Koha::Biblioitem=HASH(0x55c9a1905a40)') called at /home/koha/src/misc/maintenance/search_for_data_inconsistencies.pl line 196
Comment 1 Katrin Fischer 2022-07-30 20:51:29 UTC
Which mapping caused the error?
Comment 2 Fridolin Somers 2022-08-05 18:31:27 UTC
(In reply to Katrin Fischer from comment #1)
> Which mapping caused the error?

biblio.medium on 200$b UNIMARC
Comment 3 Jonathan Druart 2022-08-09 14:09:32 UTC
How to recreate the problem in ktd using MARC21?
Comment 4 Fridolin Somers 2022-08-10 06:12:07 UTC
(In reply to Jonathan Druart from comment #3)
> How to recreate the problem in ktd using MARC21?

1) Go to framework BKS configuration
2) Edit 245$a to use an authorized value (in 'Other options'), like COUNTRY
3) Run ./misc/maintenance/search_for_data_inconsistencies.pl :
You get error :
== Wrong values linked to authorised values ==
The method Koha::Biblioitem->title is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Object.pm line 875
Koha::Object::AUTOLOAD('Koha::Biblioitem=HASH(0x564382f7c7e8)') called at misc/maintenance/search_for_data_inconsistencies.pl line 246
Comment 5 Jonathan Druart 2022-08-10 08:54:44 UTC
Yes, that's what I tried yesterday, but I don't get the error.
Comment 6 Jonathan Druart 2022-08-10 09:12:46 UTC
(In reply to Jonathan Druart from comment #5)
> Yes, that's what I tried yesterday, but I don't get the error.

I didn't have any biblio with frameworkcode="", I do recreate now.
Comment 7 Jonathan Druart 2022-08-10 09:18:10 UTC
Created attachment 138963 [details] [review]
Bug 31239: Fix incorrect AV linked to a mapped biblio column

If an AV is linked to a MARC field mapped with a biblio column, the
search_for_data_inconsistencies.pl script might explode with

The method Koha::Biblioitem->title is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Object.pm line 875
Koha::Object::AUTOLOAD('Koha::Biblioitem=HASH(0x556b67fa7168)') called at misc/maintenance/search_for_data_inconsistencies.pl line 246

Test plan:
For a given framework, pick a biblio using it
Link 245$a with an authorised value category
Run the script
=> Notice that with this script applied you will see the warning
=> Without this patch you got the error
Comment 8 Fridolin Somers 2022-08-10 23:10:27 UTC
I will QA
Comment 9 David Nind 2022-09-11 20:58:06 UTC
Created attachment 140413 [details] [review]
Bug 31239: Fix incorrect AV linked to a mapped biblio column

If an AV is linked to a MARC field mapped with a biblio column, the
search_for_data_inconsistencies.pl script might explode with

The method Koha::Biblioitem->title is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Object.pm line 875
Koha::Object::AUTOLOAD('Koha::Biblioitem=HASH(0x556b67fa7168)') called at misc/maintenance/search_for_data_inconsistencies.pl line 246

Test plan:
For a given framework, pick a biblio using it
Link 245$a with an authorised value category
Run the script
=> Notice that with this script applied you will see the warning
=> Without this patch you got the error

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 Marcel de Rooy 2022-09-16 09:42:53 UTC
Created attachment 140698 [details] [review]
Bug 31239: Fix incorrect AV linked to a mapped biblio column

If an AV is linked to a MARC field mapped with a biblio column, the
search_for_data_inconsistencies.pl script might explode with

The method Koha::Biblioitem->title is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Object.pm line 875
Koha::Object::AUTOLOAD('Koha::Biblioitem=HASH(0x556b67fa7168)') called at misc/maintenance/search_for_data_inconsistencies.pl line 246

Test plan:
For a given framework, pick a biblio using it
Link 245$a with an authorised value category
Run the script
=> Notice that with this script applied you will see the warning
=> Without this patch you got the error

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2022-09-16 09:42:57 UTC
Created attachment 140699 [details] [review]
Bug 31239: (QA follow-up) Fixing ternary formatting

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Jonathan Druart 2022-09-16 13:25:41 UTC
(In reply to Marcel de Rooy from comment #11)
> Created attachment 140699 [details] [review] [review]
> Bug 31239: (QA follow-up) Fixing ternary formatting
> 
> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

I totally disagree with this patch :D
It's the formatting of perltidy, and this patch does not make it better.
Comment 13 Tomás Cohen Arazi 2022-09-16 13:34:38 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 14 Tomás Cohen Arazi 2022-09-16 19:44:04 UTC
(In reply to Jonathan Druart from comment #12)
> (In reply to Marcel de Rooy from comment #11)
> > Created attachment 140699 [details] [review] [review] [review]
> > Bug 31239: (QA follow-up) Fixing ternary formatting
> > 
> > Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
> 
> I totally disagree with this patch :D
> It's the formatting of perltidy, and this patch does not make it better.

Damn, between the time I applied on my working branch and pushing you added this comment. I had doubts myself about that follow-up, as I tend to just call perltidy and this looked weird.

I will keep an eye on those.
Comment 15 Marcel de Rooy 2022-09-19 06:40:15 UTC
(In reply to Tomás Cohen Arazi from comment #14)
> (In reply to Jonathan Druart from comment #12)
> > (In reply to Marcel de Rooy from comment #11)
> > > Created attachment 140699 [details] [review] [review] [review] [review]
> > > Bug 31239: (QA follow-up) Fixing ternary formatting
> > > 
> > > Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
> > 
> > I totally disagree with this patch :D
> > It's the formatting of perltidy, and this patch does not make it better.
> 
> Damn, between the time I applied on my working branch and pushing you added
> this comment. I had doubts myself about that follow-up, as I tend to just
> call perltidy and this looked weird.
> 
> I will keep an eye on those.

Matter of taste?
Comment 16 Tomás Cohen Arazi 2022-09-19 10:43:28 UTC
(In reply to Marcel de Rooy from comment #15)
> (In reply to Tomás Cohen Arazi from comment #14)
> > (In reply to Jonathan Druart from comment #12)
> > > (In reply to Marcel de Rooy from comment #11)
> > > > Created attachment 140699 [details] [review] [review] [review] [review] [review]
> > > > Bug 31239: (QA follow-up) Fixing ternary formatting
> > > > 
> > > > Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
> > > 
> > > I totally disagree with this patch :D
> > > It's the formatting of perltidy, and this patch does not make it better.
> > 
> > Damn, between the time I applied on my working branch and pushing you added
> > this comment. I had doubts myself about that follow-up, as I tend to just
> > call perltidy and this looked weird.
> > 
> > I will keep an eye on those.
> 
> Matter of taste?

It is not really relevant, Marcel. I just wanted to point perltidy generates nice formatting and is encouraged, but it is all a matter of taste after all.
Comment 17 Fridolin Somers 2022-09-23 06:50:16 UTC
Maybe we should not use ternary inside ternary with if elsif else.
But it works, that's the most important ;)
Comment 18 Lucas Gass 2022-10-31 21:46:45 UTC
Backported to 22.05.x for upcoming 22.05.07 release
Comment 19 Arthur Suzuki 2022-11-14 14:26:05 UTC
applied to 21.11 for 21.11.14
Comment 20 Victor Grousset/tuxayo 2022-11-24 20:37:41 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document it seems, marking resolved.