Bug 24083 - Koha should support "seen" vs "unseen" renewals
Summary: Koha should support "seen" vs "unseen" renewals
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Andrew Isherwood
QA Contact: Testopia
URL:
Keywords:
Depends on: 18936
Blocks:
  Show dependency treegraph
 
Reported: 2019-11-21 13:52 UTC by Andrew Isherwood
Modified: 2020-10-23 10:11 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 24083: Required atomic updates & DB update (11.80 KB, patch)
2019-11-28 09:07 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add circ rules management (3.99 KB, patch)
2019-11-28 09:07 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add support for unseen_renewals (37.16 KB, patch)
2019-11-28 09:07 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add unit tests (3.67 KB, patch)
2019-11-28 09:07 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Required atomic updates & DB update (11.80 KB, patch)
2019-11-28 16:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add circ rules management (3.99 KB, patch)
2019-11-28 16:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add support for unseen_renewals (38.07 KB, patch)
2019-11-28 16:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add unit tests (3.67 KB, patch)
2019-11-28 16:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Required atomic updates & DB update (10.92 KB, patch)
2020-02-12 13:38 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add circ rules management (6.53 KB, patch)
2020-02-12 13:38 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add support for unseen_renewals (39.36 KB, patch)
2020-02-12 13:38 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add unit tests (3.69 KB, patch)
2020-02-12 13:38 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Required atomic updates & DB update (10.92 KB, patch)
2020-02-12 17:02 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add circ rules management (6.63 KB, patch)
2020-02-12 17:02 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add support for unseen_renewals (43.50 KB, patch)
2020-02-12 17:02 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add unit tests (3.69 KB, patch)
2020-02-12 17:02 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Required atomic updates & DB update (10.92 KB, patch)
2020-03-06 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add circ rules management (6.39 KB, patch)
2020-03-06 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add support for unseen_renewals (43.50 KB, patch)
2020-03-06 11:00 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add unit tests (3.69 KB, patch)
2020-03-06 11:00 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Required atomic updates & DB update (11.00 KB, patch)
2020-03-09 13:51 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add circ rules management (6.47 KB, patch)
2020-03-09 13:51 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add support for unseen_renewals (43.51 KB, patch)
2020-03-09 13:51 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add unit tests (3.76 KB, patch)
2020-03-09 13:51 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix params to AddRenewal (3.44 KB, patch)
2020-03-09 16:06 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Required atomic updates & DB update (10.96 KB, patch)
2020-09-23 15:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add circ rules management (6.46 KB, patch)
2020-09-23 15:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add support for unseen_renewals (44.12 KB, patch)
2020-09-23 15:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add unit tests (3.48 KB, patch)
2020-09-23 15:31 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix params to AddRenewal (2.47 KB, patch)
2020-09-23 15:31 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Required atomic updates & DB update (10.96 KB, patch)
2020-10-02 12:45 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add circ rules management (6.46 KB, patch)
2020-10-02 12:45 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add support for unseen_renewals (42.14 KB, patch)
2020-10-02 12:45 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add unit tests (3.18 KB, patch)
2020-10-02 12:45 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix params to AddRenewal (2.47 KB, patch)
2020-10-02 12:46 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Respond to QA feedback (9.23 KB, patch)
2020-10-02 12:46 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Required atomic updates & DB update (10.96 KB, patch)
2020-10-06 11:24 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add circ rules management (6.46 KB, patch)
2020-10-06 11:25 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add support for unseen_renewals (42.14 KB, patch)
2020-10-06 11:25 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add unit tests (3.18 KB, patch)
2020-10-06 11:25 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix params to AddRenewal (2.47 KB, patch)
2020-10-06 11:25 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Respond to QA feedback (9.23 KB, patch)
2020-10-06 11:25 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Include SelfCheckInModule (4.45 KB, patch)
2020-10-06 11:25 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix error in checkouts JS (1.52 KB, patch)
2020-10-06 11:25 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix GetRenewCount return (1.01 KB, patch)
2020-10-06 11:25 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix unit tests (8.60 KB, patch)
2020-10-06 11:25 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix bad rebase (6.22 KB, patch)
2020-10-06 11:26 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Required atomic updates & DB update (11.00 KB, patch)
2020-10-16 11:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add circ rules management (6.46 KB, patch)
2020-10-16 11:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add support for unseen_renewals (42.15 KB, patch)
2020-10-16 11:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add unit tests (3.18 KB, patch)
2020-10-16 11:20 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix params to AddRenewal (2.47 KB, patch)
2020-10-16 11:20 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Respond to QA feedback (9.26 KB, patch)
2020-10-16 11:20 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Include SelfCheckInModule (4.45 KB, patch)
2020-10-16 11:20 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix error in checkouts JS (1.52 KB, patch)
2020-10-16 11:20 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix GetRenewCount return (1.01 KB, patch)
2020-10-16 11:20 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix unit tests (8.60 KB, patch)
2020-10-16 11:20 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix bad rebase (6.22 KB, patch)
2020-10-16 11:20 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix bad rebase (1.29 KB, patch)
2020-10-16 11:20 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Required atomic updates & DB update (11.00 KB, patch)
2020-10-23 10:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add circ rules management (6.46 KB, patch)
2020-10-23 10:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add support for unseen_renewals (42.15 KB, patch)
2020-10-23 10:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: Add unit tests (3.18 KB, patch)
2020-10-23 10:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix params to AddRenewal (2.47 KB, patch)
2020-10-23 10:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Respond to QA feedback (9.26 KB, patch)
2020-10-23 10:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Include SelfCheckInModule (4.45 KB, patch)
2020-10-23 10:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix error in checkouts JS (1.52 KB, patch)
2020-10-23 10:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix GetRenewCount return (1.01 KB, patch)
2020-10-23 10:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix unit tests (8.60 KB, patch)
2020-10-23 10:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix bad rebase (6.22 KB, patch)
2020-10-23 10:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix bad rebase (1.29 KB, patch)
2020-10-23 10:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix misaligned columns (1.79 KB, patch)
2020-10-23 10:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24083: (follow-up) Fix QA issues (8.12 KB, patch)
2020-10-23 10:10 UTC, Andrew Isherwood
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-11-21 13:52:58 UTC
When carrying out a renewal transaction Koha should check for both the maximum number of renewals allowed and also the number of unseen renewals. If it finds a value equal to or greater than the number of renewals already carried out against that loan, it prevents the renewal.

The purpose of this is to prevent borrowers from continually renewing a book when they have, in fact, lost or misplaced it!

When a renewal takes place via a method that doesn't require visible sight or possession of the item (e.g. OPAC, automatic renewal) then the number of unseen renewals for that item should be incremented. SIP2 renewals can be considered seen since they will require the physical item to be scanned.

