We propose adding a charge field (scan fee) to the patron category configuration screen. Cost for Article scan request: $____
Created attachment 120144 [details] [review] Bug 27946: Add tests
Created attachment 120145 [details] [review] Bug 27946: Add article_request_fee to categories table and ARTICLE_REQUEST account debit type
Created attachment 120146 [details] [review] [DO NOT PUSH] Koha 27946: Update categories schema
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
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.
[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.
[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
[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.
Read: cancel an article request after paying fee ..
[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.
[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.
Some of these 5 points justify a status change here now.
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
(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.
(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.
Created attachment 122529 [details] [review] Bug 27946: Add tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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>
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>
Created attachment 122532 [details] [review] Bug 27946: [DO NOT PUSH] Update DBIx schema
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.
(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.
(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 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 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 ); } ```
Created attachment 122552 [details] [review] Bug 27946: (follow-up) Use bug 28656
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.
Hmm a dependency on a NEW bug report?
(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.
(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'...
(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.
(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?
(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
Created attachment 123503 [details] [review] Bug 27946: Add tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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>
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>
Created attachment 123506 [details] [review] Bug 27946: [DO NOT PUSH] Update DBIx schema
Created attachment 123507 [details] [review] Bug 27946: (follow-up) Use bug 28656
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.
*** Bug 26088 has been marked as a duplicate of this bug. ***
Tomas, I am keen to signoff this one, but wouldnt it be helpful if you would add patches ;)
And what about the dependency ?
(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.
Tomas: Add some patches ;)
(In reply to Marcel de Rooy from comment #43) > Tomas: Add some patches ;) Good idea :-P
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>
Created attachment 128661 [details] [review] Bug 27946: DBIC schema changes [DO NOT PUSH] Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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
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
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
+ $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).
Few comments forthcoming still
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
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 ?
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?
(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!
(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.
(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!
(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.
(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.
(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.
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>
Created attachment 128869 [details] [review] Bug 27946: DBIC schema changes [DO NOT PUSH] Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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
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
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
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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
(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.
Pushed to master for 22.05, thanks to everybody involved [U+1F984]