[Koha-bugs] [Bug 19532] Recalls for Koha

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

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

--- Comment #358 from Martin Renvoize <martin.renvoize at ptfs-europe.com> ---
I've uploaded an updated patchset with some simple followups added.

However, I've got a few code review comments, and I've not got to the full
testing of the feature itself yet. I don't think this is likely to be ready for
18.11 personally, but it would be good to get the groundwork done to get it in
at the beginning of the next cycle.

Review:
This really feels like it should be built atop the existing Holds/Reserves
system. Everyone I've spoken to, to date, has been really surprised that it
appears to be an entirely distinct feature.  It also feels like allot of code
was borrowed from reserves, leading to repetition. Putting that aside for now,
however:
1) Please update the POD for C4::Circulation::transferbook to state how the
functionality has changed
2) There is commented out code in the above routine which looks as though it
needs more thought
3) Shouldn't the librarian be able to override a recalls at checkout the same
way they can override a reserve (based on sysprefs I believe)
4) Please document the recalls status codes somewhere
5) C4::Circulation::AddReturn - There are superfluous DB calls here I believe
6) Reserves/Holds take precedence over Recalls during renewal, is that
deliberate (everywhere else it seems to be the opposite way around)
7) Superfluous `use Data::Dumper` in C4::Letters
8) I couldn't work out from the code how this affects fines.. only that is does
9) I don't understand why you've changed Koha/Patron.pm
10) Superfluous `use Data::Dumper` in catalogue/detail.pl
11) Why 3 distinct DB hits in catalogue/detail.pl
12) circ/recall-pickup-slip.pl appears to contain unused imports
13) circ/recalls_overdue.pl, circ/recalls_queue.pl, circ/recalls_waiting all
have references to Fast Cataloguing.. not sure why
14) Why the mix of filenames with underscores and hyphens?
15) circ/returns.pl has commented out code, leaving the feeling that it needs
further thought
16) atomicupdate .sql files should be converted to .perl and use the template.
17) Rebase issue has resulted in SR_SLIP being contained within many of the sql
files
18) All JS should be pushed to the bottom of the file now as per recent(ish)
guidelines.. I think this has partially been done, but some cases have been
missed.

Lots of little things.. now I need to fully test the feature to really get to
grips with why it's entirely distinct from Reserves.  I wish there was a full
RFC we could see and a Work In Progress branch for updating the manual. I'm
struggling to pick out the whole modus operandi of this patch as it's not what
anyone here understands 'recalls' to be.

Hopefully, we can keep this moving along.. sorry it's not all good news, but I
am keen to help get it moving again.

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