[Bug 33556] New: $c->validation->output should be avoided
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Bug ID: 33556 Summary: $c->validation->output should be avoided Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com CC: tomascohen@gmail.com Talking to the OpenAPI plugin maintainer, he mentioned the use of $c->validation->output should be avoided as the plugin is not designed to have a stable behavior there, and he even thought of just removing the method. That method returns an internal data structure the plugin uses to validate things, and then updates the request itself. Take the following example: GET /patrons x-koha-embed: checkouts,library without the OpenAPI plugin, requesting the header like this: $c->req->headers->header('x-koha-embed') would return a scalar, the string 'checkouts,library'. When using the plugin, and with `x-koha-embed` being defined as collectionFormat: csv, that header is entirely replaced by an arrayref. That's how the plugin works and how it is expected to be used. So we need to replace the uses of $c->validation->format, with normal Mojo usage to avoid future headaches. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |33852 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33852 [Bug 33852] jobs.t is not testing only_current -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 152212 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152212&action=edit Bug 33556: Path parameters are handled explicitly in the controllers In the case of $c->objects->search_rs, the variable is just not used. In the case of /acq/orders, it's a leftover from when we removed in the helper. Check there are tests with path params everywhere (including orders) and it has no effect. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 152213 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152213&action=edit Bug 33556: Avoid relying on $c->validation Talking to the OpenAPI plugin maintainer, he mentioned the use of $c->validation->output should be avoided as the plugin is not designed to have a stable behavior there, and he even thought of just removing the method. That method returns an internal data structure the plugin uses to validate things, and then updates the request itself. Take the following example: GET /patrons/123 x-koha-embed: checkouts,library without the OpenAPI plugin, requesting the header like this: $c->req->headers->header('x-koha-embed') would return a scalar, the string 'checkouts,library'. When using the plugin, and with `x-koha-embed` being defined as collectionFormat: csv, that header is entirely replaced by an arrayref. That's how the plugin works and how it is expected to be used. So we need to replace the uses of $c->validation format, with normal Mojo usage to avoid future headaches. This patch changes: * $c->validation->param => $c->param * $c->validation->param('body') => $c->req->json To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/*.t => SUCCESS: Tests pass! 2. Apply this patches 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #3 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 152214 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152214&action=edit Bug 33556: Easy ERM-related changes I left the (easy) ERM cases out because it throws this when running the QA script: Processing files before patches |========================>| 51 / 51 (100.00%) An error occurred : Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Titles::Local': current merge results [ Koha::REST::V1::ERM::EHoldings::Titles::Local, ] merging failed on 'Mojolicious::Controller' at /kohadevbox/qa-test-tools/koha-qa.pl line 112. and didn't want this to pollute the rest of the changes. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|$c->validation->output |$c->validation should be |should be avoided |avoided CC| |jonathan.druart+koha@gmail. | |com, | |kyle@bywatersolutions.com, | |martin.renvoize@ptfs-europe | |.com, | |pedro.amorim@ptfs-europe.co | |m -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- I did the trivial ones. Which could be pushed as-is while we figure the tricky stuff. The idea behind the previous sentence is we can make devs rely on better examples for ongoing stuff. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #5 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- All looks good to me (review, tests (api+cypress) and a couple of manual tests). I am good with pushing as it, but we need a plan for the remaining occurrences. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152212|0 |1 is obsolete| | --- Comment #6 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 152693 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152693&action=edit Bug 33556: Path parameters are handled explicitly in the controllers In the case of $c->objects->search_rs, the variable is just not used. In the case of /acq/orders, it's a leftover from when we removed in the helper. Check there are tests with path params everywhere (including orders) and it has no effect. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152213|0 |1 is obsolete| | --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 152694 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152694&action=edit Bug 33556: Avoid relying on $c->validation Talking to the OpenAPI plugin maintainer, he mentioned the use of $c->validation->output should be avoided as the plugin is not designed to have a stable behavior there, and he even thought of just removing the method. That method returns an internal data structure the plugin uses to validate things, and then updates the request itself. Take the following example: GET /patrons/123 x-koha-embed: checkouts,library without the OpenAPI plugin, requesting the header like this: $c->req->headers->header('x-koha-embed') would return a scalar, the string 'checkouts,library'. When using the plugin, and with `x-koha-embed` being defined as collectionFormat: csv, that header is entirely replaced by an arrayref. That's how the plugin works and how it is expected to be used. So we need to replace the uses of $c->validation format, with normal Mojo usage to avoid future headaches. This patch changes: * $c->validation->param => $c->param * $c->validation->param('body') => $c->req->json To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/*.t => SUCCESS: Tests pass! 2. Apply this patches 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152214|0 |1 is obsolete| | --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 152695 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152695&action=edit Bug 33556: Easy ERM-related changes I left the (easy) ERM cases out because it throws this when running the QA script: Processing files before patches |========================>| 51 / 51 (100.00%) An error occurred : Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Titles::Local': current merge results [ Koha::REST::V1::ERM::EHoldings::Titles::Local, ] merging failed on 'Mojolicious::Controller' at /kohadevbox/qa-test-tools/koha-qa.pl line 112. and didn't want this to pollute the rest of the changes. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Signed Off --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I think this all looks solid.. so adding my signoff line. As for catching cases introduced in the future.. perhaps a QA script addition to spot `validation->param` introductions? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #10 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- https://gitlab.com/koha-community/qa-test-tools/-/issues/68 I'll try to find a moment to have a look at submitting something for that. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to Martin Renvoize from comment #10)
https://gitlab.com/koha-community/qa-test-tools/-/issues/68
I'll try to find a moment to have a look at submitting something for that.
Done -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #12 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Martin Renvoize from comment #11)
(In reply to Martin Renvoize from comment #10)
https://gitlab.com/koha-community/qa-test-tools/-/issues/68
I'll try to find a moment to have a look at submitting something for that.
Done
I let you a comment there. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- We are failing patches in qa tools with a reference to a patch that does not even apply??? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #14 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Marcel de Rooy from comment #13)
We are failing patches in qa tools with a reference to a patch that does not even apply???
We don't want new occurrences. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152693|0 |1 is obsolete| | Attachment #152694|0 |1 is obsolete| | Attachment #152695|0 |1 is obsolete| | --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 153725 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153725&action=edit Bug 33556: Path parameters are handled explicitly in the controllers In the case of $c->objects->search_rs, the variable is just not used. In the case of /acq/orders, it's a leftover from when we removed in the helper. Check there are tests with path params everywhere (including orders) and it has no effect. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #16 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 153726 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153726&action=edit Bug 33556: Avoid relying on $c->validation Talking to the OpenAPI plugin maintainer, he mentioned the use of $c->validation->output should be avoided as the plugin is not designed to have a stable behavior there, and he even thought of just removing the method. That method returns an internal data structure the plugin uses to validate things, and then updates the request itself. Take the following example: GET /patrons/123 x-koha-embed: checkouts,library without the OpenAPI plugin, requesting the header like this: $c->req->headers->header('x-koha-embed') would return a scalar, the string 'checkouts,library'. When using the plugin, and with `x-koha-embed` being defined as collectionFormat: csv, that header is entirely replaced by an arrayref. That's how the plugin works and how it is expected to be used. So we need to replace the uses of $c->validation format, with normal Mojo usage to avoid future headaches. This patch changes: * $c->validation->param => $c->param * $c->validation->param('body') => $c->req->json To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/*.t => SUCCESS: Tests pass! 2. Apply this patches 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #17 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 153727 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153727&action=edit Bug 33556: Easy ERM-related changes I left the (easy) ERM cases out because it throws this when running the QA script: Processing files before patches |========================>| 51 / 51 (100.00%) An error occurred : Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Titles::Local': current merge results [ Koha::REST::V1::ERM::EHoldings::Titles::Local, ] merging failed on 'Mojolicious::Controller' at /kohadevbox/qa-test-tools/koha-qa.pl line 112. and didn't want this to pollute the rest of the changes. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |33974 --- Comment #18 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Minor rebase against bug 33974 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33974 [Bug 33974] Add ability to search biblios endpoint any biblioitem attribute -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Seeing it too: An error occurred : Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Resources::Local': current merge results [ Koha::REST::V1::ERM::EHoldings::Resources::Local, ] merging failed on 'Mojolicious::Controller' at /app/qatools/koha-qa.pl line 112. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Please clarify -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #21 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Marcel de Rooy from comment #20)
Please clarify
I can't, and won't. It is an underlying problem with the modules design, which the author(s) should address. My feeling is it is related to this: use Mojo::Base 'Mojolicious::Controller'; use Koha::ERM::EHoldings::Titles; . We can leave out the ERM-related stuff from this bug if you ask me, as it is incomplete anyways. The trivial changes I'm making are trivial an unrelated to the error. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|$c->validation should be |$c->validation should be |avoided |avoided (part 1) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153725|0 |1 is obsolete| | Attachment #153726|0 |1 is obsolete| | Attachment #153727|0 |1 is obsolete| | --- Comment #22 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 153784 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153784&action=edit Bug 33556: Path parameters are handled explicitly in the controllers In the case of $c->objects->search_rs, the variable is just not used. In the case of /acq/orders, it's a leftover from when we removed in the helper. Check there are tests with path params everywhere (including orders) and it has no effect. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |34339 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34339 [Bug 34339] $c->validation should be avoided (part 2) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #23 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 153785 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153785&action=edit Bug 33556: Avoid relying on $c->validation Talking to the OpenAPI plugin maintainer, he mentioned the use of $c->validation->output should be avoided as the plugin is not designed to have a stable behavior there, and he even thought of just removing the method. That method returns an internal data structure the plugin uses to validate things, and then updates the request itself. Take the following example: GET /patrons/123 x-koha-embed: checkouts,library without the OpenAPI plugin, requesting the header like this: $c->req->headers->header('x-koha-embed') would return a scalar, the string 'checkouts,library'. When using the plugin, and with `x-koha-embed` being defined as collectionFormat: csv, that header is entirely replaced by an arrayref. That's how the plugin works and how it is expected to be used. So we need to replace the uses of $c->validation format, with normal Mojo usage to avoid future headaches. This patch changes: * $c->validation->param => $c->param * $c->validation->param('body') => $c->req->json To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/*.t => SUCCESS: Tests pass! 2. Apply this patches 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #24 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Tomás Cohen Arazi from comment #21)
(In reply to Marcel de Rooy from comment #20)
Please clarify
I can't, and won't. It is an underlying problem with the modules design, which the author(s) should address. My feeling is it is related to this:
use Mojo::Base 'Mojolicious::Controller';
use Koha::ERM::EHoldings::Titles;
.
We can leave out the ERM-related stuff from this bug if you ask me, as it is incomplete anyways. The trivial changes I'm making are trivial an unrelated to the error.
I think we simply asking to clarify the situation. You are first saying you don't want to go with ERM because of the errors, but the patch is still there. I moved it to bug 34339 so that this first step can be pushed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #25 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #24)
I think we simply asking to clarify the situation. You are first saying you don't want to go with ERM because of the errors, but the patch is still there.
I moved it to bug 34339 so that this first step can be pushed.
It's fine, I'm just answering I don't have an answer. And the change was so small... :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #26 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #24)
I think we simply asking to clarify the situation.
I remember now. We got communication issues. There are two different problems with the ERM-related code, that are (1) addressed but separatedly and (2) left out: (1) I put the (minor) ERM changes on a separate bug so it doesn't pollute the other changes that are a lot and harder to rebase, because it fails badly and didn't allow to run 'qa' on the big one. (2) The ERM missing changes I'm referring to, are not those on that patch, but the fact this modules make use of `my $args = $c->validation->output` to be passed to the specific classes implementing the business logic for each case, and I didn't 100% understand how things worked (they feel a bit hacky) so wasn't sure about the right way to do it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #27 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Tomás Cohen Arazi from comment #26)
I remember now. We got communication issues. There are two different problems with the ERM-related code, that are (1) addressed but separatedly and (2) left out:
(1) I put the (minor) ERM changes on a separate bug so it doesn't pollute the other changes that are a lot and harder to rebase, because it fails badly and didn't allow to run 'qa' on the big one.
No, it was on this bug report, hence the confusion.
(2) The ERM missing changes I'm referring to, are not those on that patch, but the fact this modules make use of `my $args = $c->validation->output` to be passed to the specific classes implementing the business logic for each case, and I didn't 100% understand how things worked (they feel a bit hacky) so wasn't sure about the right way to do it.
It certainly feels hacky, but I will someone to explain how to fix them properly. We are not manipulating usual Koha::Objects here, we are pulling data for an external service and serve it to the Koha ERM Vue app. So we cannot use the regular REST::Plugin::, we are trying to behave the same as the other objects so that the app does not need to do different for those endpoints. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #28 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #27)
(2) The ERM missing changes I'm referring to, are not those on that patch, but the fact this modules make use of `my $args = $c->validation->output` to be passed to the specific classes implementing the business logic for each case, and I didn't 100% understand how things worked (they feel a bit hacky) so wasn't sure about the right way to do it.
It certainly feels hacky, but I will someone to explain how to fix them properly. We are not manipulating usual Koha::Objects here, we are pulling data for an external service and serve it to the Koha ERM Vue app. So we cannot use the regular REST::Plugin::, we are trying to behave the same as the other objects so that the app does not need to do different for those endpoints.
I mean this: my $args = $c->validation->output; my $ebsco = Koha::ERM::Providers::EBSCO->new; ... my $total = $result->{totalResults}; $total = 10000 if $total > 10000; $c->add_pagination_headers( { base_total => $base_total, page => $page, per_page => $per_page, query_params => $args, total => $total, } ); and similar. I wasn't sure how relevant the $args are here. I also feels like it is not entirely correct, as $c->validation->output actually contains path parameters, not just query params. So I felt like I didn't have enough information to fix it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |23.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 --- Comment #29 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|23.11.00 |23.11.00,23.05.03 released in| | Status|Pushed to master |Pushed to stable CC| |fridolin.somers@biblibre.co | |m --- Comment #30 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.05.x for 23.05.03 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33556 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to stable |RESOLVED --- Comment #31 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Does not apply to 22.11.x. Not pushing. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org