Bug 19686 - Add to_model and to_api params to objects.search helper
Summary: Add to_model and to_api params to objects.search helper
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Julian Maurice
URL:
Keywords:
Depends on: 19410
Blocks: 19757 19250 20004
  Show dependency treegraph
 
Reported: 2017-11-23 19:58 UTC by Tomás Cohen Arazi
Modified: 2019-10-14 19:57 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19686: Unit tests (2.82 KB, patch)
2017-11-23 20:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19686: Add objects.search a 'to_model' param (2.91 KB, patch)
2017-11-23 20:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19686: Unit tests (2.88 KB, patch)
2017-12-01 21:55 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19686: Add objects.search a 'to_model' param (2.96 KB, patch)
2017-12-01 21:56 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19686: (followup) Add to_api param for completeness (5.31 KB, patch)
2018-01-17 17:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19686: Unit tests (2.93 KB, patch)
2018-02-16 11:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19686: Add objects.search a 'to_model' param (3.02 KB, patch)
2018-02-16 11:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19686: (followup) Add to_api param for completeness (5.36 KB, patch)
2018-02-16 11:53 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2017-11-23 19:58:00 UTC
Bug 19410 introduces a handy helper function to perform searches on Koha::Objects-based clases, plus several add-ons making use of other helpers.

But it is still missing the ability to solve field mappings between the exposed API and the DB structure.

We should add such a parameter.
Comment 1 Tomás Cohen Arazi 2017-11-23 20:19:26 UTC
Created attachment 69315 [details] [review]
Bug 19686: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2017-11-23 20:19:33 UTC
Created attachment 69316 [details] [review]
Bug 19686: Add objects.search a 'to_model' param

This patch introduces a new parameter to the objects.search Mojo helper.
The idea behind this, is that if there's any attribute name mapping
between the DB objects and the API exposed ones, we should be able to
pass it to objects.search so the filtering query params are mapped
correctly for building the DBIC query, like this example:

  my $patrons_set = Koha::Patrons->new;
  my @patrons = $c->objects->search( $patrons_set,
                                     \&to_model
                                    )->as_list;
  # and probably
  @patrons = map {to_api($_)} @patrons;

The to_model function needs to avoid autovivification, to prevent
messing with the query parameters (undef params).

To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass! Params get mapped!
- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 David Bourgault 2017-11-24 20:34:02 UTC
fatal: sha1 information is lacking or useless (t/db_dependent/Koha/REST/Plugin/Objects.t).
Comment 4 Tomás Cohen Arazi 2017-11-27 14:51:15 UTC
David, please apply the dependencies in this specific order:

- 19234
- 19369
- 19287
- 19370
- 19410

I've just tested it and it applies correctly.
Comment 5 David Bourgault 2017-12-01 21:50:39 UTC
So this fails for me.

I get failed test messages that look like this:

    #   Failed test 'exact match for JSON Pointer "/0/firstname"'
    #   at t/db_dependent/Koha/REST/Plugin/Objects.t line 168.
    #          got: 'Emmanuel'
    #     expected: 'Manuel'


But I also get this error at the beginning of the prove command:

t/db_dependent/Koha/REST/Plugin/Objects.t .. DBD::mysql::st execute failed: Cannot delete or update a parent row: a foreign key constraint fails (`koha_master_dev_inlibro`.`issues`, CONSTRAINT `issues_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON UPDATE CASCADE) [for Statement "DELETE FROM `borrowers`"] at /usr/local/share/perl/5.22.1/DBIx/Class/Storage/DBI.pm line 1836.
No method delete found for Koha::Patrons DBIx::Class::Storage::DBI::_dbh_execute(): Cannot delete or update a parent row: a foreign key constraint fails (`koha_master_dev_inlibro`.`issues`, CONSTRAINT `issues_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON UPDATE CASCADE) at /inlibro/git/koha-master-dev-inlibro/Koha/Objects.pm line 388
 at t/db_dependent/Koha/REST/Plugin/Objects.t line 76.

Is this a patch error or could it be a DB error?
Comment 6 David Bourgault 2017-12-01 21:55:44 UTC
Created attachment 69463 [details] [review]
Bug 19686: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 7 David Bourgault 2017-12-01 21:56:21 UTC
Created attachment 69464 [details] [review]
Bug 19686: Add objects.search a 'to_model' param

This patch introduces a new parameter to the objects.search Mojo helper.
The idea behind this, is that if there's any attribute name mapping
between the DB objects and the API exposed ones, we should be able to
pass it to objects.search so the filtering query params are mapped
correctly for building the DBIC query, like this example:

  my $patrons_set = Koha::Patrons->new;
  my @patrons = $c->objects->search( $patrons_set,
                                     \&to_model
                                    )->as_list;
  # and probably
  @patrons = map {to_api($_)} @patrons;

The to_model function needs to avoid autovivification, to prevent
messing with the query parameters (undef params).

To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass! Params get mapped!
- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 8 David Bourgault 2017-12-01 21:58:18 UTC
My bad. I tested again with a fresh BD and it worked.

Test passed, QA tool green.
Comment 9 Tomás Cohen Arazi 2018-01-17 17:35:00 UTC
Created attachment 70626 [details] [review]
Bug 19686: (followup) Add to_api param for completeness

This patch adds (yet) another param to objects.search: a reference
to a to_api function to be applied when processing the search results
for output.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Test count raised, and tests pass!
- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Kyle M Hall 2018-02-16 11:53:43 UTC
Created attachment 71742 [details] [review]
Bug 19686: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Kyle M Hall 2018-02-16 11:53:51 UTC
Created attachment 71743 [details] [review]
Bug 19686: Add objects.search a 'to_model' param

This patch introduces a new parameter to the objects.search Mojo helper.
The idea behind this, is that if there's any attribute name mapping
between the DB objects and the API exposed ones, we should be able to
pass it to objects.search so the filtering query params are mapped
correctly for building the DBIC query, like this example:

  my $patrons_set = Koha::Patrons->new;
  my @patrons = $c->objects->search( $patrons_set,
                                     \&to_model
                                    )->as_list;
  # and probably
  @patrons = map {to_api($_)} @patrons;

The to_model function needs to avoid autovivification, to prevent
messing with the query parameters (undef params).

To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass! Params get mapped!
- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Kyle M Hall 2018-02-16 11:53:54 UTC
Created attachment 71744 [details] [review]
Bug 19686: (followup) Add to_api param for completeness

This patch adds (yet) another param to objects.search: a reference
to a to_api function to be applied when processing the search results
for output.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Test count raised, and tests pass!
- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Jonathan Druart 2018-02-16 17:06:43 UTC
Pushed to master for 18.05, thanks to everybody involved!