Bug 23916 - Issuer should be recorded and visible in patron circulation history
Summary: Issuer should be recorded and visible in patron circulation history
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Andrew Isherwood
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 24321
Blocks:
  Show dependency treegraph
 
Reported: 2019-10-29 11:49 UTC by Andrew Isherwood
Modified: 2020-11-22 00:36 UTC (History)
11 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Text to go in the release notes:
This new enhancement enables the recording, and subsequent display, of who checked out an item. When viewing a checkout in a patron's circulation history and an item's checkout history, details of who carried out the checkout is displayed. The recording of this information is controlled by the new system preference "RecordStaffUserOnCheckout".
Version(s) released in:
20.11.00


Attachments
Bug 23916: Add issues.issuer column and FK (6.63 KB, patch)
2019-10-31 14:40 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add "RecordIssuer" syspref (3.37 KB, patch)
2019-10-31 14:40 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Record and display item issuer (7.44 KB, patch)
2019-10-31 14:40 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Anonymise 'issuer' when required (1.04 KB, patch)
2019-10-31 14:40 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add unit tests (5.49 KB, patch)
2019-10-31 14:40 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix column name collision (1.93 KB, patch)
2019-11-18 12:33 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add issues.issuer column and FK (6.68 KB, patch)
2019-11-18 13:48 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add "RecordIssuer" syspref (3.42 KB, patch)
2019-11-18 13:48 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Record and display item issuer (7.49 KB, patch)
2019-11-18 13:48 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Anonymise 'issuer' when required (1.09 KB, patch)
2019-11-18 13:48 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add unit tests (5.54 KB, patch)
2019-11-18 13:48 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix column name collision (1.98 KB, patch)
2019-11-18 13:48 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add issues.issuer column and FK (6.68 KB, patch)
2020-01-22 13:44 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add "RecordIssuer" syspref (3.42 KB, patch)
2020-01-22 13:44 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Record and display item issuer (7.36 KB, patch)
2020-01-22 13:44 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Anonymise 'issuer' when required (1.09 KB, patch)
2020-01-22 13:44 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add unit tests (5.39 KB, patch)
2020-01-22 13:44 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix column name collision (1.98 KB, patch)
2020-01-22 13:44 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add issues.issuer column and FK (6.73 KB, patch)
2020-02-07 20:17 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 23916: Add "RecordIssuer" syspref (3.46 KB, patch)
2020-02-07 20:17 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 23916: Record and display item issuer (7.38 KB, patch)
2020-02-07 20:17 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 23916: Anonymise 'issuer' when required (1.15 KB, patch)
2020-02-07 20:17 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 23916: Add unit tests (5.43 KB, patch)
2020-02-07 20:17 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix column name collision (2.03 KB, patch)
2020-02-07 20:17 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 24495: Reword change collection feature (3.14 KB, patch)
2020-02-07 21:13 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 23916: Add issues.issuer column and FK (6.74 KB, patch)
2020-03-02 11:06 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add "RecordIssuer" syspref (3.47 KB, patch)
2020-03-02 11:06 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Record and display item issuer (6.74 KB, patch)
2020-03-02 11:06 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Anonymise 'issuer' when required (1.15 KB, patch)
2020-03-02 11:06 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add unit tests (5.45 KB, patch)
2020-03-02 11:06 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix column name collision (2.04 KB, patch)
2020-03-02 11:06 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add issues.issuer column and FK (6.73 KB, patch)
2020-09-24 13:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add "RecordIssuer" syspref (3.66 KB, patch)
2020-09-24 13:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Record and display item issuer (6.70 KB, patch)
2020-09-24 13:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Anonymise 'issuer' when required (1.15 KB, patch)
2020-09-24 13:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add unit tests (5.35 KB, patch)
2020-09-24 13:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix column name collision (2.04 KB, patch)
2020-09-24 13:11 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Implement feedback (3.49 KB, patch)
2020-09-24 13:11 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add issues.issuer column and FK (6.73 KB, patch)
2020-10-12 09:33 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add "RecordIssuer" syspref (3.66 KB, patch)
2020-10-12 09:33 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Record and display item issuer (6.70 KB, patch)
2020-10-12 09:33 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Anonymise 'issuer' when required (1.15 KB, patch)
2020-10-12 09:33 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add unit tests (5.35 KB, patch)
2020-10-12 09:33 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix column name collision (2.04 KB, patch)
2020-10-12 09:33 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Implement feedback (3.49 KB, patch)
2020-10-12 09:34 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Respond to feedback (12.25 KB, patch)
2020-10-12 09:34 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add issues.issuer column and FK (6.73 KB, patch)
2020-10-19 12:41 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add "RecordIssuer" syspref (3.70 KB, patch)
2020-10-19 12:41 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Record and display item issuer (6.70 KB, patch)
2020-10-19 12:41 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Anonymise 'issuer' when required (1.15 KB, patch)
2020-10-19 12:41 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add unit tests (5.60 KB, patch)
2020-10-19 12:41 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix column name collision (2.04 KB, patch)
2020-10-19 12:41 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Implement feedback (3.49 KB, patch)
2020-10-19 12:41 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Respond to feedback (12.35 KB, patch)
2020-10-19 12:41 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Respond to feedback (2.95 KB, patch)
2020-10-19 12:41 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add issues.issuer column and FK (6.73 KB, patch)
2020-10-23 11:20 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add "RecordIssuer" syspref (3.70 KB, patch)
2020-10-23 11:20 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Record and display item issuer (6.70 KB, patch)
2020-10-23 11:21 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Anonymise 'issuer' when required (1.15 KB, patch)
2020-10-23 11:21 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add unit tests (5.60 KB, patch)
2020-10-23 11:21 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix column name collision (2.04 KB, patch)
2020-10-23 11:21 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Implement feedback (3.49 KB, patch)
2020-10-23 11:21 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Respond to feedback (12.35 KB, patch)
2020-10-23 11:21 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Respond to feedback (2.95 KB, patch)
2020-10-23 11:21 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix unit test (1.51 KB, patch)
2020-10-23 11:21 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add issues.issuer column and FK (6.74 KB, patch)
2020-10-24 03:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23916: Add "RecordIssuer" syspref (3.70 KB, patch)
2020-10-24 03:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23916: Record and display item issuer (6.74 KB, patch)
2020-10-24 03:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23916: Anonymise 'issuer' when required (1.15 KB, patch)
2020-10-24 03:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23916: Add unit tests (5.61 KB, patch)
2020-10-24 03:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix column name collision (2.04 KB, patch)
2020-10-24 03:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23916: (follow-up) Don't anonymise issuer and don't update action_logs on upgrade (3.55 KB, patch)
2020-10-24 03:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix terminology, use patron-title and make column consistent in issues and old_issues (12.48 KB, patch)
2020-10-24 03:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23916: (follow-up) Rebase unit tests (2.95 KB, patch)
2020-10-24 03:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix unit test (1.51 KB, patch)
2020-10-24 03:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23916: (QA follow-up) Remove stray changes and fix typo (2.70 KB, patch)
2020-10-24 03:05 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23916: Add issues.issuer column and FK (6.73 KB, patch)
2020-11-05 09:05 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add "RecordIssuer" syspref (3.70 KB, patch)
2020-11-05 09:05 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Record and display item issuer (6.78 KB, patch)
2020-11-05 09:05 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Anonymise 'issuer' when required (1.15 KB, patch)
2020-11-05 09:05 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add unit tests (5.60 KB, patch)
2020-11-05 09:05 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix column name collision (2.04 KB, patch)
2020-11-05 09:05 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Don't anonymise issuer and don't update action_logs on upgrade (3.53 KB, patch)
2020-11-05 09:06 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix terminology, use patron-title and make column consistent in issues and old_issues (12.41 KB, patch)
2020-11-05 09:06 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Rebase unit tests (2.95 KB, patch)
2020-11-05 09:06 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix unit test (1.51 KB, patch)
2020-11-05 09:06 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: (QA follow-up) Remove stray changes and fix typo (2.70 KB, patch)
2020-11-05 09:06 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23916: Add issues.issuer column and FK (6.78 KB, patch)
2020-11-09 15:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23916: Add "RecordIssuer" syspref (3.75 KB, patch)
2020-11-09 15:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23916: Record and display item issuer (6.83 KB, patch)
2020-11-09 15:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23916: Anonymise 'issuer' when required (1.21 KB, patch)
2020-11-09 15:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23916: Add unit tests (5.65 KB, patch)
2020-11-09 15:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix column name collision (2.10 KB, patch)
2020-11-09 15:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23916: (follow-up) Don't anonymise issuer and don't update action_logs on upgrade (3.59 KB, patch)
2020-11-09 15:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix terminology, use patron-title and make column consistent in issues and old_issues (12.47 KB, patch)
2020-11-09 15:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23916: (follow-up) Rebase unit tests (3.01 KB, patch)
2020-11-09 15:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix unit test (1.56 KB, patch)
2020-11-09 15:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23916: (QA follow-up) Remove stray changes and fix typo (2.75 KB, patch)
2020-11-09 15:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23916: (follow-up) Add unit tests (1002 bytes, patch)
2020-11-09 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23916: Add issues.issuer column and FK (6.84 KB, patch)
2020-11-10 11:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23916: Add "RecordIssuer" syspref (3.81 KB, patch)
2020-11-10 11:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23916: Record and display item issuer (6.90 KB, patch)
2020-11-10 11:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23916: Anonymise 'issuer' when required (1.26 KB, patch)
2020-11-10 11:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23916: Add unit tests (5.71 KB, patch)
2020-11-10 11:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix column name collision (2.15 KB, patch)
2020-11-10 11:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23916: (follow-up) Don't anonymise issuer and don't update action_logs on upgrade (3.65 KB, patch)
2020-11-10 12:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix terminology, use patron-title and make column consistent in issues and old_issues (12.54 KB, patch)
2020-11-10 12:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23916: (follow-up) Rebase unit tests (3.06 KB, patch)
2020-11-10 12:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23916: (follow-up) Fix unit test (1.62 KB, patch)
2020-11-10 12:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23916: (QA follow-up) Remove stray changes and fix typo (2.81 KB, patch)
2020-11-10 12:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23916: (follow-up) Add unit tests (1.04 KB, patch)
2020-11-10 12:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23916: (QA follow-up) Rename issuer => issuer_id [DB] (10.97 KB, patch)
2020-11-10 12:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23916: Add new attribute to API (962 bytes, patch)
2020-11-10 12:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23916: (QA follow-up) Adapt all the things to the new column and accessor names (8.51 KB, patch)
2020-11-10 12:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23916: Allow embedding the issuer object (1.63 KB, patch)
2020-11-10 12:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23916: (follow-up) Adapt all the things to the new column and accessor names (3.47 KB, patch)
2020-11-10 13:46 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2019-10-29 11:49:22 UTC
Currently, when an item is issued, the issuer (member of staff, or self check or whatever) is not recorded directly with the issue. If the syspref is turned on then it is recorded in the associated action log. The issuer should be directly recorded with the issue (in a new "issues.issuer" column) and then displayed in the patron's circulation history.

