Bug 17560 - Hold fee placement at point of checkout
Summary: Hold fee placement at point of checkout
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-06 19:48 UTC by Jonathan Druart
Modified: 2017-12-07 22:18 UTC (History)
14 users (show)

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


Attachments
Bug 17560: Add a third option to HoldFeeMode (3.49 KB, patch)
2016-11-06 20:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17560: Update current code (5.84 KB, patch)
2016-11-06 20:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17560: Hold fee placement at point of checkout (11.24 KB, patch)
2016-11-06 20:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17560: Update the wording when a patron places a hold at the OPAC (1.38 KB, patch)
2016-11-06 20:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17560: Add a message when a patron checking in using the self checkout (3.36 KB, patch)
2016-11-06 20:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17560: Add a third option to HoldFeeMode (3.56 KB, patch)
2016-11-07 07:46 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17560: Update current code (5.91 KB, patch)
2016-11-07 07:46 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17560: Hold fee placement at point of checkout (11.31 KB, patch)
2016-11-07 07:46 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17560: Update the wording when a patron places a hold at the OPAC (1.45 KB, patch)
2016-11-07 07:46 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17560: Add a message when a patron checking in using the self checkout (3.42 KB, patch)
2016-11-07 07:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17560: Improve strenght of hold existence test (4.26 KB, patch)
2017-03-10 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17560: Improve wordings (2.34 KB, patch)
2017-03-20 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17560: Improve wordings (2.48 KB, patch)
2017-03-29 11:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17560: Add a message when a patron collecting a hold (staff interface) (2.51 KB, patch)
2017-03-29 17:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17560: Add a third option to HoldFeeMode (3.64 KB, patch)
2017-03-31 06:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17560: Update current code (5.99 KB, patch)
2017-03-31 06:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17560: Hold fee placement at point of checkout (11.39 KB, patch)
2017-03-31 06:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17560: Update the wording when a patron places a hold at the OPAC (1.53 KB, patch)
2017-03-31 06:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17560: Add a message when a patron checking in using the self checkout (3.51 KB, patch)
2017-03-31 06:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17560: Improve strenght of hold existence test (4.36 KB, patch)
2017-03-31 06:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17560: Improve wordings (2.48 KB, patch)
2017-03-31 06:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-11-06 19:48:11 UTC
Currently, Koha charges all patrons a hold fee in all circumstances, if a hold fee is applicable to their patron category.
This is immediately applied at point of request.
However, it would be useful to let patrons make requests without a charge
being incurred until they physically have the item in their hands and checked out to their cards.

The hold fee will only be added to the account  as soon as the item is checked out to the requesting patron​.
With this scenario, we will be certain that patrons have the correct item, and they are happy with what has been supplied.
It also means that patrons can place holds via the OPAC without reaching the usage limit that has been selected.
Comment 1 Jonathan Druart 2016-11-06 20:11:52 UTC
Created attachment 57233 [details] [review]
Bug 17560: Add a third option to HoldFeeMode

This patch adds a new 'any_time_is_collected' option to the HoldFeeMode
syspref.
The existing 'always' option is renamed with 'any_time_is_placed''

Sponsored-by: Cheshire Libraries
Comment 2 Jonathan Druart 2016-11-06 20:11:58 UTC
Created attachment 57234 [details] [review]
Bug 17560: Update current code

This patch updates the current code to make it works with the new
option's name of the syspref.
It also refactor the tests to make them more reusable and robust.

Sponsored-by: Cheshire Libraries
Comment 3 Jonathan Druart 2016-11-06 20:12:02 UTC
Created attachment 57235 [details] [review]
Bug 17560: Hold fee placement at point of checkout

Currently, Koha charges all patrons a hold fee in all circumstances, if
a hold fee is applicable to their patron category.
This is immediately applied at point of request.
However, it would be useful to let patrons make requests without a
charge
being incurred until they physically have the item in their hands and
checked out to their cards.

The hold fee will only be added to the account  as soon as the item is
checked out to the requesting patron.
With this scenario, we will be certain that patrons have the correct
item, and they are happy with what has been supplied.
It also means that patrons can place holds via the OPAC without reaching
the usage limit that has been selected.