The number of allowed unseen renewals before a renewal is denied should be specifiable in the circulation rules.

If a renewal is denied due to the unseen maximum having been reached, the flow should be the same as when a renewal limit has been reached, though with appropriate messages displayed to the user. It should be possible to override the denial in the same was as when a renewal limit is reached.
Comment 1 Andrew Isherwood 2019-11-28 09:07:14 UTC
Created attachment 95861 [details] [review]
Bug 24083: Required atomic updates & DB update

This patch adds:

- An "UnseenRenewals" circulation syspref that enables/disables the
  functionality added in this bug

- A change to the issuingrules table schema and a corresponding database
  upgrade to add issuingrules.unseen_renewals_allowed

- A change to the issues & old_issues table schemas and corresponding
  database upgrades to add issues.unseen_renewals &
  old_issues.unseen_renewals

- An update for the renewals letter to include a message for unseen
  renewals
Comment 2 Andrew Isherwood 2019-11-28 09:07:16 UTC
Created attachment 95862 [details] [review]
Bug 24083: Add circ rules management

This patch adds the ability to set the "Unseen renewals allowed" value
in the circulation rules
Comment 3 Andrew Isherwood 2019-11-28 09:07:19 UTC
Created attachment 95863 [details] [review]
Bug 24083: Add support for unseen_renewals

This patch adds support for unseen renewals.

Here we retrofit knowledge of unseen renewals and add the display of unseen
renewal counts and warnings, in addition to adding the ability to
specify a renewal as being "unseen".

The functionality added here is goverened by the UnseenRenewals syspref.
Comment 4 Andrew Isherwood 2019-11-28 09:07:22 UTC
Created attachment 95864 [details] [review]
Bug 24083: Add unit tests

This patch adds unit tests for unseen renewals
Comment 5 Andrew Isherwood 2019-11-28 16:09:23 UTC
Created attachment 95865 [details] [review]
Bug 24083: Required atomic updates & DB update

This patch adds:

- An "UnseenRenewals" circulation syspref that enables/disables the
  functionality added in this bug

- A change to the issuingrules table schema and a corresponding database
  upgrade to add issuingrules.unseen_renewals_allowed

- A change to the issues & old_issues table schemas and corresponding
  database upgrades to add issues.unseen_renewals &
  old_issues.unseen_renewals

- An update for the renewals letter to include a message for unseen
  renewals
Comment 6 Andrew Isherwood 2019-11-28 16:09:26 UTC
Created attachment 95866 [details] [review]
Bug 24083: Add circ rules management

This patch adds the ability to set the "Unseen renewals allowed" value
in the circulation rules
Comment 7 Andrew Isherwood 2019-11-28 16:09:29 UTC
Created attachment 95867 [details] [review]
Bug 24083: Add support for unseen_renewals

This patch adds support for unseen renewals.

Here we retrofit knowledge of unseen renewals and add the display of unseen
renewal counts and warnings, in addition to adding the ability to
specify a renewal as being "unseen".

The functionality added here is goverened by the UnseenRenewals syspref.
Comment 8 Andrew Isherwood 2019-11-28 16:09:32 UTC
Created attachment 95868 [details] [review]
Bug 24083: Add unit tests

This patch adds unit tests for unseen renewals
Comment 9 Andrew Isherwood 2019-11-28 16:20:32 UTC
- Apply patches (if on a platform requiring this to be done manually)
- Run DB updates and rebuild schema (if on a platform requiring this to be done manually)
- Rebuild CSS (if on a platform requiring this to be done manually)

- Issue an item to a patron
- Navigate to Koha Administration > Circulation and fines rules
=> TEST: Observe that there *isn't* a column titled "Unseen renewals allowed (count)"
- Navigate to a patron's checkouts list
=> TEST: Observe that in the "Renew" column, only details of their remaining renewals are shown, unseen renewals are not mentioned
- Navigate to a patron's "Your account" page
=> TEST: Observe that in the "Renew" column, only details of their remaining renewals are shown, unseen renewals are not mentioned
- Turn on the "UnseenRenewals" syspref
- Turn on the "OpacRenewalAllowed" syspref
- Navigate to Koha Administration > Circulation and fines rules
- If you have no circulation rules defined, define one that will:
  - Apply to all patron categories
  - Apply to item types
  - Allow renewals
=> TEST: Observe that there *is* a column titled "Unseen renewals allowed (count)" and the value is empty
- Add a value for "Unseen renewals allowed", make the value reasonably low so we can test what happens when that value is reached
- Navigate to a patron's checkouts list in the staff client
=> TEST: Observe that in the "Renew" column, details of their remaining unseen renewals are now shown
- Navigate to a patron's "Your account" page
=> TEST: Observe that in the "Renew" column, details of their remaining unseen renewals are now shown
- In the OPAC, renew an item
=> TEST: Observe that, since the renewal took place in the OPAC, the number of unseen renewals left is decremented
- In the OPAC, repeatedly renew the item until the unseen renewals limit is reached
=> TEST: Observe that the item can no longer be renewed and the borrower is advised to take the item to the library
- Navigate to the patron's checkouts list in the staff client
=> TEST: Observe that the item is listed as "Must be renewed at the library" and is blocked from renewal
- Select the "Override renewal restrictions" checkbox
=> TEST: Observe that the item can now be selected for renewal
- Select the item and click "Renew or check in selected items"
=> TEST: Observe that now the item has been renewed by the library, the unseen renewals remaining count has been reset
- Select the item for renewal again and check the "Renew as unseen if appropriate" checkbox, click "Renew or check in selected items"
=> TEST: Observe that, since the item was renewed as "unseen", the unseen renewals remaining has been decremented
- Navigate to Circulation > Renew
=> TEST: Observe that a new "Record renewal as unseen if appropriate" checkbox is displayed
- Renew the item, do not check the unseen checkbox
=> TEST: Observe in the patron's checkouts list that the unseen renewals count has not been decremented
- Renew the item, check the unseen checkbox
=> TEST: Observe in the patron's checkouts list that the unseen renewals count has been decremented
- Navigate to Tools > Notices & Slips, edit the "AUTO_RENEWALS" Email notice
=> TEST: Observe that there is now a test for the "too_unseen" error, with an appropriate message

SIP RENEWALS:
- Use sip_cli_emulator.pl to send a renew message for the item, for example:

./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su <kiosk_username> -sp <kiosk_password> --patron <patron_username> --password <patron_password> -l CPL -i <item_barcode>  -m renew

- TEST: Observe that, as the SIP renewal is considered to be "seen", the item's unseen renewals count has been reset
Comment 10 Jonathan Druart 2020-02-12 11:37:27 UTC
This needs to be rebuilt on top of bug 18936.
And even bug 15522 I'd say.
Comment 11 Andrew Isherwood 2020-02-12 13:38:13 UTC
Created attachment 98749 [details] [review]
Bug 24083: Required atomic updates & DB update

