https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- *** Bug 17926 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org, | |kyle.m.hall@gmail.com, | |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=17932 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 59431 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59431&action=edit Bug 17932: Add a TO_JSON method to Koha::Object(s) Bug 17927 fixes data types on the current REST endpoints. If you test those endpoints, you will notice trying to access them (for listing or retrieving single objects yields a data type error. Notably on booleans but also on integers. Integers fail due to https://rt.cpan.org/Ticket/Display.html?id=119904 but it needs some global solution until there's a DBD::mysql release backported to the supported distros. There's the option to use http://search.cpan.org/~frew/DBIx-Class-Helpers-2.033002/lib/DBIx/Class/Help... to get the integer columns fixed as a workaround: __PACKAGE__->add_columns( borrowernumber => { data_type => 'integer', is_nullable => 0, is_numeric => 1, } ); I didn't find bug reports related to this (maybe because we don't use warnings everywhere) But I don't think is worth going such a heavy overhead. A similar situation takes place for Boolean values. They need to be prepared for JSON output. This could have been done using DBIx filters as pointed out by Martin: __PACKAGE__->filter_column( lost => { filter_to_storage => sub { $_[1] ? 1 : 0 }, filter_from_storage => sub { $_[1] ? Mojo::JSON->true : Mojo::JSON->false } } ); but this could have other consequences that are worth exploring on another bug (i.e. it would mean we need to take care of every place where this boolean data is used/set needs to handle this data types nicely. Such would be the case if we were a Mojo-only app, but we aren't. We use Koha::Obect(s) in the whole app. Period. This patch adds the need to specify on the schema files, columns that are actually boolean, because we have no way to detect them for now (i.e. they are all tinyint, but we use tinyint for non-boolean stuff too). So if this patch is accepted, we would need to specify boolean columns like this: __PACKAGE__->add_columns( '+lost' => { is_boolean => 1 } ); This patch adds a TO_JSON method for Koha::Object(s) to be used for serializing Koha::Object-derived objects into JSON strings. To test it (as Koha::Object(s) need to be instantiated) I provide tests on top of the Koha::Patron(s) classes in the followup patches. [1] Yes, we use TINYINT(1) for booleans, but from DBIC's perspective there's no way to read the (1) in runtime. Sponsored-by: ByWater Solutions -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59432|0 |1 is obsolete| | --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 59499 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59499&action=edit Bug 17932: (followup) Fix /patrons endpoint Bug 17927 introduced data type fixes on the /patrons endpoint (integer and boolean types got fixed). This led to the /patrons endpoint not to work because the underlying code didn't provide the right data. With the introduction of TO_JSON on Koha::Object(s) we now have a way to output the proper data types. This patch does so by: - Adding is_boolean => 1 to the relevant columns on the Borrower.pm schema file. - Tweaking the controller class for the /patrons endpoint so it doesn't use the $object(s)->unblessed call but just let the Mojo::JSON library pick out TO_JSON implementation instead on rendering the output. - It adds a new test for booleans. To test: - Have 17927 applied - Run: $ prove t/db_dependent/api/v1/patrons.t => FAIL: Tests fail [1] - Apply this patches - Run: $ prove t/db_dependent/api/v1/patrons.t => SUCCESS: Tests pass! - Sign off! :-D [1] It is self explanatory to just try the API using any of the available tools (I use HttpRequester on Firefox) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 --- Comment #7 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 59501 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59501&action=edit Bug 17932: (followup) Tidy tests This patch makes the tests create its own data instead of searching the DB for a branchcode and a categorycode. It does so on each subtest, because there shouldn't be side effects between subtests. I also wrapped each subtest inside a transaction, for the same reason. To test: - Apply this patch - Run: $ prove t/db_dependent/Koha/Object.t => SUCCESS: Tests return green with this patch - 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=17932 --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Martin Renvoize from comment #8)

Looks good to me :)

Is it a signoff? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59499|0 |1 is obsolete| | --- Comment #11 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 59697 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59697&action=edit Bug 17932: (followup) Fix /patrons endpoint Bug 17927 introduced data type fixes on the /patrons endpoint (integer and boolean types got fixed). This led to the /patrons endpoint not to work because the underlying code didn't provide the right data. With the introduction of TO_JSON on Koha::Object(s) we now have a way to output the proper data types. This patch does so by: - Adding is_boolean => 1 to the relevant columns on the Borrower.pm schema file. - Tweaking the controller class for the /patrons endpoint so it doesn't use the $object(s)->unblessed call but just let the Mojo::JSON library pick out TO_JSON implementation instead on rendering the output. - It adds a new test for booleans. To test: - Have 17927 applied - Run: $ prove t/db_dependent/api/v1/patrons.t => FAIL: Tests fail [1] - Apply this patches - Run: $ prove t/db_dependent/api/v1/patrons.t => SUCCESS: Tests pass! - Sign off! :-D [1] It is self explanatory to just try the API using any of the available tools (I use HttpRequester on Firefox) Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59501|0 |1 is obsolete| | --- Comment #13 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 59699 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59699&action=edit Bug 17932: (followup) Tidy tests This patch makes the tests create its own data instead of searching the DB for a branchcode and a categorycode. It does so on each subtest, because there shouldn't be side effects between subtests. I also wrapped each subtest inside a transaction, for the same reason. To test: - Apply this patch - Run: $ prove t/db_dependent/Koha/Object.t => SUCCESS: Tests return green with this patch - Sign off :-D Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Marcel de Rooy from comment #14)

