Bug 28960

Summary: EDI transfer_items uses a relationship where it's looking for a field
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: AcquisitionsAssignee: Martin Renvoize <martin.renvoize>
Status: Pushed to oldoldoldstable --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: fridolin.somers, jon.turner, jonathan.druart, kyle, victor, wainuiwitikapark
Version: Main   
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:
21.11.00,21.05.05,20.11.11,20.05.17,19.11.23
Attachments: Bug 28960: Explicitly call get_column
Bug 28960: Explicitly call get_column

Description Martin Renvoize 2021-09-07 11:12:40 UTC
In Koha::EDI::transfer_items we do a branchcode comparison. Unfortunately, the comparison is called against a DBIx::Class relationship accessor instead of explicitly calling get_column to get the corresponding field value instead of a full fledged DBIx::Class::Result object.

This can cause issues in proceedings.
Comment 1 Martin Renvoize 2021-09-07 11:15:31 UTC
Created attachment 124572 [details] [review]
Bug 28960: Explicitly call get_column

This patch adds get_column to ensure we are returning a string value of
a field rather than accidentally triggering a relationship accessor.
Comment 2 Kyle M Hall 2021-09-07 12:36:45 UTC
Created attachment 124578 [details] [review]
Bug 28960: Explicitly call get_column

This patch adds get_column to ensure we are returning a string value of
a field rather than accidentally triggering a relationship accessor.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 3 Jonathan Druart 2021-09-09 08:59:23 UTC
Isn't the correct fix to call $item->home_branch->branchcode?

From Koha::Item

sub home_branch {
    my ($self) = @_;

    $self->{_home_branch} ||= Koha::Libraries->find( $self->homebranch() );

    return $self->{_home_branch};
}
Comment 4 Martin Renvoize 2021-09-09 12:51:14 UTC
(In reply to Jonathan Druart from comment #3)
> Isn't the correct fix to call $item->home_branch->branchcode?
> 
> From Koha::Item
> 
> sub home_branch {
>     my ($self) = @_;
> 
>     $self->{_home_branch} ||= Koha::Libraries->find( $self->homebranch() );
> 
>     return $self->{_home_branch};
> }

There are two things with that.

1) $item is a DBIx::Class::Result and not a Koha::Item in EDI (Yes, we should certainly convert to Koha Objects here at some point)
2) if all we need is the code, why call through another DB hitting accessor rather than use the field in the first set directly?
Comment 5 Jonathan Druart 2021-09-10 07:55:36 UTC
I hadn't realized we weren't manipulating a Koha::Object. And my remark was silly anyway as $item->branchcode is working if $item is a Koha::Item.

Sorry for the noise.
Comment 6 Martin Renvoize 2021-09-10 08:45:01 UTC
:).. it's certainly on my list to switch over to objects.. but before that I need to blitz some tests.. why is EDI so darn hard!
Comment 7 Jonathan Druart 2021-09-20 12:48:06 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 8 Kyle M Hall 2021-10-01 17:29:54 UTC
Pushed to 21.05.x for 21.05.x
Comment 9 Fridolin Somers 2021-10-06 20:04:21 UTC
Pushed to 20.11.x for 20.11.11
Comment 10 Victor Grousset/tuxayo 2021-10-15 20:15:56 UTC
Backported: Pushed to 20.05.x branch for 20.05.17
Comment 11 wainuiwitikapark 2021-10-18 02:16:26 UTC
Backported to 19.11.x for 19.11.23