Bug 27945

Summary: Limit the number of active article requests per patron category
Product: Koha Reporter: Nick Clemens <nick>
Component: CirculationAssignee: Agustín Moyano <agustinmoyano>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: caroline.cyr-la-rose, david, gmcharlt, jonathan.druart, kyle.m.hall, kyle, m.de.rooy, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26129
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This enhancement lets you limit the number of active article requests a patron can make each day. Edit the patron category and enter the 'Maximum active article requests'.
Version(s) released in:
21.11.00
Bug Depends on: 27944    
Bug Blocks: 29168, 32262, 27947, 29169    
Attachments: Bug 27945: Add tests
Bug 27945: Add article_request_limit in categories table
Bug 27945: [DO NOT PUSH] Update DBIx schema
Bug 27945: Add limit article request feature
Bug 27945: Add limit article request feature
Bug 27945: (QA follow-up) Fix exception POD
Bug 27945: Add tests
Bug 27945: Add article_request_limit in categories table
Bug 27945: [DO NOT PUSH] Update DBIx schema
Bug 27945: Add limit article request feature
Bug 27945: (QA follow-up) Fix exception POD
Bug 27945: (follow-up) Fix column header title and edit form label
Bug 27945: Add tests
Bug 27945: Add article_request_limit in categories table
Bug 27945: [DO NOT PUSH] Update DBIx schema
Bug 27945: Add limit article request feature
Bug 27945: (QA follow-up) Fix exception POD
Bug 27945: (follow-up) Fix column header title and edit form label
Bug 27945: Add tests
Bug 27945: Add article_request_limit in categories table
Bug 27945: [DO NOT PUSH] Update DBIx schema
Bug 27945: Add limit article request feature
Bug 27945: (QA follow-up) Fix exception POD
Bug 27945: (follow-up) Fix column header title and edit form label
Bug 27945: (follow-up) fix limit check in Patron.pm, regexp in categories.pl and removed unwanted use Koha::ArticleRequests in ArticleRequest.pm
Bug 27945: (follow-up) fix limit check in Patron.pm, regexp in categories.pl and removed unwanted use Koha::ArticleRequests in ArticleRequest.pm
Bug 27945: (QA follow-up) Fix limit check
Bug 27945: (QA follow-up) Terminology
Bug 27945: (follow-up) Fix tests for the adjusted check
Bug 27945: Add tests
Bug 27945: Add article_request_limit in categories table
Bug 27945: [DO NOT PUSH] Update DBIx schema
Bug 27945: Add limit article request feature
Bug 27945: (QA follow-up) Fix exception POD
Bug 27945: (follow-up) Fix column header title and edit form label
Bug 27945: (follow-up) fix limit check in Patron.pm, regexp in categories.pl and removed unwanted use Koha::ArticleRequests in ArticleRequest.pm
Bug 27945: (QA follow-up) Fix limit check
Bug 27945: (QA follow-up) Terminology
Bug 27945: (follow-up) Fix tests for the adjusted check
Bug 27945: Add tests
Bug 27945: Add article_request_limit in categories table
Bug 27945: [DO NOT PUSH] Update DBIx schema
Bug 27945: Add limit article request feature
Bug 27945: (QA follow-up) Fix exception POD
Bug 27945: (follow-up) Fix column header title and edit form label
Bug 27945: (follow-up) fix limit check in Patron.pm, regexp in categories.pl and removed unwanted use Koha::ArticleRequests in ArticleRequest.pm
Bug 27945: (QA follow-up) Fix limit check
Bug 27945: (QA follow-up) Terminology
Bug 27945: (follow-up) Fix tests for the adjusted check
Bug 27945: Move atomicupdate to new format
Bug 27945: (QA follow-up) Fix and move tests to proper file
Bug 27945: Move atomicupdate to new format
Bug 27945: (QA follow-up) Fix and move tests to proper file
Bug 27945: (QA follow-up) Clarify behavior in POD
Bug 27945: (QA follow-up) Validate the input in the form
Bug 27945: Clarify 'same day' behavior
Bug 27945: (follow-up) Clarify 'same day' behavior
Bug 27945: Add tests
Bug 27945: Add limit article request feature
Bug 27945: Clarify 'same day' behavior
Bug 27945: Add max_daily_article_requests circulation rule
Bug 27945: Implement limits using circulation rules
Bug 27945: Fix error handling and translatability
Bug 27945: Don't save rule if not defined
Bug 27945: Terminology: max daily => open requests limit
Bug 27945: (follow-up) Add tests