This patch adds:

- An "UnseenRenewals" circulation syspref that enables/disables the
  functionality added in this bug

- Add unseen_renewals_allowed to circulation_rules

- A change to the issues & old_issues table schemas and corresponding
  database upgrades to add issues.unseen_renewals &
  old_issues.unseen_renewals

- An update for the renewals letter to include a message for unseen
  renewals
Comment 12 Andrew Isherwood 2020-02-12 13:38:16 UTC
Created attachment 98750 [details] [review]
Bug 24083: Add circ rules management

This patch adds the ability to set the "Unseen renewals allowed" value
in the circulation rules
Comment 13 Andrew Isherwood 2020-02-12 13:38:20 UTC
Created attachment 98751 [details] [review]
Bug 24083: Add support for unseen_renewals

This patch adds support for unseen renewals.

Here we retrofit knowledge of unseen renewals and add the display of unseen
renewal counts and warnings, in addition to adding the ability to
specify a renewal as being "unseen".

The functionality added here is goverened by the UnseenRenewals syspref.
Comment 14 Andrew Isherwood 2020-02-12 13:38:23 UTC
Created attachment 98752 [details] [review]
Bug 24083: Add unit tests

This patch adds unit tests for unseen renewals
Comment 15 Andrew Isherwood 2020-02-12 17:02:36 UTC
Created attachment 98765 [details] [review]
Bug 24083: Required atomic updates & DB update

This patch adds:

- An "UnseenRenewals" circulation syspref that enables/disables the
  functionality added in this bug

- Add unseen_renewals_allowed to circulation_rules

- A change to the issues & old_issues table schemas and corresponding
  database upgrades to add issues.unseen_renewals &
  old_issues.unseen_renewals

- An update for the renewals letter to include a message for unseen
  renewals
Comment 16 Andrew Isherwood 2020-02-12 17:02:39 UTC
Created attachment 98766 [details] [review]
Bug 24083: Add circ rules management

This patch adds the ability to set the "Unseen renewals allowed" value
in the circulation rules
Comment 17 Andrew Isherwood 2020-02-12 17:02:42 UTC
Created attachment 98767 [details] [review]
Bug 24083: Add support for unseen_renewals

This patch adds support for unseen renewals.

Here we retrofit knowledge of unseen renewals and add the display of unseen
renewal counts and warnings, in addition to adding the ability to
specify a renewal as being "unseen".

The functionality added here is goverened by the UnseenRenewals syspref.
Comment 18 Andrew Isherwood 2020-02-12 17:02:45 UTC
Created attachment 98768 [details] [review]
Bug 24083: Add unit tests

This patch adds unit tests for unseen renewals
Comment 19 Andrew Isherwood 2020-02-14 12:19:57 UTC
- Apply patches (if on a platform requiring this to be done manually)
- Run DB updates and rebuild schema (if on a platform requiring this to be done manually)
- Rebuild CSS (if on a platform requiring this to be done manually)

- Issue an item to a patron
- Navigate to Koha Administration > Circulation and fines rules
=> TEST: Observe that there *isn't* a column titled "Unseen renewals allowed (count)"
- Navigate to a patron's checkouts list
=> TEST: Observe that in the "Renew" column, only details of their remaining renewals are shown, unseen renewals are not mentioned
- Turn on the "OpacRenewalAllowed" syspref
- Navigate to a patron's "Your account" page
=> TEST: Observe that in the "Renew" column, only details of their remaining renewals are shown, unseen renewals are not mentioned
- Turn on the "UnseenRenewals" syspref
- Navigate to Koha Administration > Circulation and fines rules
- If you have no circulation rules defined, define one that will:
  - Apply to all patron categories
  - Apply to item types
  - Allow renewals
=> TEST: Observe that there *is* a column titled "Unseen renewals allowed (count)" and the value is empty
- Add a value for "Unseen renewals allowed", make the value reasonably low so we can test what happens when that value is reached
- Navigate to a patron's checkouts list in the staff client
=> TEST: Observe that in the "Renew" column, details of their remaining unseen renewals are now shown
- Navigate to a patron's "Your account" page
=> TEST: Observe that in the "Renew" column, details of their remaining unseen renewals are now shown
- In the OPAC, renew an item
=> TEST: Observe that, since the renewal took place in the OPAC, the number of unseen renewals left is decremented
- In the OPAC, repeatedly renew the item until the unseen renewals limit is reached
=> TEST: Observe that the item can no longer be renewed and the borrower is advised to take the item to the library
- Navigate to the patron's checkouts list in the staff client
=> TEST: Observe that the item is listed as "Must be renewed at the library" and is blocked from renewal
- Select the "Override renewal restrictions" checkbox
=> TEST: Observe that the item can now be selected for renewal
- Select the item and click "Renew or check in selected items"
=> TEST: Observe that now the item has been renewed by the library, the unseen renewals remaining count has been reset
- Select the item for renewal again and check the "Renew as unseen if appropriate" checkbox, click "Renew or check in selected items"
=> TEST: Observe that, since the item was renewed as "unseen", the unseen renewals remaining has been decremented
- Navigate to Circulation > Renew
=> TEST: Observe that a new "Record renewal as unseen if appropriate" checkbox is displayed
- Renew the item, do not check the unseen checkbox
=> TEST: Observe in the patron's checkouts list that the unseen renewals count has not been decremented
- Renew the item, check the unseen checkbox
=> TEST: Observe in the patron's checkouts list that the unseen renewals count has been decremented
- Navigate to Tools > Notices & Slips, edit the "AUTO_RENEWALS" Email notice
=> TEST: Observe that there is now a test for the "too_unseen" error, with an appropriate message

SIP RENEWALS:
- Use sip_cli_emulator.pl to send a renew message for the item, for example:

./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su <kiosk_username> -sp <kiosk_password> --patron <patron_username> --password <patron_password> -l CPL -i <item_barcode>  -m renew

- TEST: Observe that, as the SIP renewal is considered to be "seen", the item's unseen renewals count has been reset
Comment 20 Andrew Isherwood 2020-03-06 10:59:56 UTC
Created attachment 100244 [details] [review]
Bug 24083: Required atomic updates & DB update

This patch adds:

- An "UnseenRenewals" circulation syspref that enables/disables the
  functionality added in this bug

- Add unseen_renewals_allowed to circulation_rules

- A change to the issues & old_issues table schemas and corresponding
  database upgrades to add issues.unseen_renewals &
  old_issues.unseen_renewals

- An update for the renewals letter to include a message for unseen
  renewals