As part of the DB upgrade, an attempt should also be made to retrospectively populate issuers using any available action logs.
Comment 1 Katrin Fischer 2019-10-29 21:25:31 UTC
With data privacy in mind... wouldn't recording the issue_id in statistics or action_logs maybe be enough? Or could this be made optional?
I think having basically the same data in multiple places might not be the right way, especially with no tools to clean/anonymize the data.
Comment 2 Andrew Isherwood 2019-10-30 10:50:57 UTC
Hi Katrin

To me it seems that the correct place to record this is with the issue, storing an issue ID in the action logs feels a bit like a hack. We have been known to purge action_logs at a customer's request during an upgrade. This would mean that relationship between the issue and they issuer would then be lost. Storing the issuer with the issue will be forever! :)

I'm not sure it raises any data privacy issues that do not equally apply to storing the borrower with the issue, which we already do, as well as in the action_logs.
Comment 3 Andrew Isherwood 2019-10-30 10:53:20 UTC
Development note: The data being stored here (issuer) should also be made visible in a biblio's "Checkout history" page
Comment 4 Katrin Fischer 2019-10-30 19:39:04 UTC
For the borrower-issue information we have a means of anonymizing via the GUI - so libraries can clean the data and it's not stored forever. The action_logs can also be cleaned by cronjob if the library chooses to. Cleaning/anonymizing statistics has been discussed in the GDPR meetings (https://wiki.koha-community.org/wiki/Gdpr)

I think we should make it a general habit not to add more person related information, without thinking about how to limit/anonymize/remove it if required, keeping the principle of data minimization in mind. (GDPR etc.)

Libraries here are rather critical about reading histories and the like already, so that's why I was asking for optional. :) I'd be really interested what the use case is. - Always good to learn about others needs/requirements.
Comment 5 Chris Cormack 2019-10-30 19:43:47 UTC
NZ is just going through privacy legislation rule, and will bring us much closer to the GDPR too, so I agree this should be optional

(The library will need to assume responsibility for a data breach, and report it and be liable for damages etc, so it's a choice they should conciously make)
Comment 6 Andrew Isherwood 2019-10-31 09:24:56 UTC
Hi both

Thanks for the input. I can put it behind a syspref. I can also ensure that Koha::Patrons::anonymise_issue_history anonymises the 'issuer' column, in addition to 'borrowernumber'. Would this address the concerns?

As far as a use case is concern, it's displaying the issuer, so staff can see an issue's issuer
Comment 7 Katrin Fischer 2019-10-31 12:39:20 UTC
> As far as a use case is concern, it's displaying the issuer, so staff can
> see an issue's issuer

Still wondering in which context the person who issued something is interesting :)
Comment 8 Andrew Isherwood 2019-10-31 14:40:03 UTC
Created attachment 94914 [details] [review]
Bug 23916: Add issues.issuer column and FK

This patch adds the issues.issuer column which is a foreign key to
borrowers.borrowernumber
Comment 9 Andrew Isherwood 2019-10-31 14:40:06 UTC
Created attachment 94915 [details] [review]
Bug 23916: Add "RecordIssuer" syspref

This patch adds the "RecordIssuer" syspref, which is disabled by default
Comment 10 Andrew Isherwood 2019-10-31 14:40:09 UTC
Created attachment 94916 [details] [review]
Bug 23916: Record and display item issuer

This patch adds the recording and display of the item issuer. This
behaviour is governed by the RecordIssuer syspref, if disabled (the
default), no recording or display of issuer will take place.
Comment 11 Andrew Isherwood 2019-10-31 14:40:12 UTC
Created attachment 94917 [details] [review]
Bug 23916: Anonymise 'issuer' when required

This patch adds the anonymisation of 'old_issues.issuer' alongside the
existing anonymisation of 'old_issues.borrowernumber'
Comment 12 Andrew Isherwood 2019-10-31 14:40:16 UTC
Created attachment 94918 [details] [review]
Bug 23916: Add unit tests

This patch adds unit tests for the new functions
Comment 13 Andrew Isherwood 2019-10-31 16:36:36 UTC
Test plan:
---------

1.  Ensure the new "RecordIssuer" is turned off (the default) setting.
2.  Issue an item to a patron
3.  Check the patron's circulation history
4.  TEST => Observe that the table does not contain a "Checked out by" column
5.  Check the item's checkout history
6.  TEST => Observe that the table does not contain a "Checked out by" column
7.  Turn the "RecordIssuer" syspref on
8.  Issue another item to the patron
9.  Check the patron's circulation history
10. TEST => Observe that the table now contains an "Checked out by" column
11. TEST => Observe that the column contains the details of the issuing user with a link to their patron record ONLY for the issue for which the syspref was turned on
11. Check the item's checkout history
12. TEST => Observe that the table now contains an "Checked out by" column
13. TEST => Observe that the column contains the details of the issuing user with a link to their patron record ONLY for the issue for which the syspref was turned on
Comment 14 Andrew Isherwood 2019-11-18 12:33:23 UTC
Created attachment 95503 [details] [review]
Bug 23916: (follow-up) Fix column name collision

Seems that joining 'borrowers' into the query run in
C4::Members::GetAllIssues caused a column name collision, which blatted
the retrieval of issues.branchcode. So we now are being a bit more
specific in what is coming from where in the query.
Comment 15 Andrew Isherwood 2019-11-18 13:48:06 UTC
Created attachment 95504 [details] [review]
Bug 23916: Add issues.issuer column and FK

This patch adds the issues.issuer column which is a foreign key to
borrowers.borrowernumber

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Comment 16 Andrew Isherwood 2019-11-18 13:48:09 UTC
Created attachment 95505 [details] [review]
Bug 23916: Add "RecordIssuer" syspref

This patch adds the "RecordIssuer" syspref, which is disabled by default

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Comment 17 Andrew Isherwood 2019-11-18 13:48:12 UTC
Created attachment 95506 [details] [review]
Bug 23916: Record and display item issuer

