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'
Created attachment 119020 [details] [review] Bug 27944: Add new letter in atomicupdate, and modify status column in article_requests table
Created attachment 119021 [details] [review] Bug 27944: Add AR_REQUESTED message in sample_notices.yml and modify AR_PENDING
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.
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 ?
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
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.
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
(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);
(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?
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.
(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.
(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.
(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?
(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?
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.
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.
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)?
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?
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>
Created attachment 122836 [details] [review] Bug 27944: Add new letter in atomicupdate, and modify status column in article_requests table
Created attachment 122837 [details] [review] Bug 27944: Add AR_REQUESTED message in sample_notices.yml and modify AR_PENDING
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.
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>
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.
Created attachment 122842 [details] [review] Bug 27944: (follow-up) Improving consistency between notice names
Created attachment 122843 [details] [review] Bug 27944: (follow-up) Move existing statuses, rename idempotent
Changing status temporarily, finishing this tomorrow
- `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.
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);
Created attachment 122857 [details] [review] Bug 27944: (follow-up) Move statuses to constants
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.
Created attachment 122984 [details] [review] Bug 27944: Add new letter in atomicupdate, and modify status column in article_requests table
Created attachment 122985 [details] [review] Bug 27944: Add AR_REQUESTED message in sample_notices.yml and modify AR_PENDING
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.
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>
Created attachment 122988 [details] [review] Bug 27944: (follow-up) Improving consistency between notice names
Created attachment 122989 [details] [review] Bug 27944: (follow-up) Move existing statuses, rename idempotent
Created attachment 122990 [details] [review] Bug 27944: (follow-up) Move statuses to constants
(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
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.
(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.
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)
(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..
(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!
(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.
(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.
(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.
(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 :) ?
(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 ?
(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.
Created attachment 123710 [details] [review] Bug 27944: REQUESTED is the new default status Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 123711 [details] [review] Bug 27944: (QA follow-up) Rename ->open for ->set_pending Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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.
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?
(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.
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.
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>
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>
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>
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>
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>
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>
Created attachment 123949 [details] [review] Bug 27944: (follow-up) Move statuses to constants Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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>
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>
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>
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.
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Created attachment 124976 [details] [review] Bug 27944: (QA follow-up) Add span to fix pipe separator Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(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?
(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
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>
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.
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;
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>
(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
Can be done on a follow-up bug, yes. But I will wait for the tests to push.
Created attachment 125565 [details] [review] Bug 27944: Add missing tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 125567 [details] [review] Bug 27944: Add missing tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(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.
(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.
Pushed to master for 21.11, thanks to everybody involved!
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
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'
(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.
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>
(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.