Quantcast

[Bug 17932] New: Koha::Object should provide a TO_JSON method

classic Classic list List threaded Threaded
35 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] New: Koha::Object should provide a TO_JSON method

bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

            Bug ID: 17932
           Summary: Koha::Object should provide a TO_JSON method
 Change sponsored?: ---
           Product: Koha
           Version: master
          Hardware: All
                OS: All
            Status: NEW
          Severity: enhancement
          Priority: P5 - low
         Component: Architecture, internals, and plumbing
          Assignee: [hidden email]
          Reporter: [hidden email]
        QA Contact: [hidden email]

The Koha::Object class should provide a TO_JSON method that prepares data for
JSON output. Right now, booleans are returned as integer values, and thus
invalid for JSON output as boolean values.

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

--- Comment #1 from Tomás Cohen Arazi <[hidden email]> ---
*** Bug 17926 has been marked as a duplicate of this bug. ***

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

Tomás Cohen Arazi <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|[hidden email]          |[hidden email]

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

Tomás Cohen Arazi <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]-c
                   |                            |ommunity.org,
                   |                            |[hidden email],
                   |                            |martin.renvoize@ptfs-europe
                   |                            |.com

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

Tomás Cohen Arazi <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |17927


Referenced Bugs:

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17927
[Bug 17927] REST API: /holds and /patrons attributes have wrong types
--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

--- Comment #2 from Tomás Cohen Arazi <[hidden email]> ---
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/Helper/Row/NumifyGet.pm
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

--- Comment #3 from Tomás Cohen Arazi <[hidden email]> ---
Created attachment 59432
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59432&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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

Tomás Cohen Arazi <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |Needs Signoff

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

Tomás Cohen Arazi <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #59431|0                           |1
        is obsolete|                            |

--- Comment #4 from Tomás Cohen Arazi <[hidden email]> ---
Created attachment 59498
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59498&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/Helper/Row/NumifyGet.pm
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

Tomás Cohen Arazi <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #59432|0                           |1
        is obsolete|                            |

--- Comment #5 from Tomás Cohen Arazi <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

--- Comment #6 from Tomás Cohen Arazi <[hidden email]> ---
Created attachment 59500
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59500&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

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

--- Comment #7 from Tomás Cohen Arazi <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

--- Comment #8 from Martin Renvoize <[hidden email]> ---
Looks good to me :)

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

--- Comment #9 from Jonathan Druart <[hidden email]> ---
(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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

Tomás Cohen Arazi <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |17992


Referenced Bugs:

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17992
[Bug 17992] REST api: Cities controller should not use ->unblessed
--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

Marcel de Rooy <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

Josef Moravec <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

Nick Clemens <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Needs Signoff               |Signed Off

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

Nick Clemens <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #59498|0                           |1
        is obsolete|                            |

--- Comment #10 from Nick Clemens <[hidden email]> ---
Created attachment 59696
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59696&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/Helper/Row/NumifyGet.pm
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 <[hidden email]>

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

Nick Clemens <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #59499|0                           |1
        is obsolete|                            |

--- Comment #11 from Nick Clemens <[hidden email]> ---
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 <[hidden email]>

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

Nick Clemens <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #59500|0                           |1
        is obsolete|                            |

--- Comment #12 from Nick Clemens <[hidden email]> ---
Created attachment 59698
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59698&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 <[hidden email]>

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

Nick Clemens <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #59501|0                           |1
        is obsolete|                            |

--- Comment #13 from Nick Clemens <[hidden email]> ---
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 <[hidden email]>

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

--- Comment #14 from Marcel de Rooy <[hidden email]> ---
Thanks, Nick. You beat me in signing off :)

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

--- Comment #15 from Tomás Cohen Arazi <[hidden email]> ---
(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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

--- Comment #16 from Marcel de Rooy <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

--- Comment #17 from Tomás Cohen Arazi <[hidden email]> ---
(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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

--- Comment #18 from Marcel de Rooy <[hidden email]> ---
(In reply to Tomás Cohen Arazi from comment #17)
> 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

Good argument.

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

--- Comment #19 from Marcel de Rooy <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

Marcel de Rooy <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         QA Contact|[hidden email]-communit |[hidden email]
                   |y.org                       |

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Bug 17932] Koha::Object should provide a TO_JSON method

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17932

Marcel de Rooy <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #59696|0                           |1
        is obsolete|                            |

--- Comment #20 from Marcel de Rooy <[hidden email]> ---
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/Helper/Row/NumifyGet.pm
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 <[hidden email]>
Signed-off-by: Marcel de Rooy <[hidden email]>

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
12
Loading...