This patch adds the recording and display of the item issuer. This
behaviour is governed by the RecordIssuer syspref, if disabled (the
default), no recording or display of issuer will take place.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Comment 18 Andrew Isherwood 2019-11-18 13:48:15 UTC
Created attachment 95507 [details] [review]
Bug 23916: Anonymise 'issuer' when required

This patch adds the anonymisation of 'old_issues.issuer' alongside the
existing anonymisation of 'old_issues.borrowernumber'

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Comment 19 Andrew Isherwood 2019-11-18 13:48:18 UTC
Created attachment 95508 [details] [review]
Bug 23916: Add unit tests

This patch adds unit tests for the new functions

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Comment 20 Andrew Isherwood 2019-11-18 13:48:22 UTC
Created attachment 95509 [details] [review]
Bug 23916: (follow-up) Fix column name collision

Seems that joining 'borrowers' into the query run in
C4::Members::GetAllIssues caused a column name collision, which blatted
the retrieval of issues.branchcode. So we now are being a bit more
specific in what is coming from where in the query.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Comment 21 Andrew Isherwood 2020-01-22 13:44:07 UTC
Created attachment 97719 [details] [review]
Bug 23916: Add issues.issuer column and FK

This patch adds the issues.issuer column which is a foreign key to
borrowers.borrowernumber

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Comment 22 Andrew Isherwood 2020-01-22 13:44:10 UTC
Created attachment 97720 [details] [review]
Bug 23916: Add "RecordIssuer" syspref

This patch adds the "RecordIssuer" syspref, which is disabled by default

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Comment 23 Andrew Isherwood 2020-01-22 13:44:13 UTC
Created attachment 97721 [details] [review]
Bug 23916: Record and display item issuer

This patch adds the recording and display of the item issuer. This
behaviour is governed by the RecordIssuer syspref, if disabled (the
default), no recording or display of issuer will take place.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Comment 24 Andrew Isherwood 2020-01-22 13:44:16 UTC
Created attachment 97722 [details] [review]
Bug 23916: Anonymise 'issuer' when required

This patch adds the anonymisation of 'old_issues.issuer' alongside the
existing anonymisation of 'old_issues.borrowernumber'

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Comment 25 Andrew Isherwood 2020-01-22 13:44:19 UTC
Created attachment 97723 [details] [review]
Bug 23916: Add unit tests

This patch adds unit tests for the new functions

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Comment 26 Andrew Isherwood 2020-01-22 13:44:22 UTC
Created attachment 97724 [details] [review]
Bug 23916: (follow-up) Fix column name collision

Seems that joining 'borrowers' into the query run in
C4::Members::GetAllIssues caused a column name collision, which blatted
the retrieval of issues.branchcode. So we now are being a bit more
specific in what is coming from where in the query.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Comment 27 Andrew Isherwood 2020-01-22 13:44:37 UTC
Rebased
Comment 28 Bouzid Fergani 2020-02-07 20:17:18 UTC
Created attachment 98586 [details] [review]
Bug 23916: Add issues.issuer column and FK

This patch adds the issues.issuer column which is a foreign key to
borrowers.borrowernumber

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 29 Bouzid Fergani 2020-02-07 20:17:23 UTC
Created attachment 98587 [details] [review]
Bug 23916: Add "RecordIssuer" syspref

This patch adds the "RecordIssuer" syspref, which is disabled by default

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 30 Bouzid Fergani 2020-02-07 20:17:28 UTC
Created attachment 98588 [details] [review]
Bug 23916: Record and display item issuer

This patch adds the recording and display of the item issuer. This
behaviour is governed by the RecordIssuer syspref, if disabled (the
default), no recording or display of issuer will take place.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 31 Bouzid Fergani 2020-02-07 20:17:32 UTC
Created attachment 98589 [details] [review]
Bug 23916: Anonymise 'issuer' when required

This patch adds the anonymisation of 'old_issues.issuer' alongside the
existing anonymisation of 'old_issues.borrowernumber'

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 32 Bouzid Fergani 2020-02-07 20:17:36 UTC
Created attachment 98590 [details] [review]
Bug 23916: Add unit tests

This patch adds unit tests for the new functions

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 33 Bouzid Fergani 2020-02-07 20:17:40 UTC
Created attachment 98591 [details] [review]
Bug 23916: (follow-up) Fix column name collision

Seems that joining 'borrowers' into the query run in
C4::Members::GetAllIssues caused a column name collision, which blatted
the retrieval of issues.branchcode. So we now are being a bit more
specific in what is coming from where in the query.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 34 Bouzid Fergani 2020-02-07 21:13:58 UTC Comment hidden (obsolete)
Comment 35 Andrew Isherwood 2020-03-02 11:06:41 UTC
Created attachment 99886 [details] [review]
Bug 23916: Add issues.issuer column and FK

This patch adds the issues.issuer column which is a foreign key to
borrowers.borrowernumber

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 36 Andrew Isherwood 2020-03-02 11:06:44 UTC
Created attachment 99887 [details] [review]
Bug 23916: Add "RecordIssuer" syspref

This patch adds the "RecordIssuer" syspref, which is disabled by default

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 37 Andrew Isherwood 2020-03-02 11:06:47 UTC
Created attachment 99888 [details] [review]
Bug 23916: Record and display item issuer

This patch adds the recording and display of the item issuer. This
behaviour is governed by the RecordIssuer syspref, if disabled (the
default), no recording or display of issuer will take place.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 38 Andrew Isherwood 2020-03-02 11:06:50 UTC
Created attachment 99889 [details] [review]
Bug 23916: Anonymise 'issuer' when required

This patch adds the anonymisation of 'old_issues.issuer' alongside the
existing anonymisation of 'old_issues.borrowernumber'

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 39 Andrew Isherwood 2020-03-02 11:06:53 UTC
Created attachment 99890 [details] [review]
Bug 23916: Add unit tests

This patch adds unit tests for the new functions

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 40 Andrew Isherwood 2020-03-02 11:06:56 UTC
Created attachment 99891 [details] [review]
Bug 23916: (follow-up) Fix column name collision

Seems that joining 'borrowers' into the query run in
C4::Members::GetAllIssues caused a column name collision, which blatted
the retrieval of issues.branchcode. So we now are being a bit more
specific in what is coming from where in the query.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 41 Nick Clemens 2020-08-06 13:27:49 UTC
Tests need rebase, I got some fails

If this relies on a syspref then the update should not attempt to convert action_logs to the issuer column - please make this a script that libraries can optionally run, or leave it out.

When anonymising checkouts that have no issuer we convert NULL to the anonymous borrowernumber - I believe we should exclude these and preserve the NULL
Comment 42 Andrew Isherwood 2020-09-24 12:52:56 UTC
(In reply to Nick Clemens from comment #41)

Hi Nick

> Tests need rebase, I got some fails

I can't replicate this:

kohadev-koha@kohadevbox:/kohadevbox/koha$ ../qa-test-tools/koha-qa.pl -c 6
testing 6 commit(s) (applied to 3d06cbb '5a Bug 19889: (follow-up) update DB a')

Processing files before patches
|========================>| 11 / 11 (100.00%)
Processing files after patches
|========================>| 11 / 11 (100.00%)

 OK     C4/Circulation.pm
 OK     C4/Members.pm
 OK     Koha/Checkout.pm
 OK     Koha/Patrons.pm
 OK     installer/data/mysql/kohastructure.sql
 OK     installer/data/mysql/sysprefs.sql
 OK     koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/issuehistory.tt
 OK     koha-tmpl/intranet-tmpl/prog/en/modules/members/readingrec.tt
 OK     t/db_dependent/Circulation.t
 OK     t/db_dependent/Koha/Checkouts.t
 OK     t/db_dependent/Koha/Patrons.t

Processing additional checks OK!

> If this relies on a syspref then the update should not attempt to convert
> action_logs to the issuer column - please make this a script that libraries
> can optionally run, or leave it out.

Good point. The retrospective migration of old issues into action_logs wasn't ideal as there are definitely cases where it could give erroneous results. I'm going to drop it.

> When anonymising checkouts that have no issuer we convert NULL to the
> anonymous borrowernumber - I believe we should exclude these and preserve
> the NULL

OK, I'll change this
Comment 43 Andrew Isherwood 2020-09-24 13:10:35 UTC
Created attachment 110649 [details] [review]
Bug 23916: Add issues.issuer column and FK

This patch adds the issues.issuer column which is a foreign key to
borrowers.borrowernumber

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 44 Andrew Isherwood 2020-09-24 13:10:40 UTC
Created attachment 110650 [details] [review]
Bug 23916: Add "RecordIssuer" syspref

This patch adds the "RecordIssuer" syspref, which is disabled by default

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 45 Andrew Isherwood 2020-09-24 13:10:45 UTC
Created attachment 110651 [details] [review]
Bug 23916: Record and display item issuer

This patch adds the recording and display of the item issuer. This
behaviour is governed by the RecordIssuer syspref, if disabled (the
default), no recording or display of issuer will take place.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 46 Andrew Isherwood 2020-09-24 13:10:49 UTC
Created attachment 110652 [details] [review]
Bug 23916: Anonymise 'issuer' when required

This patch adds the anonymisation of 'old_issues.issuer' alongside the
existing anonymisation of 'old_issues.borrowernumber'

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 47 Andrew Isherwood 2020-09-24 13:10:56 UTC
Created attachment 110653 [details] [review]
Bug 23916: Add unit tests

This patch adds unit tests for the new functions

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 48 Andrew Isherwood 2020-09-24 13:11:02 UTC
Created attachment 110654 [details] [review]
Bug 23916: (follow-up) Fix column name collision

Seems that joining 'borrowers' into the query run in
C4::Members::GetAllIssues caused a column name collision, which blatted
the retrieval of issues.branchcode. So we now are being a bit more
specific in what is coming from where in the query.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 49 Andrew Isherwood 2020-09-24 13:11:07 UTC
Created attachment 110655 [details] [review]
Bug 23916: (follow-up) Implement feedback

This commit implements the two suggestions made by Nick in comment #41:

- Removes the attempted retrospective addition of issuers to old issues.
It was not particularly reliable, it also is not appropriate if the user
does not wish to make use of this functionality

- Removes the conversion of a NULL issuer to the anonymous borrowernumber when
anonymising checkouts
Comment 50 Katrin Fischer 2020-10-11 11:55:57 UTC
Trying to give this one a go.

1) Terminology