Test plan:
0/ All the following steps must be done with a patron using a patron category with a hold fee
1/ Make sure that the existing options for HoldFeeMode work as before
2/ Select the third option "any time a hold is collected"
3/ Place a hold on an item
4/ Note that the patron has not been charged
5/ Check this item from the staff interface
6/ Note that the patron has been charged
7/ Place another hold
8/ Use the self checkout feature at the OPAC for the checkin
9/ Note that the patron has been charged and a message is displayed to
inform about the fee.

Sponsored-by: Cheshire Libraries
Comment 4 Jonathan Druart 2016-11-06 20:12:07 UTC
Created attachment 57236 [details] [review]
Bug 17560: Update the wording when a patron places a hold at the OPAC

Sponsored-by: Cheshire Libraries
Comment 5 Jonathan Druart 2016-11-06 20:12:12 UTC
Created attachment 57237 [details] [review]
Bug 17560: Add a message when a patron checking in using the self checkout

Sponsored-by: Cheshire Libraries
Comment 6 Josef Moravec 2016-11-07 07:46:15 UTC
Created attachment 57241 [details] [review]
[SIGNED-OFF] Bug 17560: Add a third option to HoldFeeMode

This patch adds a new 'any_time_is_collected' option to the HoldFeeMode
syspref.
The existing 'always' option is renamed with 'any_time_is_placed''

Sponsored-by: Cheshire Libraries

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 7 Josef Moravec 2016-11-07 07:46:21 UTC
Created attachment 57242 [details] [review]
[SIGNED-OFF] Bug 17560: Update current code

This patch updates the current code to make it works with the new
option's name of the syspref.
It also refactor the tests to make them more reusable and robust.

Sponsored-by: Cheshire Libraries

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 8 Josef Moravec 2016-11-07 07:46:25 UTC
Created attachment 57243 [details] [review]
[SIGNED-OFF] Bug 17560: Hold fee placement at point of checkout

Currently, Koha charges all patrons a hold fee in all circumstances, if
a hold fee is applicable to their patron category.
This is immediately applied at point of request.
However, it would be useful to let patrons make requests without a
charge
being incurred until they physically have the item in their hands and
checked out to their cards.

The hold fee will only be added to the account  as soon as the item is
checked out to the requesting patron.
With this scenario, we will be certain that patrons have the correct
item, and they are happy with what has been supplied.
It also means that patrons can place holds via the OPAC without reaching
the usage limit that has been selected.

Test plan:
0/ All the following steps must be done with a patron using a patron category with a hold fee
1/ Make sure that the existing options for HoldFeeMode work as before
2/ Select the third option "any time a hold is collected"
3/ Place a hold on an item
4/ Note that the patron has not been charged
5/ Check this item from the staff interface
6/ Note that the patron has been charged
7/ Place another hold
8/ Use the self checkout feature at the OPAC for the checkin
9/ Note that the patron has been charged and a message is displayed to
inform about the fee.

Sponsored-by: Cheshire Libraries

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 9 Josef Moravec 2016-11-07 07:46:30 UTC
Created attachment 57244 [details] [review]
[SIGNED-OFF] Bug 17560: Update the wording when a patron places a hold at the OPAC

Sponsored-by: Cheshire Libraries

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 10 Josef Moravec 2016-11-07 07:46:34 UTC
Created attachment 57245 [details] [review]
[SIGNED-OFF] Bug 17560: Add a message when a patron checking in using the self checkout

Sponsored-by: Cheshire Libraries

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 11 Martin Renvoize 2016-11-07 09:46:39 UTC
This is a dupe of https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16817 right?
Comment 12 Martin Renvoize 2016-11-07 09:55:59 UTC
My bad, they are subtly different.. one is 'Charge on collection' the other is 'Charge when ready for collection'.
Comment 13 Sally 2016-11-11 16:17:11 UTC
Hi Martin,