Comment 21 Andrew Isherwood 2020-03-06 10:59:59 UTC
Created attachment 100245 [details] [review]
Bug 24083: Add circ rules management

This patch adds the ability to set the "Unseen renewals allowed" value
in the circulation rules
Comment 22 Andrew Isherwood 2020-03-06 11:00:02 UTC
Created attachment 100246 [details] [review]
Bug 24083: Add support for unseen_renewals

This patch adds support for unseen renewals.

Here we retrofit knowledge of unseen renewals and add the display of unseen
renewal counts and warnings, in addition to adding the ability to
specify a renewal as being "unseen".

The functionality added here is goverened by the UnseenRenewals syspref.
Comment 23 Andrew Isherwood 2020-03-06 11:00:06 UTC
Created attachment 100247 [details] [review]
Bug 24083: Add unit tests

This patch adds unit tests for unseen renewals
Comment 24 Andrew Isherwood 2020-03-09 13:51:44 UTC
Created attachment 100380 [details] [review]
Bug 24083: Required atomic updates & DB update

This patch adds:

- An "UnseenRenewals" circulation syspref that enables/disables the
  functionality added in this bug

- Add unseen_renewals_allowed to circulation_rules

- A change to the issues & old_issues table schemas and corresponding
  database upgrades to add issues.unseen_renewals &
  old_issues.unseen_renewals

- An update for the renewals letter to include a message for unseen
  renewals

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 25 Andrew Isherwood 2020-03-09 13:51:47 UTC
Created attachment 100381 [details] [review]
Bug 24083: Add circ rules management

This patch adds the ability to set the "Unseen renewals allowed" value
in the circulation rules

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 26 Andrew Isherwood 2020-03-09 13:51:51 UTC
Created attachment 100382 [details] [review]
Bug 24083: Add support for unseen_renewals

This patch adds support for unseen renewals.

Here we retrofit knowledge of unseen renewals and add the display of unseen
renewal counts and warnings, in addition to adding the ability to
specify a renewal as being "unseen".

The functionality added here is goverened by the UnseenRenewals syspref.

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 27 Andrew Isherwood 2020-03-09 13:51:54 UTC
Created attachment 100383 [details] [review]
Bug 24083: Add unit tests

This patch adds unit tests for unseen renewals

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 28 Andrew Isherwood 2020-03-09 16:06:54 UTC
Created attachment 100392 [details] [review]
Bug 24083: (follow-up) Fix params to AddRenewal

The merging of Bug 23051 has added a paramter that can be passed to
C4::AddRenewal. This bug does also, so the offset of this bug's new
paramter was wrong. This patch fixes it.
Comment 29 Nick Clemens 2020-04-15 13:31:42 UTC
conflicts in opac-renew.pl and opac-user.tt

the previous rebase on CanBookBeRenewed looks wrong too - it readds a lot of code

at first glance here I like that this circ rule type can be disabled, but I wish it used a more generic method. Something like a table of rule names and an 'enabled' column - then we could join that when searching for effective rules and simply not use disabled ones - not a blocker
Comment 30 Martin Renvoize 2020-04-16 07:36:41 UTC
Looks like bug 19014 was where the rebase gets confused.. I've not played with the patch with It certainly does look like a lot of code was re-introduced.
Comment 31 Andrew Isherwood 2020-09-23 15:30:49 UTC
Created attachment 110607 [details] [review]
Bug 24083: Required atomic updates & DB update

This patch adds:

- An "UnseenRenewals" circulation syspref that enables/disables the
  functionality added in this bug

- Add unseen_renewals_allowed to circulation_rules

- A change to the issues & old_issues table schemas and corresponding
  database upgrades to add issues.unseen_renewals &
  old_issues.unseen_renewals

- An update for the renewals letter to include a message for unseen
  renewals

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 32 Andrew Isherwood 2020-09-23 15:30:54 UTC
Created attachment 110608 [details] [review]
Bug 24083: Add circ rules management

This patch adds the ability to set the "Unseen renewals allowed" value
in the circulation rules

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 33 Andrew Isherwood 2020-09-23 15:30:59 UTC
Created attachment 110609 [details] [review]
Bug 24083: Add support for unseen_renewals

This patch adds support for unseen renewals.

Here we retrofit knowledge of unseen renewals and add the display of unseen
renewal counts and warnings, in addition to adding the ability to
specify a renewal as being "unseen".

The functionality added here is goverened by the UnseenRenewals syspref.

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 34 Andrew Isherwood 2020-09-23 15:31:04 UTC
Created attachment 110610 [details] [review]
Bug 24083: Add unit tests

This patch adds unit tests for unseen renewals

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 35 Andrew Isherwood 2020-09-23 15:31:12 UTC
Created attachment 110611 [details] [review]
Bug 24083: (follow-up) Fix params to AddRenewal

The merging of Bug 23051 has added a paramter that can be passed to
C4::AddRenewal. This bug does also, so the offset of this bug's new
paramter was wrong. This patch fixes it.
Comment 36 Andrew Isherwood 2020-09-23 15:32:23 UTC
- Apply patches (if on a platform requiring this to be done manually)
- Run DB updates and rebuild schema (if on a platform requiring this to be done manually)
- Rebuild CSS (if on a platform requiring this to be done manually)

- Issue an item to a patron
- Navigate to Koha Administration > Circulation and fines rules
=> TEST: Observe that there *isn't* a column titled "Unseen renewals allowed (count)"
- Navigate to a patron's checkouts list
=> TEST: Observe that in the "Renew" column, only details of their remaining renewals are shown, unseen renewals are not mentioned
- Navigate to a patron's "Your account" page
=> TEST: Observe that in the "Renew" column, only details of their remaining renewals are shown, unseen renewals are not mentioned
- Turn on the "UnseenRenewals" syspref
- Turn on the "OpacRenewalAllowed" syspref
- Navigate to Koha Administration > Circulation and fines rules
- If you have no circulation rules defined, define one that will:
  - Apply to all patron categories
  - Apply to item types
  - Allow renewals