issue is not supposed to be used according to our terminology list. We prefer checkout. But... I am not sure what the better term for the person doing the checkout would be here. In some other cases we have used "manager" (accountlines). But if we wanted to record the person doing the checkin at some point, this won't work. For the pref... RecordStaffUserOnCheckout or similar could work. Overall not an easy one.

I'll only be asking for changes in the GUI.

+            - "When issuing an item, "
+            - pref: RecordIssuer
+              choices:
+                  yes: "record"
+                  no: "don't record"
+            - "the user who issued the item."

Please change to "staff user" and "issu*" to "check* out".

Everywhere else looks good.

2) Database update and installer don't match up

for old_issues.issuer we have default null in kohastructure.sql, but not in the database update.

Maybe this has been discussed, but why not default null on issues.issuer? On first glance it would make sense for me as we will also use NULL on all the old entries.

3) Name display

+                    <a href="/cgi-bin/koha/circ/circulation.pl?borrowernumber=[% checkout.issuer | uri %]">
+                    [% checkout.issued_by.firstname | html %] [% checkout.issued_by.surname | html %]

I think we should use patron-title.inc here if possible.


4) Tests fail :(

I've run the database update and dbic manually, but the tests are failing:

Test Summary Report
-------------------
t/db_dependent/Circulation.t   (Wstat: 65280 Tests: 51 Failed: 1)
  Failed test:  51
  Non-zero exit status: 255
t/db_dependent/Koha/Checkouts.t (Wstat: 65280 Tests: 7 Failed: 1)
  Failed test:  7
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 8 tests but ran 7.
t/db_dependent/Koha/Patrons.t  (Wstat: 512 Tests: 28 Failed: 1)
  Failed test:  28
  Non-zero exit status: 2
  Parse errors: Bad plan.  You planned 41 tests but ran 28.

# Subtest: issuer
    1..3
    ok 1 - Koha::Checkout->issued_by should return a Koha::Patron
    ok 2 - Koha::Checkout->issued_by should return the correct patron
    # Looks like you planned 3 tests but ran 2.
not ok 7 - issuer

From the verbose output:

#   Failed test 'issuer'
#   at t/db_dependent/Koha/Checkouts.t line 152.
Can't call method "issuer" on an undefined value at t/db_dependent/Koha/Checkouts.t line 148.
# Looks like your test exited with 255 just after 7.
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 2/8 subtests 

item-level_itypes set but no itemtype set for item (1508) at /home/vagrant/kohaclone/Koha/Schema/Result/Item.pm line 789.
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`items`, CONSTRAINT `items_ibfk_3` FOREIGN KEY (`holdingbranch`) REFERENCES `branches` (`branchcode`) ON UPDATE CASCADE) [for Statement "UPDATE `items` SET `datelastborrowed` = ?, `holdingbranch` = ?, `issues` = ?, `onloan` = ? WHERE ( `itemnumber` = ? )" with ParamValues: 0='2020-10-02', 1="Branch4T", 2=1, 3='2020-10-01', 4=1508] at /home/vagrant/kohaclone/Koha/Object.pm line 166
    # No tests run!
not ok 28 - No tests run for subtest "get_overdues"

#   Failed test 'No tests run for subtest "get_overdues"'
#   at t/db_dependent/Koha/Patrons.t line 1454.
Broken FK constraint# Looks like your test exited with 2 just after 28.
Dubious, test returned 2 (wstat 512, 0x200)
Failed 14/41 subtests 

I hope I didn't mess up the rebase, so not attaching my patches here (had fixed a conflict in one of the test files).
Comment 51 Andrew Isherwood 2020-10-12 09:33:29 UTC
Created attachment 111485 [details] [review]
Bug 23916: Add issues.issuer column and FK

This patch adds the issues.issuer column which is a foreign key to
borrowers.borrowernumber

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 52 Andrew Isherwood 2020-10-12 09:33:35 UTC
Created attachment 111486 [details] [review]
Bug 23916: Add "RecordIssuer" syspref

This patch adds the "RecordIssuer" syspref, which is disabled by default

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 53 Andrew Isherwood 2020-10-12 09:33:42 UTC
Created attachment 111487 [details] [review]
Bug 23916: Record and display item issuer

This patch adds the recording and display of the item issuer. This
behaviour is governed by the RecordIssuer syspref, if disabled (the
default), no recording or display of issuer will take place.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 54 Andrew Isherwood 2020-10-12 09:33:47 UTC
Created attachment 111488 [details] [review]
Bug 23916: Anonymise 'issuer' when required

This patch adds the anonymisation of 'old_issues.issuer' alongside the
existing anonymisation of 'old_issues.borrowernumber'

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 55 Andrew Isherwood 2020-10-12 09:33:51 UTC
Created attachment 111489 [details] [review]
Bug 23916: Add unit tests

This patch adds unit tests for the new functions

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 56 Andrew Isherwood 2020-10-12 09:33:56 UTC
Created attachment 111490 [details] [review]
Bug 23916: (follow-up) Fix column name collision

Seems that joining 'borrowers' into the query run in
C4::Members::GetAllIssues caused a column name collision, which blatted
the retrieval of issues.branchcode. So we now are being a bit more
specific in what is coming from where in the query.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 57 Andrew Isherwood 2020-10-12 09:34:00 UTC
Created attachment 111491 [details] [review]
Bug 23916: (follow-up) Implement feedback

This commit implements the two suggestions made by Nick in comment #41:

- Removes the attempted retrospective addition of issuers to old issues.
It was not particularly reliable, it also is not appropriate if the user
does not wish to make use of this functionality

- Removes the conversion of a NULL issuer to the anonymous borrowernumber when
anonymising checkouts
Comment 58 Andrew Isherwood 2020-10-12 09:34:06 UTC
Created attachment 111492 [details] [review]
Bug 23916: (follow-up) Respond to feedback

This commit makes the changes suggested by Katrin in comment #50:

- Changed syspref from RecordIssuer to RecordStaffUserOnCheckout
- Changed terminology from "issue" to "check out" and variations
- Fixed name display to use patron-title.inc
- Made issuer column DEFAULT NULL consistently between issues and
old_issues and between the DB update and kohastructure.sql
Comment 59 Andrew Isherwood 2020-10-12 09:40:58 UTC
(In reply to Katrin Fischer from comment #50)
> 1) Terminology
> 
> issue is not supposed to be used according to our terminology list. We
> prefer checkout. But... I am not sure what the better term for the person
> doing the checkout would be here. In some other cases we have used "manager"
> (accountlines). But if we wanted to record the person doing the checkin at
> some point, this won't work. For the pref... RecordStaffUserOnCheckout or
> similar could work. Overall not an easy one.
> 
> I'll only be asking for changes in the GUI.
> 
> +            - "When issuing an item, "
> +            - pref: RecordIssuer
> +              choices:
> +                  yes: "record"
> +                  no: "don't record"
> +            - "the user who issued the item."
> 
> Please change to "staff user" and "issu*" to "check* out".
> 
> Everywhere else looks good.

