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.)
Jessie, Could you explain the workflow to get this issue? It looks like a configuration issue.
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.
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.
Created attachment 45258 [details] [review] 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. This is the easy but dirty way to fix this issue: It supposes that the data are not sensitive as they are now displayed in the html document. A better way to fix it would be to modify C4::Members::Attributes::SetBorrowerAttributes to delete only attributes we are editing. 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.
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.
Created attachment 45260 [details] [review] 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. This is the easy but dirty way to fix this issue: It supposes that the data are not sensitive as they are now displayed in the html document. A better way to fix it would be to modify C4::Members::Attributes::SetBorrowerAttributes to delete only attributes we are editing. 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. Followed test plan, works as described. Signed-off-by: Marc Véron <veron@veron.ch> http://bugs.koha-community.org/show_bug.cgi?id=15165
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.
(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
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?
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.)
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 ); +}
Created attachment 46416 [details] [review] Bug 15163: Wip - stuck
(In reply to Jonathan Druart from comment #12) > Created attachment 46416 [details] [review] [review] > Bug 15163: Wip - stuck Jesse, here is my start.
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.
(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?
Created attachment 48269 [details] [review] 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.
Created attachment 48322 [details] [review] 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> Works, sincere thanks Jonathan!
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>
Pushed to Master - Should be in the May 2016 Release. Thanks!
Patch pushed to 3.22.x, will be in 3.22.5
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?
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.
I have not tested this patch, just fixed the conflict.