Bug 40055

Summary: C4::Reserves::MoveReserve should be passed objects
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi (tcohen) <tomascohen>
Status: Passed QA ---    
Severity: enhancement CC: jonathan.druart, nick, tomascohen
Priority: P5 - low    
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: Trivial patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on:    
Bug Blocks: 40058    
Attachments: Bug 40055: Make MoveReserve be passed objects instead of ids
Bug 40055: Make MoveReserve be passed objects instead of ids
Bug 40055: Make MoveReserve be passed objects instead of ids
Bug 40055: Make MoveReserve be passed objects instead of ids

Description Tomás Cohen Arazi (tcohen) 2025-06-02 21:07:27 UTC
The `MoveReserve` method is called once, in `AddIssue`. The latter has the related `Koha::Item` object already fetched from the DB, but it passed the `itemnumber` which is used by `MoveReserve` to fetch the item from the DB. This should not happen.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-06-02 21:13:42 UTC
Created attachment 182915 [details]
Bug 40055: Make MoveReserve be passed objects instead of ids

The `MoveReserve` method is called once, in `AddIssue`. The latter has the related `Koha::Item` object already fetched from the DB, but it passed the `itemnumber` which is used by `MoveReserve` to fetch the item from the DB. This should not happen.

To test:
1. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Reserves* \
           t/db_dependent/Koha/Hold* \
           t/db_dependent/Hold* \
           t/db_dependent/Circulation*
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Sign off :-D
Comment 2 David Nind 2025-06-04 14:58:38 UTC
Created attachment 182953 [details]
Bug 40055: Make MoveReserve be passed objects instead of ids

The `MoveReserve` method is called once, in `AddIssue`. The latter has the related `Koha::Item` object already fetched from the DB, but it passed the `itemnumber` which is used by `MoveReserve` to fetch the item from the DB. This should not happen.

To test:
1. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Reserves* \
           t/db_dependent/Koha/Hold* \
           t/db_dependent/Hold* \
           t/db_dependent/Circulation*
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Jonathan Druart 2025-06-05 07:51:28 UTC
I don't like the use of 'id'. We have borrowernumber and userid on this object.
And we have patron_id for the REST API.

It might be obvious for you and me, but for new developers it can be very confusing I think.
Comment 4 Jonathan Druart 2025-06-05 07:53:47 UTC
Or we batch replace all the occurrences.
Otherwise we are just adding yet another inconsistency.
Comment 5 Tomás Cohen Arazi (tcohen) 2025-06-05 12:39:18 UTC
(In reply to Jonathan Druart from comment #3)
> I don't like the use of 'id'. We have borrowernumber and userid on this
> object.
> And we have patron_id for the REST API.
> 
> It might be obvious for you and me, but for new developers it can be very
> confusing I think.

I always felt like ->id (being the defined PRIMARY KEY) was a good practice as, in general, we end up using `$patron->id` which reads as 'the patron id' (at least in my brain).

In this case, the comparison is with a 'borrowernumber' attribute, so I'd say I will change it to `$patron->borrowernumber` as it reads well.
Comment 6 Tomás Cohen Arazi (tcohen) 2025-06-05 12:44:59 UTC
Created attachment 182989 [details]
Bug 40055: Make MoveReserve be passed objects instead of ids

The `MoveReserve` method is called once, in `AddIssue`. The latter has the related `Koha::Item` object already fetched from the DB, but it passed the `itemnumber` which is used by `MoveReserve` to fetch the item from the DB. This should not happen.

To test:
1. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Reserves* \
           t/db_dependent/Koha/Hold* \
           t/db_dependent/Hold* \
           t/db_dependent/Circulation*
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 Tomás Cohen Arazi (tcohen) 2025-06-05 12:45:37 UTC
(In reply to Jonathan Druart from comment #3)
> I don't like the use of 'id'. We have borrowernumber and userid on this
> object.

I changed it to `$patron->borrowernumber`. Thanks for reviewing it.
Comment 8 Jonathan Druart 2025-06-05 13:14:03 UTC
Created attachment 182999 [details]
Bug 40055: Make MoveReserve be passed objects instead of ids

The `MoveReserve` method is called once, in `AddIssue`. The latter has the related `Koha::Item` object already fetched from the DB, but it passed the `itemnumber` which is used by `MoveReserve` to fetch the item from the DB. This should not happen.

To test:
1. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Reserves* \
           t/db_dependent/Koha/Hold* \
           t/db_dependent/Hold* \
           t/db_dependent/Circulation*
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Sign off :-D

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>