Bug 29746

Summary: Add a handy Koha::Result::Boolean class
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: RESOLVED FIXED QA Contact: Jonathan Druart <jonathan.druart+koha>
Severity: enhancement    
Priority: P5 - low CC: dcook, jonathan.druart+koha, katrin.fischer, kyle, m.de.rooy, martin.renvoize, nick, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Text to go in the release notes:
This development introduces a new library, Koha::Result::Boolean, that will be used in method that need to return a boolean, but could also want to carry some more information such as the reason for a 'false' return value. A Koha::Result::Boolean object will be evaluated as a boolean in bool context, while retaining its object nature and methods. For example, suppose a $patron object should not be deleted because the patron it represents has a guarantee: ``` if ( $patron->safe_to_delete ) { ... } ``` will eval to false, and the code will do what we expect. If our code really wanted to know *why* it cannot be deleted, we can do: ``` my $result = $patron->safe_to_delete; unless ( $result ) { Koha::Exceptions->throw( "Cannot delete, errors: " . join( ', ', map {$_->message} @{$result->messages} ) ); } ```
Version(s) released in:
22.05.00
Bug Depends on:    
Bug Blocks: 29765, 29788    
Attachments: Bug 29746: Unit tests
Bug 29746: Add Koha::Boolean
Bug 29746: Unit tests
Bug 29746: Add Koha::Boolean
Bug 29746: Unit tests
Bug 29746: Add Koha::Result::Boolean
Bug 29746: Unit tests
Bug 29746: Add Koha::Result::Boolean
Bug 29746: (it-doesn't-hurt follow-up) More tests

Description Tomás Cohen Arazi 2021-12-21 12:42:43 UTC
The idea is to provide an OO class that can carry a boolean value, and also useful messages for feedback.

An example usage:

sub safe_to_delete {
    my ( $self ) = @_;

    my $result = Koha::Boolean->new;

    my $can = 1;

    if ( condition_1 ) {
        $can = 0;
	$result->add_message({ message => 'has_debt' });
    }
    ...
    if ( condition_n ) {
        $can = 0;
        $result->add_message({ message => 'has_guarantees' });
    }

    return $result->set_value($can);
}
Comment 1 Tomás Cohen Arazi 2021-12-21 12:49:05 UTC
Created attachment 128830 [details] [review]
Bug 29746: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2021-12-21 12:49:09 UTC
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>
Comment 3 David Nind 2021-12-21 13:53:43 UTC
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>
Comment 4 David Nind 2021-12-21 13:53:51 UTC
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>
Comment 5 Tomás Cohen Arazi 2021-12-23 12:21:23 UTC
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>
Comment 6 Tomás Cohen Arazi 2021-12-23 12:21:30 UTC
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>
Comment 7 Tomás Cohen Arazi 2021-12-23 12:45:35 UTC
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.
Comment 8 Tomás Cohen Arazi 2021-12-23 14:56:16 UTC
I went ahead and used this in bug 29765.
Comment 9 Jonathan Druart 2022-01-03 14:38:37 UTC
+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.
Comment 10 Jonathan Druart 2022-01-03 14:42:08 UTC
(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"
Comment 11 Jonathan Druart 2022-01-03 14:46:29 UTC
I don't think "Koha::Result::Boolean" is good. What is then a Koha::Result?
Comment 12 Tomás Cohen Arazi 2022-01-03 14:48:43 UTC
(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.
Comment 13 Jonathan Druart 2022-01-03 15:52:48 UTC
"Return value" sounds better to me.
Comment 14 Tomás Cohen Arazi 2022-01-03 18:38:09 UTC
(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?
Comment 15 Jonathan Druart 2022-01-04 15:20:13 UTC
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>
Comment 16 Jonathan Druart 2022-01-04 15:20:19 UTC
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>
Comment 17 Tomás Cohen Arazi 2022-01-04 19:22:55 UTC
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>
Comment 18 Fridolin Somers 2022-01-05 23:35:25 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