Bug 29062

Summary: Patron check-in slip repeats data
Product: Koha Reporter: Lucas Gass <lucas>
Component: Staff interfaceAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: andrewfh, fridolin.somers, gmcharlt, jonathan.druart, kyle, lisettepalouse+koha, m.de.rooy, nick
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05
Bug Depends on: 12224    
Bug Blocks:    
Attachments: Bug 29062: Use primary key issued_id to fetch old_issues for letters
Bug 29062: Use primary key issued_id to fetch old_issues for letters
Bug 29062: (follow-up) Adjust SendCirculationAlert
Bug 29062: (follow-up) Adjust SendCirculationAlert
Bug 29062: Use primary key issued_id to fetch old_issues for letters
Bug 29062: (follow-up) Adjust SendCirculationAlert
Bug 29062: (QA follow-up) Add comment
Bug 29062: Unit tests

Description Lucas Gass 2021-09-20 20:35:23 UTC
The patron check in slip repeats data for each time that particular itemnumber is found in old_issues.

TO recreate:

1. Check something out to patron A. Check it in.
2. Check something out to patron B. Check it in.
3. Check something out to patron C. Check it in and print the check-in slip.
4. You will see the checkin repeat itself 3 times, one for each line in old_issues.
Comment 1 Nick Clemens 2021-09-22 11:55:46 UTC
Created attachment 125129 [details] [review]
Bug 29062: Use primary key issued_id to fetch old_issues for letters

The code currently uses itemnumber to fetch old_issues for notices.
This doesn't seem to be used in any current notices except the CHECKINSLIP:
SELECTY letter.code,content FROM letter WHERE content LIKE 'old\\_%'\G

For issues we use itemnumber, however, issues has a constraint to limit issues for an itemnumber to 1
Old issues has no such constraint, we try to rectify this in the old code by adding 'ORDER BY returndate DESC LIMIT 1"

As the code is not used by default and buggy I think we can make a change to using 'issue_id' as the key and
announcing the change - it prevents leaky data

To test:
1. Check something out to patron A. Check it in.
2. Check something out to patron B. Check it in.
3. Check something out to patron C. Check it in and print the check-in slip. (Leave the checkin paghe open)
4. You will see the checkin repeat itself 3 times, one for each line in old_issues.
5. Apply patch and restart_all
6. Click the 'Print checkin slip' button again
7. You see a single checkin
8. Checkout a different item to patron A. Check it in and print the check-in slip
9. See the correct checkins
Comment 2 Lucas Gass 2021-09-22 15:38:02 UTC
Created attachment 125154 [details] [review]
Bug 29062: Use primary key issued_id to fetch old_issues for letters

The code currently uses itemnumber to fetch old_issues for notices.
This doesn't seem to be used in any current notices except the CHECKINSLIP:
SELECTY letter.code,content FROM letter WHERE content LIKE 'old\\_%'\G

For issues we use itemnumber, however, issues has a constraint to limit issues for an itemnumber to 1
Old issues has no such constraint, we try to rectify this in the old code by adding 'ORDER BY returndate DESC LIMIT 1"

As the code is not used by default and buggy I think we can make a change to using 'issue_id' as the key and
announcing the change - it prevents leaky data

To test:
1. Check something out to patron A. Check it in.
2. Check something out to patron B. Check it in.
3. Check something out to patron C. Check it in and print the check-in slip. (Leave the checkin paghe open)
4. You will see the checkin repeat itself 3 times, one for each line in old_issues.
5. Apply patch and restart_all
6. Click the 'Print checkin slip' button again
7. You see a single checkin
8. Checkout a different item to patron A. Check it in and print the check-in slip
9. See the correct checkins

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 3 Marcel de Rooy 2021-09-23 14:34:29 UTC
This might be hard to find another occurrence.
But look at this code in SendCirculationAlert

    my $issues_table = ( $type eq 'CHECKOUT' || $type eq 'RENEWAL' ) ? 'issues' : 'old_issues';