It's a very similar bug (it was our original spec) - but after some internal discussion, we altered the spec, and this bug was logged and sponsored instead.
Comment 14 Marcel de Rooy 2017-03-10 07:40:42 UTC
QA: Looking here now.
Comment 15 Marcel de Rooy 2017-03-10 07:58:14 UTC
QA Comment
Code looks good to me. Note some wording issues and one blocker in the db revision. Did not yet test the interface.

not_always is kind of a vague name for an option ;)
There is a charge of [% RESERVE_CHARGE %] for collecting this hold
There is a charge of [% RESERVE_CHARGE %] for placing this hold
Shouldn't it be better to say WHEN collecting this hold or WHEN placing this hold ?
<div class="alert">You have been charged for collecting this item.</div>
Why did they charge me? Shouldn't you mention the the term "hold fee" here too?

sco-main.pl
You check if a hold exists before issuing. Shouldn't you check for the borrowernumber too?
BTW Why not just call GetReserveFee here instead? You do not need to check holds and account_lines anymore. And additionally you can pass the exact amount in the message.
If you would still check account_lines, why not search for "Reserve Charge - $title" too? 

Please rename bug_XXXXX.sql.
Additionally, if the pref is set to 'always', you should now change the value to any_time_is_placed. Since you remove always !!

Failed QA
Comment 16 Jonathan Druart 2017-03-10 13:23:21 UTC
(In reply to Marcel de Rooy from comment #15)
> QA Comment
> Code looks good to me. Note some wording issues and one blocker in the db
> revision. Did not yet test the interface.
> 
> not_always is kind of a vague name for an option ;)
> There is a charge of [% RESERVE_CHARGE %] for collecting this hold
> There is a charge of [% RESERVE_CHARGE %] for placing this hold
> Shouldn't it be better to say WHEN collecting this hold or WHEN placing this
> hold ?
> <div class="alert">You have been charged for collecting this item.</div>
> Why did they charge me? Shouldn't you mention the the term "hold fee" here
> too?


Can we get a native English speaker confirm the wordings?


> sco-main.pl
> You check if a hold exists before issuing. Shouldn't you check for the
> borrowernumber too?

Indeed.

> BTW Why not just call GetReserveFee here instead? You do not need to check
> holds and account_lines anymore. And additionally you can pass the exact
> amount in the message.

The idea was to make sure the patron has been charged. From the circulation and reserves modules we are never really sure that what we want to happen will happen....

> If you would still check account_lines, why not search for "Reserve Charge -
> $title" too? 

To avoid to depend on a string that could change, but I can add it if you think it's safer.

> Please rename bug_XXXXX.sql.
> Additionally, if the pref is set to 'always', you should now change the
> value to any_time_is_placed. Since you remove always !!

Indeed, will fix.
Comment 17 Jonathan Druart 2017-03-10 14:02:34 UTC
Created attachment 60993 [details] [review]
Bug 17560: Improve strenght of hold existence test