=> TEST: Observe that there *is* a column titled "Unseen renewals allowed (count)" and the value is empty
- Add a value for "Unseen renewals allowed", make the value reasonably low so we can test what happens when that value is reached
- Navigate to a patron's checkouts list in the staff client
=> TEST: Observe that in the "Renew" column, details of their remaining unseen renewals are now shown
- Navigate to a patron's "Your account" page
=> TEST: Observe that in the "Renew" column, details of their remaining unseen renewals are now shown
- In the OPAC, renew an item
=> TEST: Observe that, since the renewal took place in the OPAC, the number of unseen renewals left is decremented
- In the OPAC, repeatedly renew the item until the unseen renewals limit is reached
=> TEST: Observe that the item can no longer be renewed and the borrower is advised to take the item to the library
- Navigate to the patron's checkouts list in the staff client
=> TEST: Observe that the item is listed as "Must be renewed at the library" and is blocked from renewal
- Select the "Override renewal restrictions" checkbox
=> TEST: Observe that the item can now be selected for renewal
- Select the item and click "Renew or check in selected items"
=> TEST: Observe that now the item has been renewed by the library, the unseen renewals remaining count has been reset
- Select the item for renewal again and check the "Renew as unseen if appropriate" checkbox, click "Renew or check in selected items"
=> TEST: Observe that, since the item was renewed as "unseen", the unseen renewals remaining has been decremented
- Navigate to Circulation > Renew
=> TEST: Observe that a new "Record renewal as unseen if appropriate" checkbox is displayed
- Renew the item, do not check the unseen checkbox
=> TEST: Observe in the patron's checkouts list that the unseen renewals count has been reset
- Renew the item, check the unseen checkbox
=> TEST: Observe in the patron's checkouts list that the unseen renewals count has been decremented
- Navigate to Tools > Notices & Slips, edit the "AUTO_RENEWALS" Email notice
=> TEST: Observe that there is now a test for the "too_unseen" error, with an appropriate message

SIP RENEWALS:
- Use sip_cli_emulator.pl to send a renew message for the item, for example:

./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su <kiosk_username> -sp <kiosk_password> --patron <patron_username> --password <patron_password> -l CPL -i <item_barcode>  -m renew

- TEST: Observe that, as the SIP renewal is considered to be "seen", the item's unseen renewals count has been reset
Comment 37 Andrew Isherwood 2020-09-23 15:32:31 UTC
Rebased on master
Comment 38 Katrin Fischer 2020-09-27 11:13:17 UTC
Hi Andrew,

starting with some QA work here.

Tbh I've never heard of this specific functionality in an ILS before and am a bit stumped on the use case. Can you explain a bit on how this is used in libraries?


1) QA script fails

 FAIL	C4/Circulation.pm
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: Use of DateTime->now should certainly be replaced with dt_from_string (bug 24840) (line 2933)

 FAIL	t/db_dependent/Circulation/issue.t
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   FAIL	  valid
		Can't locate Koha/IssuingRules.pm in @INC (you may need to install the Koha::IssuingRules module) (@INC contains: /home/vagrant/kohaclone /home/vagrant/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1 /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base /var/lib/koha/kohadev/plugins) 

2) Database updates

There is lots of them :) 

a) The one thing that gives me a bit of a headache is the change to the notices for existing installations. I think we should leave that one out as we cannot safely assume what people have 'done' to this notice - they will have translated it, might have decided to not inform patrons of the reasons at all, changed the structure etc. I feel we should not interfere here.

b) You have a mismatch between kohastructure.sql and the updates for issues/old_issues:
AFTER renewals
vs.
after auto_renew

3) Strings in Javascript

This is an old patch, I am aware, but not sure if you have seen yet: We are moving the strings back into the js as they can be finally translated within the file now. See bug 21156 for examples.

I am not going to block this, but it would be nice if the patches could be changed to use the new mechanism right away.

4) Terminology

We just did a major clean-up on the system preferences, that's why I am a bit nit-picky there. Every change later requires translators to do the work again too - so it might be forgiven:

+            - renewals to be recorded as "unseen" by the library, and count against the borrowers unseen renewals limit

Can you please make the borrowers patrons and add a .?

5) Web based self check

Renewals are also possible with the web based self check - so I feel some changes will be needed there as well!
Comment 39 Andrew Isherwood 2020-10-02 12:45:34 UTC
Created attachment 111147 [details] [review]
Bug 24083: Required atomic updates & DB update

This patch adds:

- An "UnseenRenewals" circulation syspref that enables/disables the
  functionality added in this bug

- Add unseen_renewals_allowed to circulation_rules

- A change to the issues & old_issues table schemas and corresponding
  database upgrades to add issues.unseen_renewals &
  old_issues.unseen_renewals

- An update for the renewals letter to include a message for unseen
  renewals

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 40 Andrew Isherwood 2020-10-02 12:45:40 UTC
Created attachment 111148 [details] [review]
Bug 24083: Add circ rules management

This patch adds the ability to set the "Unseen renewals allowed" value
in the circulation rules

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 41 Andrew Isherwood 2020-10-02 12:45:46 UTC
Created attachment 111149 [details] [review]
Bug 24083: Add support for unseen_renewals

This patch adds support for unseen renewals.

Here we retrofit knowledge of unseen renewals and add the display of unseen
renewal counts and warnings, in addition to adding the ability to
specify a renewal as being "unseen".

The functionality added here is goverened by the UnseenRenewals syspref.

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 42 Andrew Isherwood 2020-10-02 12:45:53 UTC
Created attachment 111150 [details] [review]
Bug 24083: Add unit tests

This patch adds unit tests for unseen renewals

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 43 Andrew Isherwood 2020-10-02 12:46:00 UTC
Created attachment 111151 [details] [review]
Bug 24083: (follow-up) Fix params to AddRenewal

The merging of Bug 23051 has added a paramter that can be passed to
C4::AddRenewal. This bug does also, so the offset of this bug's new
paramter was wrong. This patch fixes it.
Comment 44 Andrew Isherwood 2020-10-02 12:46:07 UTC
Created attachment 111152 [details] [review]
Bug 24083: (follow-up) Respond to QA feedback

This patch implements changes requested by Katrin in QA feedback
(comment #38):

- Fix QA script failures
- Remove notice text database update
- Fix alter table column order mismatch
- Use new convention for JS strings
- Fix terminology
Comment 45 Andrew Isherwood 2020-10-02 12:46:29 UTC
Hi Katrin

> 1) QA script fails

Fixed

> 2) Database updates

a) This is now removed
b) Fixed

> 3) Strings in Javascript

This was actually causing merge conflicts when I applied this bug to latest master, so I made this switch as part of the merging

> 4) Terminology

Fixed

> 5) Web based self check

I'm not sure what you mean here. Do you mean the renewal function in the "My account" in the OPAC? If so, unseen renewals are already handled there. I'm guessing you're referring to something else though...
Comment 46 Katrin Fischer 2020-10-02 12:51:55 UTC
> 
> > 5) Web based self check
> 
> I'm not sure what you mean here. Do you mean the renewal function in the "My
> account" in the OPAC? If so, unseen renewals are already handled there. I'm
> guessing you're referring to something else though...

Hi Andrew,

thx for the follow-ups, what I meant is the web based self check module, see SelfCheckInModule and WebBasedSelfCheck - the latter also has the option to make renewals either by scanning in an item agan (seen?) and by renewing them directly (unseen).
Comment 47 Andrew Isherwood 2020-10-06 11:24:53 UTC
Created attachment 111283 [details] [review]
Bug 24083: Required atomic updates & DB update