[etc]
            tables => {
                $issues_table => $item->{itemnumber},

This should no longer work.

How do you know if there no other occurrences like that one?
Comment 4 Nick Clemens 2021-10-04 18:44:22 UTC
Created attachment 125712 [details] [review]
Bug 29062: (follow-up) Adjust SendCirculationAlert
Comment 5 Nick Clemens 2021-10-04 18:59:13 UTC
(In reply to Marcel de Rooy from comment #3)
> This might be hard to find another occurrence.
> But look at this code in SendCirculationAlert
> This should no longer work.

Fixed

> 
> How do you know if there no other occurrences like that one?

according to tools/letters.pl only 
CHECKIN and CHECKINSLIP have the old_issues table, infact, in printslip we don't even pass the old_issues table, but we send it using the 'loops' parameter

I added a patch that fixes that call, I could find no others, and we don't indicate support elsewhere

The sample notices make no use of the old_issues table, so another option would be to remove it from the options all together
Comment 6 Marcel de Rooy 2021-10-05 11:26:34 UTC
+        $issues_id = $item->{itemnumber};

I am having serious doubts about that change ;)
Comment 7 Marcel de Rooy 2021-10-05 11:28:18 UTC
(In reply to Marcel de Rooy from comment #6)
> +        $issues_id = $item->{itemnumber};
> 
> I am having serious doubts about that change ;)

Especially the way it reads btw. Maybe find a good rename..
Comment 8 Nick Clemens 2021-10-05 17:52:20 UTC
Created attachment 125749 [details] [review]
Bug 29062: (follow-up) Adjust SendCirculationAlert
Comment 9 Marcel de Rooy 2021-10-06 13:34:21 UTC
Created attachment 125806 [details] [review]
Bug 29062: Use primary key issued_id to fetch old_issues for letters

The code currently uses itemnumber to fetch old_issues for notices.
This doesn't seem to be used in any current notices except the CHECKINSLIP:
SELECTY letter.code,content FROM letter WHERE content LIKE 'old\\_%'\G

For issues we use itemnumber, however, issues has a constraint to limit issues for an itemnumber to 1
Old issues has no such constraint, we try to rectify this in the old code by adding 'ORDER BY returndate DESC LIMIT 1"

As the code is not used by default and buggy I think we can make a change to using 'issue_id' as the key and
announcing the change - it prevents leaky data

To test:
1. Check something out to patron A. Check it in.
2. Check something out to patron B. Check it in.
3. Check something out to patron C. Check it in and print the check-in slip. (Leave the checkin paghe open)
4. You will see the checkin repeat itself 3 times, one for each line in old_issues.
5. Apply patch and restart_all
6. Click the 'Print checkin slip' button again
7. You see a single checkin
8. Checkout a different item to patron A. Check it in and print the check-in slip
9. See the correct checkins

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2021-10-06 13:34:24 UTC
Created attachment 125807 [details] [review]
Bug 29062: (follow-up) Adjust SendCirculationAlert

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2021-10-06 13:34:28 UTC
Created attachment 125809 [details] [review]
Bug 29062: (QA follow-up) Add comment

Wrapped it in a TODO ;)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Jonathan Druart 2021-10-07 08:18:27 UTC
Missing test coverage.
Comment 13 Marcel de Rooy 2021-10-07 08:24:09 UTC
(In reply to Jonathan Druart from comment #12)
> Missing test coverage.

I was permissive here for the current lack of tests of SendCirculationAlert.
Comment 14 Nick Clemens 2021-10-07 13:13:52 UTC
Created attachment 125879 [details] [review]
Bug 29062: Unit tests
Comment 15 Jonathan Druart 2021-10-07 13:45:15 UTC
(In reply to Nick Clemens from comment #14)
> Created attachment 125879 [details] [review] [review]
> Bug 29062: Unit tests

Thanks!
Comment 16 Jonathan Druart 2021-10-07 13:48:25 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 17 Kyle M Hall 2021-10-08 15:21:04 UTC
Pushed to 21.05.x for 21.05.05
Comment 18 Fridolin Somers 2021-10-21 06:05:25 UTC
Depends on Bug 12224 not in 20.11.x