Bug 14655 - Cannot checkin without Anonymous Patron
Summary: Cannot checkin without Anonymous Patron
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 9942
Blocks: 14821 17296
  Show dependency treegraph
 
Reported: 2015-08-06 13:50 UTC by Nicole C. Engard
Modified: 2016-09-14 12:38 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14655: Add a warning if the checkin will fail (3.53 KB, patch)
2015-08-18 17:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14655: Add a warning on the about page if patrons have requested privacy (2.95 KB, patch)
2015-08-18 17:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
[Signed-off] Bug 14655: Add a warning if the checkin will fail (3.73 KB, patch)
2015-08-28 08:08 UTC, Marc Véron
Details | Diff | Splinter Review
[Signed-off] Bug 14655: Add a warning on the about page if patrons have requested privacy (3.12 KB, patch)
2015-08-28 08:09 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14655: Fix wording (3.33 KB, patch)
2015-09-14 12:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 14655: Add a warning if the checkin will fail (3.77 KB, patch)
2015-09-14 20:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14655: Add a warning on the about page if patrons have requested privacy (3.17 KB, patch)
2015-09-14 20:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14655: Fix wording (3.43 KB, patch)
2015-09-14 20:00 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2015-08-06 13:50:20 UTC
3.18.9 has a huge error.  You cannot check in if you don't have an anonymous patron.  Most of our partners don't allow opacprivacy or anonymous suggestions so they don't have an anonymous patron.  You shouldn't need one if you don't do anything with anonymizing.
Comment 1 Jonathan Druart 2015-08-06 14:13:15 UTC
I don't reproduce, please return the result of the following commands/queries

mysq> select value from systempreferences where variable="OPACPrivacy" or variable="AnonymousPatron";

$ git show-branch HEAD v3.18.09 # To be sure it's a pure 3.18.09
Comment 2 Nicole C. Engard 2015-08-06 16:45:45 UTC
For the record: http://irc.koha-community.org/koha/2015-08-06#i_1713701
Comment 3 Nicole C. Engard 2015-08-06 16:52:55 UTC
Jonathan,

We are using package sites - that might be why you can't reproduce. Sorry I left out that info.

Nicole
Comment 4 Jesse Weaver 2015-08-07 23:23:17 UTC
The issue here is that there can be a large number of patrons with a privacy of 2 ("Never") even if all of the following are true:

  * OPACPrivacy is off
  * All patron categories have a default privacy of Forever or Default

