Bug 27944

Summary: Add new stages to the article request process
Product: Koha Reporter: Nick Clemens <nick>
Component: CirculationAssignee: Agustín Moyano <agustinmoyano>
Status: CLOSED FIXED QA Contact: Nick Clemens <nick>
Severity: enhancement    
Priority: P5 - low CC: caroline.cyr-la-rose, david, gmcharlt, jonathan.druart, kyle.m.hall, kyle, m.de.rooy, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20472
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28836
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
When article requests come in they may require additional processing, for example: determining the type of request or other workflows. This enhancement adds a requested stage before the pending and processing stages for the article request process.
Version(s) released in:
21.11.00
Bug Depends on:    
Bug Blocks: 27945, 29182    
Attachments: Bug 27944: Add new letter in atomicupdate, and modify status column in article_requests table
Bug 27944: Add AR_REQUESTED message in sample_notices.yml and modify AR_PENDING
Bug 27944: Add "requested" stage in article request process
Bug 27944: Add "requested" stage in article request process
Bug 27944: Add missing Status module
Bug 27944: Add new letter in atomicupdate, and modify status column in article_requests table
Bug 27944: Add AR_REQUESTED message in sample_notices.yml and modify AR_PENDING
Bug 27944: Add "requested" stage in article request process
Bug 27944: Add missing Status module
Bug 27944: (follow-up) Improving consistency between notice names
Bug 27944: (follow-up) Move existing statuses, rename idempotent
Bug 27944: (follow-up) Move statuses to constants
Bug 27944: Add new letter in atomicupdate, and modify status column in article_requests table
Bug 27944: Add AR_REQUESTED message in sample_notices.yml and modify AR_PENDING
Bug 27944: Add "requested" stage in article request process
Bug 27944: Add missing Status module
Bug 27944: (follow-up) Improving consistency between notice names
Bug 27944: (follow-up) Move existing statuses, rename idempotent
Bug 27944: (follow-up) Move statuses to constants
Bug 27944: REQUESTED is the new default status
Bug 27944: (QA follow-up) Rename ->open for ->set_pending
Bug 27944: (QA follow-up) Make staff home page show new article requests
Bug 27944: Add new letter in atomicupdate, and modify status column in article_requests table
Bug 27944: Add AR_REQUESTED message in sample_notices.yml and modify AR_PENDING
Bug 27944: Add "requested" stage in article request process
Bug 27944: Add missing Status module
Bug 27944: (follow-up) Improving consistency between notice names
Bug 27944: (follow-up) Move existing statuses, rename idempotent
Bug 27944: (follow-up) Move statuses to constants
Bug 27944: REQUESTED is the new default status
Bug 27944: (QA follow-up) Rename ->open for ->set_pending
Bug 27944: (QA follow-up) Make staff home page show new article requests
Bug 27944: Add new letter in atomicupdate, and modify status column in article_requests table
Bug 27944: Add AR_REQUESTED message in sample_notices.yml and modify AR_PENDING
Bug 27944: Add "requested" stage in article request process
Bug 27944: Add missing Status module
Bug 27944: (follow-up) Improving consistency between notice names
Bug 27944: (follow-up) Move existing statuses, rename idempotent
Bug 27944: (follow-up) Move statuses to constants
Bug 27944: REQUESTED is the new default status
Bug 27944: (QA follow-up) Rename ->open for ->set_pending
Bug 27944: (QA follow-up) Make staff home page show new article requests
Bug 27944: (QA follow-up) Add span to fix pipe separator
Bug 27944: (QA follow-up) Make atomic update idempotent
Bug 27944: (follow-up) Add POD
Bug 27944: Add missing tests
Bug 27944: Add missing tests
Bug 27944: Fix t/db_dependent/AuthorisedValues.t
Bug 27944: Add fr-FR AR_REQUESTED notice

Description Nick Clemens 2021-03-12 14:39:19 UTC
When requests come in they may require additional processing, i.e. for determining type of request or other workflows.

Adding a 'new' tab will allow for preprocessing before moving the request along to 'pending' or 'processing'
Comment 1 Agustín Moyano 2021-03-30 16:20:15 UTC
Created attachment 119020 [details] [review]
Bug 27944: Add new letter in atomicupdate, and modify status column in article_requests table
Comment 2 Agustín Moyano 2021-03-30 16:20:19 UTC
Created attachment 119021 [details] [review]
Bug 27944: Add AR_REQUESTED message in sample_notices.yml and modify AR_PENDING
Comment 3 Agustín Moyano 2021-03-30 16:20:22 UTC
Created attachment 119022 [details] [review]
Bug 27944: Add "requested" stage in article request process

This patch adds the stage "requested" in article request process, which
is previous to pending stage.

To test:
1. apply this patch
2. updatedatabase
3. enable ArticleRequests syspref
4. from staff inteface and from opac search for a record and place an
   article request
5. koha-mysql kohadev
6. query: select subject, content, letter_code from message_queue;
CHECK => There is a message for each article request with code
AR_REQUESTED
      => In opac-user.pl, in "Article requests" tab you should see a row
in the table with "Requested" status
5. in staff go to Circulation -> Article Requests
SUCCESS => You should see 3 tabs, one for Requested stage (with two
requests), one for Pending stage and one for Processing stage.
6. play with actions buttons
CHECK => you should see a new action called "Set request as pending"
SUCCESS => All action buttons behave as expected, and tab counts updates
correctly.
Comment 4 Marcel de Rooy 2021-04-02 06:41:34 UTC
Hi Augustin,

I have some questions here:

The tab is called Requested, but since all entries are requests, wouldnt it be more logical to call it New ?

If I pick a request to process, it would be handy to move it to Processing immediately from the New tab? Instead of an additional move thru Pending.

Can you explain the mail for a move to Pending? Since the patron gets an email when he submitted the request and will have an email when we hit Processing, does this mail saying 'We are doing nothing yet' make him happy :) ?

Please run qa tools. I see this failure:
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt
   FAIL   valid_template
                Attempt to reload Koha/Template/Plugin/Biblio.pm aborted.
Compilation failed in require at /usr/lib/x86_64-linux-gnu/perl5/5.24/Template/Plugins.pm line 206.
=> Is this related to biblio-view-menu.inc ?
Comment 5 Agustín Moyano 2021-04-26 13:19:07 UTC
Hi Marcel, I'll reply between lines