This patch adds:

- An "UnseenRenewals" circulation syspref that enables/disables the
  functionality added in this bug

- Add unseen_renewals_allowed to circulation_rules

- A change to the issues & old_issues table schemas and corresponding
  database upgrades to add issues.unseen_renewals &
  old_issues.unseen_renewals

- An update for the renewals letter to include a message for unseen
  renewals

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 48 Andrew Isherwood 2020-10-06 11:25:03 UTC
Created attachment 111284 [details] [review]
Bug 24083: Add circ rules management

This patch adds the ability to set the "Unseen renewals allowed" value
in the circulation rules

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 49 Andrew Isherwood 2020-10-06 11:25:09 UTC
Created attachment 111285 [details] [review]
Bug 24083: Add support for unseen_renewals

This patch adds support for unseen renewals.

Here we retrofit knowledge of unseen renewals and add the display of unseen
renewal counts and warnings, in addition to adding the ability to
specify a renewal as being "unseen".

The functionality added here is goverened by the UnseenRenewals syspref.

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 50 Andrew Isherwood 2020-10-06 11:25:18 UTC
Created attachment 111286 [details] [review]
Bug 24083: Add unit tests

This patch adds unit tests for unseen renewals

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 51 Andrew Isherwood 2020-10-06 11:25:25 UTC
Created attachment 111287 [details] [review]
Bug 24083: (follow-up) Fix params to AddRenewal

The merging of Bug 23051 has added a paramter that can be passed to
C4::AddRenewal. This bug does also, so the offset of this bug's new
paramter was wrong. This patch fixes it.
Comment 52 Andrew Isherwood 2020-10-06 11:25:30 UTC
Created attachment 111288 [details] [review]
Bug 24083: (follow-up) Respond to QA feedback