Thanks, Nick. You beat me in signing off :)

You can QA it ;-) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 --- Comment #17 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Marcel de Rooy from comment #16)

(In reply to Tomás Cohen Arazi from comment #15)

...

(In reply to Marcel de Rooy from comment #14)

...

Thanks, Nick. You beat me in signing off :)

You can QA it ;-)

Good plan. Just a dumb question btw: why do you use uppercase TO_JSON? Is it that important :) Note that I see everywhere get_whatever etc.

I personally dislike ->get_whatever, and prefer ->whatever in most cases (->as_json would be acceptable, because it is clear we want the object in a specific serialization format). Anyway, this time we are just sticking to what JSON libraries expect from blessed objects to accept them for rendering as JSON strings. See http://search.cpan.org/~makamaka/JSON-2.90/lib/JSON.pm#allow_blessed -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- What about this output for /api/v1/patrons/xxx after the second patch: {"errors":[{"message":"([0] Expected string - got boolean. [1] Not null.)","path":"\/gonenoaddress"},{"message":"([0] Expected string - got number. [1] Not null.)","path":"\/flags"},{"message":"Expected string - got number.","path":"\/privacy"},{"message":"Expected string - got number.","path":"\/borrowernumber"},{"path":"\/guarantorid","message":"([0] Expected string - got number. [1] Not null.)"},{"message":"([0] Expected string - got number. [1] Not null.)","path":"\/sms_provider_id"},{"path":"\/privacy_guarantor_checkouts","message":"Expected string - got number."},{"path":"\/lost","message":"([0] Expected string - got boolean. [1] Not null.)"}]} -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59696|0 |1 is obsolete| | --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 60072 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60072&action=edit Bug 17932: Add a TO_JSON method to Koha::Object(s) Bug 17927 fixes data types on the current REST endpoints. If you test those endpoints, you will notice trying to access them (for listing or retrieving single objects yields a data type error. Notably on booleans but also on integers. Integers fail due to https://rt.cpan.org/Ticket/Display.html?id=119904 but it needs some global solution until there's a DBD::mysql release backported to the supported distros. There's the option to use http://search.cpan.org/~frew/DBIx-Class-Helpers-2.033002/lib/DBIx/Class/Help... to get the integer columns fixed as a workaround: __PACKAGE__->add_columns( borrowernumber => { data_type => 'integer', is_nullable => 0, is_numeric => 1, } ); I didn't find bug reports related to this (maybe because we don't use warnings everywhere) But I don't think is worth going such a heavy overhead. A similar situation takes place for Boolean values. They need to be prepared for JSON output. This could have been done using DBIx filters as pointed out by Martin: __PACKAGE__->filter_column( lost => { filter_to_storage => sub { $_[1] ? 1 : 0 }, filter_from_storage => sub { $_[1] ? Mojo::JSON->true : Mojo::JSON->false } } ); but this could have other consequences that are worth exploring on another bug (i.e. it would mean we need to take care of every place where this boolean data is used/set needs to handle this data types nicely. Such would be the case if we were a Mojo-only app, but we aren't. We use Koha::Obect(s) in the whole app. Period. This patch adds the need to specify on the schema files, columns that are actually boolean, because we have no way to detect them for now (i.e. they are all tinyint, but we use tinyint for non-boolean stuff too). So if this patch is accepted, we would need to specify boolean columns like this: __PACKAGE__->add_columns( '+lost' => { is_boolean => 1 } ); This patch adds a TO_JSON method for Koha::Object(s) to be used for serializing Koha::Object-derived objects into JSON strings. To test it (as Koha::Object(s) need to be instantiated) I provide tests on top of the Koha::Patron(s) classes in the followup patches. [1] Yes, we use TINYINT(1) for booleans, but from DBIC's perspective there's no way to read the (1) in runtime. Sponsored-by: ByWater Solutions Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59698|0 |1 is obsolete| | --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 60074 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60074&action=edit Bug 17932: Unit tests This patch adds unit tests for the Koha::Object::TO_JSON function. It tests on top of Koha::Patron as Koha::Object needs to be instantiated. To test: - Apply the patch - Run: $ prove -v t/db_dependent/Koha/Object.t => SUCCESS: New tests for TO_JSON are run and return green. - Sign off :-D Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #24 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Looks good to me. One small thing: the isvstring test looks somewhat useless to me. It also passes on other borrower text columns like phonepro or B_streetnumber. Could be handled on a follow-up. Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED CC| |katrin.fischer@bsz-bw.de Resolution|--- |FIXED --- Comment #26 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- This won't get ported back to 16.11.x as it is an enhancement. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 --- Comment #27 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Followup Bug 18290 fixes tests to cover a change in Mojolicious -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |18326 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18326 [Bug 18326] Add Mojolicious as a required dependency -- You are receiving this mail because: You are watching all bug changes.