Description Nick Clemens 2021-03-12 14:45:12 UTC
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.
Comment 1 Agustín Moyano 2021-07-27 15:38:09 UTC
Created attachment 123217 [details] [review]
Bug 27945: Add tests
Comment 2 Agustín Moyano 2021-07-27 15:38:13 UTC
Created attachment 123218 [details] [review]
Bug 27945: Add article_request_limit in categories table
Comment 3 Agustín Moyano 2021-07-27 15:38:17 UTC
Created attachment 123219 [details] [review]
Bug 27945: [DO NOT PUSH] Update DBIx schema
Comment 4 Agustín Moyano 2021-07-27 15:38:21 UTC
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
Comment 5 Tomás Cohen Arazi 2021-07-27 15:39:57 UTC
Maybe 'article_request_daily_limit'?
Comment 6 Marcel de Rooy 2021-07-28 11:43:42 UTC
Base does not apply
Comment 7 David Nind 2021-08-05 20:31:40 UTC
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
Comment 8 Tomás Cohen Arazi 2021-08-10 13:18:37 UTC
(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.
Comment 9 Marcel de Rooy 2021-08-10 14:39:57 UTC
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.
Comment 10 Marcel de Rooy 2021-08-10 14:41:01 UTC
(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
Comment 11 Marcel de Rooy 2021-08-10 14:42:28 UTC
(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..
Comment 12 Marcel de Rooy 2021-08-10 14:44:08 UTC
 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
Comment 13 Tomás Cohen Arazi 2021-08-10 16:58:05 UTC
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>
Comment 14 Tomás Cohen Arazi 2021-08-10 17:04:49 UTC
Created attachment 123715 [details] [review]
Bug 27945: (QA follow-up) Fix exception POD

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 David Nind 2021-08-11 10:20:45 UTC
Created attachment 123774 [details] [review]
Bug 27945: Add tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 16 David Nind 2021-08-11 10:21:08 UTC
Created attachment 123775 [details] [review]
Bug 27945: Add article_request_limit in categories table

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 David Nind 2021-08-11 10:21:27 UTC
Created attachment 123776 [details] [review]
Bug 27945: [DO NOT PUSH] Update DBIx schema

Signed-off-by: David Nind <david@davidnind.com>
Comment 18 David Nind 2021-08-11 10:21:41 UTC
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>
Comment 19 David Nind 2021-08-11 10:21:57 UTC
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>
Comment 20 David Nind 2021-08-11 11:16:05 UTC
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'.
Comment 21 Agustín Moyano 2021-08-12 22:29:59 UTC
(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
Comment 22 Agustín Moyano 2021-08-12 22:38:48 UTC
Created attachment 123820 [details] [review]
Bug 27945: (follow-up) Fix column header title and edit form label
Comment 23 Marcel de Rooy 2021-08-18 12:47:15 UTC
 FAIL   Koha/Exceptions/ArticleRequest.pm
   FAIL   pod coverage
                POD is missing for 'description'

False negative
Comment 24 Marcel de Rooy 2021-08-18 12:55:59 UTC
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.
Comment 25 Marcel de Rooy 2021-08-18 13:31:02 UTC
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?
Comment 26 Marcel de Rooy 2021-08-18 13:47:07 UTC
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.
Comment 27 Marcel de Rooy 2021-08-18 13:56:08 UTC
    } 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+$ ?
Comment 28 Marcel de Rooy 2021-08-18 13:59:27 UTC
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.
Comment 29 Agustín Moyano 2021-08-24 18:00:35 UTC
Created attachment 124066 [details] [review]
Bug 27945: Add tests

Signed-off-by: David Nind <david@davidnind.com>




Current depends:
Comment 30 Agustín Moyano 2021-08-24 18:00:42 UTC
Created attachment 124067 [details] [review]
Bug 27945: Add article_request_limit in categories table

Signed-off-by: David Nind <david@davidnind.com>
Comment 31 Agustín Moyano 2021-08-24 18:00:46 UTC
Created attachment 124068 [details] [review]
Bug 27945: [DO NOT PUSH] Update DBIx schema

Signed-off-by: David Nind <david@davidnind.com>
Comment 32 Agustín Moyano 2021-08-24 18:01:01 UTC
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>
Comment 33 Agustín Moyano 2021-08-24 18:01:06 UTC
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>
Comment 34 Agustín Moyano 2021-08-24 18:01:13 UTC
Created attachment 124071 [details] [review]
Bug 27945: (follow-up) Fix column header title and edit form label
Comment 35 Agustín Moyano 2021-08-24 20:02:21 UTC
Created attachment 124072 [details] [review]
Bug 27945: Add tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 36 Agustín Moyano 2021-08-24 20:02:26 UTC
Created attachment 124073 [details] [review]
Bug 27945: Add article_request_limit in categories table

Signed-off-by: David Nind <david@davidnind.com>
Comment 37 Agustín Moyano 2021-08-24 20:02:31 UTC
Created attachment 124074 [details] [review]
Bug 27945: [DO NOT PUSH] Update DBIx schema

Signed-off-by: David Nind <david@davidnind.com>
Comment 38 Agustín Moyano 2021-08-24 20:02:36 UTC
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>
Comment 39 Agustín Moyano 2021-08-24 20:02:41 UTC
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>
Comment 40 Agustín Moyano 2021-08-24 20:02:46 UTC
Created attachment 124077 [details] [review]
Bug 27945: (follow-up) Fix column header title and edit form label
Comment 41 Agustín Moyano 2021-08-24 20:02:50 UTC
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
Comment 42 Agustín Moyano 2021-08-24 20:07:41 UTC
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
Comment 43 Agustín Moyano 2021-08-24 20:14:34 UTC
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
Comment 44 Marcel de Rooy 2021-08-31 12:36:16 UTC
QA: Looking here
Comment 45 Marcel de Rooy 2021-08-31 12:38:23 UTC
(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.
Comment 46 Marcel de Rooy 2021-08-31 12:49:59 UTC
There is a functional dependency between this report and 27944.
See bug 27944#68
Comment 47 Marcel de Rooy 2021-08-31 13:19:58 UTC
+        { 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.
Comment 48 Marcel de Rooy 2021-08-31 13:38:09 UTC
(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" ?
Comment 49 Marcel de Rooy 2021-08-31 13:47:45 UTC
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>
Comment 50 Marcel de Rooy 2021-08-31 13:47:49 UTC
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>
Comment 51 Marcel de Rooy 2021-08-31 13:48:34 UTC
We still need one thing imo: Fix the added tests for the adjusted check.
Comment 52 Agustín Moyano 2021-09-07 22:28:58 UTC
Created attachment 124633 [details] [review]
Bug 27945: (follow-up) Fix tests for the adjusted check
Comment 53 Marcel de Rooy 2021-09-09 07:44:40 UTC
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>
Comment 54 Marcel de Rooy 2021-09-09 07:44:44 UTC
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>
Comment 55 Marcel de Rooy 2021-09-09 07:44:48 UTC
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>
Comment 56 Marcel de Rooy 2021-09-09 07:44:52 UTC
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>
Comment 57 Marcel de Rooy 2021-09-09 07:44:56 UTC
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>
Comment 58 Marcel de Rooy 2021-09-09 07:45:01 UTC
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>
Comment 59 Marcel de Rooy 2021-09-09 07:45:06 UTC
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>
Comment 60 Marcel de Rooy 2021-09-09 07:45:10 UTC
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>
Comment 61 Marcel de Rooy 2021-09-09 07:45:14 UTC
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>
Comment 62 Marcel de Rooy 2021-09-09 07:45:19 UTC
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>
Comment 63 Marcel de Rooy 2021-09-09 07:46:19 UTC
(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.
Comment 64 Jonathan Druart 2021-09-21 18:00:34 UTC
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.
Comment 65 Tomás Cohen Arazi 2021-09-21 20:00:09 UTC
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.
Comment 66 Tomás Cohen Arazi 2021-09-22 15:11:04 UTC
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>
Comment 67 Tomás Cohen Arazi 2021-09-22 15:11:12 UTC
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>
Comment 68 Tomás Cohen Arazi 2021-09-22 15:11:18 UTC
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>
Comment 69 Tomás Cohen Arazi 2021-09-22 15:11:23 UTC
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>
Comment 70 Tomás Cohen Arazi 2021-09-22 15:11:30 UTC
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>
Comment 71 Tomás Cohen Arazi 2021-09-22 15:11:37 UTC
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>
Comment 72 Tomás Cohen Arazi 2021-09-22 15:11:43 UTC
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>
Comment 73 Tomás Cohen Arazi 2021-09-22 15:11:49 UTC
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>
Comment 74 Tomás Cohen Arazi 2021-09-22 15:11:56 UTC
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>
Comment 75 Tomás Cohen Arazi 2021-09-22 15:12:02 UTC
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>
Comment 76 Tomás Cohen Arazi 2021-09-22 15:12:07 UTC
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>
Comment 77 Tomás Cohen Arazi 2021-09-22 15:12:15 UTC
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>
Comment 78 Tomás Cohen Arazi 2021-09-22 15:13:31 UTC
Rebased on top of bug 27944, fixed a problem introduced by a rebase, fixed the tests.
Comment 79 Marcel de Rooy 2021-09-23 06:21:24 UTC
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>
Comment 80 Marcel de Rooy 2021-09-23 06:21:29 UTC
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>
Comment 81 Jonathan Druart 2021-09-30 14:33:22 UTC
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?
Comment 82 Tomás Cohen Arazi 2021-09-30 18:59:53 UTC
Created attachment 125569 [details] [review]
Bug 27945: (QA follow-up) Clarify behavior in POD

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 83 Tomás Cohen Arazi 2021-09-30 19:00:34 UTC
(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.
Comment 84 Tomás Cohen Arazi 2021-09-30 20:34:16 UTC
(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?
Comment 85 Tomás Cohen Arazi 2021-09-30 20:54:23 UTC
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>
Comment 86 Jonathan Druart 2021-10-01 08:07:08 UTC
(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)?
Comment 87 Jonathan Druart 2021-10-01 08:21:00 UTC
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.
Comment 88 Marcel de Rooy 2021-10-01 08:28:56 UTC
(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.
Comment 89 Marcel de Rooy 2021-10-01 08:29:30 UTC
(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.
Comment 90 Jonathan Druart 2021-10-01 09:03:40 UTC
(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.
Comment 91 Tomás Cohen Arazi 2021-10-01 13:29:40 UTC
(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.
Comment 92 Tomás Cohen Arazi 2021-10-01 13:31:23 UTC
(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?
Comment 93 Tomás Cohen Arazi 2021-10-01 13:32:30 UTC
(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)
Comment 94 Jonathan Druart 2021-10-01 13:47:41 UTC
(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 ;)
Comment 95 Marcel de Rooy 2021-10-01 15:02:29 UTC
(In reply to Tomás Cohen Arazi from comment #92)
> Marcel: what is the right behavior?

Thats a good question.
Comment 96 Tomás Cohen Arazi 2021-10-01 15:29:02 UTC
(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.
Comment 97 Tomás Cohen Arazi 2021-10-01 20:27:10 UTC
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>
Comment 98 Marcel de Rooy 2021-10-02 06:57:28 UTC
But rhetorical questions dont need an answer ;)
Comment 99 Jonathan Druart 2021-10-04 10:36:47 UTC
Created attachment 125690 [details] [review]
Bug 27945: (follow-up) Clarify 'same day' behavior

Don't use MySQLism
Comment 100 Jonathan Druart 2021-10-04 10:42:07 UTC
Why is it a category's attribute and not a circ rule?
Comment 101 Tomás Cohen Arazi 2021-10-04 15:15:22 UTC
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>
Comment 102 Tomás Cohen Arazi 2021-10-04 15:15:29 UTC
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.
Comment 103 Tomás Cohen Arazi 2021-10-04 15:15:37 UTC
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>
Comment 104 Tomás Cohen Arazi 2021-10-04 15:15:44 UTC
Created attachment 125705 [details] [review]
Bug 27945: Add max_daily_article_requests circulation rule

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 105 Tomás Cohen Arazi 2021-10-04 15:15:51 UTC
Created attachment 125706 [details] [review]
Bug 27945: Implement limits using circulation rules
Comment 106 Tomás Cohen Arazi 2021-10-04 15:15:58 UTC
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>
Comment 107 Jonathan Druart 2021-10-05 09:32:27 UTC
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)
Comment 108 Tomás Cohen Arazi 2021-10-05 15:02:57 UTC
Created attachment 125741 [details] [review]
Bug 27945: Terminology: max daily => open requests limit

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 109 Jonathan Druart 2021-10-06 08:37:12 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 110 Jonathan Druart 2021-10-06 09:36:58 UTC
Created attachment 125761 [details] [review]
Bug 27945: (follow-up) Add tests

Restore +x flag
Comment 111 Jonathan Druart 2021-10-06 12:58:11 UTC
(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.
Comment 112 Marcel de Rooy 2022-11-16 14:30:16 UTC
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..