Bug 14655

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

For the record: http://irc.koha-community.org/koha/2015-08-06#i_1713701

Jonathan,

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

Nicole

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.

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?)

As this patch is in master - I am changing the version.

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

The suggested change looks good to me - I think that should work.

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

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

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.

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

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

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? :)

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

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?

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

That's ok, I was just not sure what you meant with your last comment.

I have filed a separate bug for the 'check-in list' issue: Bug 14821

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>

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>

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: :)

Patches pushed to master.

Thanks Jonathan!

Pushed to 3.20.x will be in 3.20.4

Pushed to 3.18.x will be in 3.18.11