This patch implements changes requested by Katrin in QA feedback
(comment #38):

- Fix QA script failures
- Remove notice text database update
- Fix alter table column order mismatch
- Use new convention for JS strings
- Fix terminology
Comment 53 Andrew Isherwood 2020-10-06 11:25:35 UTC
Created attachment 111289 [details] [review]
Bug 24083: (follow-up) Include SelfCheckInModule

As highlighted by Katrin, the selfcheckin module was not aware of this
work. This commits gives it awareness of the too_unseen renew error and
the "seen" argument that can be passed to AddRenewal
Comment 54 Andrew Isherwood 2020-10-06 11:25:40 UTC
Created attachment 111290 [details] [review]
Bug 24083: (follow-up) Fix error in checkouts JS

This commit fixes a small error in the checkouts related JS
Comment 55 Andrew Isherwood 2020-10-06 11:25:47 UTC
Created attachment 111291 [details] [review]
Bug 24083: (follow-up) Fix GetRenewCount return

GetRenewCount now returns 6 values when a call to it succeeds, a
failed call should also return the same number of values. This commit
adds these additional values.
Comment 56 Andrew Isherwood 2020-10-06 11:25:53 UTC
Created attachment 111292 [details] [review]
Bug 24083: (follow-up) Fix unit tests

Some changes in issue.t had broken the tests for unseen renewals (the
unseen tests were using variables that had been moved out of the tests'
scope).

Also now using Koha::CirculationRules::set_rules to set circ rules
rather than using SQL queries.

Fixed expected number of return values from GetRenewCount

Moved unseen tests in issue.t to the bottom of the file to remove the
risk of interference with other test circ rules.
Comment 57 Andrew Isherwood 2020-10-06 11:26:00 UTC
Created attachment 111293 [details] [review]
Bug 24083: (follow-up) Fix bad rebase

There was a real mess in C4/Circulation.pm due to a bad rebase back in
February. Frankly it's a wonder anything worked at all. This commit
fixes that problem and reinstates the correct patch for
C4/Circulation.pm
Comment 58 Andrew Isherwood 2020-10-06 11:29:49 UTC
(In reply to Katrin Fischer from comment #46)
> Hi Andrew,
> 
> thx for the follow-ups, what I meant is the web based self check module, see
> SelfCheckInModule and WebBasedSelfCheck - the latter also has the option to
> make renewals either by scanning in an item agan (seen?) and by renewing
> them directly (unseen).

Hi Katrin

Thanks for the clarification, I didn't even know this area of Koha existed! Anyway, I have now added awareness of seen vs unseen to it so should prevent renewals when appropriate with a suitable error message.

I've also fixed some unit tests that were broken as a result of changes since this was last rebased.

I've also fixed a major rebase error that crept in in February, to be honest it's amazing anything worked at all with it in, anyway it's fixed now.

Setting this back to "Signed off" with crossed fingers...
Comment 59 Katrin Fischer 2020-10-15 10:58:38 UTC
I am sorry Andrew, but I can't get the tests to pass :(

I applied the patches, ran updatedatabase and then dbic after. Can you please check?

t/db_dependent/Circulation.t ........ 1/50 
    #   Failed test 'Cannot renew, 0 unseen renewals allowed'
    #   at t/db_dependent/Circulation.t line 1174.
    #          got: '1'
    #     expected: '0'

    #   Failed test 'Cannot renew, returned code is too_unseen'
    #   at t/db_dependent/Circulation.t line 1175.
    #          got: undef
    #     expected: 'too_unseen'
    # Looks like you failed 2 tests of 89.
t/db_dependent/Circulation.t ........ 15/50 
#   Failed test 'CanBookBeRenewed tests'
#   at t/db_dependent/Circulation.t line 1288.
t/db_dependent/Circulation.t ........ 50/50 # Looks like you failed 1 test of 50.
t/db_dependent/Circulation.t ........ Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/50 subtests 
t/db_dependent/Circulation/issue.t .. ok     

Test Summary Report
-------------------
t/db_dependent/Circulation.t      (Wstat: 256 Tests: 50 Failed: 1)
  Failed test:  15
  Non-zero exit status: 1
Comment 60 Andrew Isherwood 2020-10-16 11:19:36 UTC
Created attachment 111849 [details] [review]
Bug 24083: Required atomic updates & DB update

This patch adds:

- An "UnseenRenewals" circulation syspref that enables/disables the
  functionality added in this bug

- Add unseen_renewals_allowed to circulation_rules

- A change to the issues & old_issues table schemas and corresponding
  database upgrades to add issues.unseen_renewals &
  old_issues.unseen_renewals

- An update for the renewals letter to include a message for unseen
  renewals

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 61 Andrew Isherwood 2020-10-16 11:19:44 UTC
Created attachment 111850 [details] [review]
Bug 24083: Add circ rules management

This patch adds the ability to set the "Unseen renewals allowed" value
in the circulation rules

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 62 Andrew Isherwood 2020-10-16 11:19:53 UTC
Created attachment 111851 [details] [review]
Bug 24083: Add support for unseen_renewals

This patch adds support for unseen renewals.

Here we retrofit knowledge of unseen renewals and add the display of unseen
renewal counts and warnings, in addition to adding the ability to
specify a renewal as being "unseen".

The functionality added here is goverened by the UnseenRenewals syspref.

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 63 Andrew Isherwood 2020-10-16 11:20:01 UTC
Created attachment 111852 [details] [review]
Bug 24083: Add unit tests

This patch adds unit tests for unseen renewals

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 64 Andrew Isherwood 2020-10-16 11:20:09 UTC
Created attachment 111853 [details] [review]
Bug 24083: (follow-up) Fix params to AddRenewal

The merging of Bug 23051 has added a paramter that can be passed to
C4::AddRenewal. This bug does also, so the offset of this bug's new
paramter was wrong. This patch fixes it.
Comment 65 Andrew Isherwood 2020-10-16 11:20:15 UTC
Created attachment 111854 [details] [review]
Bug 24083: (follow-up) Respond to QA feedback

This patch implements changes requested by Katrin in QA feedback
(comment #38):

- Fix QA script failures
- Remove notice text database update
- Fix alter table column order mismatch
- Use new convention for JS strings
- Fix terminology
Comment 66 Andrew Isherwood 2020-10-16 11:20:20 UTC
Created attachment 111855 [details] [review]
Bug 24083: (follow-up) Include SelfCheckInModule

As highlighted by Katrin, the selfcheckin module was not aware of this
work. This commits gives it awareness of the too_unseen renew error and
the "seen" argument that can be passed to AddRenewal
Comment 67 Andrew Isherwood 2020-10-16 11:20:29 UTC
Created attachment 111856 [details] [review]
Bug 24083: (follow-up) Fix error in checkouts JS

This commit fixes a small error in the checkouts related JS
Comment 68 Andrew Isherwood 2020-10-16 11:20:36 UTC
Created attachment 111857 [details] [review]
Bug 24083: (follow-up) Fix GetRenewCount return

GetRenewCount now returns 6 values when a call to it succeeds, a
failed call should also return the same number of values. This commit
adds these additional values.
Comment 69 Andrew Isherwood 2020-10-16 11:20:41 UTC
Created attachment 111858 [details] [review]
Bug 24083: (follow-up) Fix unit tests

Some changes in issue.t had broken the tests for unseen renewals (the
unseen tests were using variables that had been moved out of the tests'
scope).

Also now using Koha::CirculationRules::set_rules to set circ rules
rather than using SQL queries.

Fixed expected number of return values from GetRenewCount

Moved unseen tests in issue.t to the bottom of the file to remove the
risk of interference with other test circ rules.
Comment 70 Andrew Isherwood 2020-10-16 11:20:49 UTC
Created attachment 111859 [details] [review]
Bug 24083: (follow-up) Fix bad rebase

There was a real mess in C4/Circulation.pm due to a bad rebase back in
February. Frankly it's a wonder anything worked at all. This commit
fixes that problem and reinstates the correct patch for
C4/Circulation.pm
Comment 71 Andrew Isherwood 2020-10-16 11:20:55 UTC
Created attachment 111860 [details] [review]
Bug 24083: (follow-up) Fix bad rebase

This commit fixes a couple of lines that got missed during the last
rebase
Comment 72 Andrew Isherwood 2020-10-16 11:29:36 UTC
Sorry Katrin, another bad rebase. All these rebases are a nightmare for introducing errors. Anyway, hopefully it will now be good.
Comment 73 Katrin Fischer 2020-10-16 15:29:43 UTC
Could someone please take over testing the SIP bit of the test plan for me?
Comment 74 Katrin Fischer 2020-10-22 13:09:22 UTC
Done lots of successful testing, but getting stuck a bit at the mechanics of this:

1) I turned on the unseen/seen renewals feature. My total allowed renewals are 5. Unseen 4. My checked out item in the OPAC now shows:

Renew ( 2 of 5 renewals remaining / 4 of 4 renewals left before the item must be seen by the library )

In this case it's extra confusing because I have done renewals before the feature was activated. 

Then I have renewed the item by checking it out again. It now reads:

Renew ( 1 of 5 renewals remaining / 4 of 4 renewals left before the item must be seen by the library )

So it appears to have reset the renewals. But: If I renew the item in the OPAC now, I get:

Renewed! Not renewable 

I feel like it's confusing to patrons that it says I have x renewals left, but this number is smaller than the unseen renewals available. It would make sense if the "seen" renewal reset the counter, but that doesn't appear to happen.

2) Checked my item in again and back out to reset all counters.

OPAC reads:  Renew ( 5 of 5 renewals remaining / 4 of 4 renewals left before the item must be seen by the library )

Renew once in the OPAC.

Renew ( 4 of 5 renewals remaining / 3 of 4 renewals left before the item must be seen by the library )

Renew using the renew tab in staff client:

( 3 of 5 renewals remaining / 4 of 4 renewals left before the item must be seen by the library )

So this appears to have reset the "unseen" renewals, which I think is correct. 

My issue here is that I think it will be very confusing to the patrons, do they have 3 or 4 renewals left now? (actually only 3, otherwise the item has to be checked in and out again and everything will be reset.

Maybe we should just show the patrons how many more online renewals are possible before they have to take the item to the library for renewal?

OR:
( 3 of 5 renewals remaining / 4 of 4 renewals left before the item must be seen by the library )

Would change to the following, when the unseen renewals are higher than the actual remaining ones:
( 3 of 5 renewals remaining ) 

AND:
( 4 of 5 renewals remaining / 2 of 2 renewals left before the item must be seen by the library )

Could be:
( 2 online renewals remaining before the item must be seen by the library ) 


I guess besides the confusing display I wonder if the count of renewals should be reset by a seen renewal?
Comment 75 Katrin Fischer 2020-10-22 13:18:41 UTC
3) New test case:
- 5 renewals allowed
- 2 renewals unseen
- Item checked out
- Renewed twice via the OPAC
- Now no renewal information displays, the cell is empty

Expected: 
Must be renewed at the library and some information about remaining renewals. 

Where it's very verbose before, once you reach that state, there is no information to the user at all.
Comment 76 Katrin Fischer 2020-10-22 13:30:32 UTC
4) Please add the new test for too_unseen to the AUTO_RENEW default notices, so that new installations will have it and we have a default notice to refer to.


Addition to 1) and 2), trying to be clearer about the issue:

In my last test case I see:

Renew ( 4 of 5 renewals remaining / 1 of 2 renewals left before the item must be seen by the library )

This does kind of make sense to me now.