(In reply to Marcel de Rooy from comment #4)
> Hi Augustin,
> 
> I have some questions here:
> 
> The tab is called Requested, but since all entries are requests, wouldnt it
> be more logical to call it New ?

I'll change it right away. For me "new" sounded like it was recently created, but I thought you may have old unprocessed requests, so in that context it seemed reasonable to call it "requested".

> 
> If I pick a request to process, it would be handy to move it to Processing
> immediately from the New tab? Instead of an additional move thru Pending.

I wasn't told that was allowed, but it can be added.

> 
> Can you explain the mail for a move to Pending? Since the patron gets an
> email when he submitted the request and will have an email when we hit
> Processing, does this mail saying 'We are doing nothing yet' make him happy
> :) ?
> 

I believe it's better to give as much feedback to patrons as we can.. Even if that message means "Your request was moved from the drawer of the left, to the drawer on the right". It might save the librarian a call like "what happend with my request"

> Please run qa tools. I see this failure:
>  FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt
>    FAIL   valid_template
>                 Attempt to reload Koha/Template/Plugin/Biblio.pm aborted.
> Compilation failed in require at
> /usr/lib/x86_64-linux-gnu/perl5/5.24/Template/Plugins.pm line 206.
> => Is this related to biblio-view-menu.inc ?

I believe it's a bug in qa tools, but I really got no idea why this error appears. I'll check it out once more
Comment 6 Agustín Moyano 2021-05-04 13:02:15 UTC
Created attachment 120454 [details] [review]
Bug 27944: Add "requested" stage in article request process

This patch adds the stage "requested" in article request process, which
is previous to pending stage.

To test:
1. apply this patch
2. updatedatabase
3. enable ArticleRequests syspref
4. from staff inteface and from opac search for a record and place an
   article request
5. koha-mysql kohadev
6. query: select subject, content, letter_code from message_queue;
CHECK => There is a message for each article request with code
AR_REQUESTED
      => In opac-user.pl, in "Article requests" tab you should see a row
in the table with "Requested" status
5. in staff go to Circulation -> Article Requests
SUCCESS => You should see 3 tabs, one for Requested stage (with two
requests), one for Pending stage and one for Processing stage.
6. play with actions buttons
CHECK => you should see a new action called "Set request as pending"
SUCCESS => All action buttons behave as expected, and tab counts updates
correctly.
Comment 7 Marcel de Rooy 2021-06-21 13:53:40 UTC
Agustin:
I still have some doubts about the atomicupdate.

You do an UPDATE for notice AR_PENDING and an insert for the new AR_REQUESTED.
Generally we should be very careful with UPDATES on existing notices. This one is too generic and will unnecessarily? remove customized texts.
The insert could be an INSERT IGNORE btw.
I would suggest to use a new name for the former AR_PENDING notice and only rename it in the atomicupdate. Like AR_RECEIVED. Do not touch the text.
And after that add a 'new' notice AR_PENDING (..) with the new text.

In the second patch you add AR_REQUESTED but note that you insert the text for the PENDING state ! So you mix them up here.
Obviously, the changes for patch1 should reflect in patch2.

