We propose adding an "Article Request Limit" field to the patron category configuration. "Allow __ article requests per day" This limit would be enforced by querying the existing article requests in the database that have been placed during the day and allowing the patron to proceed or deny the request.
Created attachment 123217 [details] [review] Bug 27945: Add tests
Created attachment 123218 [details] [review] Bug 27945: Add article_request_limit in categories table
Created attachment 123219 [details] [review] Bug 27945: [DO NOT PUSH] Update DBIx schema
Created attachment 123220 [details] [review] Bug 27945: Add limit article request feature This patch makes it possible to limit article requests per patron per day. To test: 1. Apply patches 2. updatedatabase 3. Enable ArticleRequests preference 4. Edit a patron category and set an article request limit to 1 CHECK => if you set the limit to anything else but a positive number or empty string, a warning appears 5. In staff search biblios and request an article for a patron of the modified category 6. Repeat step 5 SUCCESS => if limit is reached, when you select the user to request an article a warning appears saying that the limit was reached 7. Repeat steps 5 and 6 but this time in opac SUCCESS => Patron is not allowed to request another article if limit is reached 8. prove t/db_dependent/ArticleRequests.t
Maybe 'article_request_daily_limit'?
Base does not apply
Hi Agustín. There are errors when running the database update: root@kohadevbox:koha(bz27945)$ updatedatabase DEV atomic update: bug_27945.perl Atomic update generated errors: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Unknown column 'article_request_fee' in 'categories' [for Statement " ALTER TABLE `categories` ADD COLUMN `article_request_limit` tinyint(4) DEFAULT NULL COMMENT 'Limit article scan requests per day' AFTER `article_request_fee` "] at /usr/share/perl5/DBIx/Class/Schema.pm line 1118. DBIx::Class::Schema::throw_exception(Koha::Schema=HASH(0x55c0aed443e8), "DBI Exception: DBD::mysql::db do failed: Unknown column 'arti"...) called at /usr/share/perl5/DBIx/Class/Storage.pm line 113 DBIx::Class::Storage::throw_exception(DBIx::Class::Storage::DBI::mysql=HASH(0x55c0b29ba4b0), "DBI Exception: DBD::mysql::db do failed: Unknown column 'arti"...) called at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1501 DBIx::Class::Storage::DBI::__ANON__("DBD::mysql::db do failed: Unknown column 'article_request_fee"..., DBI::db=HASH(0x55c0b4469f90), undef) called at (eval 1487) line 7 eval '$DBversion = \'XXX\'; # will be replaced by the RM if( CheckVersion( $DBversion ) ) { # you can use $dbh here like: # $dbh->do( "ALTER TABLE biblio ADD COLUMN badtaste int" ); # or perform some test and warn if( !column_exists( \'categories\', \'article_request_limit\' ) ) { $dbh->do(q{ ALTER TABLE `categories` ADD COLUMN `article_request_limit` tinyint(4) DEFAULT NULL COMMENT \'Limit article scan requests per day\' AFTER `article_request_fee` }); } # Always end with this (adjust the bug info) NewVersion( $DBversion, 27945, "Add article_request_limit to categories table"); } ' called at /kohadevbox/koha/installer/data/mysql/updatedatabase.pl line 24596 DEV atomic update: bug_27946.perl Upgrade to XXX done [20:29:34]: Bug 27946 - Add article_request_fee to categories table, add debit_line_id column to article_requests table and add ARTICLE_REQUEST as a debit type
(In reply to David Nind from comment #7) > Hi Agustín. > > There are errors when running the database update: Hi David, thanks for taking the time to test this! I've just applied bug 27946 and then this one, and the database update ran smoothly. Please try again! It seems that you didn't have 27946 DB upgrade applied.
On top of 27946: Applying: Bug 27945: Add tests Using index info to reconstruct a base tree... M t/db_dependent/ArticleRequests.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/ArticleRequests.t CONFLICT (content): Merge conflict in t/db_dependent/ArticleRequests.t error: Failed to merge in the changes.
(In reply to Marcel de Rooy from comment #9) > On top of 27946: > > Applying: Bug 27945: Add tests > Using index info to reconstruct a base tree... > M t/db_dependent/ArticleRequests.t > Falling back to patching base and 3-way merge... > Auto-merging t/db_dependent/ArticleRequests.t > CONFLICT (content): Merge conflict in t/db_dependent/ArticleRequests.t > error: Failed to merge in the changes. Wait thats not correct
(In reply to Marcel de Rooy from comment #10) > (In reply to Marcel de Rooy from comment #9) > > On top of 27946: > > > > Applying: Bug 27945: Add tests > > Using index info to reconstruct a base tree... > > M t/db_dependent/ArticleRequests.t > > Falling back to patching base and 3-way merge... > > Auto-merging t/db_dependent/ArticleRequests.t > > CONFLICT (content): Merge conflict in t/db_dependent/ArticleRequests.t > > error: Failed to merge in the changes. > > Wait thats not correct It does apply correctly. Wrong base..
FAIL opac/opac-request-article.pl OK critic FAIL forbidden patterns forbidden pattern: Use of Data::Printer, should only be used for devlopment purpose (line 85) OK git manipulation OK pod SKIP spelling FAIL valid Can't locate Data/Printer.pm in @INC
Created attachment 123714 [details] [review] Bug 27945: Add limit article request feature This patch makes it possible to limit article requests per patron per day. To test: 1. Apply patches 2. updatedatabase 3. Enable ArticleRequests preference 4. Edit a patron category and set an article request limit to 1 CHECK => if you set the limit to anything else but a positive number or empty string, a warning appears 5. In staff search biblios and request an article for a patron of the modified category 6. Repeat step 5 SUCCESS => if limit is reached, when you select the user to request an article a warning appears saying that the limit was reached 7. Repeat steps 5 and 6 but this time in opac SUCCESS => Patron is not allowed to request another article if limit is reached 8. prove t/db_dependent/ArticleRequests.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 123715 [details] [review] Bug 27945: (QA follow-up) Fix exception POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 123774 [details] [review] Bug 27945: Add tests Signed-off-by: David Nind <david@davidnind.com>
Created attachment 123775 [details] [review] Bug 27945: Add article_request_limit in categories table Signed-off-by: David Nind <david@davidnind.com>
Created attachment 123776 [details] [review] Bug 27945: [DO NOT PUSH] Update DBIx schema Signed-off-by: David Nind <david@davidnind.com>
Created attachment 123777 [details] [review] Bug 27945: Add limit article request feature This patch makes it possible to limit article requests per patron per day. To test: 1. Apply patches 2. updatedatabase 3. Enable ArticleRequests preference 4. Edit a patron category and set an article request limit to 1 CHECK => if you set the limit to anything else but a positive number or empty string, a warning appears 5. In staff search biblios and request an article for a patron of the modified category 6. Repeat step 5 SUCCESS => if limit is reached, when you select the user to request an article a warning appears saying that the limit was reached 7. Repeat steps 5 and 6 but this time in opac SUCCESS => Patron is not allowed to request another article if limit is reached 8. prove t/db_dependent/ArticleRequests.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 123778 [details] [review] Bug 27945: (QA follow-up) Fix exception POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
One minor thing I noted after signing off. The column heading in patron categories is 'Maximum request limit' and the edit form says 'Maximum article scan requests'.
(In reply to David Nind from comment #20) > One minor thing I noted after signing off. > > The column heading in patron categories is 'Maximum request limit' and the > edit form says 'Maximum article scan requests'. Thanks David, I'll attach a follow-up
Created attachment 123820 [details] [review] Bug 27945: (follow-up) Fix column header title and edit form label
FAIL Koha/Exceptions/ArticleRequest.pm FAIL pod coverage POD is missing for 'description' False negative
Koha::Patron + my $count = Koha::ArticleRequests->search({ + borrowernumber => $self->borrowernumber, + status => {'!=' => 'CANCELED'}, + created_on => { + '>=' => $date->date.' 00:00:00', + '<=' => $date->date.' 23:59:59' + } + })->count; We should use Koha::Database->new->schema->storage->datetime_parser. Look in the same modules for more examples.
Side note Currently we use the following custom check to limit numbers: my $compdate = dt_from_string->add( days => -1 ); my $count = Koha::ArticleRequests->search([ { borrowernumber => $borrowernumber, status => ['PENDING','PROCESSING'] }, { borrowernumber => $borrowernumber, status => 'COMPLETED', updated_on => { '>', Koha::Database->new->schema->storage->datetime_parser->format_date($compdate) }}, ])->count; This does not take into consideration the new/requested and pending status. So pending is still new here. But it looks at the number of requests in the pipeline for the user PLUS the completed ones within the last 24 hours. Advantage illustrated by the example below: Proposed check: Say that the limit is 5. User submits 5 requests on Friday night, Saturday, Sunday and Monday morning. This makes that we need to process 20 requests coming in on Monday. "Custom" check: User may only submit 5 requests on Friday night if we did not complete requests during that day. He is not allowed any more requests during the weekend. Only when we complete say 1 request on Monday, he is allowed to submit another one on Tuesday (24 hours later). What do you think?
diff --git a/Koha/ArticleRequest.pm b/Koha/ArticleRequest.pm index e79e01610c..078f00bc70 100644 --- a/Koha/ArticleRequest.pm +++ b/Koha/ArticleRequest.pm @@ -27,7 +27,9 @@ use Koha::Items; use Koha::Libraries; use Koha::DateUtils qw( dt_from_string ); use C4::Context; +use Koha::ArticleRequests; Why do you add it here? Note that bug 27944 adds the Status module.
} elsif ($article_request_limit ne '' && $article_request_limit !~ /\d+/) { If I add 2a in the limit field (fool proof), the form says that I successfully updated the record. But actually I didnt. Your regex could include ^\d+$ ?
Overall: Looks good to me. Discussion item in comment25. Needs a few small adjustments for comment24, comment26 and comment27. Note that the dependency on 27946 is unfortunate. This could reach master sooner than 27946 which has a dependency on unfinished Accounting work.
Created attachment 124066 [details] [review] Bug 27945: Add tests Signed-off-by: David Nind <david@davidnind.com> Current depends:
Created attachment 124067 [details] [review] Bug 27945: Add article_request_limit in categories table Signed-off-by: David Nind <david@davidnind.com>
Created attachment 124068 [details] [review] Bug 27945: [DO NOT PUSH] Update DBIx schema Signed-off-by: David Nind <david@davidnind.com>
Created attachment 124069 [details] [review] Bug 27945: Add limit article request feature This patch makes it possible to limit article requests per patron per day. To test: 1. Apply patches 2. updatedatabase 3. Enable ArticleRequests preference 4. Edit a patron category and set an article request limit to 1 CHECK => if you set the limit to anything else but a positive number or empty string, a warning appears 5. In staff search biblios and request an article for a patron of the modified category 6. Repeat step 5 SUCCESS => if limit is reached, when you select the user to request an article a warning appears saying that the limit was reached 7. Repeat steps 5 and 6 but this time in opac SUCCESS => Patron is not allowed to request another article if limit is reached 8. prove t/db_dependent/ArticleRequests.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 124070 [details] [review] Bug 27945: (QA follow-up) Fix exception POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 124071 [details] [review] Bug 27945: (follow-up) Fix column header title and edit form label
Created attachment 124072 [details] [review] Bug 27945: Add tests Signed-off-by: David Nind <david@davidnind.com>
Created attachment 124073 [details] [review] Bug 27945: Add article_request_limit in categories table Signed-off-by: David Nind <david@davidnind.com>
Created attachment 124074 [details] [review] Bug 27945: [DO NOT PUSH] Update DBIx schema Signed-off-by: David Nind <david@davidnind.com>
Created attachment 124075 [details] [review] Bug 27945: Add limit article request feature This patch makes it possible to limit article requests per patron per day. To test: 1. Apply patches 2. updatedatabase 3. Enable ArticleRequests preference 4. Edit a patron category and set an article request limit to 1 CHECK => if you set the limit to anything else but a positive number or empty string, a warning appears 5. In staff search biblios and request an article for a patron of the modified category 6. Repeat step 5 SUCCESS => if limit is reached, when you select the user to request an article a warning appears saying that the limit was reached 7. Repeat steps 5 and 6 but this time in opac SUCCESS => Patron is not allowed to request another article if limit is reached 8. prove t/db_dependent/ArticleRequests.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 124076 [details] [review] Bug 27945: (QA follow-up) Fix exception POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 124077 [details] [review] Bug 27945: (follow-up) Fix column header title and edit form label
Created attachment 124078 [details] [review] Bug 27945: (follow-up) fix limit check in Patron.pm, regexp in categories.pl and removed unwanted use Koha::ArticleRequests in ArticleRequest.pm
Created attachment 124079 [details] [review] Bug 27945: (follow-up) fix limit check in Patron.pm, regexp in categories.pl and removed unwanted use Koha::ArticleRequests in ArticleRequest.pm
Hi Marcel, I removed dependency on bug 27946, implemented the check in comment 25 (with a minor modification), and addressed issues in comment 26 (that I believe it was due to a merge when I initially rebased 27946) and comment 27 Could you check it again? Thanks PS: I believe we should change texts in category form and patron's alert, because we no longer limit requests per day, but we limit "concurrent" requests per day
QA: Looking here
(In reply to Agustín Moyano from comment #43) > Hi Marcel, > > I removed dependency on bug 27946, implemented the check in comment 25 (with > a minor modification), and addressed issues in comment 26 (that I believe it > was due to a merge when I initially rebased 27946) and comment 27 > > Could you check it again? Will do. Looks good at first glance. > PS: I believe we should change texts in category form and patron's alert, > because we no longer limit requests per day, but we limit "concurrent" > requests per day Good point. The term "Maximum article request limit" could still be improved. Can understand that it is hard to get the complete meaning into a short phrase.
There is a functional dependency between this report and 27944. See bug 27944#68
+ { borrowernumber => $self->borrowernumber, status => { '!=' => ['CANCELED','COMPLETED'] } }, [2021/08/31 12:59:03] [WARN] SQL::Abstract::belch(): [SQL::Abstract::_where_field_op_ARRAYREF] Warning: A multi-element arrayref as an argument to the inequality op '!=' is technically equivalent to an always-true 1=1 (you probably wanted to say ...{ $inequality_op => [ -and => @values ] }... instead) at /usr/share/koha/Koha/Objects.pm line 601 Will fix this in a follow-up. No worries.
(In reply to Marcel de Rooy from comment #45) > (In reply to Agustín Moyano from comment #43) > > PS: I believe we should change texts in category form and patron's alert, > > because we no longer limit requests per day, but we limit "concurrent" > > requests per day > > Good point. The term "Maximum article request limit" could still be > improved. Can understand that it is hard to get the complete meaning into a > short phrase. What about "active article requests" ?
Created attachment 124284 [details] [review] Bug 27945: (QA follow-up) Fix limit check Resolve: [WARN] SQL::Abstract::belch(): [SQL::Abstract::_where_field_op_ARRAYREF] Warning: A multi-element arrayref as an argument to the inequality op '!=' is technically equivalent to an always-true 1=1 (you probably wanted to say ...{ $inequality_op => [ -and => @values ] }... instead) at /usr/share/koha/Koha/Objects.pm line 601 In this case we can simply count the statuses PENDING and PROCESSING. Note that bug 27944 will change that; PENDING should be replaced by REQUESTED there. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124285 [details] [review] Bug 27945: (QA follow-up) Terminology The term "Maximum article request limit" could be improved. In this patch we use 'Article request limit' to describe it more generic. On the field we say 'Maximum active article requests'. In the manual we need to explain that it is the sum of requests in Pending (Requested) or Processing stage and completed requests within the last 24 hours. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
We still need one thing imo: Fix the added tests for the adjusted check.
Created attachment 124633 [details] [review] Bug 27945: (follow-up) Fix tests for the adjusted check
Created attachment 124673 [details] [review] Bug 27945: Add tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124674 [details] [review] Bug 27945: Add article_request_limit in categories table Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124675 [details] [review] Bug 27945: [DO NOT PUSH] Update DBIx schema Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124676 [details] [review] Bug 27945: Add limit article request feature This patch makes it possible to limit article requests per patron per day. To test: 1. Apply patches 2. updatedatabase 3. Enable ArticleRequests preference 4. Edit a patron category and set an article request limit to 1 CHECK => if you set the limit to anything else but a positive number or empty string, a warning appears 5. In staff search biblios and request an article for a patron of the modified category 6. Repeat step 5 SUCCESS => if limit is reached, when you select the user to request an article a warning appears saying that the limit was reached 7. Repeat steps 5 and 6 but this time in opac SUCCESS => Patron is not allowed to request another article if limit is reached 8. prove t/db_dependent/ArticleRequests.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124677 [details] [review] Bug 27945: (QA follow-up) Fix exception POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124678 [details] [review] Bug 27945: (follow-up) Fix column header title and edit form label Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124679 [details] [review] Bug 27945: (follow-up) fix limit check in Patron.pm, regexp in categories.pl and removed unwanted use Koha::ArticleRequests in ArticleRequest.pm Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124680 [details] [review] Bug 27945: (QA follow-up) Fix limit check Resolve: [WARN] SQL::Abstract::belch(): [SQL::Abstract::_where_field_op_ARRAYREF] Warning: A multi-element arrayref as an argument to the inequality op '!=' is technically equivalent to an always-true 1=1 (you probably wanted to say ...{ $inequality_op => [ -and => @values ] }... instead) at /usr/share/koha/Koha/Objects.pm line 601 In this case we can simply count the statuses PENDING and PROCESSING. Note that bug 27944 will change that; PENDING should be replaced by REQUESTED there. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124681 [details] [review] Bug 27945: (QA follow-up) Terminology The term "Maximum article request limit" could be improved. In this patch we use 'Article request limit' to describe it more generic. On the field we say 'Maximum active article requests'. In the manual we need to explain that it is the sum of requests in Pending (Requested) or Processing stage and completed requests within the last 24 hours. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124682 [details] [review] Bug 27945: (follow-up) Fix tests for the adjusted check Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Agustín Moyano from comment #52) > Created attachment 124633 [details] [review] [review] > Bug 27945: (follow-up) Fix tests for the adjusted check Thx Agustin. Looks good to me now.
If I apply patches from this bug (27945) and the ones from bug 27944 I am getting the following failures: t/db_dependent/ArticleRequests.t .. 22/55 # Failed test 'Limit is 1, so patron cannot request more articles' # at t/db_dependent/ArticleRequests.t line 292. # got: '1' # expected: '0' # Failed test 'There is still an AR, so patron cannot request more articles' # at t/db_dependent/ArticleRequests.t line 309. # got: '1' # expected: '0' # Failed test 'There is still one article request' # at t/db_dependent/ArticleRequests.t line 310. # got: '2' # expected: '1' # Failed test 'A new AR was created, so patron cannot request more articles' # at t/db_dependent/ArticleRequests.t line 329. # got: '1' # expected: '0' # Failed test 'There are 2 article requests' # at t/db_dependent/ArticleRequests.t line 330. # got: '3' # expected: '2' # Failed test 'There is an old AR but not completed or cancelled, so patron cannot request more articles' # at t/db_dependent/ArticleRequests.t line 347. # got: '1' # expected: '0' # Failed test 'There are 3 current article requests' # at t/db_dependent/ArticleRequests.t line 348. # got: '4' # expected: '3' # Looks like you planned 13 tests but ran 12. # Looks like you failed 7 tests of 12 run. t/db_dependent/ArticleRequests.t .. 55/55 # Failed test 'article request limit' # at t/db_dependent/ArticleRequests.t line 350. There are 3 PQA bugs related to article requests (bug 27944, bug 27945 and bug 27947). Please make sure they don't conflict with each others and that the tests pass.
Tests pass on its own, I've now set bugs dependencies, so if other bug breaks this, we will treat it in the specific bug.
Created attachment 125140 [details] [review] Bug 27945: Add tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125142 [details] [review] Bug 27945: Add article_request_limit in categories table Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125143 [details] [review] Bug 27945: [DO NOT PUSH] Update DBIx schema Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125144 [details] [review] Bug 27945: Add limit article request feature This patch makes it possible to limit article requests per patron per day. To test: 1. Apply patches 2. updatedatabase 3. Enable ArticleRequests preference 4. Edit a patron category and set an article request limit to 1 CHECK => if you set the limit to anything else but a positive number or empty string, a warning appears 5. In staff search biblios and request an article for a patron of the modified category 6. Repeat step 5 SUCCESS => if limit is reached, when you select the user to request an article a warning appears saying that the limit was reached 7. Repeat steps 5 and 6 but this time in opac SUCCESS => Patron is not allowed to request another article if limit is reached 8. prove t/db_dependent/ArticleRequests.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125145 [details] [review] Bug 27945: (QA follow-up) Fix exception POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125146 [details] [review] Bug 27945: (follow-up) Fix column header title and edit form label Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125147 [details] [review] Bug 27945: (follow-up) fix limit check in Patron.pm, regexp in categories.pl and removed unwanted use Koha::ArticleRequests in ArticleRequest.pm Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125148 [details] [review] Bug 27945: (QA follow-up) Fix limit check Resolve: [WARN] SQL::Abstract::belch(): [SQL::Abstract::_where_field_op_ARRAYREF] Warning: A multi-element arrayref as an argument to the inequality op '!=' is technically equivalent to an always-true 1=1 (you probably wanted to say ...{ $inequality_op => [ -and => @values ] }... instead) at /usr/share/koha/Koha/Objects.pm line 601 In this case we can simply count the statuses PENDING and PROCESSING. Note that bug 27944 will change that; PENDING should be replaced by REQUESTED there. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125149 [details] [review] Bug 27945: (QA follow-up) Terminology The term "Maximum article request limit" could be improved. In this patch we use 'Article request limit' to describe it more generic. On the field we say 'Maximum active article requests'. In the manual we need to explain that it is the sum of requests in Pending (Requested) or Processing stage and completed requests within the last 24 hours. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125150 [details] [review] Bug 27945: (follow-up) Fix tests for the adjusted check Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125151 [details] [review] Bug 27945: Move atomicupdate to new format It also removes the implicit dependency on bug 27946 by changing the AFTER column. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 125152 [details] [review] Bug 27945: (QA follow-up) Fix and move tests to proper file Koha::Patron methods tests belong to the t/db_dependent/Koha/Patron.t tests file. This patch moves the tests, and also acknowledges the fact that we can use Test::Exception instead of a try/catch block. It also fixes the tests so they actually trigger the ->request method, which is the one that raises the exception if the limit is reached. At some point, because of the dependency mess, this bug ended up with the exception throwing in the wrong method, and that's why tests were also failing. To test: 1. Run: $ kshell k$ prove t/db_dependent/Koha/Patron.t \ t/db_dependent/ArticleRequests.t => FAIL: Patron.t passes, ArticleRequests.t doesn't 2. Apply this patch 3. Repeat 1 => SUCCESS: Both pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Rebased on top of bug 27944, fixed a problem introduced by a rebase, fixed the tests.
Created attachment 125178 [details] [review] Bug 27945: Move atomicupdate to new format It also removes the implicit dependency on bug 27946 by changing the AFTER column. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125179 [details] [review] Bug 27945: (QA follow-up) Fix and move tests to proper file Koha::Patron methods tests belong to the t/db_dependent/Koha/Patron.t tests file. This patch moves the tests, and also acknowledges the fact that we can use Test::Exception instead of a try/catch block. It also fixes the tests so they actually trigger the ->request method, which is the one that raises the exception if the limit is reached. At some point, because of the dependency mess, this bug ended up with the exception throwing in the wrong method, and that's why tests were also failing. To test: 1. Run: $ kshell k$ prove t/db_dependent/Koha/Patron.t \ t/db_dependent/ArticleRequests.t => FAIL: Patron.t passes, ArticleRequests.t doesn't 2. Apply this patch 3. Repeat 1 => SUCCESS: Both pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
1. +my $can_request = $borrower->can_request_article $patron is better now 2. + my $compdate = dt_from_string->add( days => -1 ); I don't understand, what are we doing here? Can you add a line in the POD about that? 3. + if ($article_request_limit ne '' && $article_request_limit !~ /^\d+$/) { + push @messages, {type => 'error', code => 'article_request_numeric_limit' }; + $op = 'add_form'; + } elsif ($article_request_limit ne '' && $article_request_limit < 0) { + push @messages, {type => 'error', code => 'article_request_negative_limit' }; + $op = 'add_form'; + } else { Why that? I am expecting to see a JS validation and an exception raised in Koha::Category->store. But not in the controller. 4. circ/request-article.pl + error_message => 'Patron cannot request more articles for today' <p>[% error_message | html %]</p> Hum? Non translatable message. Same for the exception actually: + error_message => $_->{message} And same in opac-request-article.pl 5. opac-request-article.pl + exit unless $_->[0] && $_->[0] eq 'EXIT'; This is very awkward. Why not simply having the print+exit outside of the try?
Created attachment 125569 [details] [review] Bug 27945: (QA follow-up) Clarify behavior in POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Jonathan Druart from comment #81) > 1. +my $can_request = $borrower->can_request_article > $patron is better now Done. > 2. > + my $compdate = dt_from_string->add( days => -1 ); > I don't understand, what are we doing here? Can you add a line in the POD > about that? Done.
(In reply to Jonathan Druart from comment #81) > 3. > > + if ($article_request_limit ne '' && $article_request_limit !~ /^\d+$/) { > + push @messages, {type => 'error', code => > 'article_request_numeric_limit' }; > + $op = 'add_form'; > + } elsif ($article_request_limit ne '' && $article_request_limit < 0) { > + push @messages, {type => 'error', code => > 'article_request_negative_limit' }; > + $op = 'add_form'; > + } else { > > Why that? > I am expecting to see a JS validation and an exception raised in > Koha::Category->store. But not in the controller. Is this a blocker for inclusion?
Created attachment 125571 [details] [review] Bug 27945: (QA follow-up) Validate the input in the form Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Tomás Cohen Arazi from comment #84) > (In reply to Jonathan Druart from comment #81) > > 3. > > > > + if ($article_request_limit ne '' && $article_request_limit !~ /^\d+$/) { > > + push @messages, {type => 'error', code => > > 'article_request_numeric_limit' }; > > + $op = 'add_form'; > > + } elsif ($article_request_limit ne '' && $article_request_limit < 0) { > > + push @messages, {type => 'error', code => > > 'article_request_negative_limit' }; > > + $op = 'add_form'; > > + } else { > > > > Why that? > > I am expecting to see a JS validation and an exception raised in > > Koha::Category->store. But not in the controller. > > Is this a blocker for inclusion? I think so, it does not make sense to have it there. It may be better to just remove this check as we have the JS validation. If it is really what you need, you must use a "int unsigned" at DB level. Btw why do you use tinyint(4) when other "limit" columns in the same table are smallint(6)?
I am not convinced by the COMPLETED, updated_on code: 965 Returns true if the patron can request articles. As limits apply for the patron 966 on on the same day, those completed the same day are considered as current. 976 my $dtf = Koha::Database->new->schema->storage->datetime_parser; 977 my $compdate = dt_from_string->add( days => -1 ); 978 my $count = Koha::ArticleRequests->search([ 979 { borrowernumber => $self->borrowernumber, status => ['REQUESTED','PENDING','PROCESSING'] }, 980 { borrowernumber => $self->borrowernumber, status => 'COMPLETED', updated_on => { '>', $dtf->format_date($compdate) }}, if today is Oct 1st, $dtf->format_date($compdate) will be Sept 30th and updated_on > 2021-09-30 will return the rows updated yesterday as well. There is no test coverage for that.
(In reply to Jonathan Druart from comment #86) > I think so, it does not make sense to have it there. It may be better to > just remove this check as we have the JS validation. > > If it is really what you need, you must use a "int unsigned" at DB level. Agustin added the test with \d+ before there was js validation. I agree to remove it now. Not sure if we should really specify unsigned in SQL; did we do that more often? We could just check !value < 0 in js as well.
(In reply to Jonathan Druart from comment #87) > if today is Oct 1st, $dtf->format_date($compdate) will be Sept 30th and > updated_on > 2021-09-30 will return the rows updated yesterday as well. Read 'same day' here as last 24 hours.
(In reply to Marcel de Rooy from comment #89) > (In reply to Jonathan Druart from comment #87) > > > if today is Oct 1st, $dtf->format_date($compdate) will be Sept 30th and > > updated_on > 2021-09-30 will return the rows updated yesterday as well. > > Read 'same day' here as last 24 hours. Are you sure this is correct? Sept 30th 16pm is yesterday for me, and it's Oct 1st 11am now.
(In reply to Marcel de Rooy from comment #88) > (In reply to Jonathan Druart from comment #86) > > I think so, it does not make sense to have it there. It may be better to > > just remove this check as we have the JS validation. > > > > If it is really what you need, you must use a "int unsigned" at DB level. > > Agustin added the test with \d+ before there was js validation. I agree to > remove it now. Not sure if we should really specify unsigned in SQL; did we > do that more often? We could just check !value < 0 in js as well. The DB structure and data types should help us avoid useless manual validations. But as we don't use STRICT_TRANS_TABLES, even marking the column as UNSIGNED INT will allow setting even a string... This is one of the reasons we should push for the API instead of this controller scripts (for data entry at least), because we can add the rules to the spec, and the OpenAPI plugin validates all the things for us. That said, I believe we can either keep or remove the validation. Keeping it prevents passing bad data manually, removing it cleans the controller a tiny bit. I would say let's do whatever the RM is more comfortable with so we move on.
(In reply to Jonathan Druart from comment #87) > I am not convinced by the COMPLETED, updated_on code: > > 965 Returns true if the patron can request articles. As limits apply for > the patron > 966 on on the same day, those completed the same day are considered as > current. > > 976 my $dtf = Koha::Database->new->schema->storage->datetime_parser; > 977 my $compdate = dt_from_string->add( days => -1 ); > 978 my $count = Koha::ArticleRequests->search([ > 979 { borrowernumber => $self->borrowernumber, status => > ['REQUESTED','PENDING','PROCESSING'] }, > 980 { borrowernumber => $self->borrowernumber, status => > 'COMPLETED', updated_on => { '>', $dtf->format_date($compdate) }}, > > if today is Oct 1st, $dtf->format_date($compdate) will be Sept 30th and > updated_on > 2021-09-30 will return the rows updated yesterday as well. > > There is no test coverage for that. So this is the main blocker for this dev. I will add tests: Marcel: what is the right behavior? 24 hours timespan? same day? I feel like 'same day' is the right thing. But will implement it as you want, so: how?
(In reply to Marcel de Rooy from comment #88) > do that more often? We could just check !value < 0 in js as well. The JS validation I added already checks the min value is 1. So no negative values allowed (in the form)
(In reply to Tomás Cohen Arazi from comment #91) > The DB structure and data types should help us avoid useless manual > validations. But as we don't use STRICT_TRANS_TABLES, even marking the > column as UNSIGNED INT will allow setting even a string... (nitpicky mode) It will actually raise a non-blocker warnings and the value will be ignored. The DBMS won't store a string into a int ;)
(In reply to Tomás Cohen Arazi from comment #92) > Marcel: what is the right behavior? Thats a good question.
(In reply to Marcel de Rooy from comment #95) > (In reply to Tomás Cohen Arazi from comment #92) > > Marcel: what is the right behavior? > > Thats a good question. I'm about to resubmit this patchset with some requests from the RM, and I will make it 'same day' as it was at some point. Sand promise to work on another approach on another bug if it is filed.
Created attachment 125638 [details] [review] Bug 27945: Clarify 'same day' behavior This patch introduces tests for the 'same day' check of the ability to place article requests for a patron. The limit goes against current requests, and those that have been completed on the same day. The tests cover this specific situation. The current behavior is that it takes into account a 24 hr timespan, but consensus on the QA step was that we should do it as 'same day' and use a separate feature request to change this, if required. 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: Tomas Cohen Arazi <tomascohen@theke.io>
But rhetorical questions dont need an answer ;)
Created attachment 125690 [details] [review] Bug 27945: (follow-up) Clarify 'same day' behavior Don't use MySQLism
Why is it a category's attribute and not a circ rule?
Created attachment 125702 [details] [review] Bug 27945: Add tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 125703 [details] [review] Bug 27945: Add limit article request feature This patch makes it possible to limit article requests per patron per day. To test: 1. Apply patches 2. updatedatabase 3. Enable ArticleRequests preference 4. Edit a patron category and set an article request limit to 1 CHECK => if you set the limit to anything else but a positive number or empty string, a warning appears 5. In staff search biblios and request an article for a patron of the modified category 6. Repeat step 5 SUCCESS => if limit is reached, when you select the user to request an article a warning appears saying that the limit was reached 7. Repeat steps 5 and 6 but this time in opac SUCCESS => Patron is not allowed to request another article if limit is reached 8. prove t/db_dependent/ArticleRequests.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Edit: This patchset originally changed the 'categories' table structure and relied on that for limit calculation. I removed all that code and squashed into this one, as we moved everything to the circulation_rules table.
Created attachment 125704 [details] [review] Bug 27945: Clarify 'same day' behavior This patch introduces tests for the 'same day' check of the ability to place article requests for a patron. The limit goes against current requests, and those that have been completed on the same day. The tests cover this specific situation. The current behavior is that it takes into account a 24 hr timespan, but consensus on the QA step was that we should do it as 'same day' and use a separate feature request to change this, if required. 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: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 125705 [details] [review] Bug 27945: Add max_daily_article_requests circulation rule Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 125706 [details] [review] Bug 27945: Implement limits using circulation rules
Created attachment 125707 [details] [review] Bug 27945: Fix error handling and translatability This patch adds better error handling and reporting when placing an article request fails. It also makes the error messages translatable. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 125726 [details] [review] Bug 27945: Don't save rule if not defined The DB rev added a row with NULL (unlimited), and the UI a row with '' (unlimited as well) when saved without value. Better is to not have a row (still unlimited)
Created attachment 125741 [details] [review] Bug 27945: Terminology: max daily => open requests limit Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 21.11, thanks to everybody involved!
Created attachment 125761 [details] [review] Bug 27945: (follow-up) Add tests Restore +x flag
(In reply to Jonathan Druart from comment #110) > Created attachment 125761 [details] [review] [review] > Bug 27945: (follow-up) Add tests > > Restore +x flag Pushed to master.
I added an open_article_requests_limit without patron category by script. And it works fine. Disadvantage: the record is not shown on the Circulation rules interface..