I still think it might be clearer to leave out the unseen renewals if they no longer apply, because all remaining renewals can be done online. (2 of 5, 2 of 2 => 2 of 5)


I think 3) and 4) should be fixed. 1) and 2) might be good to put some more thought into separately, not blocker for me.
Comment 77 Andrew Isherwood 2020-10-22 13:37:26 UTC
Hi Katrin

Thanks for that, and for all the detail of your testing procedures. 

> 1)

Yes, I think you're right, some improvements could be made to consolidate the various renewal counts depending on which is more pertinent, your ideas make sense. I'll see what I can do

> 2) Checked my item in again and back out to reset all counters.
> [...]
> I guess besides the confusing display I wonder if the count of renewals
> should be reset by a seen renewal?

Do you mean the unseen count should be reset by a seen renewal? That is what should happen. In all my testing it has worked correctly, am I missing your point?

> 3. New test case:
> [...]
> - Now no renewal information displays, the cell is empty

That sounds like a bug, I'll check it out

> 4) Please add the new test for too_unseen to the AUTO_RENEW default notices, so > that new installations will have it and we have a default notice to refer to.

Will do
Comment 78 Andrew Isherwood 2020-10-23 10:09:31 UTC
Created attachment 112230 [details] [review]
Bug 24083: Required atomic updates & DB update

This patch adds:

- An "UnseenRenewals" circulation syspref that enables/disables the
  functionality added in this bug

- Add unseen_renewals_allowed to circulation_rules

- A change to the issues & old_issues table schemas and corresponding
  database upgrades to add issues.unseen_renewals &
  old_issues.unseen_renewals

- An update for the renewals letter to include a message for unseen
  renewals

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 79 Andrew Isherwood 2020-10-23 10:09:40 UTC
Created attachment 112231 [details] [review]
Bug 24083: Add circ rules management

This patch adds the ability to set the "Unseen renewals allowed" value
in the circulation rules

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 80 Andrew Isherwood 2020-10-23 10:09:47 UTC
Created attachment 112232 [details] [review]
Bug 24083: Add support for unseen_renewals

This patch adds support for unseen renewals.

Here we retrofit knowledge of unseen renewals and add the display of unseen
renewal counts and warnings, in addition to adding the ability to
specify a renewal as being "unseen".

The functionality added here is goverened by the UnseenRenewals syspref.

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 81 Andrew Isherwood 2020-10-23 10:09:54 UTC
Created attachment 112233 [details] [review]
Bug 24083: Add unit tests

This patch adds unit tests for unseen renewals

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>
Comment 82 Andrew Isherwood 2020-10-23 10:10:00 UTC
Created attachment 112234 [details] [review]
Bug 24083: (follow-up) Fix params to AddRenewal

The merging of Bug 23051 has added a paramter that can be passed to
C4::AddRenewal. This bug does also, so the offset of this bug's new
paramter was wrong. This patch fixes it.
Comment 83 Andrew Isherwood 2020-10-23 10:10:06 UTC
Created attachment 112235 [details] [review]
Bug 24083: (follow-up) Respond to QA feedback

This patch implements changes requested by Katrin in QA feedback
(comment #38):

- Fix QA script failures
- Remove notice text database update
- Fix alter table column order mismatch
- Use new convention for JS strings
- Fix terminology
Comment 84 Andrew Isherwood 2020-10-23 10:10:16 UTC
Created attachment 112236 [details] [review]
Bug 24083: (follow-up) Include SelfCheckInModule

As highlighted by Katrin, the selfcheckin module was not aware of this
work. This commits gives it awareness of the too_unseen renew error and
the "seen" argument that can be passed to AddRenewal
Comment 85 Andrew Isherwood 2020-10-23 10:10:22 UTC
Created attachment 112237 [details] [review]
Bug 24083: (follow-up) Fix error in checkouts JS

This commit fixes a small error in the checkouts related JS
Comment 86 Andrew Isherwood 2020-10-23 10:10:28 UTC
Created attachment 112238 [details] [review]
Bug 24083: (follow-up) Fix GetRenewCount return

GetRenewCount now returns 6 values when a call to it succeeds, a
failed call should also return the same number of values. This commit
adds these additional values.
Comment 87 Andrew Isherwood 2020-10-23 10:10:35 UTC
Created attachment 112239 [details] [review]
Bug 24083: (follow-up) Fix unit tests

Some changes in issue.t had broken the tests for unseen renewals (the
unseen tests were using variables that had been moved out of the tests'
scope).

Also now using Koha::CirculationRules::set_rules to set circ rules
rather than using SQL queries.

Fixed expected number of return values from GetRenewCount

Moved unseen tests in issue.t to the bottom of the file to remove the
risk of interference with other test circ rules.
Comment 88 Andrew Isherwood 2020-10-23 10:10:41 UTC
Created attachment 112240 [details] [review]
Bug 24083: (follow-up) Fix bad rebase

There was a real mess in C4/Circulation.pm due to a bad rebase back in
February. Frankly it's a wonder anything worked at all. This commit
fixes that problem and reinstates the correct patch for
C4/Circulation.pm
Comment 89 Andrew Isherwood 2020-10-23 10:10:47 UTC
Created attachment 112241 [details] [review]
Bug 24083: (follow-up) Fix bad rebase

This commit fixes a couple of lines that got missed during the last
rebase
Comment 90 Andrew Isherwood 2020-10-23 10:10:52 UTC
Created attachment 112242 [details] [review]
Bug 24083: (follow-up) Fix misaligned columns

Somehow I'd never noticed this before but the columns in smart-rules.tt
were misaligned when UnseenRenewals was turned off. This was due to the
display of a <td> not being conditional when it should have been. This
is now fixed.
Comment 91 Andrew Isherwood 2020-10-23 10:10:58 UTC
Created attachment 112243 [details] [review]
Bug 24083: (follow-up) Fix QA issues

This commit fixes items 1 & 2 descibed by Katrin in comment #74 ->
comment #76.

- Fixed missing check for too_unseen in opac-user.tt, this test did used
to exist but got lost during sizeable rebase a few weeks ago :-(
- Added test for too_unseen to all AUTO_RENEWAL notice templates apart
from de-DE (as previously requested by Katrin)
Comment 92 Andrew Isherwood 2020-10-23 10:11:19 UTC
Hi Katrin

I've just fixed 1 & 2. 1 was due to a mistake during a rebase back in early October. For 2, I didn't modify the de-DE notice as you previously requested.

As far as 3 & 4 are concerned, I'd be super keen for those to be enhancements after this is PQA. As has just been proved, repeatedly rebasing this is tricky as it's quite a sprawling piece and every time introduces the potential for error. So I, for one, would be keen to get it in as it is, then address any enhancements to wording, particularly as they're likely to be subjective.