A workflow stage that performs a check on whether an ILL request being placed has already been placed in the past or not.
Created attachment 174604 [details] [review] Bug 38441: New 'ILLHistoryCheck' system preference
Created attachment 174605 [details] [review] Bug 38441: atomicupdate
Created attachment 174606 [details] [review] Bug 38441: HistoryCheck Koha class
Created attachment 174607 [details] [review] Bug 38441: ILLHistoryCheck: Staff UI
Created attachment 174608 [details] [review] Bug 38441: ILLHistoryCheck: OPAC
Created attachment 174609 [details] [review] Bug 38441: Allow / render HTML in ILL staff notes
Created attachment 174610 [details] [review] Bug 38441: Style updates for staffnotes - Use pre for staffnotes in staff illview - Ensure <pre> has same font-family as rest of the page - Render html for staff notes in list. Truncate to 100 chars max
Created attachment 174611 [details] [review] Bug 38441: Add tests Test plan: 1) Apply patches 2) Run (ignore if using a sandbox): $ reset_all 3) Visit ILL system preferences: <staff_url>/cgi-bin/koha/admin/preferences.pl?tab=interlibrary_loans 4) Enable ILLModule and ILLHistoryCheck system preferences -- STAFF testing -- 5) Create a new ILL request, visit: <staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=Standard 6) Pick type = 'Journal article', enter '123' for ISSN, '42' for cardnumber and any library. Click create. 7) Repeat 5) and 6). Notice you're now on the 'Request history check' screen. Verify your previous ILL request is listed. Click 'Submit anyway'. 8) Repeat 5) and 6). But pick a different patron cardnumber this time e.g. '23529000035676'. 9) Verify you're shown the 'Request history check' screen listing the 2 previously created requests. Click 'Submit anyway'. 10) Repeat 5 and 6). Use cardnumber '23529000035676' again. 11) Verify you're shown the 'Request history check' screen. The listing now shows the requests found for this patron first, then lists the remaining found requests for other patrons. -- OPAC testing -- 12) Place a new request through the OPAC, visit: <opac_url>/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard 13) Pick type = 'Journal article', enter '123' for ISSN. Click 'Create'. Notice it shows the 'history check' screen listing the existing 2 requests. Click 'Submit anyway'. 14) Return to the Staff UI: <staff_url>/cgi-bin/koha/ill/ill-requests.pl 15) Click on the latest ILL request. Verify 'Staff notes' contain information relating to already existing ILL requests. -- Bonus testing -- 16) On the Staff UI, create a new request for cardnumber 23529000035676 with ISSN = 321 17) On the OPAC, logged in as koha:koha, place a request with ISSN = 321. Verify you don't get shown the history check but the staff notes is still added 18) Play around with ILLModuleDisclaimerByType and ILLOpacUnauthenticatedRequest (easy) 19) Play around with ILLCheckAvailability and AutoILLBackendPriority (reach out for help using these preferences) Run tests: $ prove t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t
Created attachment 175300 [details] [review] Bug 38441: New 'ILLHistoryCheck' system preference
Created attachment 175301 [details] [review] Bug 38441: atomicupdate
Created attachment 175302 [details] [review] Bug 38441: HistoryCheck Koha class
Created attachment 175303 [details] [review] Bug 38441: ILLHistoryCheck: Staff UI
Created attachment 175304 [details] [review] Bug 38441: ILLHistoryCheck: OPAC
Created attachment 175305 [details] [review] Bug 38441: Allow / render HTML in ILL staff notes
Created attachment 175306 [details] [review] Bug 38441: Style updates for staffnotes - Use pre for staffnotes in staff illview - Ensure <pre> has same font-family as rest of the page - Render html for staff notes in list. Truncate to 100 chars max
Created attachment 175307 [details] [review] Bug 38441: Add tests Test plan: 1) Apply patches 2) Run (ignore if using a sandbox): $ reset_all 3) Visit ILL system preferences: <staff_url>/cgi-bin/koha/admin/preferences.pl?tab=interlibrary_loans 4) Enable ILLModule and ILLHistoryCheck system preferences -- STAFF testing -- 5) Create a new ILL request, visit: <staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=Standard 6) Pick type = 'Journal article', enter '123' for ISSN, '42' for cardnumber and any library. Click create. 7) Repeat 5) and 6). Notice you're now on the 'Request history check' screen. Verify your previous ILL request is listed. Click 'Submit anyway'. 8) Repeat 5) and 6). But pick a different patron cardnumber this time e.g. '23529000035676'. 9) Verify you're shown the 'Request history check' screen listing the 2 previously created requests. Click 'Submit anyway'. 10) Repeat 5 and 6). Use cardnumber '23529000035676' again. 11) Verify you're shown the 'Request history check' screen. The listing now shows the requests found for this patron first, then lists the remaining found requests for other patrons. -- OPAC testing -- 12) Place a new request through the OPAC, visit: <opac_url>/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard 13) Pick type = 'Journal article', enter '123' for ISSN. Click 'Create'. Notice it shows the 'history check' screen listing the existing 2 requests. Click 'Submit anyway'. 14) Return to the Staff UI: <staff_url>/cgi-bin/koha/ill/ill-requests.pl 15) Click on the latest ILL request. Verify 'Staff notes' contain information relating to already existing ILL requests. -- Bonus testing -- 16) On the Staff UI, create a new request for cardnumber 23529000035676 with ISSN = 321 17) On the OPAC, logged in as koha:koha, place a request with ISSN = 321. Verify you don't get shown the history check but the staff notes is still added 18) Play around with ILLModuleDisclaimerByType and ILLOpacUnauthenticatedRequest (easy) 19) Play around with ILLCheckAvailability and AutoILLBackendPriority (reach out for help using these preferences) Run tests: $ prove t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t
Created attachment 176507 [details] [review] Bug 38441: New 'ILLHistoryCheck' system preference
Created attachment 176508 [details] [review] Bug 38441: atomicupdate
Created attachment 176509 [details] [review] Bug 38441: HistoryCheck Koha class
Created attachment 176510 [details] [review] Bug 38441: ILLHistoryCheck: Staff UI
Created attachment 176511 [details] [review] Bug 38441: ILLHistoryCheck: OPAC
Created attachment 176512 [details] [review] Bug 38441: Allow / render HTML in ILL staff notes
Created attachment 176513 [details] [review] Bug 38441: Style updates for staffnotes - Use pre for staffnotes in staff illview - Ensure <pre> has same font-family as rest of the page - Render html for staff notes in list. Truncate to 100 chars max
Created attachment 176514 [details] [review] Bug 38441: Add tests Test plan: 1) Apply patches 2) Run (ignore if using a sandbox): $ reset_all 3) Visit ILL system preferences: <staff_url>/cgi-bin/koha/admin/preferences.pl?tab=interlibrary_loans 4) Enable ILLModule and ILLHistoryCheck system preferences -- STAFF testing -- 5) Create a new ILL request, visit: <staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=Standard 6) Pick type = 'Journal article', enter '123' for ISSN, '42' for cardnumber and any library. Click create. 7) Repeat 5) and 6). Notice you're now on the 'Request history check' screen. Verify your previous ILL request is listed. Click 'Submit anyway'. 8) Repeat 5) and 6). But pick a different patron cardnumber this time e.g. '23529000035676'. 9) Verify you're shown the 'Request history check' screen listing the 2 previously created requests. Click 'Submit anyway'. 10) Repeat 5 and 6). Use cardnumber '23529000035676' again. 11) Verify you're shown the 'Request history check' screen. The listing now shows the requests found for this patron first, then lists the remaining found requests for other patrons. -- OPAC testing -- 12) Place a new request through the OPAC, visit: <opac_url>/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard 13) Pick type = 'Journal article', enter '123' for ISSN. Click 'Create'. Notice it shows the 'history check' screen listing the existing 2 requests. Click 'Submit anyway'. 14) Return to the Staff UI: <staff_url>/cgi-bin/koha/ill/ill-requests.pl 15) Click on the latest ILL request. Verify 'Staff notes' contain information relating to already existing ILL requests. -- Bonus testing -- 16) On the Staff UI, create a new request for cardnumber 23529000035676 with ISSN = 321 17) On the OPAC, logged in as koha:koha, place a request with ISSN = 321. Verify you don't get shown the history check but the staff notes is still added 18) Play around with ILLModuleDisclaimerByType and ILLOpacUnauthenticatedRequest (easy) 19) Play around with ILLCheckAvailability and AutoILLBackendPriority (reach out for help using these preferences) Run tests: $ prove t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t
Rebased to make use of bug 38685
Created attachment 176561 [details] [review] Bug 38441: (QA follow-up): Handle undef userenv C4::Context->userenv may be undef (after a reset_all, trying to place an unauthenticated request without being logged). This change prevents a nasty 500 error trying use undefined as hash
Created attachment 176566 [details] [review] Bug 38441: (QA follow-up): Fix find-misplaced-executables.t It appears the atomic update had wrong permissions: prove xt/find-misplaced-executables.t
Created attachment 176573 [details] [review] Bug 38441: (QA follow-up): Fix find-missing-filters.t prove xt/find-missing-filters.t
Created attachment 176829 [details] [review] Bug 38441: (QA follow-up): Fix query Only add query fields if they're defined.
Created attachment 179800 [details] [review] Bug 38441: New 'ILLHistoryCheck' system preference
Created attachment 179801 [details] [review] Bug 38441: atomicupdate
Created attachment 179802 [details] [review] Bug 38441: HistoryCheck Koha class
Created attachment 179803 [details] [review] Bug 38441: ILLHistoryCheck: Staff UI
Created attachment 179804 [details] [review] Bug 38441: ILLHistoryCheck: OPAC
Created attachment 179805 [details] [review] Bug 38441: Allow / render HTML in ILL staff notes
Created attachment 179806 [details] [review] Bug 38441: Style updates for staffnotes - Use pre for staffnotes in staff illview - Ensure <pre> has same font-family as rest of the page - Render html for staff notes in list. Truncate to 100 chars max
Created attachment 179807 [details] [review] Bug 38441: Add tests Test plan: 1) Apply patches 2) Run (ignore if using a sandbox): $ reset_all 3) Visit ILL system preferences: <staff_url>/cgi-bin/koha/admin/preferences.pl?tab=interlibrary_loans 4) Enable ILLModule and ILLHistoryCheck system preferences -- STAFF testing -- 5) Create a new ILL request, visit: <staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=Standard 6) Pick type = 'Journal article', enter '123' for ISSN, '42' for cardnumber and any library. Click create. 7) Repeat 5) and 6). Notice you're now on the 'Request history check' screen. Verify your previous ILL request is listed. Click 'Submit anyway'. 8) Repeat 5) and 6). But pick a different patron cardnumber this time e.g. '23529000035676'. 9) Verify you're shown the 'Request history check' screen listing the 2 previously created requests. Click 'Submit anyway'. 10) Repeat 5 and 6). Use cardnumber '23529000035676' again. 11) Verify you're shown the 'Request history check' screen. The listing now shows the requests found for this patron first, then lists the remaining found requests for other patrons. -- OPAC testing -- 12) Place a new request through the OPAC, visit: <opac_url>/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard 13) Pick type = 'Journal article', enter '123' for ISSN. Click 'Create'. Notice it shows the 'history check' screen listing the existing 2 requests. Click 'Submit anyway'. 14) Return to the Staff UI: <staff_url>/cgi-bin/koha/ill/ill-requests.pl 15) Click on the latest ILL request. Verify 'Staff notes' contain information relating to already existing ILL requests. -- Bonus testing -- 16) On the Staff UI, create a new request for cardnumber 23529000035676 with ISSN = 321 17) On the OPAC, logged in as koha:koha, place a request with ISSN = 321. Verify you don't get shown the history check but the staff notes is still added 18) Play around with ILLModuleDisclaimerByType and ILLOpacUnauthenticatedRequest (easy) 19) Play around with ILLCheckAvailability and AutoILLBackendPriority (reach out for help using these preferences) Run tests: $ prove t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t
Created attachment 179808 [details] [review] Bug 38441: (QA follow-up): Handle undef userenv C4::Context->userenv may be undef (after a reset_all, trying to place an unauthenticated request without being logged). This change prevents a nasty 500 error trying use undefined as hash
Created attachment 179809 [details] [review] Bug 38441: (QA follow-up): Fix find-misplaced-executables.t It appears the atomic update had wrong permissions: prove xt/find-misplaced-executables.t
Created attachment 179810 [details] [review] Bug 38441: (QA follow-up): Fix find-missing-filters.t prove xt/find-missing-filters.t
Created attachment 179811 [details] [review] Bug 38441: (QA follow-up): HistoryCheck now only considers same patron Initial implementation checked existing ILLs for same patron and other patrons. User testing reported ILLs for other patrons are not helpful, and makes this stage screen show up much more frequently than it should. This patch simplifies the original implementation to only consider the patron of the request being placed for an ILL history check. It also changes the ID matching to only 'DOI' or only 'Pubmed ID' if any of those is set. This is to minimize scenarios where different articles from the same journal come up as matching results e.g. same ISSN but different DOI. It is also updating tests accordingly. Other QA improvements Improve HistoryCheck request list: - Make link external and show icon (to prevent staff member from clicking away from the historyCheck screen unintentionally) - Show requests' status - Show requests' placed date - Add id_prefix to request ID if possible
Rebased and tidied on top of tidied bug 36197 and bug 38685
Created attachment 180034 [details] [review] Bug 38441: New 'ILLHistoryCheck' system preference Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180035 [details] [review] Bug 38441: atomicupdate Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180036 [details] [review] Bug 38441: HistoryCheck Koha class Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180037 [details] [review] Bug 38441: ILLHistoryCheck: Staff UI Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180038 [details] [review] Bug 38441: ILLHistoryCheck: OPAC Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180039 [details] [review] Bug 38441: Allow / render HTML in ILL staff notes Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180040 [details] [review] Bug 38441: Style updates for staffnotes - Use pre for staffnotes in staff illview - Ensure <pre> has same font-family as rest of the page - Render html for staff notes in list. Truncate to 100 chars max Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180041 [details] [review] Bug 38441: Add tests Test plan: 1) Apply patches 2) Run (ignore if using a sandbox): $ reset_all 3) Visit ILL system preferences: <staff_url>/cgi-bin/koha/admin/preferences.pl?tab=interlibrary_loans 4) Enable ILLModule and ILLHistoryCheck system preferences -- STAFF testing -- 5) Create a new ILL request, visit: <staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=Standard 6) Pick type = 'Journal article', enter '123' for ISSN, '42' for cardnumber and any library. Click create. 7) Repeat 5) and 6). Notice you're now on the 'Request history check' screen. Verify your previous ILL request is listed. Click 'Submit anyway'. 8) Repeat 5) and 6). But pick a different patron cardnumber this time e.g. '23529000035676'. 9) Verify you're shown the 'Request history check' screen listing the 2 previously created requests. Click 'Submit anyway'. 10) Repeat 5 and 6). Use cardnumber '23529000035676' again. 11) Verify you're shown the 'Request history check' screen. The listing now shows the requests found for this patron first, then lists the remaining found requests for other patrons. -- OPAC testing -- 12) Place a new request through the OPAC, visit: <opac_url>/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard 13) Pick type = 'Journal article', enter '123' for ISSN. Click 'Create'. Notice it shows the 'history check' screen listing the existing 2 requests. Click 'Submit anyway'. 14) Return to the Staff UI: <staff_url>/cgi-bin/koha/ill/ill-requests.pl 15) Click on the latest ILL request. Verify 'Staff notes' contain information relating to already existing ILL requests. -- Bonus testing -- 16) On the Staff UI, create a new request for cardnumber 23529000035676 with ISSN = 321 17) On the OPAC, logged in as koha:koha, place a request with ISSN = 321. Verify you don't get shown the history check but the staff notes is still added 18) Play around with ILLModuleDisclaimerByType and ILLOpacUnauthenticatedRequest (easy) 19) Play around with ILLCheckAvailability and AutoILLBackendPriority (reach out for help using these preferences) Run tests: $ prove t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180042 [details] [review] Bug 38441: (QA follow-up): Handle undef userenv C4::Context->userenv may be undef (after a reset_all, trying to place an unauthenticated request without being logged). This change prevents a nasty 500 error trying use undefined as hash Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180043 [details] [review] Bug 38441: (QA follow-up): Fix find-misplaced-executables.t It appears the atomic update had wrong permissions: prove xt/find-misplaced-executables.t Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180044 [details] [review] Bug 38441: (QA follow-up): Fix find-missing-filters.t prove xt/find-missing-filters.t Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180045 [details] [review] Bug 38441: (QA follow-up): HistoryCheck now only considers same patron Initial implementation checked existing ILLs for same patron and other patrons. User testing reported ILLs for other patrons are not helpful, and makes this stage screen show up much more frequently than it should. This patch simplifies the original implementation to only consider the patron of the request being placed for an ILL history check. It also changes the ID matching to only 'DOI' or only 'Pubmed ID' if any of those is set. This is to minimize scenarios where different articles from the same journal come up as matching results e.g. same ISSN but different DOI. It is also updating tests accordingly. Other QA improvements Improve HistoryCheck request list: - Make link external and show icon (to prevent staff member from clicking away from the historyCheck screen unintentionally) - Show requests' status - Show requests' placed date - Add id_prefix to request ID if possible Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
TODOs from hackfest QA session: 1) HistoryCheck matched requests: Properly format placed date with Koha date tt filter for history check 2) HistoryCheck matched requests: Details such as branchcode, cardnumber backend need to be capitalized and properly named 3) HistoryCheck matched requests: OPAC and Staff UI should be identical (same data presented in the same way) 4) HistoryCheck matched requests: Show the request status description, not the status code 5) Remove ISSN check from history check matching logic 6) Change UI message "been placed by this patron koha (42) :" to "been placed by koha (42) :" 7) Change UI message "if an ILL request has already been placed during the request process." to "if an ILL request has already been placed by this patron." 8) Change UI message "previous requests" information from staff notes into 'previous_requests' pipe-separated list of ids
Created attachment 180907 [details] [review] Bug 38441: New 'ILLHistoryCheck' system preference Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180908 [details] [review] Bug 38441: atomicupdate Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180909 [details] [review] Bug 38441: HistoryCheck Koha class Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180910 [details] [review] Bug 38441: ILLHistoryCheck: Staff UI Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180911 [details] [review] Bug 38441: ILLHistoryCheck: OPAC Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180912 [details] [review] Bug 38441: Allow / render HTML in ILL staff notes Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180913 [details] [review] Bug 38441: Style updates for staffnotes - Use pre for staffnotes in staff illview - Ensure <pre> has same font-family as rest of the page - Render html for staff notes in list. Truncate to 100 chars max Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180914 [details] [review] Bug 38441: Add tests Test plan: 1) Apply patches 2) Run (ignore if using a sandbox): $ reset_all 3) Visit ILL system preferences: <staff_url>/cgi-bin/koha/admin/preferences.pl?tab=interlibrary_loans 4) Enable ILLModule and ILLHistoryCheck system preferences -- STAFF testing -- 5) Create a new ILL request, visit: <staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=Standard 6) Pick type = 'Journal article', enter '123' for DOI, '42' for cardnumber and any library. Click create. 7) Repeat 5) and 6). Notice you're now on the 'Request history check' screen. Verify your previous ILL request is listed. Click 'Submit anyway'. 8) Repeat 5) and 6). But pick a different patron cardnumber this time e.g. '23529000035676'. 9) Verify you're not shown the 'Request history check' screen as its a different patron. -- OPAC testing -- 10) Place a new request through the OPAC, visit: <opac_url>/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard 11) Pick type = 'Journal article', enter '123' for DOI. Click 'Create'. Notice it shows the 'history check' screen listing the existing requests. Click 'Submit anyway'. 12) Return to the Staff UI: <staff_url>/cgi-bin/koha/ill/ill-requests.pl 13) Click on the latest ILL request. Verify 'History check requests' is displayed and contains information relating to the already existing ILL requests. -- Bonus testing -- 16) Play around with ILLModuleDisclaimerByType and ILLOpacUnauthenticatedRequest enabled at the same time as HistoryCheck (easy) 17) Play around with ILLCheckAvailability and AutoILLBackendPriority enabled at the same time as HistoryCheck (reach out for help using these preferences) Run tests: $ prove t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180915 [details] [review] Bug 38441: (QA follow-up): Handle undef userenv C4::Context->userenv may be undef (after a reset_all, trying to place an unauthenticated request without being logged). This change prevents a nasty 500 error trying use undefined as hash Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180916 [details] [review] Bug 38441: (QA follow-up): Fix find-misplaced-executables.t It appears the atomic update had wrong permissions: prove xt/find-misplaced-executables.t Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180917 [details] [review] Bug 38441: (QA follow-up): Fix find-missing-filters.t prove xt/find-missing-filters.t Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180918 [details] [review] Bug 38441: (QA follow-up): HistoryCheck now only considers same patron Initial implementation checked existing ILLs for same patron and other patrons. User testing reported ILLs for other patrons are not helpful, and makes this stage screen show up much more frequently than it should. This patch simplifies the original implementation to only consider the patron of the request being placed for an ILL history check. It also changes the ID matching to only 'DOI' or only 'Pubmed ID' if any of those is set. This is to minimize scenarios where different articles from the same journal come up as matching results e.g. same ISSN but different DOI. It is also updating tests accordingly. Other QA improvements Improve HistoryCheck request list: - Make link external and show icon (to prevent staff member from clicking away from the historyCheck screen unintentionally) - Show requests' status - Show requests' placed date - Add id_prefix to request ID if possible Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 180919 [details] [review] Bug 38441: (QA follow-up): Hackfest feedback This commit addresses all Hackfest QA related feedback: 1) Update of all strings that required update, including UI messages and sys pref description 2) Moved the history check information from staffnotes to its own pipe-separated illrequestattribute 'historycheck_requests' 3) Fix display of ILL request information on the history check screen (Staff), including proper label naming (e.g. branchcode -> library), show status description instead of code, uppercase labels. 4) Updated OPAC display to match Staff UI's display of matching history requests 5) Removed ISSN criteria for matching requests check 6) Update tests accordingly
Hi guys thank you for your patience here. I've now rebased the patch set on top of the updated bug 36197 and bug 38685 and followed up with all the QA feedback provided at Hackfest. Thank you in advance for the time spent here.
Created attachment 181072 [details] [review] Bug 38441: (QA follow-up): Only look for matchin requests if patron Previous commit 'HistoryCheck now only considers same patron' added the following regression: HistoryCheck is performed and shown in the OPAC even if no user is authenticated (using ILLOpacUnauthenticatedRequest). This fixes that. History check is skipped unless a user exists.
Hi, it all looks great. Can you check this failing test: ```shell kohadev-koha@kohadevbox:koha(qa_38441)$ prove t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t .. 1/2 # Looks like you planned 2 tests but ran 1. # Failed test 'after_request_created' # at t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t line 188. Can't call method "value" on an undefined value at t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t line 182. # Looks like your test exited with 11 just after 2. t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t .. Dubious, test returned 11 (wstat 2816, 0xb00) Failed 1/2 subtests Test Summary Report ------------------- t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t (Wstat: 2816 (exited 11) Tests: 2 Failed: 1) Failed test: 2 Non-zero exit status: 11 Files=1, Tests=2, 2 wallclock secs ( 0.01 usr 0.02 sys + 1.19 cusr 0.62 csys = 1.84 CPU) Result: FAIL ```
Created attachment 181111 [details] [review] Bug 38441: (QA follow-up): Only look for matchin requests if patron Previous commit 'HistoryCheck now only considers same patron' added the following regression: HistoryCheck is performed and shown in the OPAC even if no user is authenticated (using ILLOpacUnauthenticatedRequest). This fixes that. History check is skipped unless a user exists.
(In reply to Tomás Cohen Arazi (tcohen) from comment #71) > Hi, it all looks great. Can you check this failing test: Thank you Tomas, my last patch fixed a bug but made it evident that some of the tests were not enforcing the presence of a cardnumber (which the bugfix now checks). I've squashed the update to the tests to the same patch "Only look for matchin requests if patron" and resubmitted.
Created attachment 181112 [details] [review] Bug 38441: New 'ILLHistoryCheck' system preference Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181113 [details] [review] Bug 38441: atomicupdate Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181114 [details] [review] Bug 38441: HistoryCheck Koha class Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181115 [details] [review] Bug 38441: ILLHistoryCheck: Staff UI Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181116 [details] [review] Bug 38441: ILLHistoryCheck: OPAC Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181117 [details] [review] Bug 38441: Allow / render HTML in ILL staff notes Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181118 [details] [review] Bug 38441: Style updates for staffnotes - Use pre for staffnotes in staff illview - Ensure <pre> has same font-family as rest of the page - Render html for staff notes in list. Truncate to 100 chars max Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181119 [details] [review] Bug 38441: Add tests Test plan: 1) Apply patches 2) Run (ignore if using a sandbox): $ reset_all 3) Visit ILL system preferences: <staff_url>/cgi-bin/koha/admin/preferences.pl?tab=interlibrary_loans 4) Enable ILLModule and ILLHistoryCheck system preferences -- STAFF testing -- 5) Create a new ILL request, visit: <staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=Standard 6) Pick type = 'Journal article', enter '123' for DOI, '42' for cardnumber and any library. Click create. 7) Repeat 5) and 6). Notice you're now on the 'Request history check' screen. Verify your previous ILL request is listed. Click 'Submit anyway'. 8) Repeat 5) and 6). But pick a different patron cardnumber this time e.g. '23529000035676'. 9) Verify you're not shown the 'Request history check' screen as its a different patron. -- OPAC testing -- 10) Place a new request through the OPAC, visit: <opac_url>/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard 11) Pick type = 'Journal article', enter '123' for DOI. Click 'Create'. Notice it shows the 'history check' screen listing the existing requests. Click 'Submit anyway'. 12) Return to the Staff UI: <staff_url>/cgi-bin/koha/ill/ill-requests.pl 13) Click on the latest ILL request. Verify 'History check requests' is displayed and contains information relating to the already existing ILL requests. -- Bonus testing -- 16) Play around with ILLModuleDisclaimerByType and ILLOpacUnauthenticatedRequest enabled at the same time as HistoryCheck (easy) 17) Play around with ILLCheckAvailability and AutoILLBackendPriority enabled at the same time as HistoryCheck (reach out for help using these preferences) Run tests: $ prove t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181120 [details] [review] Bug 38441: (QA follow-up): Handle undef userenv C4::Context->userenv may be undef (after a reset_all, trying to place an unauthenticated request without being logged). This change prevents a nasty 500 error trying use undefined as hash Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181121 [details] [review] Bug 38441: (QA follow-up): Fix find-misplaced-executables.t It appears the atomic update had wrong permissions: prove xt/find-misplaced-executables.t Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181122 [details] [review] Bug 38441: (QA follow-up): Fix find-missing-filters.t prove xt/find-missing-filters.t Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181123 [details] [review] Bug 38441: (QA follow-up): HistoryCheck now only considers same patron Initial implementation checked existing ILLs for same patron and other patrons. User testing reported ILLs for other patrons are not helpful, and makes this stage screen show up much more frequently than it should. This patch simplifies the original implementation to only consider the patron of the request being placed for an ILL history check. It also changes the ID matching to only 'DOI' or only 'Pubmed ID' if any of those is set. This is to minimize scenarios where different articles from the same journal come up as matching results e.g. same ISSN but different DOI. It is also updating tests accordingly. Other QA improvements Improve HistoryCheck request list: - Make link external and show icon (to prevent staff member from clicking away from the historyCheck screen unintentionally) - Show requests' status - Show requests' placed date - Add id_prefix to request ID if possible Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181124 [details] [review] Bug 38441: (QA follow-up): Hackfest feedback This commit addresses all Hackfest QA related feedback: 1) Update of all strings that required update, including UI messages and sys pref description 2) Moved the history check information from staffnotes to its own pipe-separated illrequestattribute 'historycheck_requests' 3) Fix display of ILL request information on the history check screen (Staff), including proper label naming (e.g. branchcode -> library), show status description instead of code, uppercase labels. 4) Updated OPAC display to match Staff UI's display of matching history requests 5) Removed ISSN criteria for matching requests check 6) Update tests accordingly Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181125 [details] [review] Bug 38441: (QA follow-up): Only look for matchin requests if patron Previous commit 'HistoryCheck now only considers same patron' added the following regression: HistoryCheck is performed and shown in the OPAC even if no user is authenticated (using ILLOpacUnauthenticatedRequest). This fixes that. History check is skipped unless a user exists. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181126 [details] [review] Bug 38441: (QA follow-up) Minor change Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Tried to apply this on top of 36197, but got: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 38441: New 'ILLHistoryCheck' system preference Applying: Bug 38441: atomicupdate Applying: Bug 38441: HistoryCheck Koha class Applying: Bug 38441: ILLHistoryCheck: Staff UI error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt). error: could not build fake ancestor Patch failed at 0001 Bug 38441: ILLHistoryCheck: Staff UI hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-38441-ILLHistoryCheck-Staff-UI-3hshu7iy.patch Can you please rebase ASAP?
Created attachment 181593 [details] [review] Bug 38441: New 'ILLHistoryCheck' system preference Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181594 [details] [review] Bug 38441: atomicupdate Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181595 [details] [review] Bug 38441: HistoryCheck Koha class Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181596 [details] [review] Bug 38441: ILLHistoryCheck: Staff UI Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181597 [details] [review] Bug 38441: ILLHistoryCheck: OPAC Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181598 [details] [review] Bug 38441: Allow / render HTML in ILL staff notes Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181599 [details] [review] Bug 38441: Style updates for staffnotes - Use pre for staffnotes in staff illview - Ensure <pre> has same font-family as rest of the page - Render html for staff notes in list. Truncate to 100 chars max Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181600 [details] [review] Bug 38441: Add tests Test plan: 1) Apply patches 2) Run (ignore if using a sandbox): $ reset_all 3) Visit ILL system preferences: <staff_url>/cgi-bin/koha/admin/preferences.pl?tab=interlibrary_loans 4) Enable ILLModule and ILLHistoryCheck system preferences -- STAFF testing -- 5) Create a new ILL request, visit: <staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=Standard 6) Pick type = 'Journal article', enter '123' for DOI, '42' for cardnumber and any library. Click create. 7) Repeat 5) and 6). Notice you're now on the 'Request history check' screen. Verify your previous ILL request is listed. Click 'Submit anyway'. 8) Repeat 5) and 6). But pick a different patron cardnumber this time e.g. '23529000035676'. 9) Verify you're not shown the 'Request history check' screen as its a different patron. -- OPAC testing -- 10) Place a new request through the OPAC, visit: <opac_url>/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard 11) Pick type = 'Journal article', enter '123' for DOI. Click 'Create'. Notice it shows the 'history check' screen listing the existing requests. Click 'Submit anyway'. 12) Return to the Staff UI: <staff_url>/cgi-bin/koha/ill/ill-requests.pl 13) Click on the latest ILL request. Verify 'History check requests' is displayed and contains information relating to the already existing ILL requests. -- Bonus testing -- 16) Play around with ILLModuleDisclaimerByType and ILLOpacUnauthenticatedRequest enabled at the same time as HistoryCheck (easy) 17) Play around with ILLCheckAvailability and AutoILLBackendPriority enabled at the same time as HistoryCheck (reach out for help using these preferences) Run tests: $ prove t/db_dependent/Koha/ILL/Request/Workflow/HistoryCheck.t Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181601 [details] [review] Bug 38441: (QA follow-up): Handle undef userenv C4::Context->userenv may be undef (after a reset_all, trying to place an unauthenticated request without being logged). This change prevents a nasty 500 error trying use undefined as hash Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181602 [details] [review] Bug 38441: (QA follow-up): Fix find-misplaced-executables.t It appears the atomic update had wrong permissions: prove xt/find-misplaced-executables.t Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181603 [details] [review] Bug 38441: (QA follow-up): Fix find-missing-filters.t prove xt/find-missing-filters.t Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181604 [details] [review] Bug 38441: (QA follow-up): HistoryCheck now only considers same patron Initial implementation checked existing ILLs for same patron and other patrons. User testing reported ILLs for other patrons are not helpful, and makes this stage screen show up much more frequently than it should. This patch simplifies the original implementation to only consider the patron of the request being placed for an ILL history check. It also changes the ID matching to only 'DOI' or only 'Pubmed ID' if any of those is set. This is to minimize scenarios where different articles from the same journal come up as matching results e.g. same ISSN but different DOI. It is also updating tests accordingly. Other QA improvements Improve HistoryCheck request list: - Make link external and show icon (to prevent staff member from clicking away from the historyCheck screen unintentionally) - Show requests' status - Show requests' placed date - Add id_prefix to request ID if possible Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181605 [details] [review] Bug 38441: (QA follow-up): Hackfest feedback This commit addresses all Hackfest QA related feedback: 1) Update of all strings that required update, including UI messages and sys pref description 2) Moved the history check information from staffnotes to its own pipe-separated illrequestattribute 'historycheck_requests' 3) Fix display of ILL request information on the history check screen (Staff), including proper label naming (e.g. branchcode -> library), show status description instead of code, uppercase labels. 4) Updated OPAC display to match Staff UI's display of matching history requests 5) Removed ISSN criteria for matching requests check 6) Update tests accordingly Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181606 [details] [review] Bug 38441: (QA follow-up): Only look for matchin requests if patron Previous commit 'HistoryCheck now only considers same patron' added the following regression: HistoryCheck is performed and shown in the OPAC even if no user is authenticated (using ILLOpacUnauthenticatedRequest). This fixes that. History check is skipped unless a user exists. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 181607 [details] [review] Bug 38441: (QA follow-up) Minor change Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed for 25.05! Well done everyone, thank you!