Bug 27946

Summary: Add a charge per article request to patron categories
Product: Koha Reporter: Nick Clemens <nick>
Component: CirculationAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, gmcharlt, kyle.m.hall, kyle, m.de.rooy, martin.renvoize, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28656
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26088
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00
Bug Depends on:    
Bug Blocks: 28652, 29819, 29759, 30852    
Attachments: Bug 27946: Add tests
Bug 27946: Add article_request_fee to categories table and ARTICLE_REQUEST account debit type
[DO NOT PUSH] Koha 27946: Update categories schema
Bug 27946: Add article request fee feature
Bug 27946: Add article request fee feature
Bug 27946: Add tests
Bug 27946: Add article_request_fee to categories table and ARTICLE_REQUEST account debit type
Bug 27946: Add article request fee feature
Bug 27946: [DO NOT PUSH] Update DBIx schema
Bug 27946: (follow-up) Use bug 28656
Bug 27946: Add tests
Bug 27946: Add article_request_fee to categories table and ARTICLE_REQUEST account debit type
Bug 27946: Add article request fee feature
Bug 27946: [DO NOT PUSH] Update DBIx schema
Bug 27946: (follow-up) Use bug 28656
Bug 27946: Add article_requests.debit_id and ARTICLE_REQUEST account debit type
Bug 27946: DBIC schema changes [DO NOT PUSH]
Bug 27946: Add article request fee to circulation rules
Bug 27946: Article request fee methods in Koha::Patron
Bug 27946: Make Koha::ArticleRequest->request add a fee if required
Bug 27946: Add UI handling of AR fees
Bug 27946: Add article_requests.debit_id and ARTICLE_REQUEST account debit type
Bug 27946: DBIC schema changes [DO NOT PUSH]
Bug 27946: Add article request fee to circulation rules
Bug 27946: Article request fee methods in Koha::Patron
Bug 27946: Make Koha::ArticleRequest->request add a fee if required
Bug 27946: Add UI handling of AR fees
Bug 27946: Add article_requests.debit_id and ARTICLE_REQUEST account debit type
Bug 27946: DBIC schema changes [DO NOT PUSH]
Bug 27946: Add article request fee to circulation rules
Bug 27946: Article request fee methods in Koha::Patron
Bug 27946: Make Koha::ArticleRequest->request add a fee if required
Bug 27946: Add UI handling of AR fees
Bug 27946: (QA follow-up) Remove 'scan' terminology, requests may not be scans
Bug 27946: Add article_requests.debit_id and ARTICLE_REQUEST account debit type
Bug 27946: DBIC schema changes [DO NOT PUSH]
Bug 27946: Add article request fee to circulation rules
Bug 27946: Article request fee methods in Koha::Patron
Bug 27946: Make Koha::ArticleRequest->request add a fee if required
Bug 27946: Add UI handling of AR fees
Bug 27946: (QA follow-up) Remove 'scan' terminology, requests may not be scans

Description Nick Clemens 2021-03-12 14:46:49 UTC
We propose adding a charge field (scan fee) to the patron category configuration screen.
Cost for Article scan request: $____
Comment 1 Agustín Moyano 2021-04-24 00:09:58 UTC
Created attachment 120144 [details] [review]
Bug 27946: Add tests
Comment 2 Agustín Moyano 2021-04-24 00:10:05 UTC
Created attachment 120145 [details] [review]
Bug 27946: Add article_request_fee to categories table and ARTICLE_REQUEST account debit type
Comment 3 Agustín Moyano 2021-04-24 00:10:10 UTC
Created attachment 120146 [details] [review]
[DO NOT PUSH] Koha 27946: Update categories schema
Comment 4 Agustín Moyano 2021-04-24 00:10:16 UTC
Created attachment 120147 [details] [review]
Bug 27946: Add article request fee feature

This patch adds the ability to charge a fee for article scan requests.

To test:
1. apply patches
2. updatedatabase
3. restart_all
4. enable ArticleRequest in preferences
5. grab a patron category and set a fee for article requests
6. grab a patron of that category (patron a) and another fron different category (patron b)
7. place an article request for both patrons
SUCCESS => only for patron a, a warning is displayed saying a fee will be charged
8. check the account for both patrons
SUCCESS => only for patron a there is a debit of type ARTICLE_REQUEST
9. cancel the article request for patron a
SUCCESS => the debit for patron a gets canceled
10. repeat step 7 only for patron a
11. change article request fee for the patron a's category
12. repeat step 10
13. check account for patron a
SUCCESS => there are 2 debits with different amount of typ ARTICLE_REQUEST
14. cancel only one article request
SUCCESS => only the corresponding debit gets canceled
15. prove t/db_dependent/ArticleRequests.t
Comment 5 Marcel de Rooy 2021-06-22 14:43:32 UTC
Tested. Looks like it would work when configured correctly. But found some cases that need to be addressed in some form. Adding a few comments for it.
Comment 6 Marcel de Rooy 2021-06-22 14:43:58 UTC
[1] No item link.
Shouldnt we save a link to item in the accounts when the article request contains one ? I added two article requests, one with an item and one without. Both were in the database without an item reference. Accountlines table has an itemnumber column. Showing up sometimes as a barcode on a form.
Comment 7 Marcel de Rooy 2021-06-22 14:44:22 UTC
[2] Warning Use of uninitialized value in string eq at /usr/share/koha/Koha/ArticleRequest.pm line 110.
 my $line = Koha::Account::Lines->find($self->debit_line_id);
      $line->cancel(
        {
          branch => C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef,
          staff_id => C4::Context->userenv ? C4::Context->userenv->{'number'} : undef,
        }
      ) unless $line->status eq 'CANCELLED' || $line->amount != $line->amountoutstanding;
    }
