Bug 22531 - Allow for multiple requests to partners and display partners in audit log
Summary: Allow for multiple requests to partners and display partners in audit log
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Pedro Amorim
QA Contact:
URL:
Keywords: rel_22_11_candidate
Depends on:
Blocks: 32737
  Show dependency treegraph
 
Reported: 2019-03-18 14:20 UTC by Andrew Isherwood
Modified: 2023-08-28 15:02 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 22531: Add logging & display of req partners (14.53 KB, patch)
2019-03-18 16:34 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22531: Add unit tests (7.06 KB, patch)
2019-03-19 14:17 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22531: Add logging & display of req partners (14.94 KB, patch)
2021-03-26 10:47 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22531: Add unit tests (7.64 KB, patch)
2021-03-26 10:47 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22531: Add logging & display of req partners (14.99 KB, patch)
2021-07-07 08:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22531: Add unit tests (7.68 KB, patch)
2021-07-07 08:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22531: Add logging & display of req partners (14.99 KB, patch)
2021-09-17 11:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22531: Add unit tests (7.54 KB, patch)
2021-09-17 11:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22531: Add logging & display of req partners (15.05 KB, patch)
2022-03-02 10:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22531: Add unit tests (7.60 KB, patch)
2022-03-02 10:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22531: (QA follow-up) Fix filter and tab issues (4.48 KB, patch)
2022-03-02 10:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22531: (QA follow-up) Fix Unit Tests (2.32 KB, patch)
2022-03-02 10:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22531: Add logging & display of req partners (15.12 KB, patch)
2022-04-24 09:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22531: Add unit tests (7.66 KB, patch)
2022-04-24 09:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22531: (QA follow-up) Fix filter and tab issues (4.53 KB, patch)
2022-04-24 09:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22531: (QA follow-up) Fix Unit Tests (2.37 KB, patch)
2022-04-24 09:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22531: Add logging & display of req partners (15.09 KB, patch)
2023-01-27 12:00 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 22531: Add logging & display of req partners (15.09 KB, patch)
2023-01-27 12:37 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 22531: Add unit tests (7.66 KB, patch)
2023-01-27 12:37 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 22531: (QA follow-up) Fix filter and tab issues (4.53 KB, patch)
2023-01-27 12:37 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 22531: (QA follow-up) Fix Unit Tests (2.37 KB, patch)
2023-01-27 12:37 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2019-03-18 14:20:08 UTC
Currently, when an ILL request is sent to partners it is not possible to send to further partners, should the first partner not be able to fulfil the request.

A better approach would be to allow the "Place request with partners" option to be selected even when this action has already taken place. This will allow the user to try multiple partners subsequently.

As such, we should log which partners a request has been sent to and this information should be displayed in the "Display request log" functionality added in Bug 20750
Comment 1 Andrew Isherwood 2019-03-18 16:34:42 UTC
Created attachment 86723 [details] [review]
Bug 22531: Add logging & display of req partners

This patch adds:

- The ability to send request to partners multiple times.
- Logging of requested partners
- Display of requested partners in "ILL Request Log" display
Comment 2 Andrew Isherwood 2019-03-19 14:17:05 UTC
Created attachment 86744 [details] [review]
Bug 22531: Add unit tests

Add tests for new and modified methods
Comment 3 Andrew Isherwood 2019-03-19 14:25:27 UTC
Test plan:

- Ensure you have at least two "partners" set up. These are patrons in the category defined in <interlibrary_loans> -> <partner_code> in koha-conf.xml, the patrons must also have a primary email address defined.
- Create an ILL request using the FreeForm backend
- From the "Manage ILL request" screen, select "Place request with partners"
- In "Select partner libraries" choose one partner library, then click "Send email"
- From the "Manage ILL request" screen, select "ILL request log"
=> TEST: Observe that the status was changed *to* "Requested from partners (first_partner_email_address_here)
- From the "Manage ILL request" screen, select "Place request with partners"
- In "Select partner libraries" choose a different partner library, then click "Send email"
- From the "Manage ILL request" screen, select "ILL request log"
=> TEST: Observe that the status was changed *from* "Requested from partners (first_partner_email_address_here)
=> TEST: Observe that the status was changed *TO* "Requested from partners (second_partner_email_address_here)
Comment 4 Andrew Isherwood 2019-03-19 14:28:18 UTC
Test plan: (ignore the plan in comment #3)

- Ensure you have at least two "partners" set up. These are patrons in the category defined in <interlibrary_loans> -> <partner_code> in koha-conf.xml, the patrons must also have a primary email address defined.
- Create an ILL request using the FreeForm backend
- From the "Manage ILL request" screen, select "Place request with partners"
- In "Select partner libraries" choose one partner library, then click "Send email"
- Go to the "Manage ILL request" screen
=> TEST: Observe that the request "Status" is "Requested from partners : first_partner_email_address_here"
=> TEST: Observe that the first_partner_email_address links to the appropriate patron screen
- From the "Manage ILL request" screen, select "ILL request log"
=> TEST: Observe that the status was changed *to* "Requested from partners (first_partner_email_address_here)
- From the "Manage ILL request" screen, select "Place request with partners"
- In "Select partner libraries" choose a different partner library, then click "Send email"
- From the "Manage ILL request" screen, select "ILL request log"
=> TEST: Observe that the status was changed *from* "Requested from partners (first_partner_email_address_here)
=> TEST: Observe that the status was changed *TO* "Requested from partners (second_partner_email_address_here)
=> TEST: Observe that the email addresses displayed link to the appropriate patron page
Comment 5 Martin Renvoize 2021-03-19 14:37:20 UTC
Hmm, I had a go at rebasing this one.. and it didn't appear too bad.. but my rebased set had all the unit tests failing.

Going to ask Andrew for a rebase.
Comment 6 Andrew Isherwood 2021-03-26 10:47:52 UTC
Created attachment 118843 [details] [review]
Bug 22531: Add logging & display of req partners

This patch adds:

- The ability to send request to partners multiple times.
- Logging of requested partners
- Display of requested partners in "ILL Request Log" display
Comment 7 Andrew Isherwood 2021-03-26 10:47:55 UTC
Created attachment 118844 [details] [review]
Bug 22531: Add unit tests

Add tests for new and modified methods
Comment 8 Andrew Isherwood 2021-03-26 10:49:20 UTC
Revised test plan:

- Ensure you have at least two "partners" set up. These are patrons in the category defined in <interlibrary_loans> -> <partner_code> in koha-conf.xml, the patrons must also have a primary email address defined.
- Ensure the ILLLOG syspref is turned on
- Create an ILL request using the FreeForm backend
- From the "Manage ILL request" screen, select "Place request with partners"
- In "Select partner libraries" choose one partner library, then click "Send email"
- Go to the "Manage ILL request" screen
=> TEST: Observe that the request "Status" is "Requested from partners : first_partner_email_address_here"
=> TEST: Observe that the first_partner_email_address links to the appropriate patron screen
- From the "Manage ILL request" screen, select "ILL request log"
=> TEST: Observe that the status was changed *to* "Requested from partners (first_partner_email_address_here)
- From the "Manage ILL request" screen, select "Place request with partners"
- In "Select partner libraries" choose a different partner library, then click "Send email"
- From the "Manage ILL request" screen, select "ILL request log"
=> TEST: Observe that the status was changed *from* "Requested from partners (first_partner_email_address_here)
=> TEST: Observe that the status was changed *TO* "Requested from partners (second_partner_email_address_here)
=> TEST: Observe that the email addresses displayed link to the appropriate patron page
Comment 9 Martin Renvoize 2021-07-07 08:40:21 UTC
Created attachment 122643 [details] [review]
Bug 22531: Add logging & display of req partners

This patch adds:

- The ability to send request to partners multiple times.
- Logging of requested partners
- Display of requested partners in "ILL Request Log" display

Signed-ff-by: Barry Cannon <bc@interleaf.ie>
Comment 10 Martin Renvoize 2021-07-07 08:40:26 UTC
Created attachment 122644 [details] [review]
Bug 22531: Add unit tests

Add tests for new and modified methods

Signed-ff-by: Barry Cannon <bc@interleaf.ie>
Comment 11 Martin Renvoize 2021-07-07 08:41:21 UTC
Signed off by Barry Cannon on behalf of the Institutes of Technology, whom he supports.  Testing was undertaken at multiple sites and is now in live use.
Comment 12 Kyle M Hall 2021-09-10 15:25:18 UTC
Applying: Bug 22531: Add logging & display of req partners
Applying: Bug 22531: Add unit tests
error: sha1 information is lacking or useless (Koha/Illrequest/Logger.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 22531: Add unit tests
Comment 13 Andrew Isherwood 2021-09-17 11:30:22 UTC
Created attachment 124979 [details] [review]
Bug 22531: Add logging & display of req partners

This patch adds:

- The ability to send request to partners multiple times.
- Logging of requested partners
- Display of requested partners in "ILL Request Log" display

Signed-ff-by: Barry Cannon <bc@interleaf.ie>
Comment 14 Andrew Isherwood 2021-09-17 11:30:26 UTC
Created attachment 124980 [details] [review]
Bug 22531: Add unit tests

Add tests for new and modified methods

Signed-off-by: Barry Cannon <bc@interleaf.ie>
Comment 15 Katrin Fischer 2021-11-01 12:12:48 UTC
I am sorry, but I am having a bit of trouble with this one:

prove t/db_dependent/Illrequests.t
t/db_dependent/Illrequests.t .. 3/13 
    #   Failed test 'new node + core_status_graph = bigger status graph'
    #   at t/db_dependent/Illrequests.t line 272.
    # Compared array length of $data->{"GENREQ"}{"next_actions"}
    #    got : array with 3 element(s)
    # expect : array with 2 element(s)
    # {
    #   'CANCREQ' => {
    #     'id' => 'CANCREQ',
    #     'method' => 0,
    #     'name' => 'Cancellation requested',
    #     'next_actions' => [
    #       'KILL',
    #       'REQ'
    #     ],
    #     'prev_actions' => [
    #       'NEW'
    #     ],
    #     'ui_method_icon' => 0,
    #     'ui_method_name' => 0
    #   },
    #   'CHK' => {
    #     'id' => 'CHK',
    #     'method' => 'check_out',
    #     'name' => 'Checked out',
    #     'needs_all' => [
    #       sub { "DUMMY" }
    #     ],
    #     'needs_perms' => [
    #       'user_circulate_circulate_remaining_permissions'
    #     ],
    #     'needs_prefs' => [
    #       'CirculateILL'
    #     ],
    #     'next_actions' => [],
    #     'prev_actions' => [
    #       'REQ',
    #       'GENREQ',
    #       'COMP'
    #     ],
    #     'ui_method_icon' => 'fa-upload',
    #     'ui_method_name' => 'Check out'
    #   },
    #   'COMP' => {
    #     'id' => 'COMP',
    #     'method' => 'mark_completed',
    #     'name' => 'Completed',
    #     'next_actions' => [
    #       'CHK'
    #     ],
    #     'prev_actions' => [
    #       'REQ'
    #     ],
    #     'ui_method_icon' => 'fa-check',
    #     'ui_method_name' => 'Mark completed'
    #   },
    #   'GENREQ' => {
    #     'id' => 'GENREQ',
    #     'method' => 'generic_confirm',
    #     'name' => 'Requested from partners',
    #     'next_actions' => [
    #       'COMP',
    #       'CHK',
    #       'GENREQ'
    #     ],
    #     'prev_actions' => [
    #       'NEW',
    #       'REQREV',
    #       'GENREQ'
    #     ],
    #     'ui_method_icon' => 'fa-send-o',
    #     'ui_method_name' => 'Place request with partners'
    #   },
    #   'KILL' => {
    #     'id' => 'KILL',
    #     'method' => 'delete',
    #     'name' => 0,
    #     'next_actions' => [],
    #     'prev_actions' => [
    #       'QUEUED',
    #       'REQREV',
    #       'NEW',
    #       'CANCREQ'
    #     ],
    #     'ui_method_icon' => 'fa-trash',
    #     'ui_method_name' => 'Delete request'
    #   },
    #   'NEW' => {
    #     'id' => 'NEW',
    #     'method' => 'create',
    #     'name' => 'New request',
    #     'next_actions' => [
    #       'REQ',
    #       'GENREQ',
    #       'KILL'
    #     ],
    #     'prev_actions' => [],
    #     'ui_method_icon' => 'fa-plus',
    #     'ui_method_name' => 'New request'
    #   },
    #   'QUEUED' => {
    #     'id' => 'QUEUED',
    #     'method' => 0,
    #     'name' => 'Queued request',
    #     'next_actions' => [
    #       'REQ',
    #       'KILL'
    #     ],
    #     'prev_actions' => [],
    #     'ui_method_icon' => 0,
    #     'ui_method_name' => 0
    #   },
    #   'REQ' => {
    #     'id' => 'REQ',
    #     'method' => 'confirm',
    #     'name' => 'Requested',
    #     'next_actions' => [
    #       'REQREV',
    #       'COMP',
    #       'CHK'
    #     ],
    #     'prev_actions' => [
    #       'NEW',
    #       'REQREV',
    #       'QUEUED',
    #       'CANCREQ'
    #     ],
    #     'ui_method_icon' => 'fa-check',
    #     'ui_method_name' => 'Confirm request'
    #   },
    #   'REQREV' => {
    #     'id' => 'REQREV',
    #     'method' => 'cancel',
    #     'name' => 'Request reverted',
    #     'next_actions' => [
    #       'REQ',
    #       'GENREQ',
    #       'KILL'
    #     ],
    #     'prev_actions' => [
    #       'REQ'
    #     ],
    #     'ui_method_icon' => 'fa-times',
    #     'ui_method_name' => 'Revert Request'
    #   },
    #   'RET' => {
    #     'id' => 'RET',
    #     'method' => 'check_in',
    #     'name' => 'Returned to library',
    #     'next_actions' => [
    #       'COMP'
    #     ],
    #     'prev_actions' => [
    #       'CHK'
    #     ],
    #     'ui_method_icon' => 'fa-download',
    #     'ui_method_name' => 'Check in'
    #   },
    #   'TEST' => {
    #     'id' => 'TEST',
    #     'next_actions' => [],
    #     'prev_actions' => []
    #   }
    # }
    # Looks like you failed 1 test of 5.
t/db_dependent/Illrequests.t .. 5/13 
#   Failed test 'Status Graph tests'
#   at t/db_dependent/Illrequests.t line 380.
t/db_dependent/Illrequests.t .. 11/13     # Looks like you planned 3 tests but ran 5.

#   Failed test 'Custom statuses'
#   at t/db_dependent/Illrequests.t line 1343.
t/db_dependent/Illrequests.t .. 13/13 # Looks like you failed 2 tests of 13.
t/db_dependent/Illrequests.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/13 subtests 

Test Summary Report
-------------------
t/db_dependent/Illrequests.t (Wstat: 512 Tests: 13 Failed: 2)
  Failed tests:  5, 12
  Non-zero exit status: 2


There are also some minor QA fails (tab characters and missing TT filters). I would have fixed those, but holding off now as the tests might need a little work anyway.
Comment 16 Martin Renvoize 2022-03-02 10:44:34 UTC
Created attachment 131234 [details] [review]
Bug 22531: Add logging & display of req partners

This patch adds:

- The ability to send request to partners multiple times.
- Logging of requested partners
- Display of requested partners in "ILL Request Log" display

Signed-ff-by: Barry Cannon <bc@interleaf.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2022-03-02 10:44:38 UTC
Created attachment 131235 [details] [review]
Bug 22531: Add unit tests

Add tests for new and modified methods

Signed-off-by: Barry Cannon <bc@interleaf.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2022-03-02 10:44:42 UTC
Created attachment 131236 [details] [review]
Bug 22531: (QA follow-up) Fix filter and tab issues

Correction to template filters and replace tabs with spaces in code to
pass the QA script.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2022-03-02 10:44:45 UTC
Created attachment 131237 [details] [review]
Bug 22531: (QA follow-up) Fix Unit Tests

This patch series adds the ability to generate a partners request
multiple times from the same ILL request. It does this be adding the
'GENREQ' action to both 'prev_actions' and 'next_actions' in the
'GENREQ' status segment of the status graph.

This QA followup updates the status graph tests to reflect this change.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Katrin Fischer 2022-04-24 09:52:31 UTC
Created attachment 133708 [details] [review]
Bug 22531: Add logging & display of req partners

This patch adds:

- The ability to send request to partners multiple times.
- Logging of requested partners
- Display of requested partners in "ILL Request Log" display

Signed-ff-by: Barry Cannon <bc@interleaf.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2022-04-24 09:52:36 UTC
Created attachment 133709 [details] [review]
Bug 22531: Add unit tests

Add tests for new and modified methods

Signed-off-by: Barry Cannon <bc@interleaf.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Katrin Fischer 2022-04-24 09:52:41 UTC
Created attachment 133710 [details] [review]
Bug 22531: (QA follow-up) Fix filter and tab issues

Correction to template filters and replace tabs with spaces in code to
pass the QA script.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2022-04-24 09:52:46 UTC
Created attachment 133711 [details] [review]
Bug 22531: (QA follow-up) Fix Unit Tests

This patch series adds the ability to generate a partners request
multiple times from the same ILL request. It does this be adding the
'GENREQ' action to both 'prev_actions' and 'next_actions' in the
'GENREQ' status segment of the status graph.

This QA followup updates the status graph tests to reflect this change.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Tomás Cohen Arazi 2022-04-24 18:45:39 UTC
I like the idea. Isn't this a bit hacky?
Comment 25 Andrew Isherwood 2022-04-25 08:13:25 UTC
(In reply to Tomás Cohen Arazi from comment #24)
> I like the idea. Isn't this a bit hacky?

Possibly, as I noted in the patch, I wasn't 100% happy with the solution. I can't remember any of the thought processes involved when I did it, it was written over three years ago, but I'm sure the decisions I made were made for good reasons.

I'm not inclined to revisit it now, it has been in use by multiple customers over the past three years with no issues. It would be really nice to see it finally merged after all this time.
Comment 26 Tomás Cohen Arazi 2022-09-23 12:35:57 UTC
Please rebase. Doesn't seem to apply after I pushed 30484
Comment 27 Katrin Fischer 2023-01-13 11:12:20 UTC
This was marked rel_22_11_candidate, but currently has no assignee. 

Martin, since it sounds like it was needed for some of your customers, do you plan to move forward with it?
Comment 28 Pedro Amorim 2023-01-27 12:00:31 UTC Comment hidden (obsolete)
Comment 29 Pedro Amorim 2023-01-27 12:37:22 UTC
Created attachment 145727 [details] [review]
Bug 22531: Add logging & display of req partners

This patch adds:

- The ability to send request to partners multiple times.
- Logging of requested partners
- Display of requested partners in "ILL Request Log" display

Sponsored-by: UKHSA

Signed-ff-by: Barry Cannon <bc@interleaf.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Pedro Amorim 2023-01-27 12:37:27 UTC
Created attachment 145728 [details] [review]
Bug 22531: Add unit tests

Add tests for new and modified methods

Signed-off-by: Barry Cannon <bc@interleaf.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Pedro Amorim 2023-01-27 12:37:32 UTC
Created attachment 145729 [details] [review]
Bug 22531: (QA follow-up) Fix filter and tab issues

Correction to template filters and replace tabs with spaces in code to
pass the QA script.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 32 Pedro Amorim 2023-01-27 12:37:38 UTC
Created attachment 145730 [details] [review]
Bug 22531: (QA follow-up) Fix Unit Tests

This patch series adds the ability to generate a partners request
multiple times from the same ILL request. It does this be adding the
'GENREQ' action to both 'prev_actions' and 'next_actions' in the
'GENREQ' status segment of the status graph.

This QA followup updates the status graph tests to reflect this change.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 33 Nick Clemens (kidclamp) 2023-03-30 13:06:59 UTC
Reading through the code here, my best guess is that many decisions were made in the interest of preserving functionality of existing backends, which I think can be fine, but a few things I would like to see comments, maybe FIXMEs for:
1 - Accepting either a hash or a scalar seems a bit odd, at leats an explanation would help - probably a FIXME
2 - Retrieving ILL partner patrons using a string of emails seems possibly dangerous. I imagine past logs contain only these. If going forward we could use borrowernumbers or something else, that would be ideal. If that would break existing backends an explanation would be nice
3 - Some function names and variables could be made clearer, i.e.:

[% FOREACH add IN log.info.additional.partner_email_previous.split('; ') %]
add could be 'additional_borrower'

Koha::Illrequest::Logger->logged_requested_partners($log);
maybe 'return_requested_partners'? the current seems unclear

I think with some clarity and cleanup this one can move forward