Bug 28960 - EDI transfer_items uses a relationship where it's looking for a field
Summary: EDI transfer_items uses a relationship where it's looking for a field
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-07 11:12 UTC by Martin Renvoize
Modified: 2021-10-18 02:16 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
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 (1.27 KB, patch)
2021-09-07 11:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28960: Explicitly call get_column (1.34 KB, patch)
2021-09-07 12:36 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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