Changing status
Comment 8 Jonathan Druart 2021-06-25 14:09:55 UTC
(In reply to Marcel de Rooy from comment #4)
> Please run qa tools. I see this failure:
>  FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt
>    FAIL   valid_template
>                 Attempt to reload Koha/Template/Plugin/Biblio.pm aborted.
> Compilation failed in require at
> /usr/lib/x86_64-linux-gnu/perl5/5.24/Template/Plugins.pm line 206.
> => Is this related to biblio-view-menu.inc ?

The following change should fix that.

diff --git a/Koha/ArticleRequest.pm b/Koha/ArticleRequest.pm
index 3ea325d83dc..692099ca6bd 100644
--- a/Koha/ArticleRequest.pm
+++ b/Koha/ArticleRequest.pm
@@ -27,6 +27,7 @@ use Koha::Biblios;
 use Koha::Items;
 use Koha::Libraries;
 use Koha::DateUtils qw(dt_from_string);
+use Koha::ArticleRequest::Status;

 use base qw(Koha::Object);
Comment 9 Marcel de Rooy 2021-06-29 11:34:35 UTC
(In reply to Jonathan Druart from comment #8)
> (In reply to Marcel de Rooy from comment #4)
> > Please run qa tools. I see this failure:
> >  FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt
> >    FAIL   valid_template
> >                 Attempt to reload Koha/Template/Plugin/Biblio.pm aborted.
> > Compilation failed in require at
> > /usr/lib/x86_64-linux-gnu/perl5/5.24/Template/Plugins.pm line 206.
> > => Is this related to biblio-view-menu.inc ?
> 
> The following change should fix that.
> 
> diff --git a/Koha/ArticleRequest.pm b/Koha/ArticleRequest.pm
> index 3ea325d83dc..692099ca6bd 100644
> --- a/Koha/ArticleRequest.pm
> +++ b/Koha/ArticleRequest.pm
> @@ -27,6 +27,7 @@ use Koha::Biblios;
>  use Koha::Items;
>  use Koha::Libraries;
>  use Koha::DateUtils qw(dt_from_string);
> +use Koha::ArticleRequest::Status;
> 
>  use base qw(Koha::Object);

Sorry, but it does not with me:

diff --git a/Koha/ArticleRequest.pm b/Koha/ArticleRequest.pm
index 3ea325d83d..692099ca6b 100644
--- a/Koha/ArticleRequest.pm
+++ b/Koha/ArticleRequest.pm
@@ -27,6 +27,7 @@ use Koha::Biblios;
 use Koha::Items;
 use Koha::Libraries;
 use Koha::DateUtils qw(dt_from_string);
+use Koha::ArticleRequest::Status;

 use base qw(Koha::Object);

 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt
   FAIL   valid_template
                Attempt to reload Koha/Template/Plugin/Biblio.pm aborted.
Compilation failed in require at /usr/lib/x86_64-linux-gnu/perl5/5.28/Template/Plugins.pm line 206.

There is still something else triggering a compile warn or error?
Comment 10 Agustín Moyano 2021-07-02 19:18:02 UTC
Hi Marcel

(In reply to Marcel de Rooy from comment #7)
> Agustin:
> I still have some doubts about the atomicupdate.
> 
> You do an UPDATE for notice AR_PENDING and an insert for the new
> AR_REQUESTED.
> Generally we should be very careful with UPDATES on existing notices. This
> one is too generic and will unnecessarily? remove customized texts.
> The insert could be an INSERT IGNORE btw.

Thanks, missed that

> I would suggest to use a new name for the former AR_PENDING notice and only
> rename it in the atomicupdate. Like AR_RECEIVED. Do not touch the text.
> And after that add a 'new' notice AR_PENDING (..) with the new text.
> 
> In the second patch you add AR_REQUESTED but note that you insert the text
> for the PENDING state ! So you mix them up here.
> Obviously, the changes for patch1 should reflect in patch2.

That was intentional, AR_REQUESTED should have the same text as AR_PENDING now has.. it tells the patron that the request was received. 

I did not realize those messages were customizable, so, what I'll do is in atomicupdate I'll first update message code from AR_PENDING to AR_REQUESTED (keeping that way customized messages) and then I'll insert (ignore) the new AR_PENDING message.

Sounds good?

Cheers.
Comment 11 Marcel de Rooy 2021-07-05 06:02:02 UTC
(In reply to Agustín Moyano from comment #10)
> I did not realize those messages were customizable, so, what I'll do is in
> atomicupdate I'll first update message code from AR_PENDING to AR_REQUESTED
> (keeping that way customized messages) and then I'll insert (ignore) the new
> AR_PENDING message.

Sounds good.
Comment 12 Jonathan Druart 2021-07-06 10:07:24 UTC
(In reply to Marcel de Rooy from comment #9)
> Sorry, but it does not with me:

Tried again.

I squashed the patches, and apply the changes from those 4 files only:
 Koha/ArticleRequest/Status.pm                                   | 21 ++++++++++++++++-----
 Koha/ArticleRequests.pm                                         | 11 +++++++++++
 Koha/Template/Plugin/Biblio.pm                                  |  1 +
 koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt |  2 ++

And the failure appears randomly.

It's a weird one, I don't understand what's going on but it's hiding something for sure.
Comment 13 Nick Clemens 2021-07-06 10:47:23 UTC
(In reply to Jonathan Druart from comment #12)
> (In reply to Marcel de Rooy from comment #9)
> > Sorry, but it does not with me:
> 
> Tried again.
> 
> I squashed the patches, and apply the changes from those 4 files only:
>  Koha/ArticleRequest/Status.pm                                   | 21
> ++++++++++++++++-----
>  Koha/ArticleRequests.pm                                         | 11
> +++++++++++
>  Koha/Template/Plugin/Biblio.pm                                  |  1 +
>  koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt |  2 ++
> 
> And the failure appears randomly.
> 
> It's a weird one, I don't understand what's going on but it's hiding
> something for sure.

I think it is the POD in Koha/ArticleRequest/Status.pm

If I remove the POD changes, commit, restart_all - the error disappears

Remove my commit/readd the pod, qa passes
restart_all - it fails

Some somehow POD and something being cached?
Comment 14 Marcel de Rooy 2021-07-06 14:44:13 UTC
(In reply to Nick Clemens from comment #13)
> I think it is the POD in Koha/ArticleRequest/Status.pm
> 
> If I remove the POD changes, commit, restart_all - the error disappears
> 
> Remove my commit/readd the pod, qa passes
> restart_all - it fails
> 
> Some somehow POD and something being cached?

Altough the POD could be improved, podchecker does not complain about it. The coverage is not complete, but no problem? The relation with something in the cache is weird?
Comment 15 Marcel de Rooy 2021-07-08 12:20:18 UTC
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt
index 5431aa782a..927d37993c 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt
@@ -309,7 +309,7 @@

         <div class="col-sm-2 col-sm-pull-10">
             <aside>
-                [% INCLUDE 'biblio-view-menu.inc' %]
+
             </aside>
         </div> <!-- /.col-sm-2.col-sm-pull-10 -->
      </div> <!-- /.row -->

=> This UNUSABLE patch would make the error go away. It definitely has to do something imo with the Koha::Template::Plugin::Biblio which is used in the include.
Comment 16 Marcel de Rooy 2021-07-08 12:21:50 UTC
diff --git a/Koha/ArticleRequest/Status.pm b/Koha/ArticleRequest/Status.pm
index 42de00d551..830024dccd 100644
--- a/Koha/ArticleRequest/Status.pm
+++ b/Koha/ArticleRequest/Status.pm
@@ -19,6 +19,8 @@ package Koha::ArticleRequest::Status;

 use Modern::Perl;

+sub Requested { 'REQUESTED' }
+
 sub Pending {
     return 'PENDING';
 }
diff --git a/Koha/Template/Plugin/Biblio.pm b/Koha/Template/Plugin/Biblio.pm
index e9d7a73009..5ec3542294 100644
--- a/Koha/Template/Plugin/Biblio.pm
+++ b/Koha/Template/Plugin/Biblio.pm
@@ -44,6 +44,7 @@ sub ArticleRequestsActiveCount {
             biblionumber => $biblionumber,
             status       => [
                 -or => [
+                    status => Koha::ArticleRequest::Status::Requested,
                     status => Koha::ArticleRequest::Status::Pending,
                     status => Koha::ArticleRequest::Status::Processing
                 ]
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt
index e59703482e..c708a2ba86 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt
@@ -12,6 +12,7 @@
 </head>

 <body id="circ_request-article" class="circ">
+<div testmr="1"/>
     [% INCLUDE 'header.inc' %]
     [% INCLUDE 'circ-search.inc' %]

=> And this unusable patch is an easy way to show the error.. If I would remove the Plugin/Biblio from the patch, we clear the warning again.
Comment 17 Marcel de Rooy 2021-07-08 12:28:24 UTC
Jonathan: Might this be an error in qa tools too somehow? The fact that it randomly appears seems to be related with the fact that Template Toolkit around line 206 tries Template/Plugin and Koha/Template/Plugin in random order to find and load (require) the plugin.

If it loaded already, perl sets an entry in %INC. Would it make a difference when qa tools reset the situation before it runs the next file? Clear the INC hash (not to be confused with @INC)?
Comment 18 Marcel de Rooy 2021-07-08 12:41:44 UTC
Last question before I really let this go: Should we actually allow includes to do [% USE Biblio %] etc. or should we 'use' them already earlier in the process, in other words at the top of the template files that call them?
Comment 19 Marcel de Rooy 2021-07-08 12:48:15 UTC
Created attachment 122673 [details] [review]
Bug 27944: Add missing Status module

Test plan:
perl -c the module or run qa tools

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Agustín Moyano 2021-07-14 13:27:09 UTC
Created attachment 122836 [details] [review]
Bug 27944: Add new letter in atomicupdate, and modify status column in article_requests table
Comment 21 Agustín Moyano 2021-07-14 13:27:13 UTC
Created attachment 122837 [details] [review]
Bug 27944: Add AR_REQUESTED message in sample_notices.yml and modify AR_PENDING
Comment 22 Agustín Moyano 2021-07-14 13:27:18 UTC
Created attachment 122838 [details] [review]
Bug 27944: Add "requested" stage in article request process

This patch adds the stage "requested" in article request process, which
is previous to pending stage.

To test:
1. apply this patch
2. updatedatabase
3. enable ArticleRequests syspref
4. from staff inteface and from opac search for a record and place an
   article request
5. koha-mysql kohadev
6. query: select subject, content, letter_code from message_queue;
CHECK => There is a message for each article request with code
AR_REQUESTED
      => In opac-user.pl, in "Article requests" tab you should see a row
in the table with "Requested" status
5. in staff go to Circulation -> Article Requests
SUCCESS => You should see 3 tabs, one for Requested stage (with two
requests), one for Pending stage and one for Processing stage.
6. play with actions buttons
CHECK => you should see a new action called "Set request as pending"
SUCCESS => All action buttons behave as expected, and tab counts updates
correctly.
Comment 23 Agustín Moyano 2021-07-14 13:27:23 UTC
Created attachment 122839 [details] [review]
Bug 27944: Add missing Status module

Test plan:
perl -c the module or run qa tools

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2021-07-14 14:15:52 UTC
We still have this stubborn warn from qa tools and unfortunately it is still unclear what the exact cause is:

 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt
   FAIL   valid_template
                Attempt to reload Koha/Template/Plugin/Biblio.pm aborted.
Compilation failed in require at /usr/lib/x86_64-linux-gnu/perl5/5.28/Template/Plugins.pm line 206.
Comment 25 Marcel de Rooy 2021-07-14 15:02:42 UTC
Created attachment 122842 [details] [review]
Bug 27944: (follow-up) Improving consistency between notice names
Comment 26 Marcel de Rooy 2021-07-14 15:02:46 UTC
Created attachment 122843 [details] [review]
Bug 27944: (follow-up) Move existing statuses, rename idempotent
Comment 27 Marcel de Rooy 2021-07-14 15:03:33 UTC
Changing status temporarily, finishing this tomorrow
Comment 28 Marcel de Rooy 2021-07-14 15:04:11 UTC
-                `name` = 'Article requested'
+                `name` = REPLACE(name, '- open', '- new')
Might be safer when touching translated notices.
Added a follow-up for making the names a bit more consistent.

We also need to move the existing request from Pending to Requested.
And make the move idempotent.
Comment 29 Marcel de Rooy 2021-07-15 06:08:56 UTC
The 'open' function is in both the perl code and the js code, now refers to setting to pending status. I think that this will lead to confusion and potential future bugs. Why not call it pending or set_pending or similar?

-        return $self->open;
+        return $self->request;

+                <a class="ar-open-request" href="#" onclick="HandleMulti( Open, [% id_arg | html %], $(this) ); return false;">
+                    <i class="fa fa-bars"></i>
+                    Set request as pending

+        function Open( id, a ) {

sub open {
    my ($self) = @_;
    $self->status(Koha::ArticleRequest::Status::Pending);
Comment 30 Marcel de Rooy 2021-07-15 11:49:13 UTC
Created attachment 122857 [details] [review]
Bug 27944: (follow-up) Move statuses to constants
Comment 31 Marcel de Rooy 2021-07-15 11:52:18 UTC
Note comment24 (maybe Tomas could have a look too?)
Note comment29

And a few other points needing attention still:

TODO Behavior of "Set request as pending"
=> The New(number) is not updated, nor the Pending(number)
=> Same for Process btw now.

TODO The staff home page does no longer show New requests. It shows pending requests.
Comment 32 Agustín Moyano 2021-07-20 16:59:53 UTC
Created attachment 122984 [details] [review]
Bug 27944: Add new letter in atomicupdate, and modify status column in article_requests table
Comment 33 Agustín Moyano 2021-07-20 16:59:57 UTC
Created attachment 122985 [details] [review]
Bug 27944: Add AR_REQUESTED message in sample_notices.yml and modify AR_PENDING
Comment 34 Agustín Moyano 2021-07-20 17:00:02 UTC
Created attachment 122986 [details] [review]
Bug 27944: Add "requested" stage in article request process

This patch adds the stage "requested" in article request process, which
is previous to pending stage.

To test:
1. apply this patch
2. updatedatabase
3. enable ArticleRequests syspref
4. from staff inteface and from opac search for a record and place an
   article request
5. koha-mysql kohadev
6. query: select subject, content, letter_code from message_queue;
CHECK => There is a message for each article request with code
AR_REQUESTED
      => In opac-user.pl, in "Article requests" tab you should see a row
in the table with "Requested" status
5. in staff go to Circulation -> Article Requests
SUCCESS => You should see 3 tabs, one for Requested stage (with two
requests), one for Pending stage and one for Processing stage.
6. play with actions buttons
CHECK => you should see a new action called "Set request as pending"
SUCCESS => All action buttons behave as expected, and tab counts updates
correctly.
Comment 35 Agustín Moyano 2021-07-20 17:00:06 UTC
Created attachment 122987 [details] [review]
Bug 27944: Add missing Status module

Test plan:
perl -c the module or run qa tools

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Agustín Moyano 2021-07-20 17:00:11 UTC
Created attachment 122988 [details] [review]
Bug 27944: (follow-up) Improving consistency between notice names
Comment 37 Agustín Moyano 2021-07-20 17:00:15 UTC
Created attachment 122989 [details] [review]
Bug 27944: (follow-up) Move existing statuses, rename idempotent
Comment 38 Agustín Moyano 2021-07-20 17:00:19 UTC
Created attachment 122990 [details] [review]
Bug 27944: (follow-up) Move statuses to constants
Comment 39 Agustín Moyano 2021-07-21 19:37:46 UTC
(In reply to Marcel de Rooy from comment #31)
> Note comment24 (maybe Tomas could have a look too?)
> Note comment29
> 
> And a few other points needing attention still:
> 
> TODO Behavior of "Set request as pending"
> => The New(number) is not updated, nor the Pending(number)
> => Same for Process btw now.
> 
> TODO The staff home page does no longer show New requests. It shows pending
> requests.

Hi Marcel, sorry about that.. Set request as pending worked well before rebase. Now its fixed.

Cheers
Comment 40 Marcel de Rooy 2021-07-26 09:13:18 UTC
   FAIL   valid_template
                Attempt to reload Koha/Template/Plugin/Biblio.pm aborted.
Compilation failed in require at /usr/lib/x86_64-linux-gnu/perl5/5.28/Template/Plugins.pm line 206.
Comment 41 Marcel de Rooy 2021-07-26 09:17:47 UTC
(In reply to Agustín Moyano from comment #39)
> > TODO The staff home page does no longer show New requests. It shows pending
> > requests.
> 
> Hi Marcel, sorry about that.. Set request as pending worked well before
> rebase. Now its fixed.

This still seems to look at pending instead of new.
Comment 42 Jonathan Druart 2021-07-26 15:30:58 UTC
Tomas asked me to look at the following QA errors:

Prototype mismatch: sub Koha::ArticleRequest::Status::Completed: none vs () at /usr/share/perl/5.28/constant.pm line 171.                                                                                         
Prototype mismatch: sub Koha::ArticleRequest::Status::Processing: none vs () at /usr/share/perl/5.28/constant.pm line 171.                                                                                        
Prototype mismatch: sub Koha::ArticleRequest::Status::Pending: none vs () at /usr/share/perl/5.28/constant.pm line 171.                                                                                           
Prototype mismatch: sub Koha::ArticleRequest::Status::Canceled: none vs () at /usr/share/perl/5.28/constant.pm line 171. 

I'd say we can ignore them.

What's happening (from what I understand):
The module are reloaded when the git branch is checked out:

1. "use Koha::ArticleRequest::Status" is done on master
2. processing tests before patches
3. Checkout the branch with the patches
4. reload the modules
5. processing tests after patches

The error appears during 3.
https://gitlab.com/koha-community/qa-test-tools/-/blob/bca1e51151665ba4965b219a5f48cdac3affa6e5/koha-qa.pl#L102

            eval "require $module";
            $module->import;

Requested (and friends) went from sub to constant and so Perl is warning us.
Does that make sense?

(related with bug 17600 comment 109 and following)
Comment 43 Marcel de Rooy 2021-07-27 11:16:22 UTC
(In reply to Jonathan Druart from comment #42)
> Tomas asked me to look at the following QA errors:
> 
> Prototype mismatch: sub Koha::ArticleRequest::Status::Completed: none vs ()
> at /usr/share/perl/5.28/constant.pm line 171.                               
> 
> Prototype mismatch: sub Koha::ArticleRequest::Status::Processing: none vs ()
> at /usr/share/perl/5.28/constant.pm line 171.                               
> 
> Prototype mismatch: sub Koha::ArticleRequest::Status::Pending: none vs () at
> /usr/share/perl/5.28/constant.pm line 171.                                  
> 
> Prototype mismatch: sub Koha::ArticleRequest::Status::Canceled: none vs ()
> at /usr/share/perl/5.28/constant.pm line 171. 
> 
> I'd say we can ignore them.

Yeah thats what I concluded too.
The more intriguing problem is the one about Koha::Template::Plugin::Biblio.
Last mentioned in comment40, but discussed before that too.
You did already have a look, and I actually asked Tomas to have a look. but he did not yet..
Comment 44 Tomás Cohen Arazi 2021-07-27 11:22:33 UTC
(In reply to Marcel de Rooy from comment #43)
> (In reply to Jonathan Druart from comment #42)
> > Tomas asked me to look at the following QA errors:
> > 
> > Prototype mismatch: sub Koha::ArticleRequest::Status::Completed: none vs ()
> > at /usr/share/perl/5.28/constant.pm line 171.                               
> > 
> > Prototype mismatch: sub Koha::ArticleRequest::Status::Processing: none vs ()
> > at /usr/share/perl/5.28/constant.pm line 171.                               
> > 
> > Prototype mismatch: sub Koha::ArticleRequest::Status::Pending: none vs () at
> > /usr/share/perl/5.28/constant.pm line 171.                                  
> > 
> > Prototype mismatch: sub Koha::ArticleRequest::Status::Canceled: none vs ()
> > at /usr/share/perl/5.28/constant.pm line 171. 
> > 
> > I'd say we can ignore them.
> 
> Yeah thats what I concluded too.
> The more intriguing problem is the one about Koha::Template::Plugin::Biblio.
> Last mentioned in comment40, but discussed before that too.
> You did already have a look, and I actually asked Tomas to have a look. but
> he did not yet..

I've been looking at this yesterday morning, with no luck. Will give it another run today!
Comment 45 Tomás Cohen Arazi 2021-08-07 12:28:08 UTC
(In reply to Marcel de Rooy from comment #18)
> Last question before I really let this go: Should we actually allow includes
> to do [% USE Biblio %] etc. or should we 'use' them already earlier in the
> process, in other words at the top of the template files that call them?

I tried removing the USE-es in biblio-view-menu.inc and this 'random' failure showed on all files that used it (which I changed by adding the USE).

I noticed that the qa tools just try to load the template and I added a warn on /usr/lib/x86_64-linux-gnu/perl5/5.28/Template/Plugins.pm (which takes care of loading those USE):

206             $ok = eval { require "$file.pm" };
207             use Data::Printer colored => 1;
208             p($@);

The errors show up randomly (it was all green the first run :-/) but this is what $@ says:

Can't locate Koha/Template/Plugin/String.pm in @INC (you may need to install the Koha::Template::Plugin::String module) (@INC contains: /kohadevbox/koha /kohadevbox/koha/lib /kohadevbox/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.28.1 /usr/local/share/perl/5.28.1 /usr/lib/x86_64-linux-gnu/perl5/5.28 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.28 /usr/share/perl/5.28 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base /kohadevbox/koha_plugin) at /usr/lib/x86_64-linux-gnu/perl5/5.28/Template/Plugins.pm line 206.

Can't locate Koha/Template/Plugin/JSON/Escape.pm in @INC (you may need to install the Koha::Template::Plugin::JSON::Escape module) (@INC contains: /kohadevbox/koha /kohadevbox/koha/lib /kohadevbox/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.28.1 /usr/local/share/perl/5.28.1 /usr/lib/x86_64-linux-gnu/perl5/5.28 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.28 /usr/share/perl/5.28 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base /kohadevbox/koha_plugin) at /usr/lib/x86_64-linux-gnu/perl5/5.28/Template/Plugins.pm line 206.

=> This are upstream TT plugins that are looked up in the wrong place

Can't locate Template/Plugin/Biblio.pm in @INC (you may need to install the Template::Plugin::Biblio module) (@INC contains: /kohadevbox/koha /kohadevbox/koha/lib /kohadevbox/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.28.1 /usr/local/share/perl/5.28.1 /usr/lib/x86_64-linux-gnu/perl5/5.28 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.28 /usr/share/perl/5.28 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base /kohadevbox/koha_plugin) at /usr/lib/x86_64-linux-gnu/perl5/5.28/Template/Plugins.pm line 206.

=> This is a Koha plugin that is looked up in the wrong place

Maybe a path problem baked into the QA tools? I don't think it is fair that this issue blocks this bug.
Comment 46 Marcel de Rooy 2021-08-09 09:26:31 UTC
(In reply to Tomás Cohen Arazi from comment #45)

Thx for giving it another try!

My first qa tools run here again:
Attempt to reload Koha/Template/Plugin/Biblio.pm aborted.
Compilation failed in require at /usr/lib/x86_64-linux-gnu/perl5/5.28/Template/Plugins.pm line 206.
So unfortunately bug 17600 did not resolve this problem ;)

Note the pbase array used in /usr/lib/x86_64-linux-gnu/perl5/5.28/Template/Plugins.pm looks like:
$VAR1 = [
          'Koha::Template::Plugin',
          'Template::Plugin'
        ];
This actually means that every upstream plugin is looked up in the wrong place first, but that should not be a problem: TT ignores the Can't locate file messages.
When you saw the message "Can't locate Template/Plugin/Biblio.pm in @INC", TT already failed on Koha::Template::Plugin. Which is the actual problem.


> Maybe a path problem baked into the QA tools? I don't think it is fair that
> this issue blocks this bug.

I agree. Half of the QA team already had a look and cant find the real source of the issue.
But I am not sure if the issue is in QA tools, Template Toolkit or in a Koha dependency.
The randomness of the failure does not make it easier ;)
There seems to be a connection with the fact that the module is used in an INCLUDE of the template under consideration.

But note also that Agustin still needs to respond to comment41. This is not fixed yet.
Comment 47 Tomás Cohen Arazi 2021-08-10 13:10:46 UTC
(In reply to Marcel de Rooy from comment #46)
>
> But note also that Agustin still needs to respond to comment41. This is not
> fixed yet.

It works for me.
Comment 48 Marcel de Rooy 2021-08-10 14:33:20 UTC
(In reply to Tomás Cohen Arazi from comment #47)
> (In reply to Marcel de Rooy from comment #46)
> >
> > But note also that Agustin still needs to respond to comment41. This is not
> > fixed yet.
> 
> It works for me.

Hmm. I guess you need to explain me :) Or read earlier comments more closely?

The staff interface tells me now Article requests: 1.
If I click on the link, I come in the New tab which lists no requests. The Pending tab contains one request, Processing 2.

And now the code:
The template shows: <span class="pending-number-link">[% pending_article_requests | html %]</span>
Hote that pending is not the correct term anymore. These should be New.
mainpage.pl contains:
my $pending_article_requests = Koha::ArticleRequests->search_limited(
    {
        status => Koha::ArticleRequest::Status::Pending,
        $branch ? ( 'me.branchcode' => $branch ) : (),
    }
)->count;
Again, dont be misled by the bad variable names here. We should refer to New.
Could you please explain how this produces the number of New requests on the main page :) ?
Comment 49 Marcel de Rooy 2021-08-10 14:36:34 UTC
(In reply to Marcel de Rooy from comment #46)
> But note also that Agustin still needs to respond to comment41. This is not
> fixed yet.

Or comment31 ?
Comment 50 Tomás Cohen Arazi 2021-08-10 14:46:01 UTC
(In reply to Marcel de Rooy from comment #48)
> (In reply to Tomás Cohen Arazi from comment #47)
> > (In reply to Marcel de Rooy from comment #46)
> > >
> > > But note also that Agustin still needs to respond to comment41. This is not
> > > fixed yet.
> > 
> > It works for me.
> 
> Hmm. I guess you need to explain me :) Or read earlier comments more closely?

I was looking at the wrong thing: if it was possible to set a request as pending. Which was Agustin's answer. Taking a closer look now.
Comment 51 Tomás Cohen Arazi 2021-08-10 16:44:10 UTC
Created attachment 123710 [details] [review]
Bug 27944: REQUESTED is the new default status

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 52 Tomás Cohen Arazi 2021-08-10 16:44:15 UTC
Created attachment 123711 [details] [review]
Bug 27944: (QA follow-up) Rename ->open for ->set_pending

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 53 Tomás Cohen Arazi 2021-08-10 16:44:21 UTC
Created attachment 123712 [details] [review]
Bug 27944: (QA follow-up) Make staff home page show new article requests

As exposed on comment 29, the current code is showing the 'pending'
requests when it should display 'requested' ones (i.e. the new ones).

This patch changes that accordingly.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 54 Tomás Cohen Arazi 2021-08-10 16:49:29 UTC
Marcel, I think I addressed the issues you highlighted.

- Tests pass (they weren't passing)
- Renamed 'open' to set_pending/pending. We could even open a new bug report to refactor the whole thing. I think it doesn't comply with our current practices in the codebase. Will address on a separate bug.
- You were right about the bad search, fixed it.
Comment 55 Katrin Fischer 2021-08-10 21:00:26 UTC
Been following the comments here all day and keep wondering - what are the new status named?

I have seen new and pending - but they kind of are the same to me?
Comment 56 Marcel de Rooy 2021-08-18 11:24:00 UTC
(In reply to Katrin Fischer from comment #55)
> Been following the comments here all day and keep wondering - what are the
> new status named?
> 
> I have seen new and pending - but they kind of are the same to me?

No, new and pending are not the same. You could say that a pending request is a new request that has been put on hold since there is a reason why you cannot fill the request right away.
Comment 57 Katrin Fischer 2021-08-18 11:40:24 UTC
Hm, I am not sure how clear that is from the naming, although I understand the intention better now. A little worried about the terminology clashing with what we have for holds in translation.
Comment 58 Marcel de Rooy 2021-08-18 12:28:31 UTC
Created attachment 123943 [details] [review]
Bug 27944: Add new letter in atomicupdate, and modify status column in article_requests table

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 59 Marcel de Rooy 2021-08-18 12:28:35 UTC
Created attachment 123944 [details] [review]
Bug 27944: Add AR_REQUESTED message in sample_notices.yml and modify AR_PENDING

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 60 Marcel de Rooy 2021-08-18 12:28:39 UTC
Created attachment 123945 [details] [review]
Bug 27944: Add "requested" stage in article request process

This patch adds the stage "requested" in article request process, which
is previous to pending stage.

To test:
1. apply this patch
2. updatedatabase
3. enable ArticleRequests syspref
4. from staff inteface and from opac search for a record and place an
   article request
5. koha-mysql kohadev
6. query: select subject, content, letter_code from message_queue;
CHECK => There is a message for each article request with code
AR_REQUESTED
      => In opac-user.pl, in "Article requests" tab you should see a row
in the table with "Requested" status
5. in staff go to Circulation -> Article Requests
SUCCESS => You should see 3 tabs, one for Requested stage (with two
requests), one for Pending stage and one for Processing stage.
6. play with actions buttons
CHECK => you should see a new action called "Set request as pending"
SUCCESS => All action buttons behave as expected, and tab counts updates
correctly.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 61 Marcel de Rooy 2021-08-18 12:28:43 UTC
Created attachment 123946 [details] [review]
Bug 27944: Add missing Status module

Test plan:
perl -c the module or run qa tools

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 62 Marcel de Rooy 2021-08-18 12:28:47 UTC
Created attachment 123947 [details] [review]
Bug 27944: (follow-up) Improving consistency between notice names

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 63 Marcel de Rooy 2021-08-18 12:28:51 UTC
Created attachment 123948 [details] [review]
Bug 27944: (follow-up) Move existing statuses, rename idempotent

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 64 Marcel de Rooy 2021-08-18 12:28:55 UTC
Created attachment 123949 [details] [review]
Bug 27944: (follow-up) Move statuses to constants

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 65 Marcel de Rooy 2021-08-18 12:28:59 UTC
Created attachment 123950 [details] [review]
Bug 27944: REQUESTED is the new default status

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 66 Marcel de Rooy 2021-08-18 12:29:03 UTC
Created attachment 123951 [details] [review]
Bug 27944: (QA follow-up) Rename ->open for ->set_pending

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 67 Marcel de Rooy 2021-08-18 12:29:08 UTC
Created attachment 123952 [details] [review]
Bug 27944: (QA follow-up) Make staff home page show new article requests

As exposed on comment 29, the current code is showing the 'pending'
requests when it should display 'requested' ones (i.e. the new ones).

This patch changes that accordingly.

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 68 Marcel de Rooy 2021-08-31 12:48:34 UTC
If bug 27945 might pass before this one, we could add here:

+++ b/Koha/Patron.pm
@@ -975,7 +975,7 @@ sub can_request_article {

-        { borrowernumber => $self->borrowernumber, status => { '!=' => ['CANCELED','COMPLETED'] } },
+        { borrowernumber => $self->borrowernumber, status => { '!=' => ['PENDING', 'CANCELED', 'COMPLETED'] } },

It feels better to not count the PENDING reqs here.
Comment 69 Nick Clemens 2021-09-17 11:10:06 UTC
Created attachment 124966 [details] [review]
Bug 27944: Add new letter in atomicupdate, and modify status column in article_requests table

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 70 Nick Clemens 2021-09-17 11:10:10 UTC
Created attachment 124967 [details] [review]
Bug 27944: Add AR_REQUESTED message in sample_notices.yml and modify AR_PENDING

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 71 Nick Clemens 2021-09-17 11:10:13 UTC
Created attachment 124968 [details] [review]
Bug 27944: Add "requested" stage in article request process

This patch adds the stage "requested" in article request process, which
is previous to pending stage.

To test:
1. apply this patch
2. updatedatabase
3. enable ArticleRequests syspref
4. from staff inteface and from opac search for a record and place an
   article request
5. koha-mysql kohadev
6. query: select subject, content, letter_code from message_queue;
CHECK => There is a message for each article request with code
AR_REQUESTED
      => In opac-user.pl, in "Article requests" tab you should see a row
in the table with "Requested" status
5. in staff go to Circulation -> Article Requests
SUCCESS => You should see 3 tabs, one for Requested stage (with two
requests), one for Pending stage and one for Processing stage.
6. play with actions buttons
CHECK => you should see a new action called "Set request as pending"
SUCCESS => All action buttons behave as expected, and tab counts updates
correctly.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 72 Nick Clemens 2021-09-17 11:10:18 UTC
Created attachment 124969 [details] [review]
Bug 27944: Add missing Status module

Test plan:
perl -c the module or run qa tools

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 73 Nick Clemens 2021-09-17 11:10:22 UTC
Created attachment 124970 [details] [review]
Bug 27944: (follow-up) Improving consistency between notice names

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 74 Nick Clemens 2021-09-17 11:10:27 UTC
Created attachment 124971 [details] [review]
Bug 27944: (follow-up) Move existing statuses, rename idempotent

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 75 Nick Clemens 2021-09-17 11:10:33 UTC
Created attachment 124972 [details] [review]
Bug 27944: (follow-up) Move statuses to constants

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 76 Nick Clemens 2021-09-17 11:10:37 UTC
Created attachment 124973 [details] [review]
Bug 27944: REQUESTED is the new default status

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 77 Nick Clemens 2021-09-17 11:10:42 UTC
Created attachment 124974 [details] [review]
Bug 27944: (QA follow-up) Rename ->open for ->set_pending

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 78 Nick Clemens 2021-09-17 11:10:48 UTC
Created attachment 124975 [details] [review]
Bug 27944: (QA follow-up) Make staff home page show new article requests

As exposed on comment 29, the current code is showing the 'pending'
requests when it should display 'requested' ones (i.e. the new ones).

This patch changes that accordingly.

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 79 Nick Clemens 2021-09-17 11:10:59 UTC
Created attachment 124976 [details] [review]
Bug 27944: (QA follow-up) Add span to fix pipe separator

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 80 Nick Clemens 2021-09-17 11:13:14 UTC
(In reply to Marcel de Rooy from comment #68)
> If bug 27945 might pass before this one, we could add here:
> 
> +++ b/Koha/Patron.pm
> @@ -975,7 +975,7 @@ sub can_request_article {
> 
> -        { borrowernumber => $self->borrowernumber, status => { '!=' =>
> ['CANCELED','COMPLETED'] } },
> +        { borrowernumber => $self->borrowernumber, status => { '!=' =>
> ['PENDING', 'CANCELED', 'COMPLETED'] } },
> 
> It feels better to not count the PENDING reqs here.

The bugs do not apply on top of each other - one will need to be rebased to be pushed before the other, Agustin, can you take care of that and add the follow-up?
Comment 81 Tomás Cohen Arazi 2021-09-21 19:59:05 UTC
(In reply to Nick Clemens from comment #80)
> (In reply to Marcel de Rooy from comment #68)
> > If bug 27945 might pass before this one, we could add here:
> > 
> > +++ b/Koha/Patron.pm
> > @@ -975,7 +975,7 @@ sub can_request_article {
> > 
> > -        { borrowernumber => $self->borrowernumber, status => { '!=' =>
> > ['CANCELED','COMPLETED'] } },
> > +        { borrowernumber => $self->borrowernumber, status => { '!=' =>
> > ['PENDING', 'CANCELED', 'COMPLETED'] } },
> > 
> > It feels better to not count the PENDING reqs here.
> 
> The bugs do not apply on top of each other - one will need to be rebased to
> be pushed before the other, Agustin, can you take care of that and add the
> follow-up?

I've decided this  one will depend on bug 27945. Just because someone had to make the decision :-D
Comment 82 Tomás Cohen Arazi 2021-09-22 18:15:06 UTC
Created attachment 125158 [details] [review]
Bug 27944: (QA follow-up) Make atomic update idempotent

This patch checks the DB structure to see if the status column
definition contains 'REQUESTED' as a valid ENUM value.

The script is also moved into the new style.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 83 Marcel de Rooy 2021-09-23 06:02:06 UTC
Our old friend:
   FAIL   valid_template
                Attempt to reload Koha/Template/Plugin/Biblio.pm aborted.
Compilation failed in require at /usr/lib/x86_64-linux-gnu/perl5/5.28/Template/Plugins.pm line 206.
Comment 84 Jonathan Druart 2021-09-30 14:33:58 UTC
1. Missing POD in Koha::ArticleRequest

2. Koha::ArticleRequest->request is not covered by tests

3. Changes to Koha::ArticleRequest->cancel are not covered by tests

4. Koha::ArticleRequests->requested is not covered by tests

4. Not a big deal (not blocker of course) but
  $reason = $av->lib_opac ? $av->lib_opac : $av->lib if $av;
Is more readable (IMO) like:
  $reason = $av->lib_opac || $av->lib if $av;
Comment 85 Tomás Cohen Arazi 2021-09-30 14:57:54 UTC
Created attachment 125544 [details] [review]
Bug 27944: (follow-up) Add POD

The library didn't contain real POD. This patch adds it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 86 Tomás Cohen Arazi 2021-09-30 16:14:47 UTC
(In reply to Jonathan Druart from comment #84)
> 1. Missing POD in Koha::ArticleRequest
> 
> 2. Koha::ArticleRequest->request is not covered by tests
> 
> 3. Changes to Koha::ArticleRequest->cancel are not covered by tests
> 
> 4. Koha::ArticleRequests->requested is not covered by tests
> 
> 4. Not a big deal (not blocker of course) but
>   $reason = $av->lib_opac ? $av->lib_opac : $av->lib if $av;
> Is more readable (IMO) like:
>   $reason = $av->lib_opac || $av->lib if $av;

Can this be pushed as is? I move the tests to another file in a later bug and can happily add tests. Some already exist on Article requests.t
Comment 87 Jonathan Druart 2021-09-30 16:28:09 UTC
Can be done on a follow-up bug, yes. But I will wait for the tests to push.
Comment 88 Tomás Cohen Arazi 2021-09-30 18:07:26 UTC Comment hidden (obsolete)
Comment 89 Tomás Cohen Arazi 2021-09-30 18:36:37 UTC
Created attachment 125567 [details] [review]
Bug 27944: Add missing tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 90 Tomás Cohen Arazi 2021-09-30 18:38:20 UTC
(In reply to Jonathan Druart from comment #84)
> 1. Missing POD in Koha::ArticleRequest

Done.

> 2. Koha::ArticleRequest->request is not covered by tests

Done.

> 3. Changes to Koha::ArticleRequest->cancel are not covered by tests

Done.

> 4. Koha::ArticleRequests->requested is not covered by tests

Done.

> 4. Not a big deal (not blocker of course) but
>   $reason = $av->lib_opac ? $av->lib_opac : $av->lib if $av;
> Is more readable (IMO) like:
>   $reason = $av->lib_opac || $av->lib if $av;

This is another bug, will fix in the right place.
Comment 91 Jonathan Druart 2021-10-06 08:36:40 UTC
(In reply to Tomás Cohen Arazi from comment #90)
> (In reply to Jonathan Druart from comment #84)
> > 4. Not a big deal (not blocker of course) but
> >   $reason = $av->lib_opac ? $av->lib_opac : $av->lib if $av;
> > Is more readable (IMO) like:
> >   $reason = $av->lib_opac || $av->lib if $av;
> 
> This is another bug, will fix in the right place.

Not done yet.
Comment 92 Jonathan Druart 2021-10-06 08:37:07 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 93 Jonathan Druart 2021-10-06 09:36:27 UTC
xt/sample_notices.t ................ 1/? 
#   Failed test 'No sample notice to add'
#   at xt/sample_notices.t line 86.
# Sample notices to add in fr-FR/1-Obligatoire/sample_notices.sql: AR_REQUESTED

#   Failed test 'No sample notice to add'
#   at xt/sample_notices.t line 86.
# Sample notices to add in fr-CA/obligatoire/sample_notices.sql: AR_REQUESTED

#   Failed test 'No sample notice to add'
#   at xt/sample_notices.t line 86.
# Sample notices to add in nb-NO/1-Obligatorisk/sample_notices.sql: AR_REQUESTED

Fix or help:
Bug 27101 - Remove fr-CA installer data
Bug 27622 - Remove nb-NO installer data

fr-FR is needed as it won't be ready for 21.11
Comment 94 Jonathan Druart 2021-10-06 09:43:58 UTC
Created attachment 125762 [details] [review]
Bug 27944: Fix t/db_dependent/AuthorisedValues.t

#   Failed test 'The second category should be correct (ordered by category name)'
 #   at t/db_dependent/AuthorisedValues.t line 141.
 #          got: 'AR_CANCELLATION'
 #     expected: 'av_for_testing'
Comment 95 Jonathan Druart 2021-10-06 12:58:03 UTC
(In reply to Jonathan Druart from comment #94)
> Created attachment 125762 [details] [review] [review]
> Bug 27944: Fix t/db_dependent/AuthorisedValues.t
> 
> #   Failed test 'The second category should be correct (ordered by category
> name)'
>  #   at t/db_dependent/AuthorisedValues.t line 141.
>  #          got: 'AR_CANCELLATION'
>  #     expected: 'av_for_testing'

Pushed to master.
Comment 96 Tomás Cohen Arazi 2021-10-06 13:35:36 UTC
Created attachment 125813 [details] [review]
Bug 27944: Add fr-FR AR_REQUESTED notice

This patch adds the missing notice.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 97 Jonathan Druart 2021-10-07 13:48:40 UTC
(In reply to Tomás Cohen Arazi from comment #96)
> Created attachment 125813 [details] [review] [review]
> Bug 27944: Add fr-FR AR_REQUESTED notice
> 
> This patch adds the missing notice.
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Pushed to master.