|
Description
Andrew Isherwood
2019-10-29 11:49:22 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. 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. Development note: The data being stored here (issuer) should also be made visible in a biblio's "Checkout history" page 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. 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) 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
> 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 :)
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 Created attachment 94915 [details] [review] Bug 23916: Add "RecordIssuer" syspref This patch adds the "RecordIssuer" syspref, which is disabled by default 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. 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' Created attachment 94918 [details] [review] Bug 23916: Add unit tests This patch adds unit tests for the new functions 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 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. 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> Rebased 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> 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> 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> 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> 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> 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> Created attachment 98594 [details] [review] Bug 24495: Reword change collection feature This patch changes the text of the "Collect from patron" field to read "Amount tendered" To Test: 1) In the Point of Sale screen, check that the text inside the "Collect payment" box reads "Collect from patron" 2) Go into a patron record, and create a manual invoice. 3) Go to Make a payment, and select to "Pay an amount". Check the text reads "Collect from patron". 4) Repeat the process with the "Pay Selected", and "Pay" button (listed next to the individual invoice) 5) Install the patch 6) Repeat steps 1-4, but the text should now read "Amount tendered", instead of "Collect from patron" in all payment options in the patron record, and in the Point of Sale screen. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com> 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> 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> 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> 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> 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> 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> 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 (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 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> 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> 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> 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> 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> 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> 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 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).
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> 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> 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> 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> 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> 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> 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 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 (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! 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? (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 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 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> 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> 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> 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> 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> 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> 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 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 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 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 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 :(
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> 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> 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> 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> 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> 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> 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 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 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 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)! Hi Katrin - Thanks for that. I've now removed those lines from that file, the commit message in the last patch explains why. 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> 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> 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> 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> 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> 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> 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 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 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 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)! 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' 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 Applying: Bug 23916: Record and display item issuer CONFLICT (content): Merge conflict in C4/Circulation.pm Recorded preimage for 'C4/Circulation.pm' Please rebase. 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> 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> 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> 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> 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> 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> 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 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 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 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)! 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' Now rebased, thanks Jonathan 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> Added missing Signed-off-by line from QA. We need to adapt the REST API routes to deal with this new value. 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) (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. Also the method Koha::Patron->issued_by, should be ->issuer I think Also the method is Koha::Checkout->issued_by, should be ->issuer I think (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 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. 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> Created attachment 113379 [details] [review] Bug 23916: (QA follow-up) Rename issuer => issuer_id [DB] Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 113380 [details] [review] Bug 23916: Add new attribute to API Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> Created attachment 113382 [details] [review] Bug 23916: Allow embedding the issuer object Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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? Created attachment 113396 [details] [review] Bug 23916: (follow-up) Adapt all the things to the new column and accessor names Pushed to master for 20.11, thanks to everybody involved! enhancement will not be backported to 20.05.x |