|
Description
Julian Maurice
2023-02-16 13:40:34 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 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 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. Hi Philip, With this new patch you shouldn't get any error. 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> 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> Created attachment 155016 [details] [review] Bug 32980: (follow-up) Whitespace fix for perltidy warning 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> 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> Created attachment 155063 [details] [review] Bug 32980: (follow-up) Whitespace fix for perltidy warning Signed-off-by: Philip Orr <philip.orr@lmscloud.de> Created attachment 155201 [details] [review] Bug 32980: Use TestBuilder Also +x Any good reasons to not use TestBuilder? (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. 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
(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. (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. (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 (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) Hm, I can double check the OPAC behaviour maybe. But I think having different behaviour for staff and OPAC is also at least confusing. (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. Is this in disagreement with bug 35421? |