Bug 14711 - C4::Reserves::AddReserves should take a hashref in parameters
Summary: C4::Reserves::AddReserves should take a hashref in parameters
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 24769
  Show dependency treegraph
 
Reported: 2015-08-24 13:15 UTC by Marcel de Rooy
Modified: 2020-11-30 21:45 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 14711: We do no longer need bibitems in AddReserve (19.25 KB, patch)
2015-09-02 11:56 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 14711: Change prototype for AddReserve - pass a hashref (74.41 KB, patch)
2018-08-16 18:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14711: Change prototype for AddReserve - pass a hashref (94.15 KB, patch)
2020-02-11 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14711: Change prototype for AddReserve - pass a hashref (94.18 KB, patch)
2020-02-11 14:25 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2015-08-24 13:15:32 UTC
If bug 9809 and bug 14702 make their way into Koha, time comes to remove the bibitems parameter from AddReserve. In that case it is no longer used (code for reserve fee calculation being replaced).

Note that this parameter was not supplied in the same form consistently thru Koha. The old code could handle that; the new code does not need it.
Comment 1 Joonas Kylmälä 2015-09-02 11:56:45 UTC
Created attachment 42198 [details] [review]
Bug 14711: We do no longer need bibitems in AddReserve

Removes the unnecessary bibitems parameter from all the calls to
AddReserve function and also from the function itself.

Test plan:

1. Try some (if not all) of the pages' functionalities where the code
   was changed.
2. Make sure they work like before and there is no errors in
   the koha-error log.
3. If you didn't try all the pages make sure the code changes in the
   other pages are exactly like they were in the pages you tested.
4. Run all the test and look if they pass
Comment 2 Jonathan Druart 2015-10-06 13:22:44 UTC
Bug 9809 already updated all calls to AddReserve, to prevent a future change again it could worth to change the prototype of this subroutine to make it get a hashref.
Comment 3 Joonas Kylmälä 2015-10-09 08:47:37 UTC
(In reply to Jonathan Druart from comment #2)
> Bug 9809 already updated all calls to AddReserve, to prevent a future change
> again it could worth to change the prototype of this subroutine to make it
> get a hashref.

Attachment 42198 [details] is now obsolete, right? Shall we create a new bug report for the hashref change?
Comment 4 Jonathan Druart 2015-10-09 09:53:16 UTC
no, just submit another patch with the call updated.
Comment 5 Joonas Kylmälä 2018-08-16 12:08:27 UTC
I'm not working on this anymore, somebody else feel free to take this further.
Comment 6 Jonathan Druart 2018-08-16 18:36:06 UTC
Created attachment 77940 [details] [review]
Bug 14711: Change prototype for AddReserve - pass a hashref

The number of parameters of AddReserve makes it hard to read and
maintain.
This patch replace it with a hashref, which will make the calls more
readable.

Moreover the bibitems has been removed as it was not used by the
subroutine.

Test plan:
- Make sure the tests pass
- Read the diff and search for typos
- Place a hold on few items

Note for QA: reservation_date and expiration_date do not match the DB column's names,
should we?
Comment 7 David Nind 2020-02-10 09:55:19 UTC
Path no longer applies 8-(
Comment 8 Jonathan Druart 2020-02-11 14:03:14 UTC
Created attachment 98708 [details] [review]
Bug 14711: Change prototype for AddReserve - pass a hashref

The number of parameters of AddReserve makes it hard to read and
maintain.
This patch replace it with a hashref, which will make the calls more
readable.

Moreover the bibitems has been removed as it was not used by the
subroutine.

Test plan:
- Make sure the tests pass
- Read the diff and search for typos
- Place a hold on few items

Note for QA: reservation_date and expiration_date do not match the DB column's names,
should we?
Comment 9 Martin Renvoize 2020-02-11 14:25:58 UTC
Created attachment 98710 [details] [review]
Bug 14711: Change prototype for AddReserve - pass a hashref

The number of parameters of AddReserve makes it hard to read and
maintain.
This patch replace it with a hashref, which will make the calls more
readable.

Moreover the bibitems has been removed as it was not used by the
subroutine.

Test plan:
- Make sure the tests pass
- Read the diff and search for typos
- Place a hold on few items

Note for QA: reservation_date and expiration_date do not match the DB column's names,
should we?

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2020-02-11 14:28:05 UTC
Works as expected.. a look through the code doesn't highlight any issues regressions..

I'm tempted to PQA and push and let Jenkins do it's thing here rather than let it languish and require another rebase.

As for the note.. I think we're safe with the signature.. the db column name mismatch isn't the first and should likely get cleaned up as a distinct issue upon more to Koha::

PQA
Comment 11 Martin Renvoize 2020-02-11 14:38:55 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 12 Joy Nelson 2020-03-05 00:49:20 UTC
Enhancement not pushed to 19.11.x