Attachment 63792 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), 17.65 KB, created by Lari Taskula on 2017-05-29 09:58:03 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Lari Taskula

Created: 2017-05-29 09:58:03 UTC

Size: 17.65 KB

patch

obsolete

>From 2c948326cc8208bfb3e98bd1f9ed940b3705bd8d 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 | 186 ++++++++++++++++++++++-
> 2 files changed, 330 insertions(+), 5 deletions(-)
>
>diff --git a/Koha/Patron/Message/Preference.pm b/Koha/Patron/Message/Preference.pm
>index 9f63782..c7e2b95 100644
>--- a/Koha/Patron/Message/Preference.pm
>+++ b/Koha/Patron/Message/Preference.pm
>@@ -25,6 +25,7 @@ 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;
> 
>@@ -46,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,
> });
>@@ -62,12 +64,22 @@ You can instantiate a new object without custom validation errors, but when
> storing, validation may throw exceptions. See C<validate()> for more
> documentation.
> 
>+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);
> 
>     return $self;
> }
>@@ -144,6 +156,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
>@@ -154,7 +224,24 @@ can be thrown. See C<validate()> for documentation about exceptions.
> sub store {
>     my $self = shift;
> 
>-    return $self->validate->SUPER::store(@_);
>+    $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;
> }
> 
> =head3 validate
>@@ -252,6 +339,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 8f09826..0c98222 100644
>--- a/t/db_dependent/Koha/Patron/Message/Preferences.t
>+++ b/t/db_dependent/Koha/Patron/Message/Preferences.t
>@@ -19,7 +19,8 @@
> 
> use Modern::Perl;
> 
>-use Test::More tests => 5;
>+
>+use Test::More tests => 6;
> 
> use t::lib::Mocks;
> use t::lib::TestBuilder;
>@@ -173,6 +174,152 @@ subtest 'Add preferences from defaults' => sub {
>     $schema->storage->txn_rollback;
> };
> 
>+subtest 'Test Koha::Patron::Message::Preference->message_transport_types' => sub {
>+    plan tests => 4;
>+
>+    ok(Koha::Patron::Message::Preference->can('message_transport_types'),
>+       'Method message_transport_types available');
>+
>+    subtest 'get message_transport_types' => sub {
>+        plan tests => 4;
>+
>+        $schema->storage->txn_begin;
>+
>+        my $patron = build_a_test_patron();
>+        my ($preference, $mtt1, $mtt2) = build_a_test_complete_preference({
>+            patron => $patron
>+        });
>+        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 => $mtt1->message_transport_type,
>+        })->store;
>+        Koha::Patron::Message::Transport::Preference->new({
>+            borrower_message_preference_id => $preference->borrower_message_preference_id,
>+            message_transport_type => $mtt2->message_transport_type,
>+        })->store;
>+        my $stored_transports = Koha::Patron::Message::Transport::Preferences->search({
>+            borrower_message_preference_id => $preference->borrower_message_preference_id,
>+        });
>+        my $transport1 = Koha::Patron::Message::Transports->find({
>+            message_attribute_id => $preference->message_attribute_id,
>+            message_transport_type => $mtt1->message_transport_type,
>+        });
>+        my $transport2 = Koha::Patron::Message::Transports->find({
>+            message_attribute_id => $preference->message_attribute_id,
>+            message_transport_type => $mtt2->message_transport_type,
>+        });
>+        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},
>+           $transport1->letter_code, 'Found correct message transport type and letter code.');
>+        is($transports->{$stored_transports->next->message_transport_type},
>+           $transport2->letter_code, 'Found correct message transport type and letter code.');
>+        ok(!$preference->message_transport_types->{'nonexistent'},
>+           'Didn\'t find nonexistent transport type.');
>+
>+        $schema->storage->txn_rollback;
>+    };
>+
>+    subtest 'set message_transport_types' => sub {
>+        plan tests => 6;
>+
>+        $schema->storage->txn_begin;
>+
>+        my $patron = build_a_test_patron();
>+        my ($preference, $mtt1, $mtt2) = build_a_test_complete_preference({
>+            patron => $patron
>+        });
>+
>+        my $mtt1_str = $mtt1->message_transport_type;
>+        my $mtt2_str = $mtt2->message_transport_type;
>+        # 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($mtt1_str, $mtt2_str)->store,
>+           '1/3 Set returned true.');
>+        my $stored_transports = Koha::Patron::Message::Transport::Preferences->search({
>+            borrower_message_preference_id => $preference->borrower_message_preference_id,
>+            '-or' => [
>+                message_transport_type => $mtt1_str,
>+                message_transport_type => $mtt2_str
>+            ]
>+        });
>+        is($stored_transports->count, 2, 'Two transports selected');
>+
>+        # 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([$mtt1_str, $mtt2_str])->store,
>+           '2/3 Set returned true.');
>+        $stored_transports = Koha::Patron::Message::Transport::Preferences->search({
>+            borrower_message_preference_id => $preference->borrower_message_preference_id,
>+            '-or' => [
>+                message_transport_type => $mtt1_str,
>+                message_transport_type => $mtt2_str
>+            ]
>+        });
>+        is($stored_transports->count, 2, 'Two transports selected');
>+
>+        # 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 => [$mtt1_str, $mtt2_str]})->store,
>+           '3/3 Set returned true.');
>+        $stored_transports = Koha::Patron::Message::Transport::Preferences->search({
>+            borrower_message_preference_id => $preference->borrower_message_preference_id,
>+            '-or' => [
>+                message_transport_type => $mtt1_str,
>+                message_transport_type => $mtt2_str
>+            ]
>+        });
>+        is($stored_transports->count, 2, 'Two transports selected');
>+
>+        $schema->storage->txn_rollback;
>+    };
>+
>+    subtest 'new message_transport_types' => sub {
>+        plan tests => 3;
>+
>+        $schema->storage->txn_begin;
>+
>+        my $patron    = build_a_test_patron();
>+        my $letter    = build_a_test_letter();
>+        my $attribute = build_a_test_attribute();
>+        my $mtt       = build_a_test_transport_type();
>+        Koha::Patron::Message::Transport->new({
>+            message_attribute_id   => $attribute->message_attribute_id,
>+            message_transport_type => $mtt->message_transport_type,
>+            is_digest              => 0,
>+            letter_module          => $letter->module,
>+            letter_code            => $letter->code,
>+        })->store;
>+        ok(my $preference = Koha::Patron::Message::Preference->new({
>+            borrowernumber => $patron->borrowernumber,
>+            message_attribute_id => $attribute->message_attribute_id,
>+            wants_digest => 0,
>+            days_in_advance => undef,
>+            message_transport_types => $mtt->message_transport_type,
>+        })->store, 'Added a new messaging preference and transport types to patron.');
>+        ok($preference->message_transport_types->{$mtt->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, $mtt->message_transport_type,
>+           'The transport type is stored in the database.');
>+
>+        $schema->storage->txn_rollback;
>+    };
>+};
>+
> subtest 'Test adding a new preference with invalid parameters' => sub {
>     plan tests => 4;
> 
>@@ -203,7 +350,7 @@ subtest 'Test adding a new preference with invalid parameters' => sub {
>     };
> 
>     subtest 'Bad parameter' => sub {
>-        plan tests => 8;
>+        plan tests => 13;
> 
>         $schema->storage->txn_begin;
> 
>@@ -250,6 +397,30 @@ subtest 'Test adding a new preference with invalid parameters' => sub {
>         is($@->parameter, 'days_in_advance', 'The previous exception tells us it'
>             .' was the days_in_advance.');
> 
>+        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.');
>+
>         $schema->storage->txn_rollback;
>     };
> 
>@@ -395,4 +566,15 @@ sub build_a_test_category_preference {
>     return ($default, $mtt1, $mtt2);
> }
> 
>+sub build_a_test_complete_preference {
>+    my ($params) = @_;
>+
>+    my ($default, $mtt1, $mtt2) = build_a_test_category_preference($params);
>+    my $patron = $params->{patron};
>+    $patron->set_default_messaging_preferences;
>+    return (Koha::Patron::Message::Preferences->search({
>+        borrowernumber => $patron->borrowernumber
>+    })->next, $mtt1, $mtt2);
>+}
>+
> 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