This patch is a QA follow-up to fix several issues:
- 1 call to GetReserveFee was wrong in ModReserveFill
- Update DB entry was wrong and insufficient
- Add robustness to the tests in sco-main
Comment 18 Jonathan Druart 2017-03-10 14:05:37 UTC
(In reply to Marcel de Rooy from comment #15)
> BTW Why not just call GetReserveFee here instead? You do not need to check
> holds and account_lines anymore. And additionally you can pass the exact
> amount in the message.

I tried to display the amount, but since we do not handle the display of price yet, I do not think it is ready.
Without the currency symbol, the number alone looks weird, and with the currency symbol the display will be wrong half of the time...

> Please rename bug_XXXXX.sql.

I do not think this is necessary, I will have to rewrite the original patches and reupload all of them.
Comment 19 Marcel de Rooy 2017-03-20 13:27:26 UTC
I will resume QA here later.
Comment 20 Alex Sassmannshausen 2017-03-20 14:05:12 UTC
(In reply to Jonathan Druart from comment #16)
> (In reply to Marcel de Rooy from comment #15)
> > QA Comment
> > Code looks good to me. Note some wording issues and one blocker in the db
> > revision. Did not yet test the interface.
> > 
> > not_always is kind of a vague name for an option ;)
> > There is a charge of [% RESERVE_CHARGE %] for collecting this hold
> > There is a charge of [% RESERVE_CHARGE %] for placing this hold
> > Shouldn't it be better to say WHEN collecting this hold or WHEN placing this
> > hold ?
> > <div class="alert">You have been charged for collecting this item.</div>
> > Why did they charge me? Shouldn't you mention the the term "hold fee" here
> > too?
> 
> 
> Can we get a native English speaker confirm the wordings?
> 

Claire and I have deliberated for a bit at the hackfest, and we think the following would be minor improvements (though it kind of works as it is).

> There is a charge of [% RESERVE_CHARGE %] for collecting this hold
You will be charged a hold fee of [% _ %] for collecting this item

> There is a charge of [% RESERVE_CHARGE %] for placing this hold
You will be charged a hold fee of [% _ %] for placing this hold

> <div class="alert">You have been charged for collecting this item.</div>
A hold fee was charged to your account for collecting this item.
Comment 21 Jonathan Druart 2017-03-20 14:11:12 UTC
Created attachment 61301 [details] [review]
Bug 17560: Improve wordings
Comment 22 Marcel de Rooy 2017-03-21 15:17:05 UTC
(In reply to Alex Sassmannshausen from comment #20)
> Claire and I have deliberated for a bit at the hackfest, and we think the
> following would be minor improvements (though it kind of works as it is).

Great. Thanks.

> > There is a charge of [% RESERVE_CHARGE %] for collecting this hold
> You will be charged a hold fee of [% _ %] for collecting this item

Although I am not a native speaker, maybe we can still improve here :) Note that opac-reserve tells this to the user. So the time is placing the hold, but collecting it is still in the future. We do not charge for collecting btw, we just charge when you collect.. Shouldn't it be better to say You will be charged WHEN collecting? (Very strictly, we can't say THIS item; could be a biblio level hold. Maybe: book, material, or just 'it'.)
So I would opt for: You will be charged WHEN collecting THE item.
No blocker btw.
Comment 23 Marcel de Rooy 2017-03-21 15:22:02 UTC
Just another question: If we checkout in the staff client, wouldn't it be good to also show a message? A staff member could remind the patron that the fee is applied now, but he does not see it happening.
Comment 24 Marcel de Rooy 2017-03-21 15:38:51 UTC
(In reply to Marcel de Rooy from comment #23)
> Just another question: If we checkout in the staff client, wouldn't it be
> good to also show a message? A staff member could remind the patron that the
> fee is applied now, but he does not see it happening.

Not to talk about SIP.
This 'fortunately' does not look very promising currently in do_hold:
    AddReserve( $branch, $borrower->{borrowernumber}, $bibno, GetBiblioItemByBiblioNumber($bibno) );
    # unfortunately no meaningful return value
    $self->ok(1);
    return $self;
Comment 25 Marcel de Rooy 2017-03-23 10:38:30 UTC
Changing status to reflect need for feedback (comment23)
Comment 26 Martin Renvoize 2017-03-23 14:25:49 UTC
Comment from Colin regarding SIP (He's on leave today, so I've replied for him after a conversation about this yesterday):

"I think if user tries to issue the item you will see that there is a fee associated and the unit should take that from the user - similar to rental fees - we just need to check that is returned"
Comment 27 Fiona Borthwick 2017-03-23 14:39:43 UTC
(In reply to Marcel de Rooy from comment #23)
> Just another question: If we checkout in the staff client, wouldn't it be
> good to also show a message? A staff member could remind the patron that the
> fee is applied now, but he does not see it happening.

I think this would be a 'nice to have' feature but not essential so perhaps it could be a phase 2 follow up. Items with rental fees show an alert at checkout so an alert would be consistent with this. However in large, busy public libraries for example with lots of customers collecting holds, an alert may be more of a hindrance.
Comment 28 Marcel de Rooy 2017-03-24 07:19:35 UTC
(In reply to Martin Renvoize from comment #26)
> Comment from Colin regarding SIP (He's on leave today, so I've replied for
> him after a conversation about this yesterday):
> 
> "I think if user tries to issue the item you will see that there is a fee
> associated and the unit should take that from the user - similar to rental
> fees - we just need to check that is returned"

Martin, could you translate this for me? :)
I am not sure what you or Colin mean here.
Do you want a change, or are you comfortable with the current status?
Comment 29 Colin Campbell 2017-03-24 09:20:15 UTC
(In reply to Marcel de Rooy from comment #28)
> (In reply to Martin Renvoize from comment #26)
> > Comment from Colin regarding SIP (He's on leave today, so I've replied for
> > him after a conversation about this yesterday):
> > 
> > "I think if user tries to issue the item you will see that there is a fee
> > associated and the unit should take that from the user - similar to rental
> > fees - we just need to check that is returned"
> 
> Martin, could you translate this for me? :)
> I am not sure what you or Colin mean here.
> Do you want a change, or are you comfortable with the current status?

Sip calls CanBookBeIssued and will need to be told in the return that the issue requires a fee to be paid - It will then handle it much as it does rental fees.
Comment 30 Marcel de Rooy 2017-03-24 10:00:04 UTC
(In reply to Fiona Borthwick from comment #27)
> (In reply to Marcel de Rooy from comment #23)
> > Just another question: If we checkout in the staff client, wouldn't it be
> > good to also show a message? A staff member could remind the patron that the
> > fee is applied now, but he does not see it happening.
> 
> I think this would be a 'nice to have' feature but not essential so perhaps
> it could be a phase 2 follow up. Items with rental fees show an alert at
> checkout so an alert would be consistent with this. However in large, busy
> public libraries for example with lots of customers collecting holds, an
> alert may be more of a hindrance.

Thx Fiona for your feedback.
A busy librarian may ignore alerts at his own responsibility. (In this case the user has been informed when he placed the hold.)
But yes, I agree that it would be consistent to show the alert just as a rental charge is shown under the attention label of staff checkout. And I would rather see it on this report; it is a small change and within scope.
Comment 31 Marcel de Rooy 2017-03-24 10:16:43 UTC
(In reply to Colin Campbell from comment #29)
> Sip calls CanBookBeIssued and will need to be told in the return that the
> issue requires a fee to be paid - It will then handle it much as it does
> rental fees.

Thx Colin for your quick response.
There is a fundamental difference with this fee. The user cannot acknowledge it anymore as with a rental fee. He already agreed with this fee when he placed the hold. In other words, CanBookBeIssued does not look at it and AddIssue will just charge the fee via MoveReserve/ModReserveFill.
But just as we show the fee on the self checkout, we need to show a message imo on the sip terminal about the fee applied.
Comment 32 Marcel de Rooy 2017-03-24 10:21:47 UTC
Just for completeness:
Colin: See comment 24. The currently applied hold fee at the time of placing a hold is not shown in SIP. I guess that placing a hold via SIP is not that common as checking out.
Comment 33 Alex Sassmannshausen 2017-03-27 09:24:59 UTC
(In reply to Marcel de Rooy from comment #22)
> (In reply to Alex Sassmannshausen from comment #20)
> > Claire and I have deliberated for a bit at the hackfest, and we think the
> > following would be minor improvements (though it kind of works as it is).
> 
> Great. Thanks.
> 
> > > There is a charge of [% RESERVE_CHARGE %] for collecting this hold
> > You will be charged a hold fee of [% _ %] for collecting this item
> 
> Although I am not a native speaker, maybe we can still improve here :) Note
> that opac-reserve tells this to the user. So the time is placing the hold,
> but collecting it is still in the future. We do not charge for collecting
> btw, we just charge when you collect.. Shouldn't it be better to say You
> will be charged WHEN collecting? (Very strictly, we can't say THIS item;
> could be a biblio level hold. Maybe: book, material, or just 'it'.)
> So I would opt for: You will be charged WHEN collecting THE item.
> No blocker btw.

Fwiw, I agree with this reasoning you, Marcel (though I think the tense 'collecting' is not quite right):
You will be charged a hold fee of [% RESERVE_CHARGE %] when you collect the item.

Jonathan, would you be happy for me to add a patch for this?

Alex
Comment 34 Colin Campbell 2017-03-27 11:15:19 UTC
(In reply to Marcel de Rooy from comment #32)
> Just for completeness:
> Colin: See comment 24. The currently applied hold fee at the time of placing
> a hold is not shown in SIP. I guess that placing a hold via SIP is not that
> common as checking out.

I suspect the hold messages are not used in practice. Hence the code only returns failure messages in its current skeletal state. If any are used it would be the cancel and possibly alter functions, I dont think self checks have the interface to let you search for an item to hold in order to place it, whereas it could possible retrieve a user's holds to allow them to cancel them.

I would imagine that items are not checked out to the holder via self check as they have to collect the item from a staffed hold shelf.

I may be wrong but I think if we are going to change the sip code I'd like to have a user's scenario of what the workflow is rather than concocting it from possibly mistaken abstractions. So I suggest any sip implications be dealt with in a separate call
Comment 35 Marcel de Rooy 2017-03-27 11:25:18 UTC
(In reply to Colin Campbell from comment #34)
> (In reply to Marcel de Rooy from comment #32)
> > Just for completeness:
> > Colin: See comment 24. The currently applied hold fee at the time of placing
> > a hold is not shown in SIP. I guess that placing a hold via SIP is not that
> > common as checking out.
> 
> I suspect the hold messages are not used in practice. Hence the code only
> returns failure messages in its current skeletal state. If any are used it
> would be the cancel and possibly alter functions, I dont think self checks
> have the interface to let you search for an item to hold in order to place
> it, whereas it could possible retrieve a user's holds to allow them to
> cancel them.
> 
> I would imagine that items are not checked out to the holder via self check
> as they have to collect the item from a staffed hold shelf.
> 
> I may be wrong but I think if we are going to change the sip code I'd like
> to have a user's scenario of what the workflow is rather than concocting it
> from possibly mistaken abstractions. So I suggest any sip implications be
> dealt with in a separate call

Well, at least I learned a new word :) Concoct. My message stated more or less the same: placing a hold in SIP is probably useless.
If we put the SIP change in another report, you may have the effect that this patch is pushed into 17.05 and the sip followup is not..
Comment 36 Jonathan Druart 2017-03-27 14:27:01 UTC
Marcel, by chance could you rephrase with what you want on QAing?
Comment 37 Jonathan Druart 2017-03-28 12:18:50 UTC Comment hidden (obsolete)
Comment 38 Marcel de Rooy 2017-03-29 11:20:47 UTC
Created attachment 61669 [details] [review]
Bug 17560: Improve wordings

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended as per comment33 of Alex Sassmannshausen.
Comment 39 Marcel de Rooy 2017-03-29 11:21:41 UTC
(In reply to Alex Sassmannshausen from comment #33)
> Fwiw, I agree with this reasoning you, Marcel (though I think the tense
> 'collecting' is not quite right):
> You will be charged a hold fee of [% RESERVE_CHARGE %] when you collect the
> item.

Amended last patch. Thanks.
Comment 40 Marcel de Rooy 2017-03-29 11:33:27 UTC
(In reply to Jonathan Druart from comment #36)
> Marcel, by chance could you rephrase with what you want on QAing?

Of course :)

[1] If we checkout in the staff client, wouldn't it be good to also show a message? (Just like the sco message.) 
See comment23, 27 and 30. Show the fee under Attention just like rental fees.

[2] Provide a message to the SIP terminal at checkout time. (a.o. comment31, 34)
Note that Colin suggested to do this on another report.
In that case we could perhaps add a small "disclaimer" to the preference that SIP does apply, but not show the fee at checkout time.
And as a side note, SIP currently does not show a fee when placing a hold. But this is just a theoretical thing; not really used.

[3] Since we need to show the same message at three locations, we could consolidate that code (now in sco-main.pl)
Comment 41 Jonathan Druart 2017-03-29 17:15:32 UTC
(In reply to Marcel de Rooy from comment #40)
> [1] If we checkout in the staff client, wouldn't it be good to also show a
> message? (Just like the sco message.) 
> See comment23, 27 and 30. Show the fee under Attention just like rental fees.

I would consider this as a new enhancement, the info did not show up before this patch set.
I will submit a patch anyway.

> [2] Provide a message to the SIP terminal at checkout time. (a.o. comment31,
> 34)
> Note that Colin suggested to do this on another report.
> In that case we could perhaps add a small "disclaimer" to the preference
> that SIP does apply, but not show the fee at checkout time.
> And as a side note, SIP currently does not show a fee when placing a hold.
> But this is just a theoretical thing; not really used.

Same as previously, the message did not show up before. Why should we add a warning to the pref?

> [3] Since we need to show the same message at three locations, we could
> consolidate that code (now in sco-main.pl)

Messages are now in staff and opac, there are no way to centralise them.
Comment 42 Jonathan Druart 2017-03-29 17:15:54 UTC
Created attachment 61691 [details] [review]
Bug 17560: Add a message when a patron collecting a hold (staff interface)
Comment 43 Marcel de Rooy 2017-03-30 09:39:23 UTC
+                $hold_existed = Koha::Holds->search({ -or => { 'biblionumber' => $item->biblionumber, 'itemnumber' => $item->itemnumber}})->count;

versus

+            my $had_an_hold_placed_on_it = Koha::Holds->search({ borrowernumber => $borrower->{borrowernumber}, itemnumber => $item->itemnumber })->count;

This difference may actually illustrate why a consolidation would be nice.
Will amend it.
Comment 44 Marcel de Rooy 2017-03-30 10:12:34 UTC
(In reply to Jonathan Druart from comment #41)
> (In reply to Marcel de Rooy from comment #40)
> > [1] If we checkout in the staff client, wouldn't it be good to also show a
> > message? (Just like the sco message.) 
> > See comment23, 27 and 30. Show the fee under Attention just like rental fees.
> 
> I would consider this as a new enhancement, the info did not show up before
> this patch set.
> I will submit a patch anyway.

The info did not show up before since the hold fee was not charged at checkout time before. So this is not a new enhancement.
I am still looking at your patch btw. You may have a point somehow, since the rental charge is not presented as an individual charge but as a total amount.
Comment 45 Marcel de Rooy 2017-03-30 10:13:54 UTC
> > [2] Provide a message to the SIP terminal at checkout time. (a.o. comment31,
> > 34)
> > Note that Colin suggested to do this on another report.
> > In that case we could perhaps add a small "disclaimer" to the preference
> > that SIP does apply, but not show the fee at checkout time.
> > And as a side note, SIP currently does not show a fee when placing a hold.
> > But this is just a theoretical thing; not really used.
> 
> Same as previously, the message did not show up before. Why should we add a
> warning to the pref?
Same as before too. You changed behavior. But as stated before, this could go to another report.
Comment 46 Marcel de Rooy 2017-03-30 11:05:11 UTC
I obsoleted the last patch. It is not really what I had in mind.
I found out what my problem was in the charges display:
HoldsInNoissuesCharge = Dont include
RentalsInNoissuesCharge = Include
If you set HoldsInNoissuesCharge to include, you will have the same display as a rental fee. But what would be the side effects?

I am not sure if the current code really works fine if you would e.g. combine a hold fee with a rental fee (playing with noissuescharge too). But we are leaving the scope of this bug here.

In the issuing process CanBookBeIssued is called, it does not care about the hold fee charged later. And in this case it should not block the issue here for a hold fee only.

What may happen is this: I set rental to 6, hold fee to 7 and noissuecharge to 5 (a weird combination), Koha still allows me to checkout the book, charges me 13 AND says Cannot check out ! This is definitely a bug but not from this report.

In conclusion: Since Koha does not handle these charges correctly here, I would not enforce point 1 from comment 40. Since we move point 2 to another report, point 3 makes no sense yet..

Before passing QA: Do you still have any feedback to these last findings?
Comment 47 Marcel de Rooy 2017-03-30 11:10:41 UTC
(In reply to Marcel de Rooy from comment #46)
> What may happen is this: I set rental to 6, hold fee to 7 and noissuecharge
> to 5 (a weird combination), Koha still allows me to checkout the book,
> charges me 13 AND says Cannot check out ! This is definitely a bug but not
> from this report.

Or my interpretation. Should we read it as: Can no longer check out !
Comment 48 Jonathan Druart 2017-03-30 15:48:51 UTC
As you may have noticed the Circulation code is not robust and does not give us useful return values or enough trust in the actions that have been done (or maybe not).
I would like not to add anything to this enhancement to add feedback to the users, it is not easy to provide a good patch to do such a job at the moment (i.e. with the current code).
Comment 49 Marcel de Rooy 2017-03-31 06:54:40 UTC
Created attachment 61744 [details] [review]
Bug 17560: Add a third option to HoldFeeMode

This patch adds a new 'any_time_is_collected' option to the HoldFeeMode
syspref.
The existing 'always' option is renamed with 'any_time_is_placed''

Sponsored-by: Cheshire Libraries

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 50 Marcel de Rooy 2017-03-31 06:54:46 UTC
Created attachment 61745 [details] [review]
Bug 17560: Update current code

This patch updates the current code to make it works with the new
option's name of the syspref.
It also refactor the tests to make them more reusable and robust.

Sponsored-by: Cheshire Libraries

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 51 Marcel de Rooy 2017-03-31 06:54:50 UTC
Created attachment 61746 [details] [review]
Bug 17560: Hold fee placement at point of checkout

Currently, Koha charges all patrons a hold fee in all circumstances, if
a hold fee is applicable to their patron category.
This is immediately applied at point of request.
However, it would be useful to let patrons make requests without a
charge
being incurred until they physically have the item in their hands and
checked out to their cards.

The hold fee will only be added to the account  as soon as the item is
checked out to the requesting patron.
With this scenario, we will be certain that patrons have the correct
item, and they are happy with what has been supplied.
It also means that patrons can place holds via the OPAC without reaching
the usage limit that has been selected.

Test plan:
0/ All the following steps must be done with a patron using a patron category with a hold fee
1/ Make sure that the existing options for HoldFeeMode work as before
2/ Select the third option "any time a hold is collected"
3/ Place a hold on an item
4/ Note that the patron has not been charged
5/ Check this item from the staff interface
6/ Note that the patron has been charged
7/ Place another hold
8/ Use the self checkout feature at the OPAC for the checkin
9/ Note that the patron has been charged and a message is displayed to
inform about the fee.

Sponsored-by: Cheshire Libraries

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 52 Marcel de Rooy 2017-03-31 06:54:55 UTC
Created attachment 61747 [details] [review]
Bug 17560: Update the wording when a patron places a hold at the OPAC

Sponsored-by: Cheshire Libraries

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 53 Marcel de Rooy 2017-03-31 06:55:00 UTC
Created attachment 61748 [details] [review]
Bug 17560: Add a message when a patron checking in using the self checkout

Sponsored-by: Cheshire Libraries

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 54 Marcel de Rooy 2017-03-31 06:55:04 UTC
Created attachment 61749 [details] [review]
Bug 17560: Improve strenght of hold existence test

This patch is a QA follow-up to fix several issues:
- 1 call to GetReserveFee was wrong in ModReserveFill
- Update DB entry was wrong and insufficient
- Add robustness to the tests in sco-main

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 55 Marcel de Rooy 2017-03-31 06:55:09 UTC
Created attachment 61750 [details] [review]
Bug 17560: Improve wordings

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended as per comment33 of Alex Sassmannshausen.
Comment 56 Marcel de Rooy 2017-03-31 07:01:47 UTC
Final comment:
Still not completely happy about it (see earlier comments) but this is also caused by the condition of the current code.
Note that point 2 of comment40 requires opening a new report for SIP.

Passed QA
Comment 57 Kyle M Hall 2017-03-31 12:53:15 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 58 Katrin Fischer 2017-04-02 16:58:46 UTC
This won't get ported back to 16.11.x as it is an enhancement.