[Bug 32980] New: Checkouts for guarantees are not shown if patron also has a guarantor
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 Bug ID: 32980 Summary: Checkouts for guarantees are not shown if patron also has a guarantor Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: ASSIGNED Severity: normal Priority: P5 - low Component: Circulation Assignee: julian.maurice@biblibre.com Reporter: julian.maurice@biblibre.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com Steps to reproduce: 1. Create a patron category of type 'Professional' (or any type that allows guarantors) 2. Create 3 patrons in this category. Let's call them A, B, and C 3. Do at least one checkout for A and at least one for C 4. Make B the guarantor of C 5. Go to B details page. You should see the checkouts of C in the "Relatives' checkouts" tab 6. Go to B checkout page. You should see the checkouts of C in the "Relatives' checkouts" tab 7. Make A the guarantor of B 8. Go to B details page. Now you should see the checkouts of A in the "Relatives' checkouts" tab but not the checkouts of C (KO) 9. Go to B checkout page. Now you should only the checkouts of A in the "Relatives' checkouts" tab but not the checkouts of C (KO) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 --- Comment #1 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 146731 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=146731&action=edit Bug 32980: Show checkouts of all guarantors/guarantees (staff interface) Test plan: 1. Create a patron category of type 'Professional' (or any type that allows guarantors) 2. Create 3 patrons in this category. Let's call them A, B, and C 3. Do at least one checkout for A and at least one for C 4. Make A the guarantor of B, and B the guarantor of C 5. Go to B details page. You should see the checkouts of A and C in the "Relatives' checkouts" tab 6. Go to B checkout page. You should see the checkouts of A and C in the "Relatives' checkouts" tab -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 Philip Orr <philip.orr@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |philip.orr@lmscloud.de Status|Needs Signoff |Failed QA --- Comment #2 from Philip Orr <philip.orr@lmscloud.de> --- Hi Julian, at step 2 when trying to save a new patron I get the following error: Can't call method "as_list" on an undefined value at /kohadevbox/koha/members/moremember.pl line 102 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 --- Comment #3 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 155004 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155004&action=edit Bug 32980: Make the return value of Koha::Patron::siblings consistent Without the patch, Koha::Patron::siblings can return: - undef (or an empty list in list context) if patron has no guarantors - a Koha::Patrons object if patron has at least one guarantor, which doesn't necessarily mean that they have siblings (they can be the only guarantee) For ease of use, this subroutine should always return a Koha::Patrons object, even if it represents an empty set. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #4 from Julian Maurice <julian.maurice@biblibre.com> --- Hi Philip, With this new patch you shouldn't get any error. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #146731|0 |1 is obsolete| | Attachment #155004|0 |1 is obsolete| | --- Comment #5 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 155014 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155014&action=edit Bug 32980: Show checkouts of all guarantors/guarantees (staff interface) Test plan: 1. Create a patron category of type 'Professional' (or any type that allows guarantors) 2. Create 3 patrons in this category. Let's call them A, B, and C 3. Do at least one checkout for A and at least one for C 4. Make A the guarantor of B, and B the guarantor of C 5. Go to B details page. You should see the checkouts of A and C in the "Relatives' checkouts" tab 6. Go to B checkout page. You should see the checkouts of A and C in the "Relatives' checkouts" tab Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 --- Comment #6 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 155015 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155015&action=edit Bug 32980: Make the return value of Koha::Patron::siblings consistent Without the patch, Koha::Patron::siblings can return: - undef (or an empty list in list context) if patron has no guarantors - a Koha::Patrons object if patron has at least one guarantor, which doesn't necessarily mean that they have siblings (they can be the only guarantee) For ease of use, this subroutine should always return a Koha::Patrons object, even if it represents an empty set. Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 --- Comment #7 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 155016 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155016&action=edit Bug 32980: (follow-up) Whitespace fix for perltidy warning -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 Philip Orr <philip.orr@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155014|0 |1 is obsolete| | --- Comment #8 from Philip Orr <philip.orr@lmscloud.de> --- Created attachment 155061 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155061&action=edit Bug 32980: Show checkouts of all guarantors/guarantees (staff interface) Test plan: 1. Create a patron category of type 'Professional' (or any type that allows guarantors) 2. Create 3 patrons in this category. Let's call them A, B, and C 3. Do at least one checkout for A and at least one for C 4. Make A the guarantor of B, and B the guarantor of C 5. Go to B details page. You should see the checkouts of A and C in the "Relatives' checkouts" tab 6. Go to B checkout page. You should see the checkouts of A and C in the "Relatives' checkouts" tab Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Philip Orr <philip.orr@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 Philip Orr <philip.orr@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155015|0 |1 is obsolete| | --- Comment #9 from Philip Orr <philip.orr@lmscloud.de> --- Created attachment 155062 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155062&action=edit Bug 32980: Make the return value of Koha::Patron::siblings consistent Without the patch, Koha::Patron::siblings can return: - undef (or an empty list in list context) if patron has no guarantors - a Koha::Patrons object if patron has at least one guarantor, which doesn't necessarily mean that they have siblings (they can be the only guarantee) For ease of use, this subroutine should always return a Koha::Patrons object, even if it represents an empty set. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Philip Orr <philip.orr@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 Philip Orr <philip.orr@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155016|0 |1 is obsolete| | --- Comment #10 from Philip Orr <philip.orr@lmscloud.de> --- Created attachment 155063 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155063&action=edit Bug 32980: (follow-up) Whitespace fix for perltidy warning Signed-off-by: Philip Orr <philip.orr@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 --- Comment #11 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 155201 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155201&action=edit Bug 32980: Use TestBuilder Also +x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 --- Comment #12 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Any good reasons to not use TestBuilder? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 --- Comment #13 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Jonathan Druart from comment #12) > Any good reasons to not use TestBuilder? I have some reasons... not sure if they're good enough: - I'm used to DBIx::Class API and always forget how to use TestBuilder, so it's faster for me to write it like that (okay that one is not that good, I'm just lazy). - TestBuilder populates all columns, which can make tests fail randomly. Without it I don't have to think about those cases. - TestBuilder is not really useful in a simple case like this IMO - While writing tests I like to commit transaction and explore the database to see what's going wrong. And it's a lot easier when it's not filled with "AI37AMe2_0aESHPuxgZVLAmwHXOohDl" in every column. I'm ok with your patch but I don't understand why you took the time to write it. Is it mandatory to use TestBuilder in test files ? What do we gain ? Certainly not speed because it now takes 40% more time for the test to complete. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |katrin.fischer@bsz-bw.de |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |nick@bywatersolutions.com --- Comment #14 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- This doesn't feel correct, even with the patch. What I would expect for a relationship: The guarantor can see the checkouts of their guarantees, as the label is: Show checkouts to guarantor (not to guarantees). A > B > C
= is guarantor of
Expectation: A sees B's checkouts, maybe also C's B sees C's checkouts C sees only their own. Master: A sees B's checkouts B sees A's checkouts - should see C's checkouts C sees B's checkouts - should see no other checkouts With patch: A sees B's checkouts B sees A's and C's checkouts - should only see C's checkouts C sees B's checkouts - should only see their own checkouts -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 --- Comment #15 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Katrin Fischer from comment #14)
This doesn't feel correct, even with the patch.
What I would expect for a relationship: The guarantor can see the checkouts of their guarantees, as the label is: Show checkouts to guarantor (not to guarantees).
It looks like this setting (privacy_guarantor_checkouts) only affects OPAC ? The checkouts tab is labelled as "Relatives' checkouts" and apparently included checkouts for guarantor, siblings and guarantees for a long time. It was like that before Bug 15656 (pushed in 16.05) introduced the problem that bug 32980 (this one) is trying to solve. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |caroline.cyr-la-rose@inlibr | |o.com --- Comment #16 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Julian Maurice from comment #15)
(In reply to Katrin Fischer from comment #14)
This doesn't feel correct, even with the patch.
What I would expect for a relationship: The guarantor can see the checkouts of their guarantees, as the label is: Show checkouts to guarantor (not to guarantees).
It looks like this setting (privacy_guarantor_checkouts) only affects OPAC ?
The checkouts tab is labelled as "Relatives' checkouts" and apparently included checkouts for guarantor, siblings and guarantees for a long time. It was like that before Bug 15656 (pushed in 16.05) introduced the problem that bug 32980 (this one) is trying to solve.
It feels honestly wrong to me. I have added Nick/Kyle/Caroline in CC to get more opinions. In a parent/child situation I cannot imagine it would be wanted that the children see the parents' checkouts? Also the labelling on the buttons/checkboxes suggests otherwise and they would at least need to be fixed. But I think we should not move here before we agree on the intended behavior. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 --- Comment #17 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Katrin Fischer from comment #16)
What I would expect for a relationship: The guarantor can see the checkouts of their guarantees, as the label is: Show checkouts to guarantor (not to guarantees). It looks like this setting (privacy_guarantor_checkouts) only affects OPAC ?
Also the labelling on the buttons/checkboxes suggests otherwise and they would at least need to be fixed. But I think we should not move here before we agree on the intended behavior.
Those buttons do control the OPAC, maybe it could be made more clear, but I don't see it as a blocker here
The checkouts tab is labelled as "Relatives' checkouts" and apparently included checkouts for guarantor, siblings and guarantees for a long time.
In a parent/child situation I cannot imagine it would be wanted that the children see the parents' checkouts?
I think Julian is right, this is how it worked for a long time. And we are talking about staff seeing the checkouts, not patrons. The staff shouldn't reveal that info if it goes against patron privacy, but I don't think it necessarily needs to be hidden from staff. I do think that a preference here would help. Things changed when we added multiple guarantors, and guarantors with guarantors, and there should probably be switches for both 'Allow staff to see guarantees checkouts' and 'Allow staff to view guarantors checkout' - sysprefs or permissions, I am not sure which is preferable, but I think that can be its own bug too. I do wonder how 'view_borrower_infos_from_any_libraries' should be treated. Currently we will show the guarantee/tor names, and we will show those names with the checkouts as well, though the staff may not be able to view that borrower directly. And that is probably yet another bug (In reply to Julian Maurice from comment #13)
Any good reasons to not use TestBuilder? I'm ok with your patch but I don't understand why you took the time to write it. Is it mandatory to use TestBuilder in test files ? What do we gain ? Certainly not speed because it now takes 40% more time for
(In reply to Jonathan Druart from comment #12) the test to complete.
I think we gain consistency - if we always use builder, then it makes it easy to see how to write tests. It is also safer for naming, to avoid collision with possible test data in the system generated by humans (CCCCCCC is something we could add for testing, and suddenly your tests are failing) If there are fields you expect to be empty when building, we have sub _gen_default_values and should add more cases as needed -- You are receiving this mail because: You are watching all bug changes.
I think we gain consistency - if we always use builder, then it makes it easy to see how to write tests. It is also safer for naming, to avoid collision with possible test data in the system generated by humans (CCCCCCC is something we could add for testing, and suddenly your tests are failing) If there are fields you expect to be empty when building, we have sub _gen_default_values and should add more cases as needed 'CCCC...' is a pattern for String::Random, so the risk of collision is minimal (and the same as with TestBuilder I think ?). I agree with the consistency point, but I really don't like that TestBuilder is
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 --- Comment #18 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Nick Clemens from comment #17) populating columns I don't want to populate. I'll see if I can make this behaviour configurable (in another bug report) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 --- Comment #19 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hm, I can double check the OPAC behaviour maybe. But I think having different behaviour for staff and OPAC is also at least confusing. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 --- Comment #20 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Katrin Fischer from comment #19)
Hm, I can double check the OPAC behaviour maybe. But I think having different behaviour for staff and OPAC is also at least confusing.
I didn't get back to test this one yet. I am happy to have someone else take the lead here and won't block any decision made. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|katrin.fischer@bsz-bw.de |testopia@bugs.koha-communit | |y.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=35421 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 --- Comment #21 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Is this in disagreement with bug 35421? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=37892 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32980 --- Comment #22 from Fridolin Somers <fridolin.somers@biblibre.com> --- Indeed, since Bug 37892 is pushed looks like this is obsolete. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org