Bug 32980 - Checkouts for guarantees are not shown if patron also has a guarantor
Summary: Checkouts for guarantees are not shown if patron also has a guarantor
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-02-16 13:40 UTC by Julian Maurice
Modified: 2024-04-01 14:11 UTC (History)
5 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:
Circulation function:


Attachments
Bug 32980: Show checkouts of all guarantors/guarantees (staff interface) (2.61 KB, patch)
2023-02-16 13:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 32980: Make the return value of Koha::Patron::siblings consistent (2.92 KB, patch)
2023-08-31 12:12 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 32980: Show checkouts of all guarantors/guarantees (staff interface) (2.66 KB, patch)
2023-08-31 15:03 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32980: Make the return value of Koha::Patron::siblings consistent (2.97 KB, patch)
2023-08-31 15:03 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32980: (follow-up) Whitespace fix for perltidy warning (939 bytes, patch)
2023-08-31 15:03 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32980: Show checkouts of all guarantors/guarantees (staff interface) (2.71 KB, patch)
2023-09-01 08:02 UTC, Philip Orr
Details | Diff | Splinter Review
Bug 32980: Make the return value of Koha::Patron::siblings consistent (3.02 KB, patch)
2023-09-01 08:02 UTC, Philip Orr
Details | Diff | Splinter Review
Bug 32980: (follow-up) Whitespace fix for perltidy warning (990 bytes, patch)
2023-09-01 08:02 UTC, Philip Orr
Details | Diff | Splinter Review
Bug 32980: Use TestBuilder (2.03 KB, patch)
2023-09-04 14:03 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 Julian Maurice 2023-02-16 13:40:34 UTC
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)
Comment 1 Julian Maurice 2023-02-16 13:50:06 UTC
Created attachment 146731 [details] [review]
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
Comment 2 Philip Orr 2023-03-30 08:11:09 UTC
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
Comment 3 Julian Maurice 2023-08-31 12:12:45 UTC
Created attachment 155004 [details] [review]
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.
Comment 4 Julian Maurice 2023-08-31 12:14:27 UTC
Hi Philip,

With this new patch you shouldn't get any error.
Comment 5 Owen Leonard 2023-08-31 15:03:36 UTC
Created attachment 155014 [details] [review]
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>
Comment 6 Owen Leonard 2023-08-31 15:03:38 UTC
Created attachment 155015 [details] [review]
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>
Comment 7 Owen Leonard 2023-08-31 15:03:40 UTC
Created attachment 155016 [details] [review]
Bug 32980: (follow-up) Whitespace fix for perltidy warning
Comment 8 Philip Orr 2023-09-01 08:02:35 UTC
Created attachment 155061 [details] [review]
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>
Comment 9 Philip Orr 2023-09-01 08:02:38 UTC
Created attachment 155062 [details] [review]
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>
Comment 10 Philip Orr 2023-09-01 08:02:40 UTC
Created attachment 155063 [details] [review]
Bug 32980: (follow-up) Whitespace fix for perltidy warning

Signed-off-by: Philip Orr <philip.orr@lmscloud.de>
Comment 11 Jonathan Druart 2023-09-04 14:03:27 UTC
Created attachment 155201 [details] [review]
Bug 32980: Use TestBuilder

Also +x
Comment 12 Jonathan Druart 2023-09-04 14:03:35 UTC
Any good reasons to not use TestBuilder?
Comment 13 Julian Maurice 2023-09-04 15:00:00 UTC
(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.
Comment 14 Katrin Fischer 2023-09-17 21:33:39 UTC
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
Comment 15 Julian Maurice 2023-09-18 11:48:38 UTC
(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.
Comment 16 Katrin Fischer 2023-09-18 11:52:52 UTC
(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.
Comment 17 Nick Clemens (kidclamp) 2023-09-18 13:20:01 UTC
(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)
> (In reply to Jonathan Druart from comment #12)
> > 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
> 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
Comment 18 Julian Maurice 2023-09-18 13:37:39 UTC
(In reply to Nick Clemens from comment #17)
> 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 populating columns I don't want to populate. I'll see if I can make this behaviour configurable (in another bug report)
Comment 19 Katrin Fischer 2023-09-18 13:54:36 UTC
Hm, I can double check the OPAC behaviour maybe. But I think having different behaviour for staff and OPAC is also at least confusing.
Comment 20 Katrin Fischer 2023-10-29 23:07:15 UTC
(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.
Comment 21 Katrin Fischer 2024-04-01 14:11:35 UTC
Is this in disagreement with bug 35421?