Attachment 63433 Details for Bug 17499 – Bug 17499: Improve object usability by enabling direct access to transport preferences

[patch] Bug 17499: Improve object usability by enabling direct access to transport preferences

Bug-17499-Improve-object-usability-by-enabling-dir.patch (text/plain), 16.89 KB, created by Lari Taskula on 2017-05-12 13:27:00 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Lari Taskula

Created: 2017-05-12 13:27:00 UTC

Size: 16.89 KB

patch

obsolete

>From 2e74f0bbafbacea8ec92e584a4e9036086dc65e5 Mon Sep 17 00:00:00 2001
>From: Lari Taskula <lari.taskula@jns.fi>
>Date: Wed, 26 Oct 2016 18:22:50 +0300
>Subject: [PATCH] Bug 17499: Improve object usability by enabling direct access
> to transport preferences
>
>Since messaging preference is a feature that has multiple related database tables,
>usage via Koha-objects is sometimes frustrating.
>
>This patch adds a feature for Koha::Patron::Message::Preference which enables
>access to message transport preferences directly via this object. It allows us
>to skip calls to Koha::Patron::Message::Transport::Preference(s) because we can
>now get and set via K::P::M::Preference.
>
>Get:
>$preference->message_transport_types
>Returns a hashref, where each key is stored transport type and value for the key
>is letter code for the transport.
>
>Set:
>$preference->set({ message_transport_types => ['sms'] }) or
>$preference->message_transport_types('email', 'sms') or
>$preference->message_transport_types(['email', 'sms'])
>Returns $self (Koha::Patron::Message::Preference object)
>
>To test:
>1. Run t/db_dependent/Koha/Patron/Message/Preferences.t
>---
> Koha/Patron/Message/Preference.pm                | 149 ++++++++++++++++++++++-
> t/db_dependent/Koha/Patron/Message/Preferences.t | 132 +++++++++++++++++++-
> 2 files changed, 273 insertions(+), 8 deletions(-)
>
>diff --git a/Koha/Patron/Message/Preference.pm b/Koha/Patron/Message/Preference.pm
>index 8fa1910..4ca3fe5 100644
>--- a/Koha/Patron/Message/Preference.pm
>+++ b/Koha/Patron/Message/Preference.pm
>@@ -22,8 +22,11 @@ use Modern::Perl;
> use Koha::Database;
> use Koha::Exceptions;
> use Koha::Patron::Categories;
>+use Koha::Patron::Message::Attributes;
> use Koha::Patron::Message::Preferences;
> use Koha::Patron::Message::Transport::Preferences;
>+use Koha::Patron::Message::Transport::Types;
>+use Koha::Patron::Message::Transports;
> use Koha::Patrons;
> 
> use base qw(Koha::Object);
>@@ -44,6 +47,7 @@ my $preference = Koha::Patron::Message::Preference->new({
>    borrowernumber => 123,
>    #categorycode => 'ABC',
>    message_attribute_id => 4,
>+   message_transport_types => ['email', 'sms'], # see documentation below
>    wants_digest => 1,
>    days_in_advance => 7,
> });
>@@ -54,12 +58,22 @@ either borrowernumber or categorycode, but not both.
> Throws Koha::Exceptions::DuplicateObject if trying to create a preference that
> already exists for the same message_attribute_id and borrowernumber/categorycode.
> 
>+C<message_transport_types> is a parameter that is not actually a column in this
>+Koha-object. Given this parameter, the message transport types will be added as
>+related transport types for this object. For get and set, you can access them via
>+subroutine C<message_transport_types()> in this class.
>+
> =cut
> 
> sub new {
>-    my ($class, $params) = shift;
>+    my ($class, $params) = @_;
>+
>+    my $types = $params->{'message_transport_types'};
>+    delete $params->{'message_transport_types'};
> 
>-    my $self = $class->SUPER::new(@_);
>+    my $self = $class->SUPER::new($params);
>+
>+    $self->_set_message_transport_types($types);
> 
>     my $previous = Koha::Patron::Message::Preferences->search({
>         '-and' => [
>@@ -140,6 +154,64 @@ sub new_from_default {
>     return $self;
> }
> 
>+=head3 message_transport_types
>+
>+$preference->message_transport_types
>+Returns a HASHREF of message transport types for this messaging preference, e.g.
>+if ($preference->message_transport_types->{'email'}) {
>+    # email is one of the transport preferences
>+}
>+
>+$preference->message_transport_types('email', 'sms');
>+Sets the given message transport types for this messaging preference
>+
>+=cut
>+
>+sub message_transport_types {
>+    my $self = shift;
>+
>+    unless (@_) {
>+        if ($self->{'_message_transport_types'}) {
>+            return $self->{'_message_transport_types'};
>+        }
>+        map { $self->{'_message_transport_types'}->{$_->message_transport_type} =
>+                Koha::Patron::Message::Transports->find({
>+                    message_attribute_id => $self->message_attribute_id,
>+                    message_transport_type => $_->message_transport_type,
>+                    is_digest => $self->wants_digest})->letter_code }
>+        Koha::Patron::Message::Transport::Preferences->search({
>+            borrower_message_preference_id => $self->borrower_message_preference_id,
>+        })->as_list;
>+        return $self->{'_message_transport_types'} || {};
>+    }
>+    else {
>+        $self->_set_message_transport_types(@_);
>+        return $self;
>+    }
>+}
>+
>+=head3 set
>+
>+$preference->set({
>+    message_transport_types => ['sms', 'phone'],
>+    wants_digest => 0,
>+})->store;
>+
>+Sets preference object values and additionally message_transport_types if given.
>+
>+=cut
>+
>+sub set {
>+    my ($self, $params) = @_;
>+
>+    if ($params && $params->{'message_transport_types'}) {
>+        $self->message_transport_types($params->{'message_transport_types'});
>+        delete $params->{'message_transport_types'};
>+    }
>+
>+    return $self->SUPER::set($params) if $params;
>+}
>+
> =head3 store
> 
> Makes a validation before actual Koha::Object->store so that proper exceptions
>@@ -152,9 +224,22 @@ Deletes any previous messaging preferences to avoid duplication.
> sub store {
>     my $self = shift;
> 
>-    $self->validate;
>+    $self->validate->SUPER::store(@_);
>+
>+    # store message transport types
>+    if (exists $self->{'_message_transport_types'}) {
>+        Koha::Patron::Message::Transport::Preferences->search({
>+            borrower_message_preference_id => $self->borrower_message_preference_id,
>+        })->delete;
>+        foreach my $type (keys %{$self->{'_message_transport_types'}}) {
>+            Koha::Patron::Message::Transport::Preference->new({
>+                borrower_message_preference_id => $self->borrower_message_preference_id,
>+                message_transport_type => $type,
>+            })->store;
>+        }
>+    }
> 
>-    return $self->SUPER::store(@_);
>+    return $self;
> }
> 
> =head3 validate
>@@ -203,6 +288,62 @@ sub validate {
>     return $self;
> }
> 
>+sub _set_message_transport_types {
>+    my $self = shift;
>+
>+    return unless $_[0];
>+
>+    $self->{'_message_transport_types'} = undef;
>+    my $types = ref $_[0] eq "ARRAY" ? $_[0] : [@_];
>+    return unless $types;
>+    $self->_validate_message_transport_types({ message_transport_types => $types });
>+    foreach my $type (@$types) {
>+        unless (exists $self->{'_message_transport_types'}->{$type}) {
>+            $self->{'_message_transport_types'}->{$type} =
>+            Koha::Patron::Message::Transports->find({
>+                message_attribute_id => $self->message_attribute_id,
>+                message_transport_type => $type,
>+                is_digest => $self->wants_digest})->letter_code;
>+        }
>+    }
>+    return $self;
>+}
>+
>+sub _validate_message_transport_types {
>+    my ($self, $params) = @_;
>+
>+    if (ref($params) eq 'HASH' && $params->{'message_transport_types'}) {
>+        if (ref($params->{'message_transport_types'}) ne 'ARRAY') {
>+            $params->{'message_transport_types'} = [$params->{'message_transport_types'}];
>+        }
>+        my $types = $params->{'message_transport_types'};
>+
>+        foreach my $type (@{$types}) {
>+            unless (Koha::Patron::Message::Transport::Types->find({
>+                message_transport_type => $type
>+            })) {
>+                Koha::Exceptions::BadParameter->throw(
>+                    error => "Message transport type '$type' does not exist",
>+                    parameter => 'message_transport_type',
>+                );
>+            }
>+            my $tmp = (ref($self) eq __PACKAGE__) ? $self->unblessed : $params;
>+            unless (Koha::Patron::Message::Transports->find({
>+                    message_transport_type => $type,
>+                    message_attribute_id => $tmp->{'message_attribute_id'},
>+                    is_digest => $tmp->{'wants_digest'},
>+            })) {
>+                Koha::Exceptions::BadParameter->throw(
>+                    error => "Message transport option for '$type' (".
>+                    ($tmp->{'wants_digest'} ? 'digest':'no digest').") does not exist",
>+                    parameter => 'message_transport_type',
>+                );
>+            }
>+        }
>+        return $types;
>+    }
>+}
>+
> =head3 type
> 
> =cut
>diff --git a/t/db_dependent/Koha/Patron/Message/Preferences.t b/t/db_dependent/Koha/Patron/Message/Preferences.t
>index 07a6e04..4998d80 100644
>--- a/t/db_dependent/Koha/Patron/Message/Preferences.t
>+++ b/t/db_dependent/Koha/Patron/Message/Preferences.t
>@@ -19,7 +19,7 @@
> 
> use Modern::Perl;
> 
>-use Test::More tests => 16;
>+use Test::More tests => 17;
> 
> use t::lib::Mocks;
> use t::lib::TestBuilder;
>@@ -111,7 +111,7 @@ subtest 'Add a test messaging attribute' => sub {
> };
> 
> subtest 'Add a test messaging transport' => sub {
>-    plan tests => 2;
>+    plan tests => 4;
> 
>     ok(Koha::Patron::Message::Transport->new({
>         message_attribute_id => $attribute->message_attribute_id,
>@@ -119,7 +119,21 @@ subtest 'Add a test messaging transport' => sub {
>         is_digest => 0,
>         letter_module => "circulation",
>         letter_code => "CHECKIN",
>-    })->store, "Added a new messaging transport type.");
>+    })->store, "Added a new messaging transport type test.");
>+    ok(Koha::Patron::Message::Transport->new({
>+        message_attribute_id => $attribute->message_attribute_id,
>+        message_transport_type => 'sms',
>+        is_digest => 0,
>+        letter_module => "circulation",
>+        letter_code => "CHECKIN",
>+    })->store, "Added a new messaging transport type sms.");
>+    ok(Koha::Patron::Message::Transport->new({
>+        message_attribute_id => $attribute->message_attribute_id,
>+        message_transport_type => 'email',
>+        is_digest => 0,
>+        letter_module => "circulation",
>+        letter_code => "CHECKIN",
>+    })->store, "Added a new messaging transport type email.");
>     is(Koha::Patron::Message::Transports->find({
>         message_transport_type => "test" })->message_attribute_id,
>        $attribute->message_attribute_id , "Found test messaging transport from database.");
>@@ -182,7 +196,7 @@ subtest 'Add a messaging preference to patron' => sub {
>     };
> 
>     subtest 'Attempt to add a messaging preference with invalid parameters' => sub {
>-        plan tests => 6;
>+        plan tests => 11;
> 
>         eval { Koha::Patron::Message::Preference->new->store };
>         is (ref $@, "Koha::Exceptions::MissingParameter",
>@@ -211,6 +225,29 @@ subtest 'Add a messaging preference to patron' => sub {
>             ." => Koha::Exceptions::BadParameter");
>         is ($@->parameter, "categorycode", "The previous exception tells us it"
>             ." was the categorycode.");
>+        eval { Koha::Patron::Message::Preference->new({
>+                borrowernumber => $patron->{'borrowernumber'},
>+                message_transport_types => ['nonexistent']
>+            })->store };
>+        is (ref $@, "Koha::Exceptions::BadParameter",
>+            "Adding a message preference with invalid message_transport_type"
>+            ." => Koha::Exceptions::BadParameter");
>+        is ($@->parameter, "message_transport_type", "The previous exception tells us it"
>+            ." was the message_transport_type.");
>+        eval {
>+            Koha::Patron::Message::Preference->new({
>+                borrowernumber => $patron->{'borrowernumber'},
>+                message_attribute_id => $attribute->message_attribute_id,
>+                message_transport_types => ['sms'],
>+                wants_digest => 1,
>+            })->store };
>+        is (ref $@, "Koha::Exceptions::BadParameter",
>+            "Adding a message preference with invalid message_transport_type"
>+            ." => Koha::Exceptions::BadParameter");
>+        is ($@->parameter, "message_transport_type", "The previous exception tells us it"
>+            ." was the message_transport_type.");
>+        like ($@->error, qr/^Message transport option/, "Exception s because of given"
>+            ." message_transport_type is not a valid option.");
>     };
> };
> 
>@@ -227,6 +264,93 @@ subtest 'Add a messaging transport preference to patron' => sub {
>        "Found test messaging transport preference from database.");
> };
> 
>+subtest 'Test Koha::Patron::Message::Preference->message_transport_types' => sub {
>+    plan tests => 3;
>+
>+    subtest 'Get' => sub {
>+        plan tests => 5;
>+
>+        Koha::Patron::Message::Transport::Preferences->search({
>+            borrower_message_preference_id => $preference->borrower_message_preference_id,
>+        })->delete;
>+        Koha::Patron::Message::Transport::Preference->new({
>+            borrower_message_preference_id => $preference->borrower_message_preference_id,
>+            message_transport_type => 'sms',
>+        })->store;
>+        Koha::Patron::Message::Transport::Preference->new({
>+            borrower_message_preference_id => $preference->borrower_message_preference_id,
>+            message_transport_type => 'email',
>+        })->store;
>+        my $stored_transports = Koha::Patron::Message::Transport::Preferences->search({
>+            borrower_message_preference_id => $preference->borrower_message_preference_id,
>+        });
>+
>+        ok(Koha::Patron::Message::Preference->can('message_transport_types'), "Method available-");
>+        my $transports = $preference->message_transport_types;
>+        is(keys $transports, $stored_transports->count, "->message_transport_types gets correct amount of transport types.");
>+        is($transports->{$stored_transports->next->message_transport_type}, "CHECKIN", "Found correct message transport type and letter code.");
>+        is($transports->{$stored_transports->next->message_transport_type}, "CHECKIN", "Found correct message transport type and letter code.");
>+        ok(!$preference->message_transport_types->{'nonexistent'}, "Didn't find nonexistent transport type.");
>+    };
>+
>+    subtest 'Set' => sub {
>+        plan tests => 9;
>+
>+        # 1/3, use message_transport_types(list)
>+        Koha::Patron::Message::Transport::Preferences->search({
>+            borrower_message_preference_id => $preference->borrower_message_preference_id,
>+        })->delete;
>+        ok($preference->message_transport_types('sms', 'email')->store, "1/3 Set returned true.");
>+        my $stored_transports = Koha::Patron::Message::Transport::Preferences->search({
>+            borrower_message_preference_id => $preference->borrower_message_preference_id,
>+        });
>+        is($stored_transports->next->message_transport_type, 'email', "First type is email.");
>+        is($stored_transports->next->message_transport_type, 'sms', "Second type is sms.");
>+
>+        # 2/3, use message_transport_types(ARRAYREF)
>+        Koha::Patron::Message::Transport::Preferences->search({
>+            borrower_message_preference_id => $preference->borrower_message_preference_id,
>+        })->delete;
>+        ok($preference->message_transport_types(['sms', 'email'])->store, "2/3 Set returned true.");
>+        $stored_transports = Koha::Patron::Message::Transport::Preferences->search({
>+            borrower_message_preference_id => $preference->borrower_message_preference_id,
>+        });
>+        is($stored_transports->next->message_transport_type, 'email', "First type is email.");
>+        is($stored_transports->next->message_transport_type, 'sms', "Second type is sms.");
>+
>+        # 3/3, use set({ message_transport_types => ARRAYREF })
>+        Koha::Patron::Message::Transport::Preferences->search({
>+            borrower_message_preference_id => $preference->borrower_message_preference_id,
>+        })->delete;
>+        ok($preference->set({ message_transport_types => ['sms', 'email']})->store, "3/3 Set returned true.");
>+        $stored_transports = Koha::Patron::Message::Transport::Preferences->search({
>+            borrower_message_preference_id => $preference->borrower_message_preference_id,
>+        });
>+        is($stored_transports->next->message_transport_type, 'email', "First type is email.");
>+        is($stored_transports->next->message_transport_type, 'sms', "Second type is sms.");
>+    };
>+
>+    subtest 'New' => sub {
>+        plan tests => 3;
>+
>+        $preference->delete;
>+        ok($preference = Koha::Patron::Message::Preference->new({
>+            borrowernumber => $patron->{'borrowernumber'},
>+            message_attribute_id => $attribute->message_attribute_id,
>+            wants_digest => 0,
>+            days_in_advance => 1,
>+            message_transport_types => $transport_type->message_transport_type,
>+        })->store, "Added a new messaging preference and transport types to patron.");
>+        ok($preference->message_transport_types->{$transport_type->message_transport_type},
>+           "The transport type is stored in the object.");
>+        my $stored_transports = Koha::Patron::Message::Transport::Preferences->search({
>+            borrower_message_preference_id => $preference->borrower_message_preference_id,
>+        });
>+        is($stored_transports->next->message_transport_type, $transport_type->message_transport_type,
>+           "The transport type is stored in the database.");
>+    };
>+};
>+
> $schema->storage->txn_rollback;
> 
> 1;
>-- 
>2.7.4

Actions: View | Diff | Splinter Review

Attachments on bug 17499: 63427 | 63428 | 63429 | 63430 | 63431 | 63432 | 63433 | 63434 | 63786 | 63787 | 63788 | 63789 | 63790 | 63791 | 63792 | 63793 | 63801 | 63802 | 63803 | 63804 | 63805 | 63806 | 63807 | 63808 | 63809 | 63810 | 63811 | 63812 | 63878 | 63895 | 64035 | 64036 | 64037 | 64038 | 64039 | 64040 | 64041 | 64042 | 64043 | 64044 | 64795 | 66686 | 69434 | 69435 | 69458 | 91864 | 93863 | 93865 | 93872 | 94748 | 99479 | 99634 | 99668 | 99669 | 99670 | 103441 | 103442 | 103443 | 106653 | 106654 | 106655 | 106656 | 106657 | 107612 | 107613 | 107614 | 107615 | 107616 | 113010 | 113011 | 113127 | 113129 | 113686 | 113687 | 113765 | 113766 | 113767 | 113768 | 118452 | 118453 | 119470 | 119471 | 119472 | 119473 | 119474 | 119475 | 119476 | 119477 | 119478 | 119479 | 119480 | 119481 | 119482 | 119483 | 119484 | 119485 | 119486 | 119487 | 119488 | 119489 | 119490 | 119491 | 141408 | 141409 | 141410 | 141411 | 141412 | 141413 | 141414 | 141415 | 141416 | 141417 | 141418 | 146529 | 146530 | 146531 | 146532 | 146533 | 146534 | 146535 | 146536 | 146537 | 146538 | 146539 | 146540 | 151900 | 151901 | 151902 | 151903 | 151904 | 151905 | 151906 | 151907 | 151908 | 151909 | 151910 | 151911 | 151937 | 151938 | 151939 | 151940 | 151941 | 151942 | 151943 | 151944 | 151945 | 151946 | 151947 | 151948 | 155365 | 155366 | 155367 | 155368 | 155369 | 155370 | 155371 | 155372 | 155373 | 155374 | 155375 | 155376