Fixed, I've changed the syspref name as you suggested, I've also changed "issuer" to "staff who checked out the item" and "issue" to "check out"

> 2) Database update and installer don't match up
> 
> for old_issues.issuer we have default null in kohastructure.sql, but not in
> the database update.
> 
> Maybe this has been discussed, but why not default null on issues.issuer? On
> first glance it would make sense for me as we will also use NULL on all the
> old entries.

Fixed, it is now DEFAULT NULL for both kohastructure.sql and the update, for both issues and old_issues

> 3) Name display
> 
> +                    <a
> href="/cgi-bin/koha/circ/circulation.pl?borrowernumber=[% checkout.issuer |
> uri %]">
> +                    [% checkout.issued_by.firstname | html %] [%
> checkout.issued_by.surname | html %]
> 
> I think we should use patron-title.inc here if possible.

Fixed

> 4) Tests fail :(
> 
> I've run the database update and dbic manually, but the tests are failing:

I can't replicate this (see below)

> I hope I didn't mess up the rebase, so not attaching my patches here (had
> fixed a conflict in one of the test files).

You shouldn't need to rebase, the patches apply cleanly on top of master for me. Maybe try again?

kohadev-koha@kohadevbox:/kohadevbox/koha$ ../qa-test-tools/koha-qa.pl -c 8 -v 2
testing 8 commit(s) (applied to 20022fa '21 Bug 22417: Add exec flag on .t fil')

Processing files before patches
|========================>| 11 / 11 (100.00%)
Processing files after patches
|========================>| 11 / 11 (100.00%)

 OK     C4/Circulation.pm
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   OK     pod
   OK     pod coverage
   OK     spelling
   OK     valid

 OK     C4/Members.pm
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   OK     pod
   OK     pod coverage
   OK     spelling
   OK     valid

 OK     Koha/Checkout.pm
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   OK     pod
   OK     pod coverage
   OK     spelling
   OK     valid

 OK     Koha/Patrons.pm
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   OK     pod
   OK     pod coverage
   OK     spelling
   OK     valid

 OK     installer/data/mysql/kohastructure.sql
   OK     boolean_vs_tinyint
   OK     charset_collate
   OK     git manipulation
   OK     tinyint_has_boolean_flag

 OK     installer/data/mysql/sysprefs.sql
   OK     git manipulation
   OK     semicolon
   OK     sysprefs_order

 OK     koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/issuehistory.tt
   OK     filters
   OK     forbidden patterns
   OK     git manipulation
   OK     js_in_body
   OK     spelling
   OK     tt_valid
   OK     valid_template

 OK     koha-tmpl/intranet-tmpl/prog/en/modules/members/readingrec.tt
   OK     filters
   OK     forbidden patterns
   OK     git manipulation
   OK     js_in_body
   OK     spelling
   OK     tt_valid
   OK     valid_template

 OK     t/db_dependent/Circulation.t
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   OK     pod
   OK     spelling
   OK     valid

 OK     t/db_dependent/Koha/Checkouts.t
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   OK     pod
   OK     spelling
   OK     valid

 OK     t/db_dependent/Koha/Patrons.t
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   OK     pod
   OK     spelling
   OK     valid


Processing additional checks OK!
Comment 60 Katrin Fischer 2020-10-12 09:44:10 UTC
Hi Andrew, I will have another look as soon as I can, but to avoid misunderstandings, I didn't mean the QA tests, but the unit tests. And I did have a conflict on master, but other things pushed meanwhile might have changed things again :)

Do the unit tests all pass for you?
Comment 61 Andrew Isherwood 2020-10-12 10:57:30 UTC
(In reply to Katrin Fischer from comment #60)
> Do the unit tests all pass for you?
Yes:

kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t
[...]
All tests successful.
Files=91, Tests=4075, 2515 wallclock secs ( 1.47 usr  0.33 sys + 2415.95 cusr 41.02 csys = 2458.77 CPU)                                               
Result: PASS
Comment 62 Katrin Fischer 2020-10-16 10:33:38 UTC
There is a conflict in the unit test file I am not sure about - probably not too bad, but more than fixing the number of tests, so I'd like not to mess with it:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 23916: Add issues.issuer column and FK
Applying: Bug 23916: Add "RecordIssuer" syspref
Using index info to reconstruct a base tree...
A	installer/data/mysql/sysprefs.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging installer/data/mysql/mandatory/sysprefs.sql
Applying: Bug 23916: Record and display item issuer
Applying: Bug 23916: Anonymise 'issuer' when required
Applying: Bug 23916: Add unit tests
Using index info to reconstruct a base tree...
M	t/db_dependent/Circulation.t
M	t/db_dependent/Koha/Patrons.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Koha/Patrons.t
Auto-merging t/db_dependent/Circulation.t
CONFLICT (content): Merge conflict in t/db_dependent/Circulation.t
error: Failed to merge in the changes.
Patch failed at 0001 Bug 23916: Add unit tests
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-23916-Add-unit-tests-yabswJ.patch
Comment 63 Andrew Isherwood 2020-10-19 12:41:06 UTC
Created attachment 111966 [details] [review]
Bug 23916: Add issues.issuer column and FK

This patch adds the issues.issuer column which is a foreign key to
borrowers.borrowernumber

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 64 Andrew Isherwood 2020-10-19 12:41:13 UTC
Created attachment 111967 [details] [review]
Bug 23916: Add "RecordIssuer" syspref

This patch adds the "RecordIssuer" syspref, which is disabled by default

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 65 Andrew Isherwood 2020-10-19 12:41:19 UTC
Created attachment 111968 [details] [review]
Bug 23916: Record and display item issuer

This patch adds the recording and display of the item issuer. This
behaviour is governed by the RecordIssuer syspref, if disabled (the
default), no recording or display of issuer will take place.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 66 Andrew Isherwood 2020-10-19 12:41:26 UTC
Created attachment 111969 [details] [review]
Bug 23916: Anonymise 'issuer' when required

This patch adds the anonymisation of 'old_issues.issuer' alongside the
existing anonymisation of 'old_issues.borrowernumber'

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 67 Andrew Isherwood 2020-10-19 12:41:32 UTC
Created attachment 111970 [details] [review]
Bug 23916: Add unit tests

This patch adds unit tests for the new functions

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 68 Andrew Isherwood 2020-10-19 12:41:38 UTC
Created attachment 111971 [details] [review]
Bug 23916: (follow-up) Fix column name collision

Seems that joining 'borrowers' into the query run in
C4::Members::GetAllIssues caused a column name collision, which blatted
the retrieval of issues.branchcode. So we now are being a bit more
specific in what is coming from where in the query.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 69 Andrew Isherwood 2020-10-19 12:41:44 UTC
Created attachment 111972 [details] [review]
Bug 23916: (follow-up) Implement feedback

This commit implements the two suggestions made by Nick in comment #41:

- Removes the attempted retrospective addition of issuers to old issues.
It was not particularly reliable, it also is not appropriate if the user
does not wish to make use of this functionality

- Removes the conversion of a NULL issuer to the anonymous borrowernumber when
anonymising checkouts
Comment 70 Andrew Isherwood 2020-10-19 12:41:51 UTC
Created attachment 111973 [details] [review]
Bug 23916: (follow-up) Respond to feedback

This commit makes the changes suggested by Katrin in comment #50:

- Changed syspref from RecordIssuer to RecordStaffUserOnCheckout
- Changed terminology from "issue" to "check out" and variations
- Fixed name display to use patron-title.inc
- Made issuer column DEFAULT NULL consistently between issues and
old_issues and between the DB update and kohastructure.sql
Comment 71 Andrew Isherwood 2020-10-19 12:41:58 UTC
Created attachment 111974 [details] [review]
Bug 23916: (follow-up) Respond to feedback

This commit is a response to Katrin's comments in comment #62:

- Fixed merge conflict

It also fixes a test which now fails due to an upstream change in
C4::Circulation
Comment 72 Andrew Isherwood 2020-10-19 12:43:05 UTC
Hi Katrin - thanks for that. I've fixed that, I also fixed a test that was now failing due to a change in C4::Circulation

Revised test plan:
-----------------

1.  Ensure the new "RecordStaffUserOnCheckout" is turned off (the default) setting.
2.  Issue an item to a patron
3.  Check the patron's circulation history
4.  TEST => Observe that the table does not contain a "Checked out by" column
5.  Check the item's checkout history
6.  TEST => Observe that the table does not contain a "Checked out by" column
7.  Turn the "RecordIssuer" syspref on
8.  Issue another item to the patron
9.  Check the patron's circulation history
10. TEST => Observe that the table now contains an "Checked out by" column
11. TEST => Observe that the column contains the details of the issuing user with a link to their patron record ONLY for the issue for which the syspref was turned on
11. Check the item's checkout history
12. TEST => Observe that the table now contains an "Checked out by" column
13. TEST => Observe that the column contains the details of the issuing user with a link to their patron record ONLY for the issue for which the syspref was turned on
Comment 73 Katrin Fischer 2020-10-23 00:20:25 UTC
I am sorry, but I can't get the tests to pass... updatedatabase, dbic, reset_all.. and still:

DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`items`, CONSTRAINT `items_ibfk_3` FOREIGN KEY (`holdingbranch`) REFERENCES `branches` (`branchcode`) ON UPDATE CASCADE) [for Statement "UPDATE `items` SET `datelastborrowed` = ?, `holdingbranch` = ?, `issues` = ?, `onloan` = ? WHERE ( `itemnumber` = ? )" with ParamValues: 0='2020-10-22', 1="Branch4T", 2=1, 3='2020-10-21', 4=990] at /home/vagrant/kohaclone/Koha/Object.pm line 169
    # No tests run!
