Any endpoint that is calling $c->objects->search after calling $c->req->params->remove('param_name') is having that parameter reinstated to the search object
Created attachment 184155 [details] [review] Bug 40417: Regression test
To reproduce, apply the regression test patch and run the test - it will fail
Created attachment 184160 [details] [review] Bug 40417: Regression test
Created attachment 184161 [details] [review] Bug 40417: Remove 'validation' method
This looks correct, but we need to deal with other things for this not to break existing features. Look at the dependencies I added. BTW. I added ``` use DDP; p($c->validation->output); p($c->req->params->to_hash); ``` at the beginning of `objects.search_rs` and run: ``` prove t/db_dependent/api/v1/query.t ``` and verified one thing I mentioned yesterday: when a query parameter is passed multiple times, the calculated structure is the same (i.e. `$args`). When it is only one, they differ. This might not be a real issue, but it is worth making sure we have tests and it doesn't break anything.
This looks good. I added a PR on the QA tools repo so the QA script doesn't look for `$c->validation` in the test file, as we really want it there for the regression test. Please enhance the commit message.
Created attachment 184212 [details] [review] Bug 40417: Regression test prove t/db_dependent/Koha/REST/Plugin/Objects.t
Created attachment 184213 [details] [review] Bug 40417: Remove 'validation' method We shouldn't use the ->validation method anymore as it isn't stable. We need to pass the params into the method accounting for any param changes made Test plan: 1) Apply the regression test patch and run the test 2) Test should fail 3) Apply this patch 4) Run test again 5) Test should pass
Created attachment 184317 [details] [review] Bug 40417: Regression test prove t/db_dependent/Koha/REST/Plugin/Objects.t Signed-off-by: David Nind <david@davidnind.com>
Created attachment 184318 [details] [review] Bug 40417: Remove 'validation' method We shouldn't use the ->validation method anymore as it isn't stable. We need to pass the params into the method accounting for any param changes made Test plan: 1) Apply the regression test patch and run the test 2) Test should fail 3) Apply this patch 4) Run test again 5) Test should pass Signed-off-by: David Nind <david@davidnind.com>
Created attachment 184406 [details] [review] Bug 40417: Regression test prove t/db_dependent/Koha/REST/Plugin/Objects.t Signed-off-by: David Nind <david@davidnind.com>
Created attachment 184407 [details] [review] Bug 40417: Remove 'validation' method We shouldn't use the ->validation method anymore as it isn't stable. We need to pass the params into the method accounting for any param changes made Test plan: 1) Apply the regression test patch and run the test 2) Test should fail 3) Apply this patch 4) Run test again 5) Test should pass Signed-off-by: David Nind <david@davidnind.com>
Created attachment 184409 [details] [review] Bug 40417: Regression test prove t/db_dependent/Koha/REST/Plugin/Objects.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 184410 [details] [review] Bug 40417: Remove 'validation' method We shouldn't use the ->validation method anymore as it isn't stable. We need to pass the params into the method accounting for any param changes made Test plan: 1) Apply the regression test patch and run the test 2) Test should fail 3) Apply this patch 4) Run test again 5) Test should pass Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
I PQA this. * Code is correct * Side effects from using ->validation covered by tests * QA script happy (with the linked PR applied locally for the QA test tools) Good catch Matthew and thanks for the patches!
Nice work everyone! Pushed to main for 25.11