This can occur for three major reasons:

  * The patrons had their privacy settings changed when OPACPrivacy was previously enabled, but it has since been disabled
  * The patrons were imported with a privacy of 2
  * A patron category had a default privacy setting of Never at the time the patron was created (if the category's default privacy setting is later changed, it does not affect the privacy of previously created patrons)

This isn't an edge case; we have a large number of libraries whose checkins were silently failing after a bugfix upgrade due to one of the three scenarios above.
Comment 5 Katrin Fischer 2015-08-08 08:01:10 UTC
Hm, so it looks like the main problme is a mismatch of the template logic to display the warning with the logic in the module? (privacy setting of the borrower vs. system preferences?)
Comment 6 Katrin Fischer 2015-08-08 08:01:42 UTC
As this patch is in master - I am changing the version.
Comment 7 Jonathan Druart 2015-08-10 09:22:45 UTC
(In reply to Jesse Weaver from comment #4)
> The issue here is that there can be a large number of patrons with a privacy
> of 2 ("Never") even if all of the following are true:
> 
>   * OPACPrivacy is off
>   * All patron categories have a default privacy of Forever or Default
> 
> This can occur for three major reasons:
> 
>   * The patrons had their privacy settings changed when OPACPrivacy was
> previously enabled, but it has since been disabled

Restricting the privacy of the users is really not kind. Especially if they had chosen to protect their data previously.
I'd say it's like breaking a contract :)

>   * The patrons were imported with a privacy of 2

They should not have been imported with privacy=2 if OPACPrivacy is off.

>   * A patron category had a default privacy setting of Never at the time the
> patron was created (if the category's default privacy setting is later
> changed, it does not affect the privacy of previously created patrons)

Same as before, there is an inconsistency in the configuration ('Never' with OPACPrivacy off).

> This isn't an edge case; we have a large number of libraries whose checkins
> were silently failing after a bugfix upgrade due to one of the three
> scenarios above.

You should have got a big warnings during the updatedatabase process, don't you got it?

(In reply to Katrin Fischer from comment #5)
> Hm, so it looks like the main problme is a mismatch of the template logic to
> display the warning with the logic in the module? (privacy setting of the
> borrower vs. system preferences?)

The checks look good to me.

We could apply the following changes:

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 7813e33..2e06ff2 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -2077,7 +2077,7 @@ sub MarkIssueReturned {
     my ( $borrowernumber, $itemnumber, $dropbox_branch, $returndate, $privacy ) = @_;
 
     my $anonymouspatron;
-    if ( $privacy == 2 ) {
+    if ( $privacy == 2 and C4::Context->preference('OPACPrivacy') ) {

But I still think the privacy should be respected and the data updated if inconsistencies exist.
Comment 8 Katrin Fischer 2015-08-10 09:38:42 UTC
The suggested change looks good to me - I think that should work.
Comment 9 Jesse Weaver 2015-08-13 19:10:12 UTC
(In reply to Jonathan Druart from comment #7)
> (In reply to Jesse Weaver from comment #4)
> > The issue here is that there can be a large number of patrons with a privacy
> > of 2 ("Never") even if all of the following are true:
> > 
> >   * OPACPrivacy is off
> >   * All patron categories have a default privacy of Forever or Default
> > 
> > This can occur for three major reasons:
> > 
> >   * The patrons had their privacy settings changed when OPACPrivacy was
> > previously enabled, but it has since been disabled
> 
> Restricting the privacy of the users is really not kind. Especially if they
> had chosen to protect their data previously.
> I'd say it's like breaking a contract :)
> 
> >   * The patrons were imported with a privacy of 2
> 
> They should not have been imported with privacy=2 if OPACPrivacy is off.
> 
> >   * A patron category had a default privacy setting of Never at the time the
> > patron was created (if the category's default privacy setting is later
> > changed, it does not affect the privacy of previously created patrons)
> 
> Same as before, there is an inconsistency in the configuration ('Never' with
> OPACPrivacy off).
> 
> > This isn't an edge case; we have a large number of libraries whose checkins
> > were silently failing after a bugfix upgrade due to one of the three
> > scenarios above.
> 
> You should have got a big warnings during the updatedatabase process, don't
> you got it?
> 
> (In reply to Katrin Fischer from comment #5)
> > Hm, so it looks like the main problme is a mismatch of the template logic to
> > display the warning with the logic in the module? (privacy setting of the
> > borrower vs. system preferences?)
> 
> The checks look good to me.
> 
> We could apply the following changes:
> 
> diff --git a/C4/Circulation.pm b/C4/Circulation.pm
> index 7813e33..2e06ff2 100644
> --- a/C4/Circulation.pm
> +++ b/C4/Circulation.pm
> @@ -2077,7 +2077,7 @@ sub MarkIssueReturned {
>      my ( $borrowernumber, $itemnumber, $dropbox_branch, $returndate,
> $privacy ) = @_;
>  
>      my $anonymouspatron;
> -    if ( $privacy == 2 ) {
> +    if ( $privacy == 2 and C4::Context->preference('OPACPrivacy') ) {
> 
> But I still think the privacy should be respected and the data updated if
> inconsistencies exist.

Oh, indeed. Would it be possible to show the warning if anonymouspatron is not set and a patron exists with privacy == 2? It's not necessarily a huge issue with how the backend works, the issue is when the checkin explodes with no indication as to why.

Doing this with "SELECT borrowernumber FROM borrowers WHERE privacy = 2 LIMIT 1" ran in 0.02s on one of our largest sites (~125K borrowers), so it is practical.
Comment 10 Jonathan Druart 2015-08-18 15:10:12 UTC
(In reply to Jesse Weaver from comment #9)
> Doing this with "SELECT borrowernumber FROM borrowers WHERE privacy = 2
> LIMIT 1" ran in 0.02s on one of our largest sites (~125K borrowers), so it
> is practical.

I don't understand why you want that, could you please detail?
Have a look at the updatedb entry 3.21.00.013, we were careful to raise a warning if needed.
I don't think it's worth to add a select statement for every checkin.
Comment 11 Jonathan Druart 2015-08-18 17:16:27 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-08-18 17:16:35 UTC Comment hidden (obsolete)
Comment 13 Marc Véron 2015-08-28 08:08:06 UTC Comment hidden (obsolete)
Comment 14 Marc Véron 2015-08-28 08:09:41 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2015-09-06 21:57:06 UTC
This basically works, but 2 small glitches:

- When returning the item without the proper configuration, the warning message is shown, but the item still appears in the table below, even tho it was not checked in. It shows as: Not checked out. I think it should show an error and not appear in the table, as it has not been checked in.

- The error message is not ideal - I think you can't say 'a privacy':
Error: This patron has requested a privacy on returning item but the AnonymousPatron pref is not set correctly.

Suggestion:
This patron has requested their circulation history be anonymized on check-in, but the AnonymousPatron system preference is not set correctly.

It's also showing on checkout, so would have to be changed in 2 places.
Comment 16 Jesse Weaver 2015-09-10 00:06:10 UTC
(In reply to Katrin Fischer from comment #15)
> This basically works, but 2 small glitches:
> 
> - When returning the item without the proper configuration, the warning
> message is shown, but the item still appears in the table below, even tho it
> was not checked in. It shows as: Not checked out. I think it should show an
> error and not appear in the table, as it has not been checked in.
> 

Agreed; this quirk was very confusing to us and our partners before we figured out the issue.

> - The error message is not ideal - I think you can't say 'a privacy':
> Error: This patron has requested a privacy on returning item but the
> AnonymousPatron pref is not set correctly.
> 
> Suggestion:
> This patron has requested their circulation history be anonymized on
> check-in, but the AnonymousPatron system preference is not set correctly.
> 
> It's also showing on checkout, so would have to be changed in 2 places.

May I make the small suggestion of "... the AnonymousPatron system preference is empty or incorrect."? Tiny quibble, but either is an issue.
Comment 17 Jonathan Druart 2015-09-14 12:22:36 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2015-09-14 12:24:08 UTC
(In reply to Katrin Fischer from comment #15)
> This basically works, but 2 small glitches:
> 
> - When returning the item without the proper configuration, the warning
> message is shown, but the item still appears in the table below, even tho it
> was not checked in. It shows as: Not checked out. I think it should show an
> error and not appear in the table, as it has not been checked in.

Well, I consider this completely out of the scope of this bug fix.
The error/warning messages are not correctly handled at all in this area.
I would prefer to try and fix globally this issue considering it as an enh.

> - The error message is not ideal - I think you can't say 'a privacy':
> Error: This patron has requested a privacy on returning item but the
> AnonymousPatron pref is not set correctly.
> 
> Suggestion:
> This patron has requested their circulation history be anonymized on
> check-in, but the AnonymousPatron system preference is not set correctly.
> 
> It's also showing on checkout, so would have to be changed in 2 places.

Done.
Comment 19 Katrin Fischer 2015-09-14 12:36:12 UTC
I think you are right - fixing the first bit is more urgent. I think we should handle the 'not checked in but shows as checked in' case on a separate bug (bug not enh). Compromise? :)
Comment 20 Jonathan Druart 2015-09-14 13:44:46 UTC
(In reply to Katrin Fischer from comment #19)
> I think you are right - fixing the first bit is more urgent. I think we
> should handle the 'not checked in but shows as checked in' case on a
> separate bug (bug not enh). Compromise? :)

I had a quick look at the code this morning, it looks a bit more complicated.
Different errors should also be caught.
Comment 21 Katrin Fischer 2015-09-14 13:47:29 UTC
Not sure I understand - should this move on to QA and the other problems be dealt with on another bug - or does this patch need more work?
Comment 22 Jonathan Druart 2015-09-14 14:08:44 UTC
(In reply to Katrin Fischer from comment #21)
> Not sure I understand - should this move on to QA and the other problems be
> dealt with on another bug - or does this patch need more work?

What you described already exists before this patch set.
The errors handling in this area is really bad and won't be trivial to implement.
If we don't want to block this one, it should be pushed as it.
Comment 23 Katrin Fischer 2015-09-14 15:59:54 UTC
That's ok, I was just not sure what you meant with your last comment.
Comment 24 Katrin Fischer 2015-09-14 19:41:27 UTC
I have filed a separate bug for the 'check-in list' issue: Bug 14821
Comment 25 Katrin Fischer 2015-09-14 20:00:22 UTC
Created attachment 42540 [details] [review]
[PASSED QA] Bug 14655: Add a warning if the checkin will fail

On the checkin and checkout page, the checkin will fail if the patron
has requested the privacy and the AnonymousPatron is not correctly set.

This patch adds a warning message on both pages.

Test plan:
0/ Be sure you don't have any patron with privacy=2 (Never)
1/ Set OPACPrivacy, not AnonymousPatron
2/ Go on the checkin, you should a warning (same as before this patch).
3/ Set the privacy=2 for a patron
4/ Go on the circulation page, a warning should appear (for this
specific patron)
5/ Check an item out to this patron
6/ Check the item in on the checkin page.
The item is not checked in and you get a specific message for this
patron.

Confirm other/correct situations don't trigger the messages.

Followed test plan. Works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Katrin Fischer 2015-09-14 20:00:25 UTC
Created attachment 42541 [details] [review]
[PASSED QA] Bug 14655: Add a warning on the about page if patrons have requested privacy

New warning on the about page if at least a patron has requested a
privacy on checkin but the AnonymousPatron is not set to a valid patron.

Works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Katrin Fischer 2015-09-14 20:00:29 UTC
Created attachment 42542 [details] [review]
[PASSED QA] Bug 14655: Fix wording

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Fixed a missing space after Error: :)
Comment 28 Tomás Cohen Arazi 2015-09-16 13:52:32 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 29 Chris Cormack 2015-09-22 17:11:52 UTC
Pushed to 3.20.x will be in 3.20.4
Comment 30 Liz Rea 2015-09-22 23:18:04 UTC
Pushed to 3.18.x will be in 3.18.11