t/db_dependent/Koha/Patrons.t .... 28/41 
#   Failed test 'No tests run for subtest "get_overdues"'
#   at t/db_dependent/Koha/Patrons.t line 1454.
Broken FK constraint# Looks like your test exited with 2 just after 28.
t/db_dependent/Koha/Patrons.t .... Dubious, test returned 2 (wstat 512, 0x200)
Failed 14/41 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation.t   (Wstat: 65280 Tests: 0 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 51 tests but ran 0.
t/db_dependent/Koha/Patrons.t  (Wstat: 512 Tests: 28 Failed: 1)
  Failed test:  28
  Non-zero exit status: 2
  Parse errors: Bad plan.  You planned 41 tests but ran 28.


They pass on the same db on master :(
Comment 74 Andrew Isherwood 2020-10-23 11:20:53 UTC
Created attachment 112248 [details] [review]
Bug 23916: Add issues.issuer column and FK

This patch adds the issues.issuer column which is a foreign key to
borrowers.borrowernumber

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 75 Andrew Isherwood 2020-10-23 11:20:58 UTC
Created attachment 112249 [details] [review]
Bug 23916: Add "RecordIssuer" syspref

This patch adds the "RecordIssuer" syspref, which is disabled by default

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 76 Andrew Isherwood 2020-10-23 11:21:06 UTC
Created attachment 112250 [details] [review]
Bug 23916: Record and display item issuer

This patch adds the recording and display of the item issuer. This
behaviour is governed by the RecordIssuer syspref, if disabled (the
default), no recording or display of issuer will take place.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 77 Andrew Isherwood 2020-10-23 11:21:12 UTC
Created attachment 112251 [details] [review]
Bug 23916: Anonymise 'issuer' when required

This patch adds the anonymisation of 'old_issues.issuer' alongside the
existing anonymisation of 'old_issues.borrowernumber'

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 78 Andrew Isherwood 2020-10-23 11:21:16 UTC
Created attachment 112252 [details] [review]
Bug 23916: Add unit tests

This patch adds unit tests for the new functions

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 79 Andrew Isherwood 2020-10-23 11:21:20 UTC
Created attachment 112253 [details] [review]
Bug 23916: (follow-up) Fix column name collision

Seems that joining 'borrowers' into the query run in
C4::Members::GetAllIssues caused a column name collision, which blatted
the retrieval of issues.branchcode. So we now are being a bit more
specific in what is coming from where in the query.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 80 Andrew Isherwood 2020-10-23 11:21:27 UTC
Created attachment 112254 [details] [review]
Bug 23916: (follow-up) Implement feedback

This commit implements the two suggestions made by Nick in comment #41:

- Removes the attempted retrospective addition of issuers to old issues.
It was not particularly reliable, it also is not appropriate if the user
does not wish to make use of this functionality

- Removes the conversion of a NULL issuer to the anonymous borrowernumber when
anonymising checkouts
Comment 81 Andrew Isherwood 2020-10-23 11:21:33 UTC
Created attachment 112255 [details] [review]
Bug 23916: (follow-up) Respond to feedback

This commit makes the changes suggested by Katrin in comment #50:

- Changed syspref from RecordIssuer to RecordStaffUserOnCheckout
- Changed terminology from "issue" to "check out" and variations
- Fixed name display to use patron-title.inc
- Made issuer column DEFAULT NULL consistently between issues and
old_issues and between the DB update and kohastructure.sql
Comment 82 Andrew Isherwood 2020-10-23 11:21:40 UTC
Created attachment 112256 [details] [review]
Bug 23916: (follow-up) Respond to feedback

This commit is a response to Katrin's comments in comment #62:

- Fixed merge conflict

It also fixes a test which now fails due to an upstream change in
C4::Circulation
Comment 83 Andrew Isherwood 2020-10-23 11:21:46 UTC
Created attachment 112257 [details] [review]
Bug 23916: (follow-up) Fix unit test

This commit responds to the comment made by Katrin in comment #73. I
have removed the lines cited as causing the contraint violation. The
were added in the original iteration of the bug patches and, despite
spending time trying to get to the bottom of it, I cannot figure out
why. There are no changes in the patchset relating to Koha/Patron.pm and
removing the lines does not cause any of the tests in files that *are*
affected by this patchset to fail. I do not know why these lines were
ever added in the first place, unfortunately I did not add comments
explaining (lesson learned)!
Comment 84 Andrew Isherwood 2020-10-23 11:23:01 UTC
Hi Katrin - Thanks for that. I've now removed those lines from that file, the commit message in the last patch explains why.
Comment 85 Nick Clemens 2020-10-24 03:03:52 UTC
Created attachment 112363 [details] [review]
Bug 23916: Add issues.issuer column and FK

This patch adds the issues.issuer column which is a foreign key to
borrowers.borrowernumber

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 86 Nick Clemens 2020-10-24 03:03:58 UTC
Created attachment 112364 [details] [review]
Bug 23916: Add "RecordIssuer" syspref

This patch adds the "RecordIssuer" syspref, which is disabled by default

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 87 Nick Clemens 2020-10-24 03:04:06 UTC
Created attachment 112365 [details] [review]
Bug 23916: Record and display item issuer

This patch adds the recording and display of the item issuer. This
behaviour is governed by the RecordIssuer syspref, if disabled (the
default), no recording or display of issuer will take place.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 88 Nick Clemens 2020-10-24 03:04:15 UTC
Created attachment 112366 [details] [review]
Bug 23916: Anonymise 'issuer' when required

This patch adds the anonymisation of 'old_issues.issuer' alongside the
existing anonymisation of 'old_issues.borrowernumber'

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 89 Nick Clemens 2020-10-24 03:04:20 UTC
Created attachment 112367 [details] [review]
Bug 23916: Add unit tests

This patch adds unit tests for the new functions

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 90 Nick Clemens 2020-10-24 03:04:26 UTC
Created attachment 112368 [details] [review]
Bug 23916: (follow-up) Fix column name collision

Seems that joining 'borrowers' into the query run in
C4::Members::GetAllIssues caused a column name collision, which blatted
the retrieval of issues.branchcode. So we now are being a bit more
specific in what is coming from where in the query.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 91 Nick Clemens 2020-10-24 03:04:33 UTC
Created attachment 112369 [details] [review]
Bug 23916: (follow-up) Don't anonymise issuer and don't update action_logs on upgrade

This commit implements the two suggestions made by Nick in comment #41:

- Removes the attempted retrospective addition of issuers to old issues.
It was not particularly reliable, it also is not appropriate if the user
does not wish to make use of this functionality

- Removes the conversion of a NULL issuer to the anonymous borrowernumber when
anonymising checkouts
Comment 92 Nick Clemens 2020-10-24 03:04:38 UTC
Created attachment 112370 [details] [review]
Bug 23916: (follow-up) Fix terminology, use patron-title and make column consistent in issues and old_issues

This commit makes the changes suggested by Katrin in comment #50:

- Changed syspref from RecordIssuer to RecordStaffUserOnCheckout
- Changed terminology from "issue" to "check out" and variations
- Fixed name display to use patron-title.inc
- Made issuer column DEFAULT NULL consistently between issues and
old_issues and between the DB update and kohastructure.sql
Comment 93 Nick Clemens 2020-10-24 03:04:44 UTC
Created attachment 112371 [details] [review]
Bug 23916: (follow-up) Rebase unit tests

This commit is a response to Katrin's comments in comment #62:

- Fixed merge conflict

It also fixes a test which now fails due to an upstream change in
C4::Circulation
Comment 94 Nick Clemens 2020-10-24 03:04:51 UTC
Created attachment 112372 [details] [review]
Bug 23916: (follow-up) Fix unit test

This commit responds to the comment made by Katrin in comment #73. I
have removed the lines cited as causing the contraint violation. The
were added in the original iteration of the bug patches and, despite
spending time trying to get to the bottom of it, I cannot figure out
why. There are no changes in the patchset relating to Koha/Patron.pm and
removing the lines does not cause any of the tests in files that *are*
affected by this patchset to fail. I do not know why these lines were
ever added in the first place, unfortunately I did not add comments
explaining (lesson learned)!
Comment 95 Nick Clemens 2020-10-24 03:05:01 UTC
Created attachment 112373 [details] [review]
Bug 23916: (QA follow-up) Remove stray changes and fix typo

There were a couple changes that had no effect, so cleaned those up

Typo 'is' for 'if'
Comment 96 Nick Clemens 2020-10-24 03:06:40 UTC
Works well, all tests pass, confirmed it works for SIP and SCO, excellent job!

I cleaned up a few commit messages to make them clearer, they could be squashed on push to master
Comment 97 Jonathan Druart 2020-11-04 12:24:29 UTC
Applying: Bug 23916: Record and display item issuer
CONFLICT (content): Merge conflict in C4/Circulation.pm
Recorded preimage for 'C4/Circulation.pm'

Please rebase.
Comment 98 Andrew Isherwood 2020-11-05 09:05:36 UTC
Created attachment 113035 [details] [review]
Bug 23916: Add issues.issuer column and FK

This patch adds the issues.issuer column which is a foreign key to
borrowers.borrowernumber

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 99 Andrew Isherwood 2020-11-05 09:05:40 UTC
Created attachment 113036 [details] [review]
Bug 23916: Add "RecordIssuer" syspref

This patch adds the "RecordIssuer" syspref, which is disabled by default

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 100 Andrew Isherwood 2020-11-05 09:05:45 UTC
Created attachment 113037 [details] [review]
Bug 23916: Record and display item issuer

This patch adds the recording and display of the item issuer. This
behaviour is governed by the RecordIssuer syspref, if disabled (the
default), no recording or display of issuer will take place.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 101 Andrew Isherwood 2020-11-05 09:05:49 UTC
Created attachment 113038 [details] [review]
Bug 23916: Anonymise 'issuer' when required

This patch adds the anonymisation of 'old_issues.issuer' alongside the
existing anonymisation of 'old_issues.borrowernumber'

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 102 Andrew Isherwood 2020-11-05 09:05:53 UTC
Created attachment 113039 [details] [review]
Bug 23916: Add unit tests

This patch adds unit tests for the new functions

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 103 Andrew Isherwood 2020-11-05 09:05:57 UTC
Created attachment 113040 [details] [review]
Bug 23916: (follow-up) Fix column name collision

Seems that joining 'borrowers' into the query run in
C4::Members::GetAllIssues caused a column name collision, which blatted
the retrieval of issues.branchcode. So we now are being a bit more
specific in what is coming from where in the query.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 104 Andrew Isherwood 2020-11-05 09:06:01 UTC
Created attachment 113041 [details] [review]
Bug 23916: (follow-up) Don't anonymise issuer and don't update action_logs on upgrade

This commit implements the two suggestions made by Nick in comment #41:

- Removes the attempted retrospective addition of issuers to old issues.
It was not particularly reliable, it also is not appropriate if the user
does not wish to make use of this functionality

- Removes the conversion of a NULL issuer to the anonymous borrowernumber when
anonymising checkouts
Comment 105 Andrew Isherwood 2020-11-05 09:06:05 UTC
Created attachment 113042 [details] [review]
Bug 23916: (follow-up) Fix terminology, use patron-title and make column consistent in issues and old_issues

This commit makes the changes suggested by Katrin in comment #50:

- Changed syspref from RecordIssuer to RecordStaffUserOnCheckout
- Changed terminology from "issue" to "check out" and variations
- Fixed name display to use patron-title.inc
- Made issuer column DEFAULT NULL consistently between issues and
old_issues and between the DB update and kohastructure.sql
Comment 106 Andrew Isherwood 2020-11-05 09:06:09 UTC
Created attachment 113043 [details] [review]
Bug 23916: (follow-up) Rebase unit tests

This commit is a response to Katrin's comments in comment #62:

- Fixed merge conflict

It also fixes a test which now fails due to an upstream change in
C4::Circulation
Comment 107 Andrew Isherwood 2020-11-05 09:06:14 UTC
Created attachment 113044 [details] [review]
Bug 23916: (follow-up) Fix unit test

This commit responds to the comment made by Katrin in comment #73. I
have removed the lines cited as causing the contraint violation. The
were added in the original iteration of the bug patches and, despite
spending time trying to get to the bottom of it, I cannot figure out
why. There are no changes in the patchset relating to Koha/Patron.pm and
removing the lines does not cause any of the tests in files that *are*
affected by this patchset to fail. I do not know why these lines were
ever added in the first place, unfortunately I did not add comments
explaining (lesson learned)!
Comment 108 Andrew Isherwood 2020-11-05 09:06:18 UTC
Created attachment 113045 [details] [review]
Bug 23916: (QA follow-up) Remove stray changes and fix typo

There were a couple changes that had no effect, so cleaned those up

Typo 'is' for 'if'
Comment 109 Andrew Isherwood 2020-11-05 09:06:42 UTC
Now rebased, thanks Jonathan
Comment 110 Jonathan Druart 2020-11-09 15:19:16 UTC
Created attachment 113316 [details] [review]
Bug 23916: Add issues.issuer column and FK

This patch adds the issues.issuer column which is a foreign key to
borrowers.borrowernumber

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 111 Jonathan Druart 2020-11-09 15:19:21 UTC
Created attachment 113317 [details] [review]
Bug 23916: Add "RecordIssuer" syspref

This patch adds the "RecordIssuer" syspref, which is disabled by default

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 112 Jonathan Druart 2020-11-09 15:19:25 UTC
Created attachment 113318 [details] [review]
Bug 23916: Record and display item issuer

This patch adds the recording and display of the item issuer. This
behaviour is governed by the RecordIssuer syspref, if disabled (the
default), no recording or display of issuer will take place.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 113 Jonathan Druart 2020-11-09 15:19:29 UTC
Created attachment 113319 [details] [review]
Bug 23916: Anonymise 'issuer' when required

This patch adds the anonymisation of 'old_issues.issuer' alongside the
existing anonymisation of 'old_issues.borrowernumber'

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 114 Jonathan Druart 2020-11-09 15:19:33 UTC
Created attachment 113320 [details] [review]
Bug 23916: Add unit tests

This patch adds unit tests for the new functions

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 115 Jonathan Druart 2020-11-09 15:19:37 UTC
Created attachment 113321 [details] [review]
Bug 23916: (follow-up) Fix column name collision

Seems that joining 'borrowers' into the query run in
C4::Members::GetAllIssues caused a column name collision, which blatted
the retrieval of issues.branchcode. So we now are being a bit more
specific in what is coming from where in the query.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 116 Jonathan Druart 2020-11-09 15:19:41 UTC
Created attachment 113322 [details] [review]
Bug 23916: (follow-up) Don't anonymise issuer and don't update action_logs on upgrade

This commit implements the two suggestions made by Nick in comment #41:

- Removes the attempted retrospective addition of issuers to old issues.
It was not particularly reliable, it also is not appropriate if the user
does not wish to make use of this functionality

- Removes the conversion of a NULL issuer to the anonymous borrowernumber when
anonymising checkouts

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 117 Jonathan Druart 2020-11-09 15:19:46 UTC
Created attachment 113323 [details] [review]
Bug 23916: (follow-up) Fix terminology, use patron-title and make column consistent in issues and old_issues

This commit makes the changes suggested by Katrin in comment #50:

- Changed syspref from RecordIssuer to RecordStaffUserOnCheckout
- Changed terminology from "issue" to "check out" and variations
- Fixed name display to use patron-title.inc
- Made issuer column DEFAULT NULL consistently between issues and
old_issues and between the DB update and kohastructure.sql

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 118 Jonathan Druart 2020-11-09 15:19:50 UTC
Created attachment 113324 [details] [review]
Bug 23916: (follow-up) Rebase unit tests

This commit is a response to Katrin's comments in comment #62:

- Fixed merge conflict

It also fixes a test which now fails due to an upstream change in
C4::Circulation

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 119 Jonathan Druart 2020-11-09 15:19:54 UTC
Created attachment 113325 [details] [review]
Bug 23916: (follow-up) Fix unit test

This commit responds to the comment made by Katrin in comment #73. I
have removed the lines cited as causing the contraint violation. The
were added in the original iteration of the bug patches and, despite
spending time trying to get to the bottom of it, I cannot figure out
why. There are no changes in the patchset relating to Koha/Patron.pm and
removing the lines does not cause any of the tests in files that *are*
affected by this patchset to fail. I do not know why these lines were
ever added in the first place, unfortunately I did not add comments
explaining (lesson learned)!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 120 Jonathan Druart 2020-11-09 15:19:59 UTC
Created attachment 113326 [details] [review]
Bug 23916: (QA follow-up) Remove stray changes and fix typo

There were a couple changes that had no effect, so cleaned those up

Typo 'is' for 'if'

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 121 Jonathan Druart 2020-11-09 15:20:38 UTC
Added missing Signed-off-by line from QA.
Comment 122 Jonathan Druart 2020-11-09 15:32:58 UTC
We need to adapt the REST API routes to deal with this new value.
Comment 123 Jonathan Druart 2020-11-09 15:35:56 UTC
Created attachment 113327 [details] [review]
Bug 23916: (follow-up) Add unit tests

Fix perlcritic error
Subroutine "new" called using indirect syntax at line 128, column 14.  See page 349 of PBP.  (Severity: 4)
Comment 124 Tomás Cohen Arazi 2020-11-09 15:38:47 UTC
(In reply to Jonathan Druart from comment #122)
> We need to adapt the REST API routes to deal with this new value.

Yeah, can we also rename the column? issuer_id?
I can provide the follow-up if it helps.
Comment 125 Jonathan Druart 2020-11-09 16:25:55 UTC Comment hidden (obsolete)
Comment 126 Jonathan Druart 2020-11-09 16:26:16 UTC
Also the method is Koha::Checkout->issued_by, should be ->issuer I think
Comment 127 Tomás Cohen Arazi 2020-11-09 16:26:34 UTC
(In reply to Jonathan Druart from comment #126)
> Also the method is Koha::Checkout->issued_by, should be ->issuer I think

I'll take care
Comment 128 Tomás Cohen Arazi 2020-11-10 11:51:02 UTC
Hi, I made some changes to:
- Name the new column issuer_id
- Name the method for accessing the related patron 'issuer' (which actually makes sense as it would be named the same as the DBIC relationship)
- Removed the unnecessary translation for the API
- Added the new attribute name, type and description to the API spec
- Added an optional 'issuer' attribute, with its corresponding 'x-koha-embed' so it is more useful for displaying checkouts.
Comment 129 Tomás Cohen Arazi 2020-11-10 11:59:15 UTC
Created attachment 113367 [details] [review]
Bug 23916: Add issues.issuer column and FK

This patch adds the issues.issuer column which is a foreign key to
borrowers.borrowernumber

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 130 Tomás Cohen Arazi 2020-11-10 11:59:23 UTC
Created attachment 113368 [details] [review]
Bug 23916: Add "RecordIssuer" syspref

This patch adds the "RecordIssuer" syspref, which is disabled by default

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 131 Tomás Cohen Arazi 2020-11-10 11:59:32 UTC
Created attachment 113369 [details] [review]
Bug 23916: Record and display item issuer

This patch adds the recording and display of the item issuer. This
behaviour is governed by the RecordIssuer syspref, if disabled (the
default), no recording or display of issuer will take place.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 132 Tomás Cohen Arazi 2020-11-10 11:59:40 UTC
Created attachment 113370 [details] [review]
Bug 23916: Anonymise 'issuer' when required

This patch adds the anonymisation of 'old_issues.issuer' alongside the
existing anonymisation of 'old_issues.borrowernumber'

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 133 Tomás Cohen Arazi 2020-11-10 11:59:48 UTC
Created attachment 113371 [details] [review]
Bug 23916: Add unit tests

This patch adds unit tests for the new functions

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 134 Tomás Cohen Arazi 2020-11-10 11:59:56 UTC
Created attachment 113372 [details] [review]
Bug 23916: (follow-up) Fix column name collision

Seems that joining 'borrowers' into the query run in
C4::Members::GetAllIssues caused a column name collision, which blatted
the retrieval of issues.branchcode. So we now are being a bit more
specific in what is coming from where in the query.

Signed-off-by: Ben Veasey <B.T.Veasey@lboro.ac.uk>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 135 Tomás Cohen Arazi 2020-11-10 12:00:02 UTC
Created attachment 113373 [details] [review]
Bug 23916: (follow-up) Don't anonymise issuer and don't update action_logs on upgrade

This commit implements the two suggestions made by Nick in comment #41:

- Removes the attempted retrospective addition of issuers to old issues.
It was not particularly reliable, it also is not appropriate if the user
does not wish to make use of this functionality

- Removes the conversion of a NULL issuer to the anonymous borrowernumber when
anonymising checkouts

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 136 Tomás Cohen Arazi 2020-11-10 12:00:08 UTC
Created attachment 113374 [details] [review]
Bug 23916: (follow-up) Fix terminology, use patron-title and make column consistent in issues and old_issues

This commit makes the changes suggested by Katrin in comment #50:

- Changed syspref from RecordIssuer to RecordStaffUserOnCheckout
- Changed terminology from "issue" to "check out" and variations
- Fixed name display to use patron-title.inc
- Made issuer column DEFAULT NULL consistently between issues and
old_issues and between the DB update and kohastructure.sql

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 137 Tomás Cohen Arazi 2020-11-10 12:00:14 UTC
Created attachment 113375 [details] [review]
Bug 23916: (follow-up) Rebase unit tests

This commit is a response to Katrin's comments in comment #62:

- Fixed merge conflict

It also fixes a test which now fails due to an upstream change in
C4::Circulation

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 138 Tomás Cohen Arazi 2020-11-10 12:00:21 UTC
Created attachment 113376 [details] [review]
Bug 23916: (follow-up) Fix unit test

This commit responds to the comment made by Katrin in comment #73. I
have removed the lines cited as causing the contraint violation. The
were added in the original iteration of the bug patches and, despite
spending time trying to get to the bottom of it, I cannot figure out
why. There are no changes in the patchset relating to Koha/Patron.pm and
removing the lines does not cause any of the tests in files that *are*
affected by this patchset to fail. I do not know why these lines were
ever added in the first place, unfortunately I did not add comments
explaining (lesson learned)!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 139 Tomás Cohen Arazi 2020-11-10 12:00:28 UTC
Created attachment 113377 [details] [review]
Bug 23916: (QA follow-up) Remove stray changes and fix typo

There were a couple changes that had no effect, so cleaned those up

Typo 'is' for 'if'

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 140 Tomás Cohen Arazi 2020-11-10 12:00:39 UTC
Created attachment 113378 [details] [review]
Bug 23916: (follow-up) Add unit tests

Fix perlcritic error
Subroutine "new" called using indirect syntax at line 128, column 14.  See page 349 of PBP.  (Severity: 4)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 141 Tomás Cohen Arazi 2020-11-10 12:00:46 UTC
Created attachment 113379 [details] [review]
Bug 23916: (QA follow-up) Rename issuer => issuer_id [DB]

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 142 Tomás Cohen Arazi 2020-11-10 12:00:58 UTC
Created attachment 113380 [details] [review]
Bug 23916: Add new attribute to API

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 143 Tomás Cohen Arazi 2020-11-10 12:01:08 UTC
Created attachment 113381 [details] [review]
Bug 23916: (QA follow-up) Adapt all the things to the new column and accessor names

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 144 Tomás Cohen Arazi 2020-11-10 12:01:16 UTC
Created attachment 113382 [details] [review]
Bug 23916: Allow embedding the issuer object

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 145 Jonathan Druart 2020-11-10 13:25:27 UTC
Tomas, 

About:
+            $issue->discard_changes;
Did you encounter a specific problem? This change may be moved on a separate bug report to make it backportable (or just create a separate one for stables).
Can you confirm?
Comment 146 Jonathan Druart 2020-11-10 13:46:18 UTC
Created attachment 113396 [details] [review]
Bug 23916: (follow-up) Adapt all the things to the new column and accessor names
Comment 147 Jonathan Druart 2020-11-10 14:00:42 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 148 Lucas Gass 2020-11-16 22:30:43 UTC
enhancement will not be backported to 20.05.x