=> status == NULL
Comment 8 Marcel de Rooy 2021-06-22 14:45:10 UTC
[3] Cancel a fee after paying it
Started with fee 0.35. Added one request. Changed fee to 35. Added four requests.
Balance is now 140.35. I pay 105.  Results in 35.35. OK 
I cancel in OPAC one of the requests (which already had amountoutstanding == 0 but you cant see that).
What happened ? The balance stays 35.35 ! Wrong ! It should have gone down. NEEDS FIXING
I change the fee again to 10. Add a request. Balance 45.35. OK
Change fee back to 35. Now cancel the last request. Balance goes back to 35.35 Great.
Comment 9 Marcel de Rooy 2021-06-22 14:45:59 UTC
Read: cancel an article request after paying fee ..
Comment 10 Marcel de Rooy 2021-06-22 14:46:55 UTC
[4] Test by putting negative amount in fee on patron category
=> add_debit should throw an exception..
Seems that I cannot save that value with category on the list view. 
value="[% category.article_request_fee | $Price on_editing => 1 %]"
Koha::Number::Price->new( $value )->format_for_editing
$VAR1 = '-10.00';
But -10 is saved in the DATABASE ! Not shown in STAFF (list view shows only a minus; detail form does show -10)
So, yes adding a request now crashes: Debit amount passed is not positive at /usr/share/perl5/Exception/Class/Base.pm line 88
Conclusion: The categories form should enforce the amount to be zero or POSITIVE.
Comment 11 Marcel de Rooy 2021-06-22 14:48:53 UTC
[5] There should be a limit where you cant add requests anymore. Similar to maxoutstanding OR OPACFineNoRenewals OR MaxFine OR noissuescharge. Even RentalsInNoissuesCharge.
NOTE: This leaves the most room for discussion about scope borders..
$self->borrower->account->add_debit DOES NOT check a limit.
Please note: Your account has outstanding fees & charges of 35.35. Holds are blocked because your fine balance is over the limit.
Please note: You cannot renew your books online. Reason: Your fines exceed 100.00. 
Reducing fine for item $itemnum borrower $borrowernumber from $amount to $maxIncrease - MaxFine reached
But I can just keep adding article requests without any limit applied.
Comment 12 Marcel de Rooy 2021-06-22 14:51:06 UTC
Some of these 5 points justify a status change here now.
Comment 13 Agustín Moyano 2021-07-02 03:17:05 UTC
Created attachment 122522 [details] [review]
Bug 27946: Add article request fee feature

This patch adds the ability to charge a fee for article scan requests.

