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
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
Created attachment 86744 [details] [review] Bug 22531: Add unit tests Add tests for new and modified methods
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)
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
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.
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
Created attachment 118844 [details] [review] Bug 22531: Add unit tests Add tests for new and modified methods
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
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>
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>
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.
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
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>
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>
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.
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>
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>
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>
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>
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>
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>
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>
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>
I like the idea. Isn't this a bit hacky?
(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.
Please rebase. Doesn't seem to apply after I pushed 30484
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?
Created attachment 145726 [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>
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>
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>
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>
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>
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