Description
Tomás Cohen Arazi (tcohen)
2024-04-01 21:15:45 UTC
I meant to say that when bug 33080 introduced this, we should've reviewed all controllers. Created attachment 164264 [details] [review] Bug 36483: Make API controllers use $c->objects->to_api Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 164266 [details] [review] Bug 36483: Make API controllers use $c->objects->to_api This patch makes all controller methods using direct $object->to_api, rely on $c->objects->to_api. Direct ->to_api calling prevents things like embeds, +strings, etc to be dealt with correctly. In some cases this change reduces the code as the authors extracted the embed structure from the stash and passed it explicitly. I left out two Patrons.pm cases because I need to study it a bit more. To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1 => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 164267 [details] [review] Bug 36483: Patrons.pm use case This case brought my attention as there was a different 'user' parameter being passed. It turns out we already thought about this, and `objects.to_api` is of course always passing it :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 164293 [details] [review] Bug 36483: Make API controllers use $c->objects->to_api This patch makes all controller methods using direct $object->to_api, rely on $c->objects->to_api. Direct ->to_api calling prevents things like embeds, +strings, etc to be dealt with correctly. In some cases this change reduces the code as the authors extracted the embed structure from the stash and passed it explicitly. I left out two Patrons.pm cases because I need to study it a bit more. To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1 => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Created attachment 164294 [details] [review] Bug 36483: Patrons.pm use case This case brought my attention as there was a different 'user' parameter being passed. It turns out we already thought about this, and `objects.to_api` is of course always passing it :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> > Created attachment 164294 [details] [review] [review]
> Bug 36483: Patrons.pm use case
>
> This case brought my attention as there was a different 'user' parameter
> being passed.
> It turns out we already thought about this, and `objects.to_api` is of
> course always passing it :-D
Do we have a test for this? I don't find it.
(In reply to Jonathan Druart from comment #7) > > Created attachment 164294 [details] [review] [review] [review] > > Bug 36483: Patrons.pm use case > > > > This case brought my attention as there was a different 'user' parameter > > being passed. > > It turns out we already thought about this, and `objects.to_api` is of > > course always passing it :-D > > Do we have a test for this? I don't find it. I've been thinking about those two cases. It is trivially correct that the 'user' parameter is passed (universally) by $c->objects->to_api. But I don't fully understand the semantics here. It would mean the API user could add/modify a patron, but then only get (say) the library_id in the response for adding/modifying. Am I right, Martin? Looks good to me. Do we actually have examples where passing this user makes a difference in to_api ? Seems to do nothing in Koha::Patron?? (In reply to Marcel de Rooy from comment #9) > Looks good to me. Do we actually have examples where passing this user makes > a difference in to_api ? Seems to do nothing in Koha::Patron?? For understanding it, you need to refer to: Koha/Object.pm:585~599 and once you got it, you read Koha/Patron.pm:2361~2370 It's the redaction stuff. Though talking to Tomas at hackfest I think we both agreed it doesn't entirely make sense that one could edit, add a user with fields that aren't accessible to them.. but I reckon that's a different bug.. I'll record it when I'm not on my phone. Just packed up for the day (In reply to Tomás Cohen Arazi from comment #10) > (In reply to Marcel de Rooy from comment #9) > > Looks good to me. Do we actually have examples where passing this user makes > > a difference in to_api ? Seems to do nothing in Koha::Patron?? > > For understanding it, you need to refer to: > > Koha/Object.pm:585~599 > and once you got it, you read > Koha/Patron.pm:2361~2370 (In reply to Martin Renvoize from comment #11) > It's the redaction stuff. Though talking to Tomas at hackfest I think we > both agreed it doesn't entirely make sense that one could edit, add a user > with fields that aren't accessible to them.. but I reckon that's a different > bug.. I'll record it when I'm not on my phone. Just packed up for the day Ah I vaguely remember QAing that stuff ;) Created attachment 164803 [details] [review] Bug 36483: Make API controllers use $c->objects->to_api This patch makes all controller methods using direct $object->to_api, rely on $c->objects->to_api. Direct ->to_api calling prevents things like embeds, +strings, etc to be dealt with correctly. In some cases this change reduces the code as the authors extracted the embed structure from the stash and passed it explicitly. I left out two Patrons.pm cases because I need to study it a bit more. To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1 => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 164804 [details] [review] Bug 36483: Patrons.pm use case This case brought my attention as there was a different 'user' parameter being passed. It turns out we already thought about this, and `objects.to_api` is of course always passing it :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Pushed for 24.05! Well done everyone, thank you! Pushed to 23.11.x for 23.11.06 Does not apply cleanly to 23.05.x, no backport. |