To test:
1. apply patches
2. updatedatabase
3. restart_all
4. enable ArticleRequest in preferences
5. grab a patron category and set a fee for article requests
6. grab a patron of that category (patron a) and another fron different category (patron b)
7. place an article request for both patrons
SUCCESS => only for patron a, a warning is displayed saying a fee will be charged
8. check the account for both patrons
SUCCESS => only for patron a there is a debit of type ARTICLE_REQUEST
9. cancel the article request for patron a
SUCCESS => the debit for patron a gets canceled
10. repeat step 7 only for patron a
11. change article request fee for the patron a's category
12. repeat step 10
13. check account for patron a
SUCCESS => there are 2 debits with different amount of typ ARTICLE_REQUEST
14. cancel only one article request
SUCCESS => only the corresponding debit gets canceled
15. prove t/db_dependent/ArticleRequests.t
Comment 14 Agustín Moyano 2021-07-02 03:19:22 UTC
(In reply to Marcel de Rooy from comment #12)
> Some of these 5 points justify a status change here now.

Hi Marcel, I addressed points 1 to 4. The 5th, even though I agree there should be a limit, I believe is out of scope, and should be done in a different bug.

Cheers.
Comment 15 Marcel de Rooy 2021-07-02 07:30:42 UTC
(In reply to Agustín Moyano from comment #14)
> Hi Marcel, I addressed points 1 to 4.

Great! Thx

> The 5th, even though I agree there
> should be a limit, I believe is out of scope, and should be done in a
> different bug.

I could at least open a new report for that one. Bug 28652.
Comment 16 Marcel de Rooy 2021-07-02 08:21:25 UTC
Created attachment 122529 [details] [review]
Bug 27946: Add tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2021-07-02 08:21:29 UTC
Created attachment 122530 [details] [review]
Bug 27946: Add article_request_fee to categories table and ARTICLE_REQUEST account debit type

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2021-07-02 08:21:32 UTC
Created attachment 122531 [details] [review]
Bug 27946: Add article request fee feature

This patch adds the ability to charge a fee for article scan requests.

To test:
1. apply patches
2. updatedatabase
3. restart_all
4. enable ArticleRequest in preferences
5. grab a patron category and set a fee for article requests
6. grab a patron of that category (patron a) and another fron different category (patron b)
7. place an article request for both patrons
SUCCESS => only for patron a, a warning is displayed saying a fee will be charged
8. check the account for both patrons
SUCCESS => only for patron a there is a debit of type ARTICLE_REQUEST
9. cancel the article request for patron a
SUCCESS => the debit for patron a gets canceled
10. repeat step 7 only for patron a
11. change article request fee for the patron a's category
12. repeat step 10
13. check account for patron a
SUCCESS => there are 2 debits with different amount of typ ARTICLE_REQUEST
14. cancel only one article request
SUCCESS => only the corresponding debit gets canceled
15. prove t/db_dependent/ArticleRequests.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2021-07-02 08:21:36 UTC
Created attachment 122532 [details] [review]
Bug 27946: [DO NOT PUSH] Update DBIx schema
Comment 20 Marcel de Rooy 2021-07-02 08:25:52 UTC
Martin: Are you happy with the way this patch deals with the cancellation as to the accounts handling? Testing amountoutstanding and choosing line->cancel or account->add_credit? Balance wise, it works fine.
Comment 21 Tomás Cohen Arazi 2021-07-02 12:22:50 UTC
(In reply to Marcel de Rooy from comment #20)
> Martin: Are you happy with the way this patch deals with the cancellation as
> to the accounts handling? Testing amountoutstanding and choosing
> line->cancel or account->add_credit? Balance wise, it works fine.

I think ->cancel is correct but cancelling has some caveats: some exceptions will be thrown if (say) the line is already offsetted (partially/fully payed, writen off, etc).

Those cases lead me to think ->void would suit better, but we need to (maybe) think about possible refunds.
Comment 22 Marcel de Rooy 2021-07-02 12:28:41 UTC
(In reply to Tomás Cohen Arazi from comment #21)
> (In reply to Marcel de Rooy from comment #20)
> > Martin: Are you happy with the way this patch deals with the cancellation as
> > to the accounts handling? Testing amountoutstanding and choosing
> > line->cancel or account->add_credit? Balance wise, it works fine.
> 
> I think ->cancel is correct but cancelling has some caveats: some exceptions
> will be thrown if (say) the line is already offsetted (partially/fully
> payed, writen off, etc).
> 
> Those cases lead me to think ->void would suit better, but we need to
> (maybe) think about possible refunds.

Agustin compares amount with outstanding before calling cancel. So that should be fine, I guess. It fixes the earlier problem reported.
Comment 23 Martin Renvoize 2021-07-02 13:50:37 UTC
Comment on attachment 122531 [details] [review]
Bug 27946: Add article request fee feature

Review of attachment 122531 [details] [review]:
-----------------------------------------------------------------

Generally this is looking good :)

::: Koha/ArticleRequest.pm
@@ +50,4 @@
>      my ($self) = @_;
>  
>      $self->status(Koha::ArticleRequest::Status::Pending);
> +    if(C4::Context->preference('ArticleRequests') && $self->borrower->category->article_request_fee) {

I think the `C4::Context->preference('ArticleRequests')` check here is superfluous to requirements.. I would hope we are checking that the feature is enabled prior to this point and also don't think it makes sense to check them together.

@@ +55,5 @@
> +        {
> +          amount     => $self->borrower->category->article_request_fee,
> +          user_id    => C4::Context->userenv ? C4::Context->userenv->{'number'} : undef,
> +          interface  => C4::Context->interface,
> +          library_id => C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef,

I wonder if this should default to the items home library?  I presume there's no requirement, for now at least, for these charges to be library specific?
Comment 24 Martin Renvoize 2021-07-02 14:21:56 UTC
Comment on attachment 122531 [details] [review]
Bug 27946: Add article request fee feature

Review of attachment 122531 [details] [review]:
-----------------------------------------------------------------

::: Koha/ArticleRequest.pm
@@ +103,5 @@
>  
>      $self->status(Koha::ArticleRequest::Status::Canceled);
>      $self->notes($notes) if $notes;
> +    if ( $self->debit_line_id ) {
> +        my $line = Koha::Account::Lines->find( $self->debit_line_id );

Hmm, having looked at this a little more closely now I've read the concerns/discussion above..

I think we may need to add some additional considerations here.

a) What should happen if a fee was 'Written off' or 'Reduced' (You've taken care of 'Cancelled' by the looks).
b) We could use the 'reduce' method to cover both cases and allow the passing of 'CANCELLED' credit type perhaps?

I'm thinking something along the lines of

```
my $refundable = 0;
if ( $line->amount != $line->amountoutstanding ) {
    my $refundable_offsets = Koha::Account::Offsets->search(
        {
            debit_id  => $line->id,
            credit_id => { '!=' => undef }, # Not the debt itself
            type      => { '!=' => [ 'Writeoff', 'Refund', 'Cancellation', 'Discount' ] },
            amount    => { '<' => 0 }    # credits are negative on the DB
    );

    $refundable = ( $refundable_offsets->count > 0 )
        ? $credits_offsets->total * -1    # credits are negative on the DB
        : 0;
    }

    $refundable += $line->amountoutstanding;

    $line->reduce(
        reduction_type => REQUEST_CANCELLATION,
        amount         => $refundable,
        staff_id       => C4::Context->userenv ? C4::Context->userenv->{'number'} : undef,
        interface      => C4::Context->interface,
        branchcode     => C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef
    );
}
```
Comment 25 Martin Renvoize 2021-07-02 15:22:44 UTC
Created attachment 122552 [details] [review]
Bug 27946: (follow-up) Use bug 28656
Comment 26 Martin Renvoize 2021-07-02 15:24:07 UTC
As discussed with Tomas, follow-up attached that uses some updates to the 'cancel' method as detailed in bug 22435 that makes this process of refunding simpler.
Comment 27 Marcel de Rooy 2021-07-05 06:04:54 UTC
Hmm a dependency on a NEW bug report?
Comment 28 Marcel de Rooy 2021-07-05 07:05:11 UTC
(In reply to Martin Renvoize from comment #23)
> I think the `C4::Context->preference('ArticleRequests')` check here is
> superfluous to requirements.. I would hope we are checking that the feature
> is enabled prior to this point and also don't think it makes sense to check
> them together.

Yes, that seems fair.


> > +          library_id => C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef,
> 
> I wonder if this should default to the items home library?  I presume
> there's no requirement, for now at least, for these charges to be library
> specific?

Interesting! How important is branchcode for the accounts subsytem? kohastructure refers to the place where a payment was made or an invoice created, etc.
The article request contains a branchcode for the pickup library btw.
It might not be relevant indeed, but I feel that the charge is created at the branch where the article is actually copied or scanned.
Comment 29 Martin Renvoize 2021-07-05 08:37:49 UTC
(In reply to Marcel de Rooy from comment #27)
> Hmm a dependency on a NEW bug report?

Well.. I deliberately didn't set it as a hard dependency yet.. the follow-up was meant as a hint to where we might want to go with this having discussed it with Tomas :).

There's a little debate going on as to how we should record the cancellation over-credit if you cancel a debt in full when some of that debt has already been paid.. should we just create the single 'cancel' credit, offset it up to the amount outstanding on the original debt and then allow the rest of the outstanding to be 'paid out' or 'applied' to other debts.. or do we split the cancellation into 'cancellation' and 'overpayment credit'...
Comment 30 Martin Renvoize 2021-07-05 08:38:45 UTC
(In reply to Marcel de Rooy from comment #28)
> Interesting! How important is branchcode for the accounts subsytem?
> kohastructure refers to the place where a payment was made or an invoice
> created, etc.
> The article request contains a branchcode for the pickup library btw.
> It might not be relevant indeed, but I feel that the charge is created at
> the branch where the article is actually copied or scanned.

Very good point.. I'm not sure the branchcode is at all relevant here.. but if it is, I think you're probably right that it should go to the library doing the actual copying.
Comment 31 Marcel de Rooy 2021-07-06 14:19:14 UTC
(In reply to Martin Renvoize from comment #29)
> Well.. I deliberately didn't set it as a hard dependency yet.. the follow-up
> was meant as a hint to where we might want to go with this having discussed
> it with Tomas :).

Still seems to me that the idea of Agustin on its own is simpler. Perhaps it needs a bit of attention (checks). But adding a credit and let the cronjob reconcile the balance somehow, whats actually wrong with that?
Comment 32 Agustín Moyano 2021-07-22 13:18:05 UTC
(In reply to Marcel de Rooy from comment #31)
> (In reply to Martin Renvoize from comment #29)
> > Well.. I deliberately didn't set it as a hard dependency yet.. the follow-up
> > was meant as a hint to where we might want to go with this having discussed
> > it with Tomas :).
> 
> Still seems to me that the idea of Agustin on its own is simpler. Perhaps it
> needs a bit of attention (checks). But adding a credit and let the cronjob
> reconcile the balance somehow, whats actually wrong with that?

Seems that what I implemented did not took into account some cases, for example, what happens when a patron pays a part of what was owed. 

Besides, it seems reasonable for the logic of adding a credit to be inside of cancel
Comment 33 Agustín Moyano 2021-08-05 15:35:27 UTC
Created attachment 123503 [details] [review]
Bug 27946: Add tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Agustín Moyano 2021-08-05 15:35:32 UTC
Created attachment 123504 [details] [review]
Bug 27946: Add article_request_fee to categories table and ARTICLE_REQUEST account debit type

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Agustín Moyano 2021-08-05 15:35:37 UTC
Created attachment 123505 [details] [review]
Bug 27946: Add article request fee feature

This patch adds the ability to charge a fee for article scan requests.

To test:
1. apply patches
2. updatedatabase
3. restart_all
4. enable ArticleRequest in preferences
5. grab a patron category and set a fee for article requests
6. grab a patron of that category (patron a) and another fron different category (patron b)
7. place an article request for both patrons
SUCCESS => only for patron a, a warning is displayed saying a fee will be charged
8. check the account for both patrons
SUCCESS => only for patron a there is a debit of type ARTICLE_REQUEST
9. cancel the article request for patron a
SUCCESS => the debit for patron a gets canceled
10. repeat step 7 only for patron a
11. change article request fee for the patron a's category
12. repeat step 10
13. check account for patron a
SUCCESS => there are 2 debits with different amount of typ ARTICLE_REQUEST
14. cancel only one article request
SUCCESS => only the corresponding debit gets canceled
15. prove t/db_dependent/ArticleRequests.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Agustín Moyano 2021-08-05 15:35:42 UTC
Created attachment 123506 [details] [review]
Bug 27946: [DO NOT PUSH] Update DBIx schema
Comment 37 Agustín Moyano 2021-08-05 15:35:46 UTC
Created attachment 123507 [details] [review]
Bug 27946: (follow-up) Use bug 28656
Comment 38 Marcel de Rooy 2021-09-17 09:36:18 UTC
Changing status to BLOCKED.
This bug depends on bug 28656 which is in Failed QA state.
Could we do something to get this/that one further? Or remove the dependency on this report?

@Martin: ping.
Comment 39 Marcel de Rooy 2021-09-17 09:37:39 UTC
*** Bug 26088 has been marked as a duplicate of this bug. ***
Comment 40 Marcel de Rooy 2021-11-09 12:24:06 UTC
Tomas, I am keen to signoff this one, but wouldnt it be helpful if you would add patches ;)
Comment 41 Marcel de Rooy 2021-11-09 12:24:40 UTC
And what about the dependency ?
Comment 42 Tomás Cohen Arazi 2021-11-09 13:35:18 UTC
(In reply to Marcel de Rooy from comment #41)
> And what about the dependency ?

I'm moving the cancellation handling to a separate bug. Martin and I have talked about working together on the dependency bug just after the release.
Comment 43 Marcel de Rooy 2021-12-17 14:12:21 UTC
Tomas: Add some patches ;)
Comment 44 Tomás Cohen Arazi 2021-12-17 14:13:48 UTC
(In reply to Marcel de Rooy from comment #43)
> Tomas: Add some patches ;)

Good idea :-P
Comment 45 Tomás Cohen Arazi 2021-12-17 14:15:46 UTC
Created attachment 128660 [details] [review]
Bug 27946: Add article_requests.debit_id and ARTICLE_REQUEST account debit type

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 46 Tomás Cohen Arazi 2021-12-17 14:16:00 UTC
Created attachment 128661 [details] [review]
Bug 27946: DBIC schema changes [DO NOT PUSH]

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 47 Tomás Cohen Arazi 2021-12-17 14:16:11 UTC
Created attachment 128662 [details] [review]
Bug 27946: Add article request fee to circulation rules

This patch adds a new circulation rule: article requests fee.

It can be set per library and category.

To test:
1. Try setting sdifferent numeric values, change, and delete them
=> SUCCESS: Things work as expected
2. Make sure rules are loaded correctly when re-entering the circ rules
   page
=> SUCCESS: All good
3. Verify the data on the DB is stored as it should on each of your
   tests:
   $ koha-mysql kohadev
   > SELECT * FROM circulation_rules \
     WHERE rule_name='article_request_fee';
=> SUCCESS: All good (remember NULL => 'All' in the UI)
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 48 Tomás Cohen Arazi 2021-12-17 14:16:21 UTC
Created attachment 128663 [details] [review]
Bug 27946: Article request fee methods in Koha::Patron

This patch adds article_request_fee() and add_article_request_fee_if_needed() methods to Koha::Patron.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 49 Tomás Cohen Arazi 2021-12-17 14:16:27 UTC
Created attachment 128664 [details] [review]
Bug 27946: Make Koha::ArticleRequest->request add a fee if required

This patch makes the ->request method add a fee for the patron if
required. It relies on methods defined in Koha::Patron for the task. The
debit line is linked to the AR if applies.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/ArticleRequest.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 50 Tomás Cohen Arazi 2021-12-17 14:16:34 UTC
Created attachment 128665 [details] [review]
Bug 27946: Add UI handling of AR fees

This patch:
- Adds a notice about the fee to be charged to the patron (staff and
  OPAC)
- Adds the right UI description for ARTICLE_REQUEST_FEE debit types in
  the UI.

To test:
1. Have some article request fee rules
2. Play with placing an article request in OPAC and staff
=> SUCCESS: You see a message about the fee
=> SUCCESS: The fees are correctly applied to the patron
3. Sign off :-D
Comment 51 Marcel de Rooy 2021-12-21 13:17:15 UTC
+        $debit_line = $self->account->add_debit(
+            {
+                amount     => $amount,
+                user_id    => C4::Context->userenv ? C4::Context->userenv->{'number'} : undef,

I would expect $self->borrowernumber here instead of going to context?
Note btw that this field (in debit lines) user_id is chosen VERY unfortunate. We already had the mixup on the other report. It should be renamed (but not here).
Comment 52 Marcel de Rooy 2021-12-21 13:17:42 UTC
Few comments forthcoming still
Comment 53 Marcel de Rooy 2021-12-21 13:18:50 UTC
Seeing

Argument "" isn't numeric in numeric gt (>) at /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt line 373.
Argument "" isn't numeric in numeric gt (>) at /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt line 378.

Typo  patron's categor
Comment 54 Marcel de Rooy 2021-12-21 13:20:17 UTC
Personally do not likes changes like this:

-        [%- CASE 'ACCOUNT'          -%]Account creation fee
-        [%- CASE 'ACCOUNT_RENEW'    -%]Account renewal fee
-        [%- CASE 'LOST'             -%]Lost item
-        [%- CASE 'MANUAL'           -%]Manual fee
-        [%- CASE 'NEW_CARD'         -%]New card
-        [%- CASE 'OVERDUE'          -%]Fine
-        [%- CASE 'PROCESSING'       -%]Lost item processing fee
-        [%- CASE 'RENT'             -%]Rental fee
-        [%- CASE 'RENT_DAILY'       -%]Daily rental fee
-        [%- CASE 'RENT_RENEW'       -%]Renewal of rental item
-        [%- CASE 'RENT_DAILY_RENEW' -%]Renewal of daily rental item
-        [%- CASE 'RESERVE'          -%]Hold fee
-        [%- CASE 'RESERVE_EXPIRED'  -%]Hold waiting too long
-        [%- CASE 'Payout'           -%]Payout
-        [%- CASE                    -%][% debit_type.description | html %]
+        [%- CASE 'ACCOUNT'             -%]Account creation fee
+        [%- CASE 'ACCOUNT_RENEW'       -%]Account renewal fee
+        [%- CASE 'ARTICLE_REQUEST_FEE' -%]Article scan request fee
+        [%- CASE 'LOST'                -%]Lost item
+        [%- CASE 'MANUAL'              -%]Manual fee
+        [%- CASE 'NEW_CARD'            -%]New card
+        [%- CASE 'OVERDUE'             -%]Fine
+        [%- CASE 'PROCESSING'          -%]Lost item processing fee
+        [%- CASE 'RENT'                -%]Rental fee
+        [%- CASE 'RENT_DAILY'          -%]Daily rental fee
+        [%- CASE 'RENT_RENEW'          -%]Renewal of rental item
+        [%- CASE 'RENT_DAILY_RENEW'    -%]Renewal of daily rental item
+        [%- CASE 'RESERVE'             -%]Hold fee
+        [%- CASE 'RESERVE_EXPIRED'     -%]Hold waiting too long
+        [%- CASE 'Payout'              -%]Payout
+        [%- CASE                       -%][% debit_type.description | html %]

If you want to add one line, why change 20 ?
Comment 55 Marcel de Rooy 2021-12-21 13:22:29 UTC
The most important comment as last in the row: There is no handling of the cancellation. We need to provide a credit.
Remarkable that we miss it here, since it was the reason for holding back the whole thing earlier?
Comment 56 Tomás Cohen Arazi 2021-12-21 13:27:51 UTC
(In reply to Marcel de Rooy from comment #54)
> Personally do not likes changes like this:
> 
> -        [%- CASE 'ACCOUNT'          -%]Account creation fee
> -        [%- CASE 'ACCOUNT_RENEW'    -%]Account renewal fee
> -        [%- CASE 'LOST'             -%]Lost item
> -        [%- CASE 'MANUAL'           -%]Manual fee
> -        [%- CASE 'NEW_CARD'         -%]New card
> -        [%- CASE 'OVERDUE'          -%]Fine
> -        [%- CASE 'PROCESSING'       -%]Lost item processing fee
> -        [%- CASE 'RENT'             -%]Rental fee
> -        [%- CASE 'RENT_DAILY'       -%]Daily rental fee
> -        [%- CASE 'RENT_RENEW'       -%]Renewal of rental item
> -        [%- CASE 'RENT_DAILY_RENEW' -%]Renewal of daily rental item
> -        [%- CASE 'RESERVE'          -%]Hold fee
> -        [%- CASE 'RESERVE_EXPIRED'  -%]Hold waiting too long
> -        [%- CASE 'Payout'           -%]Payout
> -        [%- CASE                    -%][% debit_type.description | html %]
> +        [%- CASE 'ACCOUNT'             -%]Account creation fee
> +        [%- CASE 'ACCOUNT_RENEW'       -%]Account renewal fee
> +        [%- CASE 'ARTICLE_REQUEST_FEE' -%]Article scan request fee
> +        [%- CASE 'LOST'                -%]Lost item
> +        [%- CASE 'MANUAL'              -%]Manual fee
> +        [%- CASE 'NEW_CARD'            -%]New card
> +        [%- CASE 'OVERDUE'             -%]Fine
> +        [%- CASE 'PROCESSING'          -%]Lost item processing fee
> +        [%- CASE 'RENT'                -%]Rental fee
> +        [%- CASE 'RENT_DAILY'          -%]Daily rental fee
> +        [%- CASE 'RENT_RENEW'          -%]Renewal of rental item
> +        [%- CASE 'RENT_DAILY_RENEW'    -%]Renewal of daily rental item
> +        [%- CASE 'RESERVE'             -%]Hold fee
> +        [%- CASE 'RESERVE_EXPIRED'     -%]Hold waiting too long
> +        [%- CASE 'Payout'              -%]Payout
> +        [%- CASE                       -%][% debit_type.description | html
> %]
> 
> If you want to add one line, why change 20 ?

We do that to keep the indentation, I would do it again anytime it is needed :-D
That said, I think I can remove the trailing _FEE and avoid it this time!
Comment 57 Tomás Cohen Arazi 2021-12-21 13:30:05 UTC
(In reply to Marcel de Rooy from comment #51)
> +        $debit_line = $self->account->add_debit(
> +            {
> +                amount     => $amount,
> +                user_id    => C4::Context->userenv ?
> C4::Context->userenv->{'number'} : undef,
> 
> I would expect $self->borrowernumber here instead of going to context?
> Note btw that this field (in debit lines) user_id is chosen VERY
> unfortunate. We already had the mixup on the other report. It should be
> renamed (but not here).

the 'user_id' parameter accounts for the currently logged user recording the debit. So the staff member that added the debit. I think it is correct to fallback to userenv as it is the accountlines.manager_id attribute.
Comment 58 Tomás Cohen Arazi 2021-12-21 13:30:34 UTC
(In reply to Marcel de Rooy from comment #53)
> Seeing
> 
> Argument "" isn't numeric in numeric gt (>) at
> /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.
> tt line 373.
> Argument "" isn't numeric in numeric gt (>) at
> /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.
> tt line 378.
> 
> Typo  patron's categor

Good catch!
Comment 59 Tomás Cohen Arazi 2021-12-21 13:32:00 UTC
(In reply to Marcel de Rooy from comment #55)
> The most important comment as last in the row: There is no handling of the
> cancellation. We need to provide a credit.
> Remarkable that we miss it here, since it was the reason for holding back
> the whole thing earlier?

Yes!

My thinking was: Koha currently provides ways to restore credit. So what we are talking about is an automatic way to do it. My plan is to do it on a separate bug.
Comment 60 Marcel de Rooy 2021-12-21 13:32:18 UTC
(In reply to Tomás Cohen Arazi from comment #57)
> 
> the 'user_id' parameter accounts for the currently logged user recording the
> debit. So the staff member that added the debit. I think it is correct to
> fallback to userenv as it is the accountlines.manager_id attribute.

Ah ok. You will be right. Well it even more demonstrates that this field name was picked very badly.
Comment 61 Marcel de Rooy 2021-12-21 13:33:26 UTC
(In reply to Tomás Cohen Arazi from comment #59)
> (In reply to Marcel de Rooy from comment #55)
> > The most important comment as last in the row: There is no handling of the
> > cancellation. We need to provide a credit.
> > Remarkable that we miss it here, since it was the reason for holding back
> > the whole thing earlier?
> 
> Yes!
> 
> My thinking was: Koha currently provides ways to restore credit. So what we
> are talking about is an automatic way to do it. My plan is to do it on a
> separate bug.

If it is just as simple as adding a credit, I would say do it here.
Comment 62 Tomás Cohen Arazi 2021-12-22 20:40:42 UTC
Created attachment 128868 [details] [review]
Bug 27946: Add article_requests.debit_id and ARTICLE_REQUEST account debit type

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 63 Tomás Cohen Arazi 2021-12-22 20:40:48 UTC
Created attachment 128869 [details] [review]
Bug 27946: DBIC schema changes [DO NOT PUSH]

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 64 Tomás Cohen Arazi 2021-12-22 20:40:53 UTC
Created attachment 128870 [details] [review]
Bug 27946: Add article request fee to circulation rules

This patch adds a new circulation rule: article requests fee.

It can be set per library and category.

To test:
1. Try setting sdifferent numeric values, change, and delete them
=> SUCCESS: Things work as expected
2. Make sure rules are loaded correctly when re-entering the circ rules
   page
=> SUCCESS: All good
3. Verify the data on the DB is stored as it should on each of your
   tests:
   $ koha-mysql kohadev
   > SELECT * FROM circulation_rules \
     WHERE rule_name='article_request_fee';
=> SUCCESS: All good (remember NULL => 'All' in the UI)
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 65 Tomás Cohen Arazi 2021-12-22 20:40:59 UTC
Created attachment 128871 [details] [review]
Bug 27946: Article request fee methods in Koha::Patron

This patch adds article_request_fee() and add_article_request_fee_if_needed() methods to Koha::Patron.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 66 Tomás Cohen Arazi 2021-12-22 20:41:04 UTC
Created attachment 128872 [details] [review]
Bug 27946: Make Koha::ArticleRequest->request add a fee if required

This patch makes the ->request method add a fee for the patron if
required. It relies on methods defined in Koha::Patron for the task. The
debit line is linked to the AR if applies.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/ArticleRequest.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 67 Tomás Cohen Arazi 2021-12-22 20:41:10 UTC
Created attachment 128873 [details] [review]
Bug 27946: Add UI handling of AR fees

This patch:
- Adds a notice about the fee to be charged to the patron (staff and
  OPAC)
- Adds the right UI description for ARTICLE_REQUEST_FEE debit types in
  the UI.

To test:
1. Have some article request fee rules
2. Play with placing an article request in OPAC and staff
=> SUCCESS: You see a message about the fee
=> SUCCESS: The fees are correctly applied to the patron
3. Sign off :-D
Comment 68 Kyle M Hall 2022-01-07 12:14:21 UTC
Created attachment 129141 [details] [review]
Bug 27946: Add article_requests.debit_id and ARTICLE_REQUEST account debit type

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 69 Kyle M Hall 2022-01-07 12:14:31 UTC
Created attachment 129142 [details] [review]
Bug 27946: DBIC schema changes [DO NOT PUSH]

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 70 Kyle M Hall 2022-01-07 12:14:35 UTC
Created attachment 129143 [details] [review]
Bug 27946: Add article request fee to circulation rules

This patch adds a new circulation rule: article requests fee.

It can be set per library and category.

To test:
1. Try setting sdifferent numeric values, change, and delete them
=> SUCCESS: Things work as expected
2. Make sure rules are loaded correctly when re-entering the circ rules
   page
=> SUCCESS: All good
3. Verify the data on the DB is stored as it should on each of your
   tests:
   $ koha-mysql kohadev
   > SELECT * FROM circulation_rules \
     WHERE rule_name='article_request_fee';
=> SUCCESS: All good (remember NULL => 'All' in the UI)
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 71 Kyle M Hall 2022-01-07 12:14:38 UTC
Created attachment 129144 [details] [review]
Bug 27946: Article request fee methods in Koha::Patron

This patch adds article_request_fee() and add_article_request_fee_if_needed() methods to Koha::Patron.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 72 Kyle M Hall 2022-01-07 12:14:42 UTC
Created attachment 129145 [details] [review]
Bug 27946: Make Koha::ArticleRequest->request add a fee if required

This patch makes the ->request method add a fee for the patron if
required. It relies on methods defined in Koha::Patron for the task. The
debit line is linked to the AR if applies.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/ArticleRequest.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 73 Kyle M Hall 2022-01-07 12:14:46 UTC
Created attachment 129146 [details] [review]
Bug 27946: Add UI handling of AR fees

This patch:
- Adds a notice about the fee to be charged to the patron (staff and
  OPAC)
- Adds the right UI description for ARTICLE_REQUEST_FEE debit types in
  the UI.

To test:
1. Have some article request fee rules
2. Play with placing an article request in OPAC and staff
=> SUCCESS: You see a message about the fee
=> SUCCESS: The fees are correctly applied to the patron
3. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 74 Kyle M Hall 2022-01-07 12:14:50 UTC
Created attachment 129147 [details] [review]
Bug 27946: (QA follow-up) Remove 'scan' terminology, requests may not be scans

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 75 Martin Renvoize 2022-01-07 14:18:14 UTC
Created attachment 129171 [details] [review]
Bug 27946: Add article_requests.debit_id and ARTICLE_REQUEST account debit type

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 76 Martin Renvoize 2022-01-07 14:18:18 UTC
Created attachment 129172 [details] [review]
Bug 27946: DBIC schema changes [DO NOT PUSH]

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 77 Martin Renvoize 2022-01-07 14:18:22 UTC
Created attachment 129173 [details] [review]
Bug 27946: Add article request fee to circulation rules

This patch adds a new circulation rule: article requests fee.

It can be set per library and category.

To test:
1. Try setting sdifferent numeric values, change, and delete them
=> SUCCESS: Things work as expected
2. Make sure rules are loaded correctly when re-entering the circ rules
   page
=> SUCCESS: All good
3. Verify the data on the DB is stored as it should on each of your
   tests:
   $ koha-mysql kohadev
   > SELECT * FROM circulation_rules \
     WHERE rule_name='article_request_fee';
=> SUCCESS: All good (remember NULL => 'All' in the UI)
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 78 Martin Renvoize 2022-01-07 14:18:26 UTC
Created attachment 129174 [details] [review]
Bug 27946: Article request fee methods in Koha::Patron

This patch adds article_request_fee() and add_article_request_fee_if_needed() methods to Koha::Patron.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 79 Martin Renvoize 2022-01-07 14:18:31 UTC
Created attachment 129175 [details] [review]
Bug 27946: Make Koha::ArticleRequest->request add a fee if required

This patch makes the ->request method add a fee for the patron if
required. It relies on methods defined in Koha::Patron for the task. The
debit line is linked to the AR if applies.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/ArticleRequest.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 80 Martin Renvoize 2022-01-07 14:18:36 UTC
Created attachment 129176 [details] [review]
Bug 27946: Add UI handling of AR fees

This patch:
- Adds a notice about the fee to be charged to the patron (staff and
  OPAC)
- Adds the right UI description for ARTICLE_REQUEST_FEE debit types in
  the UI.

To test:
1. Have some article request fee rules
2. Play with placing an article request in OPAC and staff
=> SUCCESS: You see a message about the fee
=> SUCCESS: The fees are correctly applied to the patron
3. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 81 Martin Renvoize 2022-01-07 14:18:40 UTC
Created attachment 129177 [details] [review]
Bug 27946: (QA follow-up) Remove 'scan' terminology, requests may not be scans

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 82 Martin Renvoize 2022-01-07 14:20:51 UTC
A nice simplification here, good plan splitting the cancellation stuff into it's own bug.

I'd have loved to have seen this at the item type level rather than just at patron category/library branch level.. certainly that would be the use case in the UK.

But.. it all works as designed right now for me, doesn't cause any regressions and is well tested.

QA scripts are happy too.. Passing QA
Comment 83 Fridolin Somers 2022-01-28 20:28:19 UTC
(In reply to Marcel de Rooy from comment #53)
> Seeing
> 
> Argument "" isn't numeric in numeric gt (>) at
> /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.
> tt line 373.
> Argument "" isn't numeric in numeric gt (>) at
> /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.
> tt line 378.
> 
> Typo  patron's categor

I still see those warns with QA scripts but looks like thos lines are not changed by this bug, more from Bug 29084.
Comment 84 Fridolin Somers 2022-01-28 21:12:42 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