[Koha-bugs] [Bug 19059] Move C4::Reserves::CancelReserve to the Koha::Hold->cancel

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19059

--- Comment #31 from Jonathan Druart <jonathan.druart at bugs.koha-community.org> ---
(In reply to Marcel de Rooy from comment #21)
> Looks good to me. Few minor points left:
> 
> FIXME in sub ModReserveFill
> +    # FIXME Must call Koha::Hold->cancel ? => No, should call ->filled and
> add the correct log
> Not clear to me. Please clarify.

If we call ->cancel, we will log a "cancel" log. We should call a new method
->fill[ed] to add the "FILLED" log (or whatever, but not a cancel log)

> sub _move_to_old
> The name suggests a move, but you are only copying data.

Yes, I preferred to stick on the existing pattern (Koha::Patrons), I would like
to have again 1 more subroutine of this kind to know what is best.

> circ/branchtransfers.pl
> +    } # FIXME else?
> Can you fix it now? You add the if. Actually, the script always assumed that
> CancelReserve worked. So why not only test $holds->count for the call to
> cancel ?
> Same remark for other occurrences. Incl. circ/returns.pl

I wanted to avoid a crash with the $holds->next->cancel.
Moreover previous code displayed a "ok" message even if something went wrong.
It may be unnecessary, but I preferred to add it.

-- 
You are receiving this mail because:
You are watching all bug changes.