Bug 24083

Summary: Koha should support "seen" vs "unseen" renewals
Product: Koha Reporter: Andrew Isherwood <bugzilla>
Component: CirculationAssignee: Bugs List <koha-bugs>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: andrewfh, dcook, fiona.borthwick, gmcharlt, jonathan.druart, kyle.m.hall, lucas, martin.renvoize, nick, sally.healey
Version: MainKeywords: Manual-updated
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This new feature allows the library to keep track of how many times an item has been renewed but not actually seen by the library, typically through renewing online. Additionally, this allows the library to set their circulation rules to require regular 'seen' renewals and thus prevent patrons from continually renewing an item which may actually have been lost.
Version(s) released in:
20.11.00
Bug Depends on: 18936    
Bug Blocks: 29022, 29023, 29024, 29021, 29050, 29051    
Attachments: Bug 24083: Required atomic updates & DB update
Bug 24083: Add circ rules management
Bug 24083: Add support for unseen_renewals
Bug 24083: Add unit tests
Bug 24083: Required atomic updates & DB update
Bug 24083: Add circ rules management
Bug 24083: Add support for unseen_renewals
Bug 24083: Add unit tests
Bug 24083: Required atomic updates & DB update
Bug 24083: Add circ rules management
Bug 24083: Add support for unseen_renewals
Bug 24083: Add unit tests
Bug 24083: Required atomic updates & DB update
Bug 24083: Add circ rules management
Bug 24083: Add support for unseen_renewals
Bug 24083: Add unit tests
Bug 24083: Required atomic updates & DB update
Bug 24083: Add circ rules management
Bug 24083: Add support for unseen_renewals
Bug 24083: Add unit tests
Bug 24083: Required atomic updates & DB update
Bug 24083: Add circ rules management
Bug 24083: Add support for unseen_renewals
Bug 24083: Add unit tests
Bug 24083: (follow-up) Fix params to AddRenewal
Bug 24083: Required atomic updates & DB update
Bug 24083: Add circ rules management
Bug 24083: Add support for unseen_renewals
Bug 24083: Add unit tests
Bug 24083: (follow-up) Fix params to AddRenewal
Bug 24083: Required atomic updates & DB update
Bug 24083: Add circ rules management
Bug 24083: Add support for unseen_renewals
Bug 24083: Add unit tests
Bug 24083: (follow-up) Fix params to AddRenewal
Bug 24083: (follow-up) Respond to QA feedback
Bug 24083: Required atomic updates & DB update
Bug 24083: Add circ rules management
Bug 24083: Add support for unseen_renewals
Bug 24083: Add unit tests
Bug 24083: (follow-up) Fix params to AddRenewal
Bug 24083: (follow-up) Respond to QA feedback
Bug 24083: (follow-up) Include SelfCheckInModule
Bug 24083: (follow-up) Fix error in checkouts JS
Bug 24083: (follow-up) Fix GetRenewCount return
Bug 24083: (follow-up) Fix unit tests
Bug 24083: (follow-up) Fix bad rebase
Bug 24083: Required atomic updates & DB update
Bug 24083: Add circ rules management
Bug 24083: Add support for unseen_renewals
Bug 24083: Add unit tests
Bug 24083: (follow-up) Fix params to AddRenewal
Bug 24083: (follow-up) Respond to QA feedback
Bug 24083: (follow-up) Include SelfCheckInModule
Bug 24083: (follow-up) Fix error in checkouts JS
Bug 24083: (follow-up) Fix GetRenewCount return
Bug 24083: (follow-up) Fix unit tests
Bug 24083: (follow-up) Fix bad rebase
Bug 24083: (follow-up) Fix bad rebase
Bug 24083: Required atomic updates & DB update
Bug 24083: Add circ rules management
Bug 24083: Add support for unseen_renewals
Bug 24083: Add unit tests
Bug 24083: (follow-up) Fix params to AddRenewal
Bug 24083: (follow-up) Respond to QA feedback
Bug 24083: (follow-up) Include SelfCheckInModule
Bug 24083: (follow-up) Fix error in checkouts JS
Bug 24083: (follow-up) Fix GetRenewCount return
Bug 24083: (follow-up) Fix unit tests
Bug 24083: (follow-up) Fix bad rebase
Bug 24083: (follow-up) Fix bad rebase
Bug 24083: (follow-up) Fix misaligned columns
Bug 24083: (follow-up) Fix QA issues
Bug 24083: Required atomic updates & DB update
Bug 24083: Add circ rules management
Bug 24083: Add support for unseen_renewals
Bug 24083: Add unit tests
Bug 24083: (follow-up) Fix params to AddRenewal
Bug 24083: (follow-up) Respond to QA feedback
Bug 24083: (follow-up) Include SelfCheckInModule
Bug 24083: (follow-up) Fix error in checkouts JS
Bug 24083: (follow-up) Fix GetRenewCount return
Bug 24083: (follow-up) Fix unit tests
Bug 24083: (follow-up) Fix bad rebase
Bug 24083: (follow-up) Fix bad rebase
Bug 24083: (follow-up) Fix misaligned columns
Bug 24083: (follow-up) Fix QA issues
Bug 24083: Required atomic updates & DB update
Bug 24083: Add circ rules management
Bug 24083: Add support for unseen_renewals
Bug 24083: Add unit tests
Bug 24083: (follow-up) Fix params to AddRenewal
Bug 24083: (follow-up) Respond to QA feedback
Bug 24083: (follow-up) Include SelfCheckInModule
Bug 24083: (follow-up) Squashed follow ups
Bug 24083: Delete the new rule when needed
Bug 24083: Swap the column for "Unlimited" handling
Bug 24083: Required atomic updates & DB update
Bug 24083: Add circ rules management
Bug 24083: Add support for unseen_renewals
Bug 24083: Add unit tests
Bug 24083: (follow-up) Fix params to AddRenewal
Bug 24083: (follow-up) Respond to QA feedback
Bug 24083: (follow-up) Include SelfCheckInModule
Bug 24083: (follow-up) Squashed follow ups
Bug 24083: Delete the new rule when needed
Bug 24083: Swap the column for "Unlimited" handling
Bug 24083: (follow-up) Make requested changes
Bug 24083: (QA follow-up) Fix failing tests
Bug 24083: (QA follow-up) Fix DB upgrade
Bug 24083: (QA follow-up) Fix for previous
Bug 24083: (follow-up) Assign renewals remaining string to content

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.
Comment 93 Katrin Fischer 2020-10-24 23:46:41 UTC
(In reply to Andrew Isherwood from comment #92)
> 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.

I think you meant fixed 3 and 4 and to wait with 1 and 2 :) But I am ok with this as I wrote before, improving language is not blocker for me right now.
Comment 94 Katrin Fischer 2020-10-24 23:51:57 UTC
Created attachment 112410 [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>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 95 Katrin Fischer 2020-10-24 23:52:03 UTC
Created attachment 112411 [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>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 96 Katrin Fischer 2020-10-24 23:52:11 UTC
Created attachment 112412 [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>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 97 Katrin Fischer 2020-10-24 23:52:19 UTC
Created attachment 112413 [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>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 98 Katrin Fischer 2020-10-24 23:52:25 UTC
Created attachment 112414 [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.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 99 Katrin Fischer 2020-10-24 23:52:30 UTC
Created attachment 112415 [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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 100 Katrin Fischer 2020-10-24 23:52:37 UTC
Created attachment 112416 [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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 101 Katrin Fischer 2020-10-24 23:52:42 UTC
Created attachment 112417 [details] [review]
Bug 24083: (follow-up) Fix error in checkouts JS

This commit fixes a small error in the checkouts related JS

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 102 Katrin Fischer 2020-10-24 23:52:49 UTC
Created attachment 112418 [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.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 103 Katrin Fischer 2020-10-24 23:52:55 UTC
Created attachment 112419 [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.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 104 Katrin Fischer 2020-10-24 23:53:01 UTC
Created attachment 112420 [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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 105 Katrin Fischer 2020-10-24 23:53:08 UTC
Created attachment 112421 [details] [review]
Bug 24083: (follow-up) Fix bad rebase

This commit fixes a couple of lines that got missed during the last
rebase

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 106 Katrin Fischer 2020-10-24 23:53:15 UTC
Created attachment 112422 [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.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 107 Katrin Fischer 2020-10-24 23:53:23 UTC
Created attachment 112423 [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)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 108 Jonathan Druart 2020-11-04 14:58:08 UTC
Andrew, could you try to squash some patches here?

Especially the atomicupdate file that is added then removed, and the 2 "Fix bad rebase" patches. And more if you can (the several tiny "fix" patches).
Comment 109 Andrew Isherwood 2020-11-05 10:49:58 UTC
Created attachment 113073 [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

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 110 Andrew Isherwood 2020-11-05 10:50:02 UTC
Created attachment 113074 [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>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 111 Andrew Isherwood 2020-11-05 10:50:06 UTC
Created attachment 113075 [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>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 112 Andrew Isherwood 2020-11-05 10:50:10 UTC
Created attachment 113076 [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>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 113 Andrew Isherwood 2020-11-05 10:50:14 UTC
Created attachment 113077 [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.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 114 Andrew Isherwood 2020-11-05 10:50:18 UTC
Created attachment 113078 [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
- Fix alter table column order mismatch
- Use new convention for JS strings
- Fix terminology

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 115 Andrew Isherwood 2020-11-05 10:50:22 UTC
Created attachment 113079 [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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 116 Andrew Isherwood 2020-11-05 10:50:26 UTC
Created attachment 113080 [details] [review]
Bug 24083: (follow-up) Squashed follow ups

This squashed commit fixes:
-  a small error in the checkouts related JS

- 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.

- 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.

- 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

- 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.

- This commit also 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)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 117 Andrew Isherwood 2020-11-05 10:52:25 UTC
Hi Jonathan - squashed as requested
Comment 118 Jonathan Druart 2020-11-09 15:11:41 UTC
Created attachment 113314 [details] [review]
Bug 24083: Delete the new rule when needed
Comment 119 Jonathan Druart 2020-11-09 15:11:46 UTC
Created attachment 113315 [details] [review]
Bug 24083: Swap the column for "Unlimited" handling
Comment 120 Jonathan Druart 2020-11-09 15:13:33 UTC
Hi Andrew,

I think I've found some problem with this enhancement.

1.
I have set the default circ rule:
 unseen_renewals_allowed          | 2
 renewalsallowed                  | 5 

And an issue has:
         renewals: 2
  unseen_renewals: 2

so number of unseen is reached.

I am expecting a staff member to be allowed to renew, however I get:

Cannot renew:

E Street shuffle : ( BC_1 ) has been renewed the maximum number of consecutive times without being seen by the library )

Can you confirm it's a bug?

2. If 1. is valid, why cannot we renew from circulation.pl if the max number of renewals is not reached (but number of unseen renewals is)?

3. If I am understanding correctly, the AddRenewal call in sco-main.pl always has $seen set to 1. Maybe we should then make it hardcoded in the controller (ie. no need to have the template var)

4. It seems that the too_unseen error should be handled in misc/cronjobs/automatic_renewals.pl
Don't you think?

It can still be part of 20.11 if I get answers/follow-ups before Wednesday.
Comment 121 Andrew Isherwood 2020-11-11 13:21:03 UTC
Created attachment 113484 [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

Signed-off-by: Sally Healey <sally.Healey@cheshirewestandchester.gov.uk>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 122 Andrew Isherwood 2020-11-11 13:21:07 UTC
Created attachment 113485 [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>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 123 Andrew Isherwood 2020-11-11 13:21:11 UTC
Created attachment 113486 [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>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 124 Andrew Isherwood 2020-11-11 13:21:15 UTC
Created attachment 113487 [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>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 125 Andrew Isherwood 2020-11-11 13:21:19 UTC
Created attachment 113488 [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.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 126 Andrew Isherwood 2020-11-11 13:21:23 UTC
Created attachment 113489 [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
- Fix alter table column order mismatch
- Use new convention for JS strings
- Fix terminology

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 127 Andrew Isherwood 2020-11-11 13:21:27 UTC
Created attachment 113490 [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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 128 Andrew Isherwood 2020-11-11 13:21:31 UTC
Created attachment 113491 [details] [review]
Bug 24083: (follow-up) Squashed follow ups

This squashed commit fixes:
-  a small error in the checkouts related JS

- 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.

- 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.

- 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

- 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.

- This commit also 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)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 129 Andrew Isherwood 2020-11-11 13:21:36 UTC
Created attachment 113492 [details] [review]
Bug 24083: Delete the new rule when needed
Comment 130 Andrew Isherwood 2020-11-11 13:21:40 UTC
Created attachment 113493 [details] [review]
Bug 24083: Swap the column for "Unlimited" handling
Comment 131 Andrew Isherwood 2020-11-11 13:21:44 UTC
Created attachment 113494 [details] [review]
Bug 24083: (follow-up) Make requested changes

This commit makes the changes requested in comment #120
Comment 132 Andrew Isherwood 2020-11-11 13:22:03 UTC
(In reply to Jonathan Druart from comment #120)

Hi Jonathan

> Cannot renew:
> 
> E Street shuffle : ( BC_1 ) has been renewed the maximum number of
> consecutive times without being seen by the library )
> 
> Can you confirm it's a bug?

I think you've missed the step in the test plan that says "Select the "Override renewal restrictions" checkbox" with this enabled, you will have the ability to override the restriction via a "Override limit and renew button"

> 3. If I am understanding correctly, the AddRenewal call in sco-main.pl
> always has $seen set to 1. Maybe we should then make it hardcoded in the
> controller (ie. no need to have the template var)

Good point, now changed

> 
> 4. It seems that the too_unseen error should be handled in
> misc/cronjobs/automatic_renewals.pl
> Don't you think?

Good point, now added

> It can still be part of 20.11 if I get answers/follow-ups before Wednesday.

Sorry, I somehow missed the email for this bug until this morning, hence my reply today!
Comment 133 Jonathan Druart 2020-11-11 14:29:56 UTC
(In reply to Andrew Isherwood from comment #132)
> (In reply to Jonathan Druart from comment #120)
> 
> Hi Jonathan
> 
> > Cannot renew:
> > 
> > E Street shuffle : ( BC_1 ) has been renewed the maximum number of
> > consecutive times without being seen by the library )
> > 
> > Can you confirm it's a bug?
> 
> I think you've missed the step in the test plan that says "Select the
> "Override renewal restrictions" checkbox" with this enabled, you will have
> the ability to override the restriction via a "Override limit and renew
> button"

Right, I missed that!

> > 4. It seems that the too_unseen error should be handled in
> > misc/cronjobs/automatic_renewals.pl
> > Don't you think?
> 
> Good point, now added

But then, if we decide to send the notice, the AUTO_RENEWALS template will need to be adjust. Otherwise the notice will be generated without a correct content.
Comment 134 Andrew Isherwood 2020-11-11 14:48:20 UTC
(In reply to Jonathan Druart from comment #133)

> > > 4. It seems that the too_unseen error should be handled in
> > > misc/cronjobs/automatic_renewals.pl
> > > Don't you think?
> > 
> > Good point, now added
> 
> But then, if we decide to send the notice, the AUTO_RENEWALS template will
> need to be adjust. Otherwise the notice will be generated without a correct
> content.

too_unseen is catered for in the AUTO_RENEWALS sample notice
Comment 135 Jonathan Druart 2020-11-11 15:09:17 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 136 Jonathan Druart 2020-11-11 15:10:51 UTC
Pushed but more work needed:

1. 

> > 4. It seems that the too_unseen error should be handled in
> > misc/cronjobs/automatic_renewals.pl
> > Don't you think?
> 
> Good point, now added

But then, if we decide to send the notice, the AUTO_RENEWALS template will need to be adjust. Otherwise the notice will be generated without a correct content.

2. The insert from the upgrade process is not correct. The rule_value is not passed. But even if passed it's not idempotent (ie. the row in inserted several times if run several times).
Comment 137 Jonathan Druart 2020-11-11 17:05:01 UTC
3. We have 2 failing tests

$ prove t/db_dependent/api/v1/checkouts.t
t/db_dependent/api/v1/checkouts.t .. 7/93
#   Failed test 'exact match for JSON Pointer ""'
#   at t/db_dependent/api/v1/checkouts.t line 198.
#     Structures begin differing at:
#          $got->{unseen_renewals} = '0'
#     $expected->{unseen_renewals} = Does not exist
t/db_dependent/api/v1/checkouts.t .. 84/93 
#   Failed test 'exact match for JSON Pointer ""'
#   at t/db_dependent/api/v1/checkouts.t line 217.
#     Structures begin differing at:
#          $got->{unseen_renewals} = '0'
#     $expected->{unseen_renewals} = Does not exist
# Looks like you failed 2 tests of 93.
Comment 138 Andrew Isherwood 2020-11-12 10:05:50 UTC
(In reply to Jonathan Druart from comment #136)

> 2. The insert from the upgrade process is not correct. The rule_value is not
> passed. But even if passed it's not idempotent (ie. the row in inserted
> several times if run several times).

Hi Jonathan - thanks for that. I'm afraid I'm not sure what you mean here. Are you talking about the circulation rule DB upgrade. As far as I can see it works and is idempotent. This makes me think you're talking about something else. Any chance you could clarify please?
Comment 139 Andrew Isherwood 2020-11-12 10:37:39 UTC
Created attachment 113546 [details] [review]
Bug 24083: (QA follow-up) Fix failing tests

This commit fixes the failing tests mentioned by Jonathan in comment
Comment 140 Jonathan Druart 2020-11-12 11:08:55 UTC
(In reply to Andrew Isherwood from comment #138)
> (In reply to Jonathan Druart from comment #136)
> 
> > 2. The insert from the upgrade process is not correct. The rule_value is not
> > passed. But even if passed it's not idempotent (ie. the row in inserted
> > several times if run several times).
> 
> Hi Jonathan - thanks for that. I'm afraid I'm not sure what you mean here.
> Are you talking about the circulation rule DB upgrade. As far as I can see
> it works and is idempotent. This makes me think you're talking about
> something else. Any chance you could clarify please?

No it's not:

MySQL [koha_kohadev]> select count(*) from circulation_rules;
+----------+
| count(*) |
+----------+
|       26 |
+----------+
1 row in set (0.02 sec)

MySQL [koha_kohadev]>         INSERT IGNORE INTO circulation_rules (rule_name, rule_value) VALUES ('unseen_renewals_allowed', '');
Query OK, 1 row affected (0.02 sec)

MySQL [koha_kohadev]>         INSERT IGNORE INTO circulation_rules (rule_name, rule_value) VALUES ('unseen_renewals_allowed', '');
Query OK, 1 row affected (0.01 sec)

MySQL [koha_kohadev]> select count(*) from circulation_rules;
+----------+
| count(*) |
+----------+
|       28 |
+----------+
1 row in set (0.01 sec)
Comment 141 Andrew Isherwood 2020-11-12 13:40:18 UTC
Created attachment 113556 [details] [review]
Bug 24083: (QA follow-up) Fix DB upgrade

This commit makes the addition of the 'unseen_renewals_allowed'
circulation rule idempotent, as highlighted by Jonathan in comment #140
Comment 142 Andrew Isherwood 2020-11-12 13:41:34 UTC
(In reply to Jonathan Druart from comment #140)
> (In reply to Andrew Isherwood from comment #138)
> > (In reply to Jonathan Druart from comment #136)
> > 
> > > 2. The insert from the upgrade process is not correct. The rule_value is not
> > > passed. But even if passed it's not idempotent (ie. the row in inserted
> > > several times if run several times).
> > 
> > Hi Jonathan - thanks for that. I'm afraid I'm not sure what you mean here.
> > Are you talking about the circulation rule DB upgrade. As far as I can see
> > it works and is idempotent. This makes me think you're talking about
> > something else. Any chance you could clarify please?
> 
> No it's not:
> 
> MySQL [koha_kohadev]> select count(*) from circulation_rules;
> +----------+
> | count(*) |
> +----------+
> |       26 |
> +----------+
> 1 row in set (0.02 sec)
> 
> MySQL [koha_kohadev]>         INSERT IGNORE INTO circulation_rules
> (rule_name, rule_value) VALUES ('unseen_renewals_allowed', '');
> Query OK, 1 row affected (0.02 sec)
> 
> MySQL [koha_kohadev]>         INSERT IGNORE INTO circulation_rules
> (rule_name, rule_value) VALUES ('unseen_renewals_allowed', '');
> Query OK, 1 row affected (0.01 sec)
> 
> MySQL [koha_kohadev]> select count(*) from circulation_rules;
> +----------+
> | count(*) |
> +----------+
> |       28 |
> +----------+
> 1 row in set (0.01 sec)

Aha, you're right! Now fixed.
Comment 143 Andrew Isherwood 2020-11-12 13:42:39 UTC
As an aside, while looking through updatedatabase.pl for how this is usually handled in DB upgrades, I noticed the upgrade in Bug 14866 has the same problemhttps://bugs.koha-community.org/bugzilla3/attachment.cgi?id=112225&action=diff
Comment 144 Andrew Isherwood 2020-11-12 13:46:35 UTC
Created attachment 113557 [details] [review]
Bug 24083: (QA follow-up) Fix for previous

It helps if you don't commit commented out code...
Comment 145 Jonathan Druart 2020-11-12 16:29:37 UTC
Last patches squashed and pushed, thanks Andrew!
Comment 146 Lucas Gass 2020-11-16 22:43:23 UTC
enhancement will not be backported to 20.05.x
Comment 147 Nick Clemens 2020-11-19 13:58:32 UTC
Created attachment 113814 [details] [review]
Bug 24083: (follow-up) Assign renewals remaining string to content
Comment 148 Jonathan Druart 2020-11-20 07:19:32 UTC
(In reply to Nick Clemens from comment #147)
> Created attachment 113814 [details] [review] [review]
> Bug 24083: (follow-up) Assign renewals remaining string to content

Pushed to master for 20.11
Comment 149 Andrew Fuerste-Henry 2020-12-09 21:18:42 UTC
Enhancement, not backported to 20.05