[Bug 29746] New: Add a handy Koha::Boolean class
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Bug ID: 29746 Summary: Add a handy Koha::Boolean class Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org 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); } -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Status|NEW |Needs Signoff CC| |jonathan.druart+koha@gmail. | |com, | |kyle@bywatersolutions.com, | |m.de.rooy@rijksmuseum.nl, | |martin.renvoize@ptfs-europe | |.com, | |nick@bywatersolutions.com -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 128830 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128830&action=edit Bug 29746: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 128831 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128831&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 David Nind <david@davidnind.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=29746 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128830|0 |1 is obsolete| | --- Comment #3 from David Nind <david@davidnind.com> --- Created attachment 128834 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128834&action=edit Bug 29746: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128831|0 |1 is obsolete| | --- Comment #4 from David Nind <david@davidnind.com> --- Created attachment 128835 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128835&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128834|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128835|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 128882 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128882&action=edit Bug 29746: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 --- Comment #6 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 128883 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128883&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Add a handy Koha::Boolean |Add a handy |class |Koha::Result::Boolean class -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #7 from Tomás Cohen Arazi <tomascohen@gmail.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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |29765 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29765 [Bug 29765] Make Koha::Patron->safe_to_delete return a Koha::Result::Boolean object -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 --- Comment #8 from Tomás Cohen Arazi <tomascohen@gmail.com> --- I went ahead and used this in bug 29765. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |jonathan.druart+koha@gmail. |y.org |com --- Comment #9 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- +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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 --- Comment #10 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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" -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 --- Comment #11 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- I don't think "Koha::Result::Boolean" is good. What is then a Koha::Result? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 --- Comment #13 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- "Return value" sounds better to me. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |29788 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29788 [Bug 29788] Make Koha::Item->safe_to_delete return a Koha::Result::Boolean object -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128882|0 |1 is obsolete| | --- Comment #15 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 128997 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128997&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128883|0 |1 is obsolete| | --- Comment #16 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 128998 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128998&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 --- Comment #17 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 129007 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=129007&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |22.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 --- Comment #18 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to master for 22.05, thanks to everybody involved 🦄 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |release-notes-needed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|release-notes-needed | Text to go in the| |This development introduces release notes| |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} ) ); | | | | } | |``` -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to master |RESOLVED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |31873 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31873 [Bug 31873] Can't call method "safe_delete" on an undefined value at cataloguing/additem.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29746 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |40101 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40101 [Bug 40101] Add `Koha::Patron->can_place_holds` -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org