|
Description
Nick Clemens (kidclamp)
2021-03-12 14:48:29 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? (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 Created attachment 124063 [details] [review] Bug 27947: Add authorised values list in article requests cancellation Created attachment 124064 [details] [review] Bug 27947: [DO NOT PUSH] update ArticleRequest schema 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 (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 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 ? 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 (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. (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 Created attachment 124849 [details] [review] Bug 27947: Add authorised values list in article requests cancellation Created attachment 124850 [details] [review] Bug 27947: [DO NOT PUSH] update ArticleRequest schema 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 Working on this one now 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> Created attachment 124953 [details] [review] Bug 27947: [DO NOT PUSH] update ArticleRequest schema Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 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> 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> 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> 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? Reviewing this. (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. 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> 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> 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> 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> 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> 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> Created attachment 124999 [details] [review] Bug 27947: Add regression tests for routes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> 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> 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> 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> 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> 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> 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> 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> 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. 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.
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> (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. 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> 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> 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> 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> 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> 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> 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> 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> 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. 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> Brill, thanks for the follow-up clarifications Tomas. Passing QA Last rebase after dependency changes. 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> 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> 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> 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> 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> 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> 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> 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> 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. 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> 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, ]
}
);
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?
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> Created attachment 125740 [details] [review] Bug 27947: (QA follow-up) Fix shipped notice Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to master for 21.11, thanks to everybody involved! |