Bug 27947 - Add default cancellation reasons to article requests
Summary: Add default cancellation reasons to article requests
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Agustín Moyano
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 27945
Blocks: 29082
  Show dependency treegraph
 
Reported: 2021-03-12 14:48 UTC by Nick Clemens
Modified: 2021-11-04 12:06 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Text to go in the release notes:
This feature adds a way to define a list of possible cancellation reasons for article requests. That way, they can be chosen upon cancellation.
Version(s) released in:
21.11.00


Attachments
Bug 27947: Add authorised values list in article requests cancellation (3.87 KB, patch)
2021-08-24 14:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 27947: [DO NOT PUSH] update ArticleRequest schema (1.51 KB, patch)
2021-08-24 14:31 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 27947: Add cancellation reason to article request (30.89 KB, patch)
2021-08-24 14:32 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 27947: Add authorised values list in article requests cancellation (8.54 KB, patch)
2021-09-13 22:27 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 27947: [DO NOT PUSH] update ArticleRequest schema (1.51 KB, patch)
2021-09-13 22:27 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 27947: Add cancellation reason to article request (35.17 KB, patch)
2021-09-13 22:27 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 27947: Add authorised values list in article requests cancellation (8.63 KB, patch)
2021-09-17 09:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27947: [DO NOT PUSH] update ArticleRequest schema (1.61 KB, patch)
2021-09-17 09:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27947: Add cancellation reason to article request (35.27 KB, patch)
2021-09-17 09:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27947: (follow-up) Improve atomic update (3.30 KB, patch)
2021-09-17 09:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27947: (follow-up) Add OPAC cancellation as new reason too (6.71 KB, patch)
2021-09-17 09:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27947: Add authorised values list in article requests cancellation (8.65 KB, patch)
2021-09-17 15:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: [DO NOT PUSH] update ArticleRequest schema (1.62 KB, patch)
2021-09-17 15:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: Add cancellation reason to article request (35.28 KB, patch)
2021-09-17 15:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: (follow-up) Improve atomic update (3.32 KB, patch)
2021-09-17 15:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: (follow-up) Add OPAC cancellation as new reason too (6.72 KB, patch)
2021-09-17 15:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: (QA follow-up) Refactor routes (17.76 KB, patch)
2021-09-17 15:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: Add regression tests for routes (5.16 KB, patch)
2021-09-17 15:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: Add authorised values list in article requests cancellation (8.71 KB, patch)
2021-09-17 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: [DO NOT PUSH] update ArticleRequest schema (1.68 KB, patch)
2021-09-17 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: Add cancellation reason to article request (35.35 KB, patch)
2021-09-17 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: (follow-up) Improve atomic update (3.38 KB, patch)
2021-09-17 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: (follow-up) Add OPAC cancellation as new reason too (6.78 KB, patch)
2021-09-17 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: (QA follow-up) Refactor routes (17.82 KB, patch)
2021-09-17 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: Add regression tests for routes (5.22 KB, patch)
2021-09-17 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: (QA follow-up) Remove incorrect permission check (1.04 KB, patch)
2021-09-17 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: (QA follow-up) Update test to highlight issue (3.46 KB, patch)
2021-09-17 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: (QA follow-up) Clarify permissions (5.34 KB, patch)
2021-09-17 19:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: Add authorised values list in article requests cancellation (8.71 KB, patch)
2021-09-20 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: [DO NOT PUSH] update ArticleRequest schema (1.68 KB, patch)
2021-09-20 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: Add cancellation reason to article request (35.35 KB, patch)
2021-09-20 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: (follow-up) Improve atomic update (3.38 KB, patch)
2021-09-20 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: (follow-up) Add OPAC cancellation as new reason too (6.78 KB, patch)
2021-09-20 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: (QA follow-up) Refactor routes (17.82 KB, patch)
2021-09-20 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: Add regression tests for routes (5.22 KB, patch)
2021-09-20 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: (QA follow-up) Remove incorrect permission check (1.04 KB, patch)
2021-09-20 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: (QA follow-up) Update test to highlight issue (3.46 KB, patch)
2021-09-20 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: (QA follow-up) Clarify permissions (5.34 KB, patch)
2021-09-20 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27947: Add authorised values list in article requests cancellation (8.71 KB, patch)
2021-09-22 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: [DO NOT PUSH] update ArticleRequest schema (1.68 KB, patch)
2021-09-22 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: Add cancellation reason to article request (34.84 KB, patch)
2021-09-22 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: (follow-up) Improve atomic update (3.38 KB, patch)
2021-09-22 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: (follow-up) Add OPAC cancellation as new reason too (6.78 KB, patch)
2021-09-22 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: (QA follow-up) Refactor routes (17.82 KB, patch)
2021-09-22 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: Add regression tests for routes (5.22 KB, patch)
2021-09-22 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: (QA follow-up) Remove incorrect permission check (1.04 KB, patch)
2021-09-22 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: (QA follow-up) Update test to highlight issue (3.46 KB, patch)
2021-09-22 19:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: (QA follow-up) Clarify permissions (5.34 KB, patch)
2021-09-22 19:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: (QA follow-up) Make parameters a hashref (4.01 KB, patch)
2021-10-05 14:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27947: (QA follow-up) Fix shipped notice (1.02 KB, patch)
2021-10-05 14:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-03-12 14:48:29 UTC
Similar to holds we propose to add an authorised value  list of 'reasons' for cancellation of article requests
Comment 1 Agustín Moyano 2021-08-17 13:28:44 UTC
(In reply to Nick Clemens from comment #0)
> Similar to holds we propose to add an authorised value  list of 'reasons'
> for cancellation of article requests

Hi Nick, should we add a default list of reasons?
Comment 2 Nick Clemens 2021-08-24 13:01:25 UTC
(In reply to Agustín Moyano from comment #1)
> (In reply to Nick Clemens from comment #0)
> > Similar to holds we propose to add an authorised value  list of 'reasons'
> > for cancellation of article requests
> 
> Hi Nick, should we add a default list of reasons?

I think you could use the same defaults as HOLD_CANCELLATION to start:
Damaged
Not found
Comment 3 Agustín Moyano 2021-08-24 14:31:52 UTC
Created attachment 124063 [details] [review]
Bug 27947: Add authorised values list in article requests cancellation
Comment 4 Agustín Moyano 2021-08-24 14:31:56 UTC
Created attachment 124064 [details] [review]
Bug 27947: [DO NOT PUSH] update ArticleRequest schema
Comment 5 Agustín Moyano 2021-08-24 14:32:01 UTC
Created attachment 124065 [details] [review]
Bug 27947: Add cancellation reason to article request

This bug adds a cancellation reason authorised values to article requests

To test:
1. apply this patch
2. updatedatabase
3. in staff interface go to /cgi-bin/koha/admin/authorised_values.pl
CHECK => AR_CANCELLATION category should appears
4. place several article requests
5. in staff interface go to /cgi-bin/koha/circ/article-requests.pl
6. select multiple requests, or just one and cancel them
SUCCESS => a modal pops up offering to select a cancellation reason
7. repeat steps 4 to 6 but for /cgi-bin/koha/circ/request-article.pl
8. repeat steps 4 to 6 but for /cgi-bin/koha/opac-user.pl#opac-user-article-requests in opac interface
Comment 6 Agustín Moyano 2021-08-24 15:26:30 UTC
(In reply to Agustín Moyano from comment #5)
> Created attachment 124065 [details] [review] [review]
> Bug 27947: Add cancellation reason to article request
> 
> This bug adds a cancellation reason authorised values to article requests
> 
> To test:
> 1. apply this patch
> 2. updatedatabase
> 3. in staff interface go to /cgi-bin/koha/admin/authorised_values.pl
> CHECK => AR_CANCELLATION category should appears
> 4. place several article requests
> 5. in staff interface go to /cgi-bin/koha/circ/article-requests.pl
> 6. select multiple requests, or just one and cancel them
> SUCCESS => a modal pops up offering to select a cancellation reason
> 7. repeat steps 4 to 6 but for /cgi-bin/koha/circ/request-article.pl
> 8. repeat steps 4 to 6 but for
> /cgi-bin/koha/opac-user.pl#opac-user-article-requests in opac interface

I added some default authorised values for AR_CANCELLATION, but most probably they should be modified
Comment 7 Marcel de Rooy 2021-08-31 12:00:24 UTC
Hi Agustin,

I found a few problems :)

[1] Category AR_CANCELLATION has no description. See Admin form.
[2] There are no authorized values defined for AR_CANCELLATION ? The dbrev should also add the reasons added to auth_val.yml.
[3] There is no longer a possibility to add a custom reason for cancelling (as we had). The default reasons should be additional, not excluding free text.
[4] Your request for an article from Learning Perl () has been canceled for the following reason: [blanks]
=> The selected reason is not mentioned in the notice ! Note that the reason is saved correctly in the database.
[5] Why do you add cancel_article_request to Koha/REST/V1/Patrons.pm ? Apart from checking existence of the patron id, what is its use ?
Comment 8 Agustín Moyano 2021-08-31 14:02:19 UTC
Hi Marcel,

(In reply to Marcel de Rooy from comment #7)
> Hi Agustin,
> 
> I found a few problems :)
> 
> [1] Category AR_CANCELLATION has no description. See Admin form.

Ok, I'll add a description right away

> [2] There are no authorized values defined for AR_CANCELLATION ? The dbrev
> should also add the reasons added to auth_val.yml.

In the first attach there are authorised values in auth_val.yml.. I was not sure to add them in atomicupdate, and then I saw HOLDS_CANCELLATION did not add authorised values in the update.

> [3] There is no longer a possibility to add a custom reason for cancelling
> (as we had). The default reasons should be additional, not excluding free
> text.

Ok, I'll try to restore the way it was... what happens if patron selects a reason, and then adds a custom text?.. which one wins?

> [4] Your request for an article from Learning Perl () has been canceled for
> the following reason: [blanks]
> => The selected reason is not mentioned in the notice ! Note that the reason
> is saved correctly in the database.

Ups! I'll check notices

> [5] Why do you add cancel_article_request to Koha/REST/V1/Patrons.pm ? Apart
> from checking existence of the patron id, what is its use ?

As I was making API endpoints for cancelling article requests from staff interface (we should move away from svc in any chance we've got), I thought it would be good to add the same for opac.. the endpoint that opac uses is DELETE /api/v1/public/patrons/{patron_id}/article_requests/{ar_id}, so It seemed natural to place it in Koha/REST/V1/Patrons.pm 

Cheers
Comment 9 Marcel de Rooy 2021-08-31 14:11:41 UTC
(In reply to Agustín Moyano from comment #8)

Thx for your prompt response.

> > [3] There is no longer a possibility to add a custom reason for cancelling
> > (as we had). The default reasons should be additional, not excluding free
> > text.
> 
> Ok, I'll try to restore the way it was... what happens if patron selects a
> reason, and then adds a custom text?.. which one wins?

Free text wins from a default text. But you could control that. Disable free text unless they click an option 'Custom reason' or so?

> > [5] Why do you add cancel_article_request to Koha/REST/V1/Patrons.pm ? Apart
> > from checking existence of the patron id, what is its use ?
> 
> As I was making API endpoints for cancelling article requests from staff
> interface (we should move away from svc in any chance we've got), I thought
> it would be good to add the same for opac.. the endpoint that opac uses is
> DELETE /api/v1/public/patrons/{patron_id}/article_requests/{ar_id}, so It
> seemed natural to place it in Koha/REST/V1/Patrons.pm 

I dont really mind. Maybe ask Tomas? But it seemed more logical (or consistent) to me that deleting an article request from OPAC or staff have similar end points.
Comment 10 Agustín Moyano 2021-08-31 15:49:43 UTC
(In reply to Marcel de Rooy from comment #9)
> (In reply to Agustín Moyano from comment #8)
> 
> Free text wins from a default text. But you could control that. Disable free
> text unless they click an option 'Custom reason' or so?
> 

Good idea.. I'll do that

> > > [5] Why do you add cancel_article_request to Koha/REST/V1/Patrons.pm ? Apart
> > > from checking existence of the patron id, what is its use ?
> > 
> > As I was making API endpoints for cancelling article requests from staff
> > interface (we should move away from svc in any chance we've got), I thought
> > it would be good to add the same for opac.. the endpoint that opac uses is
> > DELETE /api/v1/public/patrons/{patron_id}/article_requests/{ar_id}, so It
> > seemed natural to place it in Koha/REST/V1/Patrons.pm 
> 
> I dont really mind. Maybe ask Tomas? But it seemed more logical (or
> consistent) to me that deleting an article request from OPAC or staff have
> similar end points.

Ok, I'll check with him
Comment 11 Agustín Moyano 2021-09-13 22:27:38 UTC
Created attachment 124849 [details] [review]
Bug 27947: Add authorised values list in article requests cancellation
Comment 12 Agustín Moyano 2021-09-13 22:27:43 UTC
Created attachment 124850 [details] [review]
Bug 27947: [DO NOT PUSH] update ArticleRequest schema
Comment 13 Agustín Moyano 2021-09-13 22:27:48 UTC
Created attachment 124851 [details] [review]
Bug 27947: Add cancellation reason to article request

This bug adds a cancellation reason authorised values to article requests

To test:
1. apply this patch
2. updatedatabase
3. in staff interface go to /cgi-bin/koha/admin/authorised_values.pl
CHECK => AR_CANCELLATION category should appears
4. place several article requests
5. in staff interface go to /cgi-bin/koha/circ/article-requests.pl
6. select multiple requests, or just one and cancel them
SUCCESS => a modal pops up offering to select a cancellation reason
CHECK => message_queue table has messages with cancellation reason included
7. repeat steps 4 to 6 but for /cgi-bin/koha/circ/request-article.pl
8. cancelling article requests from opac interface should work just as before
Comment 14 Marcel de Rooy 2021-09-17 07:35:35 UTC
Working on this one now
Comment 15 Marcel de Rooy 2021-09-17 09:18:58 UTC
Created attachment 124952 [details] [review]
Bug 27947: Add authorised values list in article requests cancellation

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2021-09-17 09:19:01 UTC
Created attachment 124953 [details] [review]
Bug 27947: [DO NOT PUSH] update ArticleRequest schema

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2021-09-17 09:19:05 UTC
Created attachment 124954 [details] [review]
Bug 27947: Add cancellation reason to article request

This bug adds a cancellation reason authorised values to article requests

To test:
1. apply this patch
2. updatedatabase
3. in staff interface go to /cgi-bin/koha/admin/authorised_values.pl
CHECK => AR_CANCELLATION category should appears
4. place several article requests
5. in staff interface go to /cgi-bin/koha/circ/article-requests.pl
6. select multiple requests, or just one and cancel them
SUCCESS => a modal pops up offering to select a cancellation reason
CHECK => message_queue table has messages with cancellation reason included
7. repeat steps 4 to 6 but for /cgi-bin/koha/circ/request-article.pl
8. cancelling article requests from opac interface should work just as before

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2021-09-17 09:19:09 UTC
Created attachment 124955 [details] [review]
Bug 27947: (follow-up) Improve atomic update

We do not need to replace the whole notice text, we only need to
replace the reference to article_requests.notes.

Test plan:
Run the dbrev. Verify result for notice AR_CANCELED.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2021-09-17 09:19:14 UTC
Created attachment 124956 [details] [review]
Bug 27947: (follow-up) Add OPAC cancellation as new reason too

Something that currently comes up now already too:
If the user cancels from the OPAC page, there is no reason in the notice.
We could simply add one now.

Test plan:
Run dbrev to add the OPAC authval.
Create new AR and cancel it from OPAC.
Verify notice created for the user.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2021-09-17 09:19:49 UTC
Looks good to me. 
Added two follow-ups to refine notice update in db rev and to add OPAC cancellation as a new reason.

One thing remains (point 5 from comment7):
DELETE /public/patrons/{patron_id}/article_requests/{ar_id}
DELETE /api/v1/article_requests/{ar_id}
=> Tomas, please respond. Quoting myself: "But it seemed more logical (or consistent) to me that deleting an article request from OPAC or staff have similar end points."
In short: Why not /public/article_requests/{ar_id} ?
And out of scope: But why does public not have a version?
Comment 21 Tomás Cohen Arazi 2021-09-17 10:51:27 UTC
Reviewing this.
Comment 22 Tomás Cohen Arazi 2021-09-17 12:57:07 UTC
(In reply to Marcel de Rooy from comment #9)
> > > [5] Why do you add cancel_article_request to Koha/REST/V1/Patrons.pm ? Apart
> > > from checking existence of the patron id, what is its use ?
> > 
> > As I was making API endpoints for cancelling article requests from staff
> > interface (we should move away from svc in any chance we've got), I thought
> > it would be good to add the same for opac.. the endpoint that opac uses is
> > DELETE /api/v1/public/patrons/{patron_id}/article_requests/{ar_id}, so It
> > seemed natural to place it in Koha/REST/V1/Patrons.pm 
> 
> I dont really mind. Maybe ask Tomas? But it seemed more logical (or
> consistent) to me that deleting an article request from OPAC or staff have
> similar end points.

I generally prefer context-specific routes, so:

/patrons/:patron_id/article_requests/1
over
/article_requests/1

in the particular case of the public routes, this allows us to use the 'allow-owner' permission feature from our API framework.

What I don't like, is having the controller methods so sparse. I will submit a follow-up reorganizing that.
Comment 23 Tomás Cohen Arazi 2021-09-17 15:14:56 UTC
Created attachment 124993 [details] [review]
Bug 27947: Add authorised values list in article requests cancellation

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2021-09-17 15:15:02 UTC
Created attachment 124994 [details] [review]
Bug 27947: [DO NOT PUSH] update ArticleRequest schema

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2021-09-17 15:15:07 UTC
Created attachment 124995 [details] [review]
Bug 27947: Add cancellation reason to article request

This bug adds a cancellation reason authorised values to article requests

To test:
1. apply this patch
2. updatedatabase
3. in staff interface go to /cgi-bin/koha/admin/authorised_values.pl
CHECK => AR_CANCELLATION category should appears
4. place several article requests
5. in staff interface go to /cgi-bin/koha/circ/article-requests.pl
6. select multiple requests, or just one and cancel them
SUCCESS => a modal pops up offering to select a cancellation reason
CHECK => message_queue table has messages with cancellation reason included
7. repeat steps 4 to 6 but for /cgi-bin/koha/circ/request-article.pl
8. cancelling article requests from opac interface should work just as before

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Tomás Cohen Arazi 2021-09-17 15:15:12 UTC
Created attachment 124996 [details] [review]
Bug 27947: (follow-up) Improve atomic update

We do not need to replace the whole notice text, we only need to
replace the reference to article_requests.notes.

Test plan:
Run the dbrev. Verify result for notice AR_CANCELED.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi 2021-09-17 15:15:18 UTC
Created attachment 124997 [details] [review]
Bug 27947: (follow-up) Add OPAC cancellation as new reason too

Something that currently comes up now already too:
If the user cancels from the OPAC page, there is no reason in the notice.
We could simply add one now.

Test plan:
Run dbrev to add the OPAC authval.
Create new AR and cancel it from OPAC.
Verify notice created for the user.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi 2021-09-17 15:15:24 UTC
Created attachment 124998 [details] [review]
Bug 27947: (QA follow-up) Refactor routes

This patch refactors the route specs a bit, and also reorganizes code
for easier tracking.

Unused exceptions that were added earlier are removed for now.

A follow-up patch will add tests to this routes.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Tomás Cohen Arazi 2021-09-17 15:15:29 UTC
Created attachment 124999 [details] [review]
Bug 27947: Add regression tests for routes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Martin Renvoize 2021-09-17 16:16:13 UTC
Created attachment 125002 [details] [review]
Bug 27947: Add authorised values list in article requests cancellation

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2021-09-17 16:16:17 UTC
Created attachment 125003 [details] [review]
Bug 27947: [DO NOT PUSH] update ArticleRequest schema

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize 2021-09-17 16:16:21 UTC
Created attachment 125004 [details] [review]
Bug 27947: Add cancellation reason to article request

This bug adds a cancellation reason authorised values to article requests

To test:
1. apply this patch
2. updatedatabase
3. in staff interface go to /cgi-bin/koha/admin/authorised_values.pl
CHECK => AR_CANCELLATION category should appears
4. place several article requests
5. in staff interface go to /cgi-bin/koha/circ/article-requests.pl
6. select multiple requests, or just one and cancel them
SUCCESS => a modal pops up offering to select a cancellation reason
CHECK => message_queue table has messages with cancellation reason included
7. repeat steps 4 to 6 but for /cgi-bin/koha/circ/request-article.pl
8. cancelling article requests from opac interface should work just as before

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Martin Renvoize 2021-09-17 16:16:25 UTC
Created attachment 125005 [details] [review]
Bug 27947: (follow-up) Improve atomic update

We do not need to replace the whole notice text, we only need to
replace the reference to article_requests.notes.

Test plan:
Run the dbrev. Verify result for notice AR_CANCELED.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize 2021-09-17 16:16:30 UTC
Created attachment 125006 [details] [review]
Bug 27947: (follow-up) Add OPAC cancellation as new reason too

Something that currently comes up now already too:
If the user cancels from the OPAC page, there is no reason in the notice.
We could simply add one now.

Test plan:
Run dbrev to add the OPAC authval.
Create new AR and cancel it from OPAC.
Verify notice created for the user.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Martin Renvoize 2021-09-17 16:16:35 UTC
Created attachment 125007 [details] [review]
Bug 27947: (QA follow-up) Refactor routes

This patch refactors the route specs a bit, and also reorganizes code
for easier tracking.

Unused exceptions that were added earlier are removed for now.

A follow-up patch will add tests to this routes.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Martin Renvoize 2021-09-17 16:16:39 UTC
Created attachment 125008 [details] [review]
Bug 27947: Add regression tests for routes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Martin Renvoize 2021-09-17 16:16:43 UTC
Created attachment 125009 [details] [review]
Bug 27947: (QA follow-up) Remove incorrect permission check

The API was checking for 'reserverforothers' permission, but that
doesn't make sense to apply here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize 2021-09-17 16:16:48 UTC
Created attachment 125010 [details] [review]
Bug 27947: (QA follow-up) Update test to highlight issue

This addition to the tests highlights a permissions/security check
error.
Comment 39 Martin Renvoize 2021-09-17 16:20:21 UTC
Sorry guys.. little more needed here.

My first followup drops the 'reserveforothers' permission requirement as I don't think that relates to this functionality.. but it makes the API tests fail.. and I can't see why.. code blind on a Friday.

My second followup highlights an issue with the public route.  Although moving the route under /public/patrons/{patron_id} ensure we do a patron identity check.. there isn't actually a later check anywhere that the article your trying to delete actually belongs to the patron ;)

This final one is actually why I preferred the original /article_requests/{request_id} approach.. though of course that would require the addition of a routine to handle checking borrowernumber in the article request against the user as per the other routines for checking allow-owner.
Comment 40 Tomás Cohen Arazi 2021-09-17 19:26:19 UTC
Created attachment 125016 [details] [review]
Bug 27947: (QA follow-up) Clarify permissions

Privileged routes need permissions defined. This patch adds the minimum
required permission until there are article request-specific permissions
in Koha: circulate: circulate_remaining_permissions

It is also clarified that interacting with an article request  from
another patron, but having your own patron_id in the path would return
404 instead of 403, as technically the resource (an article request from
the patron, identified.by the supplied id) doesn't exist.

Tests are tweaked.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 41 Tomás Cohen Arazi 2021-09-17 19:38:42 UTC
(In reply to Martin Renvoize from comment #39)
> Sorry guys.. little more needed here.
> 
> My first followup drops the 'reserveforothers' permission requirement as I
> don't think that relates to this functionality.. but it makes the API tests
> fail.. and I can't see why.. code blind on a Friday.

The permissions were wrong.

You just missed the fact that privileged routes require at least one permission (catalogue: 1 being the bare minimum). I think we never fixed this: https://gitlab.com/koha-community/qa-test-tools/-/issues/11

> My second followup highlights an issue with the public route.  Although
> moving the route under /public/patrons/{patron_id} ensure we do a patron
> identity check.. there isn't actually a later check anywhere that the
> article your trying to delete actually belongs to the patron ;)

There's an implicit check by doing:

   $article_requests = $patron->article_requests->find( $article_request_id );

I clarified this with a comment in the code on the latest patch. I agree with returning 404 there, as 403 would 'leak' the fact that the ID exists... Not sure how important it is, but I think it is correctly tested to avoid security issues.

> This final one is actually why I preferred the original
> /article_requests/{request_id} approach.. though of course that would
> require the addition of a routine to handle checking borrowernumber in the
> article request against the user as per the other routines for checking
> allow-owner.

allow-owner falls short. It feels like with routes with multiple ids in the path we need to think a bit more [1]. I think object 'ownership' validation should be baked into the objects in a declarative way. But that's for another day/bug.

[1] I tried adding a validator for article requests in Koha::REST::V1::Auth, but it failed randomly depending on which 'key' was picked first (patron_id vs. article_request_id). It feels like a big refactoring is needed there. Or just keep the patron_id from the path and leave the rest to  the controllers, as in this bug.
Comment 42 Martin Renvoize 2021-09-20 08:20:02 UTC
Created attachment 125038 [details] [review]
Bug 27947: Add authorised values list in article requests cancellation

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 43 Martin Renvoize 2021-09-20 08:20:08 UTC
Created attachment 125039 [details] [review]
Bug 27947: [DO NOT PUSH] update ArticleRequest schema

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 44 Martin Renvoize 2021-09-20 08:20:12 UTC
Created attachment 125040 [details] [review]
Bug 27947: Add cancellation reason to article request

This bug adds a cancellation reason authorised values to article requests

To test:
1. apply this patch
2. updatedatabase
3. in staff interface go to /cgi-bin/koha/admin/authorised_values.pl
CHECK => AR_CANCELLATION category should appears
4. place several article requests
5. in staff interface go to /cgi-bin/koha/circ/article-requests.pl
6. select multiple requests, or just one and cancel them
SUCCESS => a modal pops up offering to select a cancellation reason
CHECK => message_queue table has messages with cancellation reason included
7. repeat steps 4 to 6 but for /cgi-bin/koha/circ/request-article.pl
8. cancelling article requests from opac interface should work just as before

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 45 Martin Renvoize 2021-09-20 08:20:16 UTC
Created attachment 125041 [details] [review]
Bug 27947: (follow-up) Improve atomic update

We do not need to replace the whole notice text, we only need to
replace the reference to article_requests.notes.

Test plan:
Run the dbrev. Verify result for notice AR_CANCELED.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 46 Martin Renvoize 2021-09-20 08:20:20 UTC
Created attachment 125042 [details] [review]
Bug 27947: (follow-up) Add OPAC cancellation as new reason too

Something that currently comes up now already too:
If the user cancels from the OPAC page, there is no reason in the notice.
We could simply add one now.

Test plan:
Run dbrev to add the OPAC authval.
Create new AR and cancel it from OPAC.
Verify notice created for the user.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 47 Martin Renvoize 2021-09-20 08:20:24 UTC
Created attachment 125043 [details] [review]
Bug 27947: (QA follow-up) Refactor routes

This patch refactors the route specs a bit, and also reorganizes code
for easier tracking.

Unused exceptions that were added earlier are removed for now.

A follow-up patch will add tests to this routes.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 48 Martin Renvoize 2021-09-20 08:20:30 UTC
Created attachment 125044 [details] [review]
Bug 27947: Add regression tests for routes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 49 Martin Renvoize 2021-09-20 08:20:40 UTC
Created attachment 125045 [details] [review]
Bug 27947: (QA follow-up) Remove incorrect permission check

The API was checking for 'reserverforothers' permission, but that
doesn't make sense to apply here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 50 Martin Renvoize 2021-09-20 08:20:46 UTC
Created attachment 125046 [details] [review]
Bug 27947: (QA follow-up) Update test to highlight issue

This addition to the tests highlights a permissions/security check
error.
Comment 51 Martin Renvoize 2021-09-20 08:20:51 UTC
Created attachment 125047 [details] [review]
Bug 27947: (QA follow-up) Clarify permissions

Privileged routes need permissions defined. This patch adds the minimum
required permission until there are article request-specific permissions
in Koha: circulate: circulate_remaining_permissions

It is also clarified that interacting with an article request  from
another patron, but having your own patron_id in the path would return
404 instead of 403, as technically the resource (an article request from
the patron, identified.by the supplied id) doesn't exist.

Tests are tweaked.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 52 Martin Renvoize 2021-09-20 08:21:30 UTC
Brill, thanks for the follow-up clarifications Tomas.

Passing QA
Comment 53 Tomás Cohen Arazi 2021-09-22 19:33:32 UTC
Last rebase after dependency changes.
Comment 54 Tomás Cohen Arazi 2021-09-22 19:36:00 UTC
Created attachment 125165 [details] [review]
Bug 27947: Add authorised values list in article requests cancellation

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 55 Tomás Cohen Arazi 2021-09-22 19:36:06 UTC
Created attachment 125166 [details] [review]
Bug 27947: [DO NOT PUSH] update ArticleRequest schema

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 56 Tomás Cohen Arazi 2021-09-22 19:36:13 UTC
Created attachment 125167 [details] [review]
Bug 27947: Add cancellation reason to article request

This bug adds a cancellation reason authorised values to article requests

To test:
1. apply this patch
2. updatedatabase
3. in staff interface go to /cgi-bin/koha/admin/authorised_values.pl
CHECK => AR_CANCELLATION category should appears
4. place several article requests
5. in staff interface go to /cgi-bin/koha/circ/article-requests.pl
6. select multiple requests, or just one and cancel them
SUCCESS => a modal pops up offering to select a cancellation reason
CHECK => message_queue table has messages with cancellation reason included
7. repeat steps 4 to 6 but for /cgi-bin/koha/circ/request-article.pl
8. cancelling article requests from opac interface should work just as before

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 57 Tomás Cohen Arazi 2021-09-22 19:36:18 UTC
Created attachment 125168 [details] [review]
Bug 27947: (follow-up) Improve atomic update

We do not need to replace the whole notice text, we only need to
replace the reference to article_requests.notes.

Test plan:
Run the dbrev. Verify result for notice AR_CANCELED.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 58 Tomás Cohen Arazi 2021-09-22 19:36:23 UTC
Created attachment 125169 [details] [review]
Bug 27947: (follow-up) Add OPAC cancellation as new reason too

Something that currently comes up now already too:
If the user cancels from the OPAC page, there is no reason in the notice.
We could simply add one now.

Test plan:
Run dbrev to add the OPAC authval.
Create new AR and cancel it from OPAC.
Verify notice created for the user.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 59 Tomás Cohen Arazi 2021-09-22 19:36:30 UTC
Created attachment 125170 [details] [review]
Bug 27947: (QA follow-up) Refactor routes

This patch refactors the route specs a bit, and also reorganizes code
for easier tracking.

Unused exceptions that were added earlier are removed for now.

A follow-up patch will add tests to this routes.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 60 Tomás Cohen Arazi 2021-09-22 19:36:35 UTC
Created attachment 125171 [details] [review]
Bug 27947: Add regression tests for routes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 61 Tomás Cohen Arazi 2021-09-22 19:36:43 UTC
Created attachment 125172 [details] [review]
Bug 27947: (QA follow-up) Remove incorrect permission check

The API was checking for 'reserverforothers' permission, but that
doesn't make sense to apply here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 62 Tomás Cohen Arazi 2021-09-22 19:37:01 UTC
Created attachment 125173 [details] [review]
Bug 27947: (QA follow-up) Update test to highlight issue

This addition to the tests highlights a permissions/security check
error.
Comment 63 Tomás Cohen Arazi 2021-09-22 19:37:08 UTC
Created attachment 125174 [details] [review]
Bug 27947: (QA follow-up) Clarify permissions

Privileged routes need permissions defined. This patch adds the minimum
required permission until there are article request-specific permissions
in Koha: circulate: circulate_remaining_permissions

It is also clarified that interacting with an article request  from
another patron, but having your own patron_id in the path would return
404 instead of 403, as technically the resource (an article request from
the patron, identified.by the supplied id) doesn't exist.

Tests are tweaked.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 64 Jonathan Druart 2021-09-30 14:33:52 UTC
1. Not blocker, but:
-    my ( $self, $notes ) = @_;
+    my ( $self, $cancellation_reason, $notes ) = @_;

You should have used a hashref, especially because we have Koha::Hold->cancel that is getting a reason as well:

my $cancel_hold = $hold->cancel(
    {
        [ charge_cancel_fee => 1||0, ]
        [ cancellation_reason => $cancellation_reason, ]
    }
);
Comment 65 Jonathan Druart 2021-10-05 10:15:02 UTC
I think there is something wrong with the notice.

From the DBrev:

 28         $dbh->do(q{
 29             UPDATE letter SET content=REPLACE(content, '<<article_requests.notes>>', '<<reason>>')
 30             WHERE module = 'circulation' AND code = 'AR_CANCELED'
 31         });

when the installer file is still using notes

 233             - "Your request for an article from <<biblio.title>> (<<items.barcode>>) has been canceled for the following reason:"
 234             - ""
 235             - "<<article_requests.notes>>"

By the way, shouldn't we copy the notes to the cancellation_reason when the article request has been canceled?
Comment 66 Tomás Cohen Arazi 2021-10-05 14:16:48 UTC
Created attachment 125739 [details] [review]
Bug 27947: (QA follow-up) Make parameters a hashref

This patch makes the Koha::ArticleRequest->cancel parameters into a
hashref. (Missing) tests are added for those parameters being set as
well.

Calls to ->cancel are updated.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/ArticleRequest.t \
           t/db_dependent/api/v1/article_requests.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 67 Tomás Cohen Arazi 2021-10-05 14:45:08 UTC
Created attachment 125740 [details] [review]
Bug 27947: (QA follow-up) Fix shipped notice

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 68 Jonathan Druart 2021-10-06 08:37:17 UTC
Pushed to master for 21.11, thanks to everybody involved!