Description
Matt Blenkinsop
2025-07-16 16:10:07 UTC
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 |