Bug 29746

Created attachment 128830 [details] [review]
Bug 29746: Unit tests

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

Created attachment 128831 [details] [review]
Bug 29746: Add Koha::Boolean

This patch introduces a new OO class that can be used as return value
from methods that need to return boolean values, but also provide some
feedback. This last bit is implemented using Koha::Object::Message
objects that can carry valuable information.

This class can also implement a `to_api()` method so it is suitable for
API usage. And so the Koha::Object::Message class. Will be done as
needed.

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/Koha/Boolean.t
=> SUCCESS: Tests pass!
3. Sign off :-D

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

Created attachment 128834 [details] [review]
Bug 29746: Unit tests

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 128835 [details] [review]
Bug 29746: Add Koha::Boolean

This patch introduces a new OO class that can be used as return value
from methods that need to return boolean values, but also provide some
feedback. This last bit is implemented using Koha::Object::Message
objects that can carry valuable information.

This class can also implement a `to_api()` method so it is suitable for
API usage. And so the Koha::Object::Message class. Will be done as
needed.

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/Koha/Boolean.t
=> SUCCESS: Tests pass!
3. Sign off :-D

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 128882 [details] [review]
Bug 29746: Unit tests

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

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 128883 [details] [review]
Bug 29746: Add Koha::Result::Boolean

This patch introduces a new OO class that can be used as return value
from methods that need to return boolean values, but also provide some
feedback. This last bit is implemented using Koha::Object::Message
objects that can carry valuable information.

This class can also implement a `to_api()` method so it is suitable for
API usage. And so the Koha::Object::Message class. Will be done as
needed.

If some other result types are required, then we can move some of the
messaging logic to a top-level Koha::Result class this one inherits from
(and the new one as well, say, Integer?).

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/Koha/Result/Boolean.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>

I've just renamed the class because this is not an attempt to introduce booleans in Perl :-D but a way to have a consistent way to return values and have them evaluated in the core codebase.

I will pick a real-life example to highlight how it can be useful: CanItemBeReserved.

This method is called like this:

C4/Circulation.pm:
@items = grep { CanItemBeReserved( ... )->{status} eq 'OK' } @items;

In this case, the reason why it is not holdable doesn't really care. We are comparing to an arbitrary string that represents a boolean value (to be fair, OK is the obvious pick :-D, but the ->{status} bit could vary from one implementation to another).

If it used this library as a return value, we would only evaluate the result in a boolean context:

@items = grep { CanItemBeReserved( ... ) } @items;

And if we wanted to know 'why', we could do:

my $result_object = CanItemBeReserved( ... );

and then have a consistent way for this method and all other methods that need to return extra info:

if ( !$result_object ) {

    foreach my $message ( @{$result_object->messages} ) {
        $template->param( do_your_thingy );
    }

}

Please feel free to comment about this and add your thoughts. As this return value has its own class instead of what we did before, we could do things like:
- implement a to_api() method that makes it render in a suitable way, have it embed the messages, etc. I can see how it can be useful.
- some other caller contexts could be considered. for instance, some methods we have return, in list context, a boolean (the result) and a payload with extra info. We could make this class do the same, for an easy transition of legacy code.

I went ahead and used this in bug 29765.

+use overload bool => \&as_bool;

I didn't notice that the first time I read this patch, and it's awesome!

I will provide you a QA feedback soon.

(In reply to Jonathan Druart from comment #9)
> +use overload bool => \&as_bool;
> 
> I didn't notice that the first time I read this patch, and it's awesome!
> 
> I will provide you a QA feedback soon.

And, to explain a bit more:

"""
use Koha::Result::Boolean;
sub ok {
    return Koha::Result::Boolean->new(1)->add_message({message => "this is not an error"});
}
sub nok {
    return Koha::Result::Boolean->new(0)->add_message({message => "this is an error"});
}

if ( ok() ) { 
    say "all good";
}
unless ( nok() ) { 
    say "there was an error";
}
"""

=> will display "all good" and "there was an error"

I don't think "Koha::Result::Boolean" is good. What is then a Koha::Result?

(In reply to Jonathan Druart from comment #9)
> +use overload bool => \&as_bool;
> 
> I didn't notice that the first time I read this patch, and it's awesome!
> 
> I will provide you a QA feedback soon.

Thank you! That was my feeling when I noticed overload existed as well :-D

(In reply to Jonathan Druart from comment #11)
> I don't think "Koha::Result::Boolean" is good. What is then a Koha::Result?

Right, I was not sure about the name. Maybe I'm going too far thinking we could need another result types. I'd go with whatever to be honest. Koha::Result sounds correct as well.

"Return value" sounds better to me.

(In reply to Jonathan Druart from comment #11)
> I don't think "Koha::Result::Boolean" is good. What is then a Koha::Result?

I thought of writing Koha::Result as a generic class with the messages bit, and then Koha::Result::Boolean, Koha::Result::Integer, inherit from it.

Are you willing to submit a follow-up renaming it to Koha::Result or similar?

Created attachment 128997 [details] [review]
Bug 29746: Unit tests

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 128998 [details] [review]
Bug 29746: Add Koha::Result::Boolean

This patch introduces a new OO class that can be used as return value
from methods that need to return boolean values, but also provide some
feedback. This last bit is implemented using Koha::Object::Message
objects that can carry valuable information.

This class can also implement a `to_api()` method so it is suitable for
API usage. And so the Koha::Object::Message class. Will be done as
needed.

If some other result types are required, then we can move some of the
messaging logic to a top-level Koha::Result class this one inherits from
(and the new one as well, say, Integer?).

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/Koha/Result/Boolean.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 129007 [details] [review]
Bug 29746: (it-doesn't-hurt follow-up) More tests

In an attempt to add (even) more tests for this library, we wanted to
add tests for the return values initialization and then hit
a wall when trying to add tests: Test::More tries to compare in string
context first [1], and when you force integer context (by using cmp_ok +
'==') it tells '==' is not overridden for the class.

So this patch adds those tests, and also the overloaded '==' operator
that is required for such tests.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/Koha/Result/Boolean.t
=> SUCCESS: Tests pass
3. Sign off :-D

[1] https://metacpan.org/pod/Test::More#Overloaded-objects

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

Pushed to master for 22.05, thanks to everybody involved ﷐[U+1F984]﷑