Bug 15163 - Patron attributes with branch limiits are not saved when invisible
Summary: Patron attributes with branch limiits are not saved when invisible
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 16504
  Show dependency treegraph
 
Reported: 2015-11-09 23:27 UTC by Jesse Weaver
Modified: 2019-06-27 09:24 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15163: Do not erase patron attributes if limited to another library (5.05 KB, patch)
2015-12-01 12:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15163: Do not erase patron attributes if limited to another library (5.28 KB, patch)
2015-12-01 12:58 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15163: Wip - stuck (15.29 KB, patch)
2016-01-08 09:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15163: Do not erase patron attributes if limited to another library (15.55 KB, patch)
2016-02-22 10:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15163: Do not erase patron attributes if limited to another library (15.61 KB, patch)
2016-02-23 21:52 UTC, Jesse Weaver
Details | Diff | Splinter Review
[PASSED QA] Bug 15163: Do not erase patron attributes if limited to another library (15.68 KB, patch)
2016-03-20 15:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
[3.20.x] Bug 15163: Do not erase patron attributes if limited to another library (15.70 KB, patch)
2016-03-23 15:44 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 Jesse Weaver 2015-11-09 23:27:02 UTC
Because only the patron attributes that are visible are saved when editing a patron, any attributes that are hidden from the member entry screen by a branch limitation will be erased when saving.

This could be solved one of two ways:

  a) Show all extended attributes when editing or
  b) Only delete the borrower attributes that are not limited or are visible at the current branch as the first step in C4::Members::Attributes::SetBorrowerAttributes.

(Marking critical because data is lost.)
Comment 1 Jonathan Druart 2015-11-10 14:48:27 UTC
Jessie,
Could you explain the workflow to get this issue?
It looks like a configuration issue.
Comment 2 Jesse Weaver 2015-11-18 17:50:17 UTC
Steps to reproduce:

  1) Log in at library "A".
  2) Create a patron attribute type that is limited to only library "A".
  3) Edit a patron, and add data for that attribute type.
  4) Log in at library "B".
  5) Edit the same patron. The attribute should, of course, not be visible. Save.
  6) Log in at library "A".
  7) Edit the same patron. The attribute data has disappeared.
Comment 3 HB-NEKLS 2015-11-18 20:04:21 UTC
Jonathan, Jesse's description is the behavior that I have seen also happen on my system. 

Additionally, I have seen this same behavior happen with patron categories that are limited to only certain branches (and maybe this needs to be a separate bug), but it seems to be the same type of problematic behavior: 

1. Login at Library A.
2. Set a patron category to be visible to only Library A.
3. Edit a patron and change the patron category to one that is only visible to Library A. 
4. Login at Library B.
5. Edit the same patron, and note that the patron category is now the first one on the list, not the one set in step 3. 
6. Login at Library A. 
7. Edit the same patron again; note that the patron category is not set to what was set in step 3.
Comment 4 Jonathan Druart 2015-12-01 12:21:31 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-12-01 12:22:23 UTC
I have failed to provide a correct patch for this fix in a respectable time.
This one should fix the issue but it is not the best way to do.
Comment 6 Marc Véron 2015-12-01 12:58:03 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2015-12-06 21:15:26 UTC
This works as described. 

With the proposed solution the values of the hidden patron attribute are visible in the HTML source of the page. I am not super happy about this, as it seems like a potential privacy problem to me.
Comment 8 Jonathan Druart 2016-01-07 15:48:16 UTC
(In reply to Katrin Fischer from comment #7)
> This works as described. 
> 
> With the proposed solution the values of the hidden patron attribute are
> visible in the HTML source of the page. I am not super happy about this, as
> it seems like a potential privacy problem to me.

I can add a syspref to choose between privacy and data lost :D
Comment 9 Katrin Fischer 2016-01-07 21:15:59 UTC
I am not sure there is a valid answer to this. 
Would it be possible to teach it not to update/change attributes not sent with the form at all?
Comment 10 Jesse Weaver 2016-01-07 23:23:51 UTC
Started working on this; Jonathan, what do you think of passing a $branchcode_limit parameter to SetBorrowerAttributes so it only deletes the correct attributes? (Either just passing 1 and using userenv, which is what AttributeTypes.pm does, or explicitly passing the branchcode, which is easier to test.)
Comment 11 Jonathan Druart 2016-01-08 09:55:25 UTC
Jesse, yes it should be the way to go but..
I have already tried something and was stuck. I have a wip branch but I don't remember what was wrong.

Looking at the diff it seems that I have tried to add a DeleteBorrowerAttributes subroutines and the code looks quite complicated:

+sub DeleteBorrowerAttributes {
+    my $borrowernumber = shift;
+    my $no_branch_limit = @_ ? shift : 0;
+    my $branch_limit = $no_branch_limit
+        ? 0
+        : C4::Context->userenv ? C4::Context->userenv->{"branch"} : 0;
+
+    my $dbh = C4::Context->dbh;
+    my $query = q{
+        DELETE FROM borrower_attributes
+        };
+    $query .= q{
+        LEFT JOIN borrower_attribute_types_branches ON bat_code = code
+        WHERE b_branchcode = ? OR b_branchcode IS NULL
+    } if $branch_limit;
+    $query .= q{
+        WHERE borrowernumber = ?
+    };
+
+    $dbh->do( $query, undef, $branch_limit ? $branch_limit : (), $borrowernumber );
+}
Comment 12 Jonathan Druart 2016-01-08 09:56:16 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2016-01-08 09:56:37 UTC
(In reply to Jonathan Druart from comment #12)
> Created attachment 46416 [details] [review] [review]
> Bug 15163: Wip - stuck

Jesse, here is my start.
Comment 14 HB-NEKLS 2016-01-14 22:01:27 UTC
Thanks Jonathan and Jesse for working on this. 

I understand the privacy concerns of some of the ways this could be resolved. The issue really does need to be resolved. I am unable to use branch specific limits on several sets of authorised_values & patron categories until this is resolved (which is not ideal). We have patrons who use multiple branches and when their accounts are edited at another branch, we risk data being lost. 

Just my two cents on how important this bug is to get resolved.
Comment 15 Jesse Weaver 2016-02-18 21:32:53 UTC
(In reply to Jonathan Druart from comment #11)
> Jesse, yes it should be the way to go but..
> I have already tried something and was stuck. I have a wip branch but I
> don't remember what was wrong.
> 
> Looking at the diff it seems that I have tried to add a
> DeleteBorrowerAttributes subroutines and the code looks quite complicated:
> 
> +sub DeleteBorrowerAttributes {
> +    my $borrowernumber = shift;
> +    my $no_branch_limit = @_ ? shift : 0;
> +    my $branch_limit = $no_branch_limit
> +        ? 0
> +        : C4::Context->userenv ? C4::Context->userenv->{"branch"} : 0;
> +
> +    my $dbh = C4::Context->dbh;
> +    my $query = q{
> +        DELETE FROM borrower_attributes
> +        };
> +    $query .= q{
> +        LEFT JOIN borrower_attribute_types_branches ON bat_code = code
> +        WHERE b_branchcode = ? OR b_branchcode IS NULL
> +    } if $branch_limit;
> +    $query .= q{
> +        WHERE borrowernumber = ?
> +    };
> +
> +    $dbh->do( $query, undef, $branch_limit ? $branch_limit : (),
> $borrowernumber );
> +}

That honestly looks exactly like what I would have done. Why send the hidden attributes in memberentrygen.tt, though, if DeleteBorrowerAttributes is branch-limited?
Comment 16 Jonathan Druart 2016-02-22 10:45:14 UTC Comment hidden (obsolete)
Comment 17 Jesse Weaver 2016-02-23 21:52:31 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2016-03-20 15:27:56 UTC
Created attachment 49329 [details] [review]
[PASSED QA] Bug 15163: Do not erase patron attributes if limited to another library

The patron attributes displayed on editing a patron are not displayed if
limited to another library.

C4::Members::Attributes::SetBorrowerAttributes will now only delete attributes
the librarian is editing.
SetBorrowerAttributes takes a new $no_branch_limit parameter. If set,
the branch limitations have not effect and all attributes are deleted
(same behavior as before this patch).

Test plan:
1/ Create 2 patron attributes, without branch limitations.
2/ Edit a patron and set a value for these attributes
3/ Limit a patron attributes to a library (one you are not logged in
with).
4/ Edit again the patron.
=> You should not see the limited attributes
5/ Edit the patron attributes and remove the branch limitation
=> Without this patch, it has been removed from the database and is not
displayed anymore.
=> With this patch, you should see it.

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Brendan Gallagher 2016-03-21 16:57:27 UTC
Pushed to Master - Should be in the May 2016 Release.  Thanks!
Comment 20 Julian Maurice 2016-03-23 06:39:42 UTC
Patch pushed to 3.22.x, will be in 3.22.5
Comment 21 Frédéric Demians 2016-03-23 12:41:02 UTC
This patch can't be applied on 3.20 branch due to Members_Attributes.t UT. Several solution: (1) skip it, (2) you propose a working UT for 3.20, (3) I skip the UT for 3.20. Any preference?
Comment 22 Jonathan Druart 2016-03-23 15:44:28 UTC
Created attachment 49448 [details] [review]
[3.20.x] Bug 15163: Do not erase patron attributes if limited to another library

The patron attributes displayed on editing a patron are not displayed if
limited to another library.

C4::Members::Attributes::SetBorrowerAttributes will now only delete attributes
the librarian is editing.
SetBorrowerAttributes takes a new $no_branch_limit parameter. If set,
the branch limitations have not effect and all attributes are deleted
(same behavior as before this patch).

Test plan:
1/ Create 2 patron attributes, without branch limitations.
2/ Edit a patron and set a value for these attributes
3/ Limit a patron attributes to a library (one you are not logged in
with).
4/ Edit again the patron.
=> You should not see the limited attributes
5/ Edit the patron attributes and remove the branch limitation
=> Without this patch, it has been removed from the database and is not
displayed anymore.
=> With this patch, you should see it.
Comment 23 Jonathan Druart 2016-03-23 15:44:57 UTC
I have not tested this patch, just fixed the conflict.