Bug 17499

Created attachment 63786 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences. This patch does not
add any extra logic into these objects.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Created attachment 63787 [details] [review]
Bug 17499: Add useful Koha::Exceptions

This patch adds some basic Koha::Exceptions that will be useful.

Created attachment 63788 [details] [review]
Bug 17499: Add basic validation for messaging preference

This patch adds simple validation for messaging preferences.

The validation includes
- check that only either borrowernumber or categorycode is given, but not both
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available

To test:
1. Run t/db_dependent/Koha/Patron/Message/Preferences.t

Created attachment 63789 [details] [review]
Bug 17499: Throw an exception on duplicate messaging preference

When trying to add a duplicate messaging preference that has the same
borrowernumber or category, and message_attribute_id as another preference,
throw Koha::Exceptions::DuplicateObject.

To test:
1. Run t/db_dependent/Koha/Patron/Message/Preferences.t

Created attachment 63790 [details] [review]
Bug 17499: Add a method for getting messaging options

This patch adds a method for getting available messaging options.

To test:
1. Run t/db_dependent/Koha/Patron/Message/Preferences.t

Created attachment 63791 [details] [review]
Bug 17499: Add a method for setting default messaging preferences

This patch adds a method Koha::Patron::Message::Preference->new_from_default that
can be used to add category's default messaging preferences to patron for a given
message type.

Example call:

Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
});

Also adds a simple method for Koha::Patron, set_default_messaging_preferences.
Usage: $patron->set_default_messaging_preferences()

To test:
1. Run t/db_dependent/Koha/Patron/Message/Preferences.t

Created attachment 63792 [details] [review]
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

Created attachment 63793 [details] [review]
Bug 17499: Simplify searching by message_name

This patch adds optional search & find parameter message_name for
Koha::Patron::Message::Preferences->search and ->find. This simplifies object
usage by allowing us to skip joins or finding the attribute id ourselves.

Walking through 8 patches, following test plans...
1 OK
1-3 OK
1-4 OK
1-5 OK
1-6 OK
1-7 OK
1-8 OK - Test plan for 8 is the same: 
         prove -v t/db_dependent/Koha/Patron/Message/Preferences.t

Created attachment 63801 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences. This patch does not
add any extra logic into these objects.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Signed-off-by: Marc Véron <veron@veron.ch>

Created attachment 63802 [details] [review]
Bug 17499: Add useful Koha::Exceptions

This patch adds some basic Koha::Exceptions that will be useful.

Signed-off-by: Marc Véron <veron@veron.ch>

Created attachment 63803 [details] [review]
Bug 17499: Add basic validation for messaging preference

This patch adds simple validation for messaging preferences.

The validation includes
- check that only either borrowernumber or categorycode is given, but not both
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available

To test:
1. Run t/db_dependent/Koha/Patron/Message/Preferences.t

Signed-off-by: Marc Véron <veron@veron.ch>

Created attachment 63804 [details] [review]
Bug 17499: Throw an exception on duplicate messaging preference

When trying to add a duplicate messaging preference that has the same
borrowernumber or category, and message_attribute_id as another preference,
throw Koha::Exceptions::DuplicateObject.

To test:
1. Run t/db_dependent/Koha/Patron/Message/Preferences.t

Signed-off-by: Marc Véron <veron@veron.ch>

Created attachment 63805 [details] [review]
Bug 17499: Add a method for getting messaging options

This patch adds a method for getting available messaging options.

To test:
1. Run t/db_dependent/Koha/Patron/Message/Preferences.t

Signed-off-by: Marc Véron <veron@veron.ch>

Created attachment 63806 [details] [review]
Bug 17499: Add a method for setting default messaging preferences

This patch adds a method Koha::Patron::Message::Preference->new_from_default that
can be used to add category's default messaging preferences to patron for a given
message type.

Example call:

Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
});

Also adds a simple method for Koha::Patron, set_default_messaging_preferences.
Usage: $patron->set_default_messaging_preferences()

To test:
1. Run t/db_dependent/Koha/Patron/Message/Preferences.t

Signed-off-by: Marc Véron <veron@veron.ch>

Created attachment 63807 [details] [review]
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

Signed-off-by: Marc Véron <veron@veron.ch>

Created attachment 63808 [details] [review]
Bug 17499: Simplify searching by message_name

This patch adds optional search & find parameter message_name for
Koha::Patron::Message::Preferences->search and ->find. This simplifies object
usage by allowing us to skip joins or finding the attribute id ourselves.

prove -v t/db_dependent/Koha/Patron/Message/Preferences.t turns green
Signed-off-by: Marc Véron <veron@veron.ch>

Created attachment 63809 [details] [review]
Bug 17499: (follow-up) Bugfix for validation

- Bugfix for days_in_advance and wants_digest validation. Have to check definedness
  of value instead the value itself.
- Fixes broken tests caused by above update.
- Adding a missing unit test for wants_digest validation
- Minor edits to exception messages.
- Throwing exception for invalid message_attribute_id. Incl. unit test

Created attachment 63810 [details] [review]
Bug 17499: (follow-up) Bugfix for validation

- Bugfix for days_in_advance and wants_digest validation. Have to check definedness
  of value instead the value itself.
- Fixes broken tests caused by above update.
- Adding a missing unit test for wants_digest validation
- Minor edits to exception messages.
- Throwing exception for invalid message_attribute_id. Incl. unit test

Unit test turns green.
Signed-off-by: Marc Véron <veron@veron.ch>

Created attachment 63811 [details] [review]
Bug 17499: (follow-up) Bugfix for validation

- Bugfix for days_in_advance and wants_digest validation. Have to check definedness
  of value instead the value itself.
- Fixes broken tests caused by above update.
- Adding a missing unit test for wants_digest validation
- Minor edits to exception messages.
- Throwing exception for invalid message_attribute_id. Incl. unit test

Created attachment 63812 [details] [review]
Bug 17499: (follow-up) Bugfix for validation

- Bugfix for days_in_advance and wants_digest validation. Have to check definedness
  of value instead the value itself.
- Fixes broken tests caused by above update.
- Adding a missing unit test for wants_digest validation
- Minor edits to exception messages.
- Throwing exception for invalid message_attribute_id. Incl. unit test

Tested again, prove -v t/db_dependent/Koha/Patron/Message/Preferences.t turns green
prove -v t/db_dependent/Koha/Patron/Message/* turns green
Signed-off-by: Marc Véron <veron@veron.ch>

Tested 
- editing messaging preferences in staff client and opac
- check out an item to have an email sent
All OK.

Signing off.

(In reply to Marc Véron from comment #30)
> Tested 
> - editing messaging preferences in staff client and opac
> - check out an item to have an email sent
> All OK.
> 
> Signing off.
Please note that the actual move from C4::Members::Messaging to these Koha-objects is in Bug 18595. This Bug just introduces new Koha-objects for messaging preferences. This Bug doesn't yet use them in action.

I did it like this because personally I am just interested in deprecating the C4::Members::Messaging methods and instead having the Koha-objects to use with REST API.

Created attachment 63878 [details] [review]
Bug 17499: (follow-up) Bugfix for validation, 2

Remove duplicate validation for digest availability. This is already checked in
$preference->validate().

Also prevents a crash when patron has messaging transport type selected for
a message for which it should not be available. This patch logs it with as
a warning, and only throws an exception when attempting to set it.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Removing Needs Signoff status, I still noticed some bugs when using this together with REST API, will also attach tests for the latest patch.

Created attachment 63895 [details] [review]
Bug 17499: (follow-up) Bugfix for validation, 2

Remove duplicate validation for digest availability. This is already checked in
$preference->validate().

Set values for current object before validating messaging transport types, not
the other way around as it was before this patch.

Also prevents a crash when patron has messaging transport type selected for
a message for which it should not be available. This patch logs it with as
a warning, and only throws an exception when attempting to set it.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Created attachment 64035 [details] [review]
[SIGNED-OFF] Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences. This patch does not
add any extra logic into these objects.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 64036 [details] [review]
[SIGNED-OFF] Bug 17499: Add useful Koha::Exceptions

This patch adds some basic Koha::Exceptions that will be useful.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 64037 [details] [review]
[SIGNED-OFF] Bug 17499: Add basic validation for messaging preference

This patch adds simple validation for messaging preferences.

The validation includes
- check that only either borrowernumber or categorycode is given, but not both
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available

To test:
1. Run t/db_dependent/Koha/Patron/Message/Preferences.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 64038 [details] [review]
[SIGNED-OFF] Bug 17499: Throw an exception on duplicate messaging preference

When trying to add a duplicate messaging preference that has the same
borrowernumber or category, and message_attribute_id as another preference,
throw Koha::Exceptions::DuplicateObject.

To test:
1. Run t/db_dependent/Koha/Patron/Message/Preferences.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 64039 [details] [review]
[SIGNED-OFF] Bug 17499: Add a method for getting messaging options

This patch adds a method for getting available messaging options.

To test:
1. Run t/db_dependent/Koha/Patron/Message/Preferences.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 64040 [details] [review]
[SIGNED-OFF] Bug 17499: Add a method for setting default messaging preferences

This patch adds a method Koha::Patron::Message::Preference->new_from_default that
can be used to add category's default messaging preferences to patron for a given
message type.

Example call:

Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
});

Also adds a simple method for Koha::Patron, set_default_messaging_preferences.
Usage: $patron->set_default_messaging_preferences()

To test:
1. Run t/db_dependent/Koha/Patron/Message/Preferences.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 64041 [details] [review]
[SIGNED-OFF] 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

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 64042 [details] [review]
[SIGNED-OFF] Bug 17499: Simplify searching by message_name

This patch adds optional search & find parameter message_name for
Koha::Patron::Message::Preferences->search and ->find. This simplifies object
usage by allowing us to skip joins or finding the attribute id ourselves.

prove -v t/db_dependent/Koha/Patron/Message/Preferences.t turns green
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 64043 [details] [review]
[SIGNED-OFF] Bug 17499: (follow-up) Bugfix for validation

- Bugfix for days_in_advance and wants_digest validation. Have to check definedness
  of value instead the value itself.
- Fixes broken tests caused by above update.
- Adding a missing unit test for wants_digest validation
- Minor edits to exception messages.
- Throwing exception for invalid message_attribute_id. Incl. unit test

Tested again, prove -v t/db_dependent/Koha/Patron/Message/Preferences.t turns green
prove -v t/db_dependent/Koha/Patron/Message/* turns green
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 64044 [details] [review]
[SIGNED-OFF] Bug 17499: (follow-up) Bugfix for validation, 2

Remove duplicate validation for digest availability. This is already checked in
$preference->validate().

Set values for current object before validating messaging transport types, not
the other way around as it was before this patch.

Also prevents a crash when patron has messaging transport type selected for
a message for which it should not be available. This patch logs it with as
a warning, and only throws an exception when attempting to set it.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Created attachment 64795 [details] [review]
Bug 17499: (follow-up) Add information on digest being the only option to get_options

If a library wants to force digest on a message type, add this information to
Koha::Patron::Message::Preferences->get_options.

QA: Looking here now

You add exceptions that are already there.

Koha::Exceptions::BadParameter
Koha::Exceptions::WrongParameter

Koha::Exceptions::ObjectNotFound
Koha::Exceptions::UnknownObject

Created attachment 66686 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences. This patch does not
add any extra logic into these objects.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Bug 17499: Add useful Koha::Exceptions

This patch adds some basic Koha::Exceptions that will be useful.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Bug 17499: Add basic validation for messaging preference

This patch adds simple validation for messaging preferences.

The validation includes
- check that only either borrowernumber or categorycode is given, but not both
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available

To test:
1. Run t/db_dependent/Koha/Patron/Message/Preferences.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Bug 17499: Throw an exception on duplicate messaging preference

When trying to add a duplicate messaging preference that has the same
borrowernumber or category, and message_attribute_id as another preference,
throw Koha::Exceptions::DuplicateObject.

To test:
1. Run t/db_dependent/Koha/Patron/Message/Preferences.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Bug 17499: Add a method for getting messaging options

This patch adds a method for getting available messaging options.

To test:
1. Run t/db_dependent/Koha/Patron/Message/Preferences.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Bug 17499: Add a method for setting default messaging preferences

This patch adds a method Koha::Patron::Message::Preference->new_from_default that
can be used to add category's default messaging preferences to patron for a given
message type.

Example call:

Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
});

Also adds a simple method for Koha::Patron, set_default_messaging_preferences.
Usage: $patron->set_default_messaging_preferences()

To test:
1. Run t/db_dependent/Koha/Patron/Message/Preferences.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

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

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Bug 17499: Simplify searching by message_name

This patch adds optional search & find parameter message_name for
Koha::Patron::Message::Preferences->search and ->find. This simplifies object
usage by allowing us to skip joins or finding the attribute id ourselves.

prove -v t/db_dependent/Koha/Patron/Message/Preferences.t turns green
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Bug 17499: (follow-up) Bugfix for validation

- Bugfix for days_in_advance and wants_digest validation. Have to check definedness
  of value instead the value itself.
- Fixes broken tests caused by above update.
- Adding a missing unit test for wants_digest validation
- Minor edits to exception messages.
- Throwing exception for invalid message_attribute_id. Incl. unit test

Tested again, prove -v t/db_dependent/Koha/Patron/Message/Preferences.t turns green
prove -v t/db_dependent/Koha/Patron/Message/* turns green
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Bug 17499: (follow-up) Bugfix for validation, 2

Remove duplicate validation for digest availability. This is already checked in
$preference->validate().

Set values for current object before validating messaging transport types, not
the other way around as it was before this patch.

Also prevents a crash when patron has messaging transport type selected for
a message for which it should not be available. This patch logs it with as
a warning, and only throws an exception when attempting to set it.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Bug 17499: (follow-up) Add information on digest being the only option to get_options

If a library wants to force digest on a message type, add this information to
Koha::Patron::Message::Preferences->get_options.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Squashed 11 commits during QA.

QA Comment:
Looks good to me overall (see also comment47).
No blockers found.

Koha::Patron::Message::Preferences
sub find_with_message_name
 my $attr = Koha::Patron::Message::Attributes->find({
            message_name => $id->{'message_name'},
        });
Isnt this actually a search? Couldnt we have multiple results on name ?
Similar question for  search_with_message_name later

my $name = $transport->get_column('message_name');
Why get_column, not just message_name ?

+=head3 search
+
+Koha::Patron::Message::Preferences->search_with_message_name({
+    borrowernumber => 123,
+    message_name => 'Hold_Filled',
+});
Wrong POD header

(In reply to Marcel de Rooy from comment #49)
> Koha::Patron::Message::Preferences
> sub find_with_message_name
>  my $attr = Koha::Patron::Message::Attributes->find({
>             message_name => $id->{'message_name'},
>         });
> Isnt this actually a search? Couldnt we have multiple results on name ?
> Similar question for  search_with_message_name later

It's a unique key
  UNIQUE KEY `message_name` (`message_name`)


> my $name = $transport->get_column('message_name');
> Why get_column, not just message_name ?

It's a column from another table.

(In reply to Marcel de Rooy from comment #47)
> You add exceptions that are already there.
> 
> Koha::Exceptions::BadParameter
> Koha::Exceptions::WrongParameter
> 
> Koha::Exceptions::ObjectNotFound
> Koha::Exceptions::UnknownObject

Lari, please adjust.
Are you ok with the squash?

Created attachment 69434 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Created attachment 69435 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

(In reply to Marcel de Rooy from comment #49)
> QA Comment:
> Looks good to me overall (see also comment47).
Fixed.

> +=head3 search
> +
> +Koha::Patron::Message::Preferences->search_with_message_name({
> +    borrowernumber => 123,
> +    message_name => 'Hold_Filled',
> +});
> Wrong POD header
Fixed.

Also cleaned commit message. I set it to Needs Signoff again as I added a new test changed the functionality of $patron->set_default_messaging_preferences to delete any previous preferences (this is expected behaviour; before this fix it would throw an exception).

Created attachment 69458 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Signed-off-by: Dominic Pichette <dominic@inlibro.com>

prove t/db_dependent/Koha/Patron/Message/*
t/db_dependent/Koha/Patron/Message/Attributes.t ............. ok   
t/db_dependent/Koha/Patron/Message/Preferences.t ............ ok   
t/db_dependent/Koha/Patron/Message/Transport/Preferences.t .. ok   
t/db_dependent/Koha/Patron/Message/Transport/Types.t ........ ok   
t/db_dependent/Koha/Patron/Message/Transports.t ............. ok   
All tests successful.
Files=5, Tests=15,  9 wallclock secs ( 0.07 usr  0.01 sys +  5.05 cusr  0.20 csys =  5.33 CPU)
Result: PASS

Patch doesn't apply, please rebase!

Created attachment 91864 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

Rebased patch.

Created attachment 93863 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Signed-off-by: Dominic Pichette <dominic@inlibro.com>

Hi Lari,

thx for the update!

Is this related to https://wiki.koha-community.org/wiki/Patrons_messaging_preferences_endpoint_RFC and ready for voting? We got a meeting tomorrow and could add it to the agenda right away.

There was a bit of discussion on the wiki page - so please check the bottom of the page!

(In reply to Katrin Fischer from comment #60)
> Hi Lari,
> 
> thx for the update!
> 
> Is this related to
> https://wiki.koha-community.org/wiki/
> Patrons_messaging_preferences_endpoint_RFC and ready for voting? We got a
> meeting tomorrow and could add it to the agenda right away.
> 
> There was a bit of discussion on the wiki page - so please check the bottom
> of the page!

Hi Katrin,

Yes, this is related to the endpoint. Current REST API implementation in Bug 17505 uses the Koha objects I provided here.

The idea is to add validations in messaging preference Koha objects and throw Koha::Exceptions when needed, so that we can easily catch any issues in REST API controllers.

The patch is now rebased on top of latest master branch.

Just realized, I got my bugs turned upside down - objects first, then the API. It might still get sense to get the API voted on early, so we can just push things through once ready. 

Hope this can get a sign-off soon!

Created attachment 93865 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Signed-off-by: Dominic Pichette <dominic@inlibro.com>

Created attachment 93872 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Signed-off-by: Dominic Pichette <dominic@inlibro.com>

Created attachment 94748 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Signed-off-by: Dominic Pichette <dominic@inlibro.com>

Can we de-squash this huge patch into meaningful pieces with easy to follow details on the resulting implementation, please? I like the results :-D

What should the status of this be after Tomás's comment? (comment 66) Needs Signoff or Failed QA?

I tried to apply the patch and got this - no sure whether this means it has applied cleanly or note:

94748 - Bug 17499: Add Koha-objects for messaging preferences

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 17499: Add Koha-objects for messaging preferences
Using index info to reconstruct a base tree...
M	Koha/Exceptions.pm
M	Koha/Patron.pm
Falling back to patching base and 3-way merge...
Auto-merging Koha/Patron.pm
Auto-merging Koha/Exceptions.pm

Also, running the test resulted in the tests failing:

prove t/db_dependent/Koha/Patron/Message/*
t/db_dependent/Koha/Patron/Message/Attributes.t ............. 1/2     # Looks like you planned 6 tests but ran 4.

#   Failed test 'Test Koha::Patron::Message::Attributes'
#   at t/db_dependent/Koha/Patron/Message/Attributes.t line 72.
Can't locate object method "_new_from_dbic" via package "Koha::MessageAttribute" (perhaps you forgot to load "Koha::MessageAttribute"?) at /kohadevbox/koha/Koha/Object.pm line 222.
# Looks like your test exited with 255 just after 2.
t/db_dependent/Koha/Patron/Message/Attributes.t ............. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/2 subtests 
t/db_dependent/Koha/Patron/Message/Preferences.t ............ 1/7         # Looks like you planned 3 tests but ran 2.

    #   Failed test 'Test for a patron'
    #   at t/db_dependent/Koha/Patron/Message/Preferences.t line 99.
    # Looks like you planned 2 tests but ran 1.
    # Looks like you failed 1 test of 1 run.

#   Failed test 'Test Koha::Patron::Message::Preferences'
#   at t/db_dependent/Koha/Patron/Message/Preferences.t line 125.
Can't locate object method "_new_from_dbic" via package "Koha::BorrowerMessagePreference" (perhaps you forgot to load "Koha::BorrowerMessagePreference"?) at /kohadevbox/koha/Koha/Object.pm line 222.
# Looks like your test exited with 255 just after 2.
t/db_dependent/Koha/Patron/Message/Preferences.t ............ Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 6/7 subtests 
t/db_dependent/Koha/Patron/Message/Transport/Preferences.t .. 1/2     # Looks like you planned 2 tests but ran 1.

#   Failed test 'Test Koha::Patron::Message::Transport::Preferences'
#   at t/db_dependent/Koha/Patron/Message/Transport/Preferences.t line 112.
Can't locate object method "_new_from_dbic" via package "Koha::BorrowerMessageTransportPreference" (perhaps you forgot to load "Koha::BorrowerMessageTransportPreference"?) at /kohadevbox/koha/Koha/Object.pm line 222.
# Looks like your test exited with 255 just after 2.
t/db_dependent/Koha/Patron/Message/Transport/Preferences.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/2 subtests 
t/db_dependent/Koha/Patron/Message/Transport/Types.t ........ 1/2     # Looks like you planned 2 tests but ran 1.

#   Failed test 'Test Koha::Patron::Message::Transport::Types'
#   at t/db_dependent/Koha/Patron/Message/Transport/Types.t line 52.
Can't locate object method "_new_from_dbic" via package "Koha::MessageTransportType" (perhaps you forgot to load "Koha::MessageTransportType"?) at /kohadevbox/koha/Koha/Object.pm line 222.
# Looks like your test exited with 255 just after 2.
t/db_dependent/Koha/Patron/Message/Transport/Types.t ........ Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/2 subtests 
t/db_dependent/Koha/Patron/Message/Transports.t ............. 1/2     # Looks like you planned 2 tests but ran 1.

#   Failed test 'Test Koha::Patron::Message::Transports'
#   at t/db_dependent/Koha/Patron/Message/Transports.t line 70.
Can't locate object method "_new_from_dbic" via package "Koha::MessageTransport" (perhaps you forgot to load "Koha::MessageTransport"?) at /kohadevbox/koha/Koha/Object.pm line 222.
# Looks like your test exited with 255 just after 2.
t/db_dependent/Koha/Patron/Message/Transports.t ............. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/2 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Patron/Message/Attributes.t           (Wstat: 65280 Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 255
t/db_dependent/Koha/Patron/Message/Preferences.t          (Wstat: 65280 Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 7 tests but ran 2.
t/db_dependent/Koha/Patron/Message/Transport/Preferences.t (Wstat: 65280 Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 255
t/db_dependent/Koha/Patron/Message/Transport/Types.t      (Wstat: 65280 Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 255
t/db_dependent/Koha/Patron/Message/Transports.t           (Wstat: 65280 Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 255
Files=5, Tests=10,  8 wallclock secs ( 0.04 usr  0.01 sys +  6.58 cusr  0.64 csys =  7.27 CPU)
Result: FAIL

Actually, I've answered my own question I think - I've run the tests and some failed, so back to Failed QA.

Created attachment 99479 [details] [review]
Bug 17499: Define koha_object(s)_class where needed

This patch adds koha_object(s)_class for needed modules
to fix issues with tests.

Sponsored-by: Koha-Suomi Oy

Tests should work now.

Keeping status as Failed QA since I'm also unsure what it should be after Tomás's comment (comment 66)

(In reply to Emmi Takkinen from comment #70)
> Tests should work now.
> 
> Keeping status as Failed QA since I'm also unsure what it should be after
> Tomás's comment (comment 66)

Can confirm, tests all pass now.

I had a very quick look at the patch. About the tests I would like to see the subroutines replaced with a call to $builder->build_object when it is the only thing they actually do.

Example:

sub build_a_test_transport_type {
    my $mtt = $builder->build({
        source => 'MessageTransportType' });

    return Koha::Patron::Message::Transport::Types->find(
        $mtt->{message_transport_type}
    );
}

can be replaced with:

  $builder->build_object({ class => 'Koha::Patron::Message::Transport::Types' });

Created attachment 99634 [details] [review]
Bug 17499: Replace test subroutines with build_object

This patch replaces subroutines used to build test
objects with call to $builder->build_object.

To test prove that tests are still running with:
prove t/db_dependent/Koha/Patron/Message/*

Sponsored-by: Koha-Suomi Oy

Created attachment 99668 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Signed-off-by: Dominic Pichette <dominic@inlibro.com>

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

Created attachment 99669 [details] [review]
Bug 17499: Define koha_object(s)_class where needed

This patch adds koha_object(s)_class for needed modules
to fix issues with tests.

Sponsored-by: Koha-Suomi Oy

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

Created attachment 99670 [details] [review]
Bug 17499: Replace test subroutines with build_object

This patch replaces subroutines used to build test
objects with call to $builder->build_object.

To test prove that tests are still running with:
prove t/db_dependent/Koha/Patron/Message/*

Sponsored-by: Koha-Suomi Oy

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

Created attachment 103441 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Signed-off-by: Dominic Pichette <dominic@inlibro.com>

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

Created attachment 103442 [details] [review]
Bug 17499: Define koha_object(s)_class where needed

This patch adds koha_object(s)_class for needed modules
to fix issues with tests.

Sponsored-by: Koha-Suomi Oy

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

Created attachment 103443 [details] [review]
Bug 17499: Replace test subroutines with build_object

This patch replaces subroutines used to build test
objects with call to $builder->build_object.

To test prove that tests are still running with:
prove t/db_dependent/Koha/Patron/Message/*

Sponsored-by: Koha-Suomi Oy

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

Lari, I have rebased the patches.

Wondering about find_with_message_name

Koha::Patron::Message::Preferences->find_with_message_name({
    borrowernumber => 123,
    message_name => 'Hold_Filled',
});

Should not we prefer something like:

 $patron->messaging_preferences->filter_by_message_name('Hold_Filled');
or even:
 $patron->messaging_preferences->search({ message_name => 'Hold_Filled' });

ie why is it a class method?

Same for the setter, there is only 1 set_default_messaging_preferences, why cannot we add set messaging preferences directly from $patron?

(In reply to Jonathan Druart from comment #80)
> Lari, I have rebased the patches.
> 
> Wondering about find_with_message_name
> 
> Koha::Patron::Message::Preferences->find_with_message_name({
>     borrowernumber => 123,
>     message_name => 'Hold_Filled',
> });
> 
> Should not we prefer something like:
> 
>  $patron->messaging_preferences->filter_by_message_name('Hold_Filled');
> or even:
>  $patron->messaging_preferences->search({ message_name => 'Hold_Filled' });
> 
> ie why is it a class method?
> 
> Same for the setter, there is only 1 set_default_messaging_preferences, why
> cannot we add set messaging preferences directly from $patron?

I don't think there is a reason why we shouldn't have getters & setters in Koha::Patron as well. The purpose of that method was just to convert human readable message name into an id number.

This needs a bit of work, there are lots of "red" entries when running the QA tools, but they should be easy to fix. Examples:

 FAIL	Koha/Patron/Message/Attributes.pm
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: Incorrect license statement (using postal address), may be a false positive if the file is coming from outside Koha (bug 24545) (line 18)
   OK	  git manipulation
   OK	  pod
   FAIL	  pod coverage
		POD is missing for 'object_class'

All the tests do pass for me.

Lari, could you take a look and fix these? I'll probably need another QA Team member to take a closer look at the code after that.

(In reply to Katrin Fischer from comment #82)
> This needs a bit of work, there are lots of "red" entries when running the
> QA tools, but they should be easy to fix. Examples:
> 
>  FAIL	Koha/Patron/Message/Attributes.pm
>    OK	  critic
>    FAIL	  forbidden patterns
> 		forbidden pattern: Incorrect license statement (using postal address), may
> be a false positive if the file is coming from outside Koha (bug 24545)
> (line 18)
>    OK	  git manipulation
>    OK	  pod
>    FAIL	  pod coverage
> 		POD is missing for 'object_class'
> 
> All the tests do pass for me.
> 
> Lari, could you take a look and fix these? I'll probably need another QA
> Team member to take a closer look at the code after that.

Hi, thanks for the comment, however I'm currently unable to work with this so if anyone wants to step in and help, it would be greatly appreciated!

Moving out of the QA queue for now - hoping someone will rescue this one soon!

Created attachment 106653 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Signed-off-by: Dominic Pichette <dominic@inlibro.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 106654 [details] [review]
Bug 17499: Define koha_object(s)_class where needed

This patch adds koha_object(s)_class for needed modules
to fix issues with tests.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 106655 [details] [review]
Bug 17499: Replace test subroutines with build_object

This patch replaces subroutines used to build test
objects with call to $builder->build_object.

To test prove that tests are still running with:
prove t/db_dependent/Koha/Patron/Message/*

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 106656 [details] [review]
Bug 17499: (QA follow-up) Fix Licence Statements

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 106657 [details] [review]
Bug 17499: (QA follow-up) Fix POD errors

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Rescued.. I've sorted the licence issues and added some POD.. Remaining QA script failure are false negatives.

I've not QA'd as a whole as I'm not close enough to the area of code to really have an opinion whether it's good or not.. back to NQA

Created attachment 107612 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Signed-off-by: Dominic Pichette <dominic@inlibro.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 107613 [details] [review]
Bug 17499: Define koha_object(s)_class where needed

This patch adds koha_object(s)_class for needed modules
to fix issues with tests.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 107614 [details] [review]
Bug 17499: Replace test subroutines with build_object

This patch replaces subroutines used to build test
objects with call to $builder->build_object.

To test prove that tests are still running with:
prove t/db_dependent/Koha/Patron/Message/*

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 107615 [details] [review]
Bug 17499: (QA follow-up) Fix Licence Statements

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 107616 [details] [review]
Bug 17499: (QA follow-up) Fix POD errors

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Reported in 2016... feeling a little 'reckless' tonight and going to pass this. There have been quite some eyes on it already. QA script and plenty of included tests pass. I hope this will trigger work to resume on bug 18595 that makes use of these new objects so we can see them in action.

I have not found anything obvious that would reject this patch set. However we need to push it alongside with bug 18595.

Created attachment 113011 [details] [review]
Bug 17499: (follow-up) More explicit exceptions

Adds and throws more detailed exceptions.

This is useful for APIs and generating translatable errors in GUI.

In short, replaces Koha::Exceptions::BadParameter with:

Koha::Exceptions::Patron::NotFound
Koha::Exceptions::Patron::Category
Koha::Exceptions::Patron::Category::NotFound
Koha::Exceptions::Patron::Message::Preference
Koha::Exceptions::Patron::Message::Preference::AttributeNotFound
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestRequired
Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired
Koha::Exceptions::Patron::Message::Preference::NoTransportType
Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

As per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18595#c114 , I have attached a patch that provides more explicit Koha::Exceptions and reset bug status from "Passed QA" back to "Needs Signoff" for that patch.

Created attachment 113127 [details] [review]
Bug 17499: (follow-up) More explicit exceptions

Adds and throws more detailed exceptions.

This is useful for APIs and generating translatable errors in GUI.

In short, replaces Koha::Exceptions::BadParameter with:

Koha::Exceptions::Patron::NotFound
Koha::Exceptions::Patron::Category
Koha::Exceptions::Patron::Category::NotFound
Koha::Exceptions::Patron::Message::Preference
Koha::Exceptions::Patron::Message::Preference::AttributeNotFound
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestRequired
Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired
Koha::Exceptions::Patron::Message::Preference::NoTransportType
Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Created attachment 113129 [details] [review]
Bug 17499: (follow-up) Fix warning

Fix warning

DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.
SQL that returns multiple rows is DEPRECATED for ->find and ->single.

Sponsored-by: The National Library of Finland

Created attachment 113686 [details] [review]
Bug 17499: (follow-up) More explicit exceptions

Adds and throws more detailed exceptions.

This is useful for APIs and generating translatable errors in GUI.

In short, replaces Koha::Exceptions::BadParameter with:

Koha::Exceptions::Patron::NotFound
Koha::Exceptions::Patron::Category
Koha::Exceptions::Patron::Category::NotFound
Koha::Exceptions::Patron::Message::Preference
Koha::Exceptions::Patron::Message::Preference::AttributeNotFound
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestRequired
Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired
Koha::Exceptions::Patron::Message::Preference::NoTransportType
Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Created attachment 113687 [details] [review]
Bug 17499: (follow-up) Fix warning

Fix warning

DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.
SQL that returns multiple rows is DEPRECATED for ->find and ->single.

Sponsored-by: The National Library of Finland

Created attachment 113765 [details] [review]
Bug 17499: (follow-up) More explicit exceptions

Adds and throws more detailed exceptions.

This is useful for APIs and generating translatable errors in GUI.

In short, replaces Koha::Exceptions::BadParameter with:

Koha::Exceptions::Patron::NotFound
Koha::Exceptions::Patron::Category
Koha::Exceptions::Patron::Category::NotFound
Koha::Exceptions::Patron::Message::Preference
Koha::Exceptions::Patron::Message::Preference::AttributeNotFound
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestRequired
Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired
Koha::Exceptions::Patron::Message::Preference::NoTransportType
Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Created attachment 113766 [details] [review]
Bug 17499: (follow-up) Fix warning

Fix warning

DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.
SQL that returns multiple rows is DEPRECATED for ->find and ->single.

Sponsored-by: The National Library of Finland

There are some issues.

1. Considering class naming - These patches add "Koha::Patron::Message::Preference::*", but to avoid confusion between "message_queue" and "messages" table, would Koha::Notice::Preference better match the purpose?

As far as I understand, message_queue and messaging preferences are related, but "messages" is something different as it is only delivered in the web browser in staff client and/or OPAC.

As of now:
- message_queue       = Koha::Notice::Messages
- messages            = Koha::Patron::Message
- message preferences = Koha::Patron::Message::Preference

Right now this class naming makes it seem as if these preferences have something to do with "messages" when in fact it is only related to "message_queue".

I believe the work on this Bug overlapped Bug 17959 so we ended up with this confusion.

Still, notices vs messages is pretty confusing. It seems "Notice" is being used for the messages that end up in message_queue. Since notices are related to messaging preferences, should we call it notice preferences instead? I've switched this Bug into "In Discussion" in order to solve this problem first.

2. There is some logic in C4/Reserves.pm, at the end of _koha_notify_reserve() that should be centralized to avoid code duplication. In C4::Reserves::_koha_notify_reserve() we have:
> next if (
>         ( $mtt eq 'email' and not $to_address ) # No email address
>      or ( $mtt eq 'sms'   and not $patron->smsalertnumber ) # No SMS number
>      or ( $mtt eq 'itiva' and C4::Context->preference('TalkingTechItivaPhoneNotification') ) # Notice is handled by TalkingTech_itiva_outbound.pl
>      or ( $mtt eq 'phone' and not $patron->phone ) # No phone number to call
> );

What this Bug currently adds is that messaging preferences can not be saved at all if the above check fails. The reason behind this was from our production experience - there were people constantly asking about undelivered notices even when the mtt was selected. Bug 18595 together with this tackle the issue with proper UI error messages.

I propose we centralize this logic into the preference class under a new method "mtt_deliverable($mtt[, $borrowernumber])".

3. Current patches do not check if "phone" and "itiva" can be used as message transport types. Those mtts are ignored here and in Bug 18595 altogether. I'll work on it.

Created attachment 113767 [details] [review]
Bug 17499: (follow-up) Validate phone and itiva transport types

Message transport types 'phone' and 'itiva' were not validated unlike
'email' and 'sms'. This patch adds equivalent checks to the missing mtts.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Created attachment 113768 [details] [review]
Bug 17499: (follow-up) Contact information vs. mtt validation

This patch adds a subroutine that handles message transport type validation by
first checking related patron contact information.

As an example, if there is no email address, we shouldn't let email be selected
as a messaging transport type.

The reason to isolate logic into a separate subroutine is that C4/Reserves.pm
_koha_notify_reserve() also uses the same logic and we should not duplicate it.
C4::Reserves::_koha_notify_reserve() will be adjusted to use this new sub in
Bug 18595.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

(In reply to Lari Taskula from comment #107)
> 1. Considering class naming - These patches add
> "Koha::Patron::Message::Preference::*", but to avoid confusion between
> "message_queue" and "messages" table, would Koha::Notice::Preference better
> match the purpose?
> 
> As far as I understand, message_queue and messaging preferences are related,
> but "messages" is something different as it is only delivered in the web
> browser in staff client and/or OPAC.
> 
> As of now:
> - message_queue       = Koha::Notice::Messages
> - messages            = Koha::Patron::Message
> - message preferences = Koha::Patron::Message::Preference
> 
> Right now this class naming makes it seem as if these preferences have
> something to do with "messages" when in fact it is only related to
> "message_queue".
> 
> I believe the work on this Bug overlapped Bug 17959 so we ended up with this
> confusion.
> 
> Still, notices vs messages is pretty confusing. It seems "Notice" is being
> used for the messages that end up in message_queue. Since notices are
> related to messaging preferences, should we call it notice preferences
> instead? I've switched this Bug into "In Discussion" in order to solve this
> problem first.

We use messaging preferences in the GUI so I would not switch to notice preference.

I see these message preferences as their own object tied to a patron (not to a patron's individual message). This would mean the name would be Koha::Patron::MessagePreference and Koha::Patron::MessagePreferences. Then this message preference is combination of a Attribute and Transports. 

Important thing to note, reading the code C4::Members::Messaging::GetMessagingPreferences it appears that one MessagePreference would correspond to one message_attribute. As far as I can tell the table message_attributes is incorrectly named / created, the columns should be in my opinion in the borrower_message_preferences table. So the final structure could be 

Koha::Patron::MessagePreference             (contains both borrower_message_preferences and message_attributes with JOIN)
Koha::Patron::MessagePreferences            (contains MessagePreference)
Koha::Patron::MessagePreference::Transport  (message_transports)
Koha::Patron::MessagePreference::Transports (contains all Transport of a MessagePreference)

The message_transports seems to contains also unrelated columns to transport types, namely the is_digest information which seems to be preference of the patron instead of something that is decided based on the transportation medium. But maybe you can fit this somehow nicely anyway? If it seems impossible we could restructure first the messaging preference related tables.

Also see comment #80 from Jonathan, I think it the $patron->messaging_preferences suggestion is a bit like what I have here but instead with an object. Using object allows now modifying all the message preference fields easily so I would go with the object.

(In reply to Joonas Kylmälä from comment #110)
> I see these message preferences as their own object tied to a patron (not to
> a patron's individual message). This would mean the name would be Koha::Patron::MessagePreference and Koha::Patron::MessagePreferences.

Good point. That makes sense. Let's go with that.

> As far as I can
> tell the table message_attributes is incorrectly named / created, the
> columns should be in my opinion in the borrower_message_preferences table.
> ...
> Koha::Patron::MessagePreference             (contains both
> borrower_message_preferences and message_attributes with JOIN)

This is basically what the current patches do, but without a join. The Message::Preference object has an accessor called "message_name" even though it's not physically part of borrower_message_preferences table.

JOIN sounds interesting. But doesn't DBIx require you to use "table.column" key format when referencing to a column of a joined table? That's not very user friendly. Would it work with DBIx's "+as" option? Then we could get rid of the "find_with_message_name" method pointed out in comment #80 by Jonathan. I wonder if it's possible to define the JOIN in DBIx schema file so that we don't have to override search/find/whatever methods manually?

While there could be improvement at database level, imo that's outside of this Bug's scope. At database level, I don't think they should be merged either. We need message_attribute for database normalization.

borrower_message_preferences could also be improved by splitting it in two; borrower_message_preferences and category_message_preferences (the default category preferences). Anyways I think it's outside of the scope here.

> The message_transports seems to contains also unrelated columns to transport
> types, namely the is_digest information which seems to be preference of the
> patron instead of something that is decided based on the transportation
> medium. But maybe you can fit this somehow nicely anyway? If it seems
> impossible we could restructure first the messaging preference related
> tables.

"message_options" would better describe its current data. It should also have a primary key integer. Then borrower_message_preferences columns "message_attribute_id" and "wants_digest" could be replaced with a foreign key to "message_option_id". So, borrower_message_preference would simply be a reference to a borrowernumber and message_option_id (and additionally having the days_in_advance setting).

But your suggestion works as well. It's just a matter of selecting where the digest information should be at.

> Also see comment #80 from Jonathan, I think it the
> $patron->messaging_preferences suggestion is a bit like what I have here but
> instead with an object.

We can implement the Koha::Patron accessors from comment #80, I've nothing against that. But $patron->messaging_preferences would be an object (of MessagingPreferences class), I'm not sure what you mean.

Created attachment 118452 [details] [review]
Bug 17499: (follow-up) Rename to Koha::Patron::MessagePreference

Koha::Patron::Message refers to "messages" table that is used for circulation
messages. MessagePreference now refers to a messaging preference that is used
for enqueued notifications.

To test:
1. prove t/db_dependent/Koha/Patron/MessagePreference/Attributes.t
2. prove t/db_dependent/Koha/Patron/MessagePreference/Transport/Preferences.t
3. prove t/db_dependent/Koha/Patron/MessagePreference/Transport/Types.t
4. prove t/db_dependent/Koha/Patron/MessagePreference/Transports.t
5. prove t/db_dependent/Koha/Patron/MessagePreferences.t

Sponsored-by: The National Library of Finland

Created attachment 118453 [details] [review]
Bug 17499: (follow-up) Add $patron->messaging_preferences accessor

To test:
1. prove t/db_dependent/Koha/Patron.t

Sponsored-by: The National Library of Finland

Hi Lari,
can you update this patch on top of master?

Thank you.

Created attachment 119470 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Signed-off-by: Dominic Pichette <dominic@inlibro.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 119471 [details] [review]
Bug 17499: Define koha_object(s)_class where needed

This patch adds koha_object(s)_class for needed modules
to fix issues with tests.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 119472 [details] [review]
Bug 17499: Replace test subroutines with build_object

This patch replaces subroutines used to build test
objects with call to $builder->build_object.

To test prove that tests are still running with:
prove t/db_dependent/Koha/Patron/Message/*

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 119473 [details] [review]
Bug 17499: (QA follow-up) Fix Licence Statements

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 119474 [details] [review]
Bug 17499: (QA follow-up) Fix POD errors

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 119475 [details] [review]
Bug 17499: (follow-up) More explicit exceptions

Adds and throws more detailed exceptions.

This is useful for APIs and generating translatable errors in GUI.

In short, replaces Koha::Exceptions::BadParameter with:

Koha::Exceptions::Patron::NotFound
Koha::Exceptions::Patron::Category
Koha::Exceptions::Patron::Category::NotFound
Koha::Exceptions::Patron::Message::Preference
Koha::Exceptions::Patron::Message::Preference::AttributeNotFound
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestRequired
Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired
Koha::Exceptions::Patron::Message::Preference::NoTransportType
Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Created attachment 119476 [details] [review]
Bug 17499: (follow-up) Fix warning

Fix warning

DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.
SQL that returns multiple rows is DEPRECATED for ->find and ->single.

Sponsored-by: The National Library of Finland

Created attachment 119477 [details] [review]
Bug 17499: (follow-up) Validate phone and itiva transport types

Message transport types 'phone' and 'itiva' were not validated unlike
'email' and 'sms'. This patch adds equivalent checks to the missing mtts.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Created attachment 119478 [details] [review]
Bug 17499: (follow-up) Contact information vs. mtt validation

This patch adds a subroutine that handles message transport type validation by
first checking related patron contact information.

As an example, if there is no email address, we shouldn't let email be selected
as a messaging transport type.

The reason to isolate logic into a separate subroutine is that C4/Reserves.pm
_koha_notify_reserve() also uses the same logic and we should not duplicate it.
C4::Reserves::_koha_notify_reserve() will be adjusted to use this new sub in
Bug 18595.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Created attachment 119479 [details] [review]
Bug 17499: (follow-up) Rename to Koha::Patron::MessagePreference

Koha::Patron::Message refers to "messages" table that is used for circulation
messages. MessagePreference now refers to a messaging preference that is used
for enqueued notifications.

To test:
1. prove t/db_dependent/Koha/Patron/MessagePreference/Attributes.t
2. prove t/db_dependent/Koha/Patron/MessagePreference/Transport/Preferences.t
3. prove t/db_dependent/Koha/Patron/MessagePreference/Transport/Types.t
4. prove t/db_dependent/Koha/Patron/MessagePreference/Transports.t
5. prove t/db_dependent/Koha/Patron/MessagePreferences.t

Sponsored-by: The National Library of Finland

Created attachment 119480 [details] [review]
Bug 17499: (follow-up) Add $patron->messaging_preferences accessor

To test:
1. prove t/db_dependent/Koha/Patron.t

Sponsored-by: The National Library of Finland

Created attachment 119481 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Signed-off-by: Dominic Pichette <dominic@inlibro.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 119482 [details] [review]
Bug 17499: Define koha_object(s)_class where needed

This patch adds koha_object(s)_class for needed modules
to fix issues with tests.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 119483 [details] [review]
Bug 17499: Replace test subroutines with build_object

This patch replaces subroutines used to build test
objects with call to $builder->build_object.

To test prove that tests are still running with:
prove t/db_dependent/Koha/Patron/Message/*

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 119484 [details] [review]
Bug 17499: (QA follow-up) Fix Licence Statements

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 119485 [details] [review]
Bug 17499: (QA follow-up) Fix POD errors

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 119486 [details] [review]
Bug 17499: (follow-up) More explicit exceptions

Adds and throws more detailed exceptions.

This is useful for APIs and generating translatable errors in GUI.

In short, replaces Koha::Exceptions::BadParameter with:

Koha::Exceptions::Patron::NotFound
Koha::Exceptions::Patron::Category
Koha::Exceptions::Patron::Category::NotFound
Koha::Exceptions::Patron::Message::Preference
Koha::Exceptions::Patron::Message::Preference::AttributeNotFound
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestRequired
Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired
Koha::Exceptions::Patron::Message::Preference::NoTransportType
Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 119487 [details] [review]
Bug 17499: (follow-up) Fix warning

Fix warning

DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.
SQL that returns multiple rows is DEPRECATED for ->find and ->single.

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 119488 [details] [review]
Bug 17499: (follow-up) Validate phone and itiva transport types

Message transport types 'phone' and 'itiva' were not validated unlike
'email' and 'sms'. This patch adds equivalent checks to the missing mtts.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 119489 [details] [review]
Bug 17499: (follow-up) Contact information vs. mtt validation

This patch adds a subroutine that handles message transport type validation by
first checking related patron contact information.

As an example, if there is no email address, we shouldn't let email be selected
as a messaging transport type.

The reason to isolate logic into a separate subroutine is that C4/Reserves.pm
_koha_notify_reserve() also uses the same logic and we should not duplicate it.
C4::Reserves::_koha_notify_reserve() will be adjusted to use this new sub in
Bug 18595.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 119490 [details] [review]
Bug 17499: (follow-up) Rename to Koha::Patron::MessagePreference

Koha::Patron::Message refers to "messages" table that is used for circulation
messages. MessagePreference now refers to a messaging preference that is used
for enqueued notifications.

To test:
1. prove t/db_dependent/Koha/Patron/MessagePreference/Attributes.t
2. prove t/db_dependent/Koha/Patron/MessagePreference/Transport/Preferences.t
3. prove t/db_dependent/Koha/Patron/MessagePreference/Transport/Types.t
4. prove t/db_dependent/Koha/Patron/MessagePreference/Transports.t
5. prove t/db_dependent/Koha/Patron/MessagePreferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 119491 [details] [review]
Bug 17499: (follow-up) Add $patron->messaging_preferences accessor

To test:
1. prove t/db_dependent/Koha/Patron.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Applying: Bug 17499: (follow-up) Add $patron->messaging_preferences accessor
error: sha1 information is lacking or useless (Koha/Patron.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 17499: (follow-up) Add $patron->messaging_preferences accessor

Created attachment 141408 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Signed-off-by: Dominic Pichette <dominic@inlibro.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 141409 [details] [review]
Bug 17499: Define koha_object(s)_class where needed

This patch adds koha_object(s)_class for needed modules
to fix issues with tests.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 141410 [details] [review]
Bug 17499: Replace test subroutines with build_object

This patch replaces subroutines used to build test
objects with call to $builder->build_object.

To test prove that tests are still running with:
prove t/db_dependent/Koha/Patron/Message/*

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 141411 [details] [review]
Bug 17499: (QA follow-up) Fix Licence Statements

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 141412 [details] [review]
Bug 17499: (QA follow-up) Fix POD errors

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 141413 [details] [review]
Bug 17499: (follow-up) More explicit exceptions

Adds and throws more detailed exceptions.

This is useful for APIs and generating translatable errors in GUI.

In short, replaces Koha::Exceptions::BadParameter with:

Koha::Exceptions::Patron::NotFound
Koha::Exceptions::Patron::Category
Koha::Exceptions::Patron::Category::NotFound
Koha::Exceptions::Patron::Message::Preference
Koha::Exceptions::Patron::Message::Preference::AttributeNotFound
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestRequired
Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired
Koha::Exceptions::Patron::Message::Preference::NoTransportType
Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 141414 [details] [review]
Bug 17499: (follow-up) Fix warning

Fix warning

DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.
SQL that returns multiple rows is DEPRECATED for ->find and ->single.

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 141415 [details] [review]
Bug 17499: (follow-up) Validate phone and itiva transport types

Message transport types 'phone' and 'itiva' were not validated unlike
'email' and 'sms'. This patch adds equivalent checks to the missing mtts.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 141416 [details] [review]
Bug 17499: (follow-up) Contact information vs. mtt validation

This patch adds a subroutine that handles message transport type validation by
first checking related patron contact information.

As an example, if there is no email address, we shouldn't let email be selected
as a messaging transport type.

The reason to isolate logic into a separate subroutine is that C4/Reserves.pm
_koha_notify_reserve() also uses the same logic and we should not duplicate it.
C4::Reserves::_koha_notify_reserve() will be adjusted to use this new sub in
Bug 18595.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 141417 [details] [review]
Bug 17499: (follow-up) Rename to Koha::Patron::MessagePreference

Koha::Patron::Message refers to "messages" table that is used for circulation
messages. MessagePreference now refers to a messaging preference that is used
for enqueued notifications.

To test:
1. prove t/db_dependent/Koha/Patron/MessagePreference/Attributes.t
2. prove t/db_dependent/Koha/Patron/MessagePreference/Transport/Preferences.t
3. prove t/db_dependent/Koha/Patron/MessagePreference/Transport/Types.t
4. prove t/db_dependent/Koha/Patron/MessagePreference/Transports.t
5. prove t/db_dependent/Koha/Patron/MessagePreferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 141418 [details] [review]
Bug 17499: (follow-up) Add $patron->messaging_preferences accessor

To test:
1. prove t/db_dependent/Koha/Patron.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

It seems Koha::MessageAttribute(s) classes were introduced before this Bug was able to move forward. As current patch set adds equivalent classes named Koha::Patron::Message::Attribute(s), they have to be removed. Also, Koha::Patron::Message::Transport(s) and Koha::Patron::Message::Transport::Type(s) should be renamed to match naming convention of Koha::MessageAttribute(s).

I'll start fixing these.

I actually prefer your original approach if I'm honest.. it doesn't look to me like MessageAttribute has been much yet.. perhaps we could go the other way and keep the work here and update core to match?

tl;dr I like Koha::Patron::Message::Attribute(s) better than Koha::MessageAttribute(s).

I'll ask Tomas to weight in.

(In reply to Martin Renvoize from comment #150)
> I actually prefer your original approach if I'm honest.. it doesn't look to
> me like MessageAttribute has been much yet.. perhaps we could go the other
> way and keep the work here and update core to match?
> 
> tl;dr I like Koha::Patron::Message::Attribute(s) better than
> Koha::MessageAttribute(s).
> 
> I'll ask Tomas to weight in.

Has there been any decision on this?

Any update ?

Koha::MessageAttribute(s) is not used anywhere (except in a test file: t/db_dependent/Illrequests.t). It should be easy to replace it with Koha::Patron::Message::Attribute(s).

Created attachment 146529 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Signed-off-by: Dominic Pichette <dominic@inlibro.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 146530 [details] [review]
Bug 17499: Define koha_object(s)_class where needed

This patch adds koha_object(s)_class for needed modules
to fix issues with tests.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 146531 [details] [review]
Bug 17499: Replace test subroutines with build_object

This patch replaces subroutines used to build test
objects with call to $builder->build_object.

To test prove that tests are still running with:
prove t/db_dependent/Koha/Patron/Message/*

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 146532 [details] [review]
Bug 17499: (QA follow-up) Fix Licence Statements

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 146533 [details] [review]
Bug 17499: (QA follow-up) Fix POD errors

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 146534 [details] [review]
Bug 17499: (follow-up) More explicit exceptions

Adds and throws more detailed exceptions.

This is useful for APIs and generating translatable errors in GUI.

In short, replaces Koha::Exceptions::BadParameter with:

Koha::Exceptions::Patron::NotFound
Koha::Exceptions::Patron::Category
Koha::Exceptions::Patron::Category::NotFound
Koha::Exceptions::Patron::Message::Preference
Koha::Exceptions::Patron::Message::Preference::AttributeNotFound
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestRequired
Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired
Koha::Exceptions::Patron::Message::Preference::NoTransportType
Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 146535 [details] [review]
Bug 17499: (follow-up) Fix warning

Fix warning

DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.
SQL that returns multiple rows is DEPRECATED for ->find and ->single.

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 146536 [details] [review]
Bug 17499: (follow-up) Validate phone and itiva transport types

Message transport types 'phone' and 'itiva' were not validated unlike
'email' and 'sms'. This patch adds equivalent checks to the missing mtts.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 146537 [details] [review]
Bug 17499: (follow-up) Contact information vs. mtt validation

This patch adds a subroutine that handles message transport type validation by
first checking related patron contact information.

As an example, if there is no email address, we shouldn't let email be selected
as a messaging transport type.

The reason to isolate logic into a separate subroutine is that C4/Reserves.pm
_koha_notify_reserve() also uses the same logic and we should not duplicate it.
C4::Reserves::_koha_notify_reserve() will be adjusted to use this new sub in
Bug 18595.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 146538 [details] [review]
Bug 17499: (follow-up) Rename to Koha::Patron::MessagePreference

Koha::Patron::Message refers to "messages" table that is used for circulation
messages. MessagePreference now refers to a messaging preference that is used
for enqueued notifications.

To test:
1. prove t/db_dependent/Koha/Patron/MessagePreference/Attributes.t
2. prove t/db_dependent/Koha/Patron/MessagePreference/Transport/Preferences.t
3. prove t/db_dependent/Koha/Patron/MessagePreference/Transport/Types.t
4. prove t/db_dependent/Koha/Patron/MessagePreference/Transports.t
5. prove t/db_dependent/Koha/Patron/MessagePreferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 146539 [details] [review]
Bug 17499: (follow-up) Add $patron->messaging_preferences accessor

To test:
1. prove t/db_dependent/Koha/Patron.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 146540 [details] [review]
Bug 17499: Replace Koha::MessageAttribute(s) with Koha::Patron::MessagePreference::Attribute(s)

To test:
1. prove t/db_dependent/Illrequests.t
2. grep -rn 'Koha::MessageAttribute', observe no results in Koha code

(In reply to Julian Maurice from comment #152)
> Any update ?
> 
> Koha::MessageAttribute(s) is not used anywhere (except in a test file:
> t/db_dependent/Illrequests.t). It should be easy to replace it with
> Koha::Patron::Message::Attribute(s).

Attached a patch that makes the replacement

Patch no longer applies:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 17499: Add Koha-objects for messaging preferences
Applying: Bug 17499: Define koha_object(s)_class where needed
Applying: Bug 17499: Replace test subroutines with build_object
Applying: Bug 17499: (QA follow-up) Fix Licence Statements
Applying: Bug 17499: (QA follow-up) Fix POD errors
Applying: Bug 17499: (follow-up) More explicit exceptions
Applying: Bug 17499: (follow-up) Fix warning
Applying: Bug 17499: (follow-up) Validate phone and itiva transport types
Applying: Bug 17499: (follow-up) Contact information vs. mtt validation
Applying: Bug 17499: (follow-up) Rename to Koha::Patron::MessagePreference
Applying: Bug 17499: (follow-up) Add $patron->messaging_preferences accessor
error: sha1 information is lacking or useless (Koha/Patron.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 17499: (follow-up) Add $patron->messaging_preferences accessor

Created attachment 151900 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Signed-off-by: Dominic Pichette <dominic@inlibro.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 151901 [details] [review]
Bug 17499: Define koha_object(s)_class where needed

This patch adds koha_object(s)_class for needed modules
to fix issues with tests.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 151902 [details] [review]
Bug 17499: Replace test subroutines with build_object

This patch replaces subroutines used to build test
objects with call to $builder->build_object.

To test prove that tests are still running with:
prove t/db_dependent/Koha/Patron/Message/*

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 151903 [details] [review]
Bug 17499: (QA follow-up) Fix Licence Statements

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 151904 [details] [review]
Bug 17499: (QA follow-up) Fix POD errors

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 151905 [details] [review]
Bug 17499: (follow-up) More explicit exceptions

Adds and throws more detailed exceptions.

This is useful for APIs and generating translatable errors in GUI.

In short, replaces Koha::Exceptions::BadParameter with:

Koha::Exceptions::Patron::NotFound
Koha::Exceptions::Patron::Category
Koha::Exceptions::Patron::Category::NotFound
Koha::Exceptions::Patron::Message::Preference
Koha::Exceptions::Patron::Message::Preference::AttributeNotFound
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestRequired
Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired
Koha::Exceptions::Patron::Message::Preference::NoTransportType
Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 151906 [details] [review]
Bug 17499: (follow-up) Fix warning

Fix warning

DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.
SQL that returns multiple rows is DEPRECATED for ->find and ->single.

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 151907 [details] [review]
Bug 17499: (follow-up) Validate phone and itiva transport types

Message transport types 'phone' and 'itiva' were not validated unlike
'email' and 'sms'. This patch adds equivalent checks to the missing mtts.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 151908 [details] [review]
Bug 17499: (follow-up) Contact information vs. mtt validation

This patch adds a subroutine that handles message transport type validation by
first checking related patron contact information.

As an example, if there is no email address, we shouldn't let email be selected
as a messaging transport type.

The reason to isolate logic into a separate subroutine is that C4/Reserves.pm
_koha_notify_reserve() also uses the same logic and we should not duplicate it.
C4::Reserves::_koha_notify_reserve() will be adjusted to use this new sub in
Bug 18595.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 151909 [details] [review]
Bug 17499: (follow-up) Rename to Koha::Patron::MessagePreference

Koha::Patron::Message refers to "messages" table that is used for circulation
messages. MessagePreference now refers to a messaging preference that is used
for enqueued notifications.

To test:
1. prove t/db_dependent/Koha/Patron/MessagePreference/Attributes.t
2. prove t/db_dependent/Koha/Patron/MessagePreference/Transport/Preferences.t
3. prove t/db_dependent/Koha/Patron/MessagePreference/Transport/Types.t
4. prove t/db_dependent/Koha/Patron/MessagePreference/Transports.t
5. prove t/db_dependent/Koha/Patron/MessagePreferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 151910 [details] [review]
Bug 17499: (follow-up) Add $patron->messaging_preferences accessor

To test:
1. prove t/db_dependent/Koha/Patron.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 151911 [details] [review]
Bug 17499: Replace Koha::MessageAttribute(s) with Koha::Patron::MessagePreference::Attribute(s)

To test:
1. prove t/db_dependent/Illrequests.t
2. grep -rn 'Koha::MessageAttribute', observe no results in Koha code

Patches rebased on master

Created attachment 151937 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Signed-off-by: Dominic Pichette <dominic@inlibro.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Created attachment 151938 [details] [review]
Bug 17499: Define koha_object(s)_class where needed

This patch adds koha_object(s)_class for needed modules
to fix issues with tests.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Created attachment 151939 [details] [review]
Bug 17499: Replace test subroutines with build_object

This patch replaces subroutines used to build test
objects with call to $builder->build_object.

To test prove that tests are still running with:
prove t/db_dependent/Koha/Patron/Message/*

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Created attachment 151940 [details] [review]
Bug 17499: (QA follow-up) Fix Licence Statements

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Created attachment 151941 [details] [review]
Bug 17499: (QA follow-up) Fix POD errors

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Created attachment 151942 [details] [review]
Bug 17499: (follow-up) More explicit exceptions

Adds and throws more detailed exceptions.

This is useful for APIs and generating translatable errors in GUI.

In short, replaces Koha::Exceptions::BadParameter with:

Koha::Exceptions::Patron::NotFound
Koha::Exceptions::Patron::Category
Koha::Exceptions::Patron::Category::NotFound
Koha::Exceptions::Patron::Message::Preference
Koha::Exceptions::Patron::Message::Preference::AttributeNotFound
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestRequired
Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired
Koha::Exceptions::Patron::Message::Preference::NoTransportType
Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Created attachment 151943 [details] [review]
Bug 17499: (follow-up) Fix warning

Fix warning

DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.
SQL that returns multiple rows is DEPRECATED for ->find and ->single.

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Created attachment 151944 [details] [review]
Bug 17499: (follow-up) Validate phone and itiva transport types

Message transport types 'phone' and 'itiva' were not validated unlike
'email' and 'sms'. This patch adds equivalent checks to the missing mtts.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Created attachment 151945 [details] [review]
Bug 17499: (follow-up) Contact information vs. mtt validation

This patch adds a subroutine that handles message transport type validation by
first checking related patron contact information.

As an example, if there is no email address, we shouldn't let email be selected
as a messaging transport type.

The reason to isolate logic into a separate subroutine is that C4/Reserves.pm
_koha_notify_reserve() also uses the same logic and we should not duplicate it.
C4::Reserves::_koha_notify_reserve() will be adjusted to use this new sub in
Bug 18595.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Created attachment 151946 [details] [review]
Bug 17499: (follow-up) Rename to Koha::Patron::MessagePreference

Koha::Patron::Message refers to "messages" table that is used for circulation
messages. MessagePreference now refers to a messaging preference that is used
for enqueued notifications.

To test:
1. prove t/db_dependent/Koha/Patron/MessagePreference/Attributes.t
2. prove t/db_dependent/Koha/Patron/MessagePreference/Transport/Preferences.t
3. prove t/db_dependent/Koha/Patron/MessagePreference/Transport/Types.t
4. prove t/db_dependent/Koha/Patron/MessagePreference/Transports.t
5. prove t/db_dependent/Koha/Patron/MessagePreferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Created attachment 151947 [details] [review]
Bug 17499: (follow-up) Add $patron->messaging_preferences accessor

To test:
1. prove t/db_dependent/Koha/Patron.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Created attachment 151948 [details] [review]
Bug 17499: Replace Koha::MessageAttribute(s) with Koha::Patron::MessagePreference::Attribute(s)

To test:
1. prove t/db_dependent/Illrequests.t
2. grep -rn 'Koha::MessageAttribute', observe no results in Koha code

Signed-off-by: Sam Lau <samalau@gmail.com>

QA: Looking here

Created attachment 155365 [details] [review]
Bug 17499: Add Koha-objects for messaging preferences

This patch adds Koha-objects for messaging preferences.

Adds simple validation for messaging preferences.

The validation includes
- throw exception if both borrowernumber or categorycode is given for a new pref
- throw exception if patron for the given borrowernumber is not found
- throw exception if category for the given categorycode is not found
- throw exception if days in advance cannot be configured but is given
- throw exception if days in advance configuration is invalid (value between 0-30)
- throw exception if digest is not available but attempted to set on
- throw exception if digest must be enabled but attempted to set off
- throw exception on duplicate messaging preference

Adds a method for getting available messaging options.

Adds a method for setting default messaging preferenes.
  $patron->set_default_messaging_preferences (where $patron is a Koha::Patron)
  ...or...
  Koha::Patron::Message::Preference->new_from_default({
    borrowernumber => 123,
    categorycode => "ABC",
    message_attribute_id => 1,
  });

Since messaging preference is a feature that has multiple related database tables,
usage via Koha-objects is sometimes frustrating. This patch adds easy access to
message transport types via
  $preference->message_transport_types                              (for getting)
  $preference->set({ message_transport_types => ['email', 'sms'] }) (for setting)
  (also supports other calling conventions, see documentation for more)

Adds optional parameter message_name for Koha::Patron::Message::Preferences->find
and ->search. Simplifies the Koha-object usage by allowing developer to skip joins
and / or querying the message name via attribute_id from message_attributes table.

Includes test coverage for basic usage.

To test:
1. prove t/db_dependent/Koha/Patron/Message/*

Following Bug 17499, check also Bug 18595 that replaces C4::Members::Messaging
with these new Koha-objects.

Signed-off-by: Dominic Pichette <dominic@inlibro.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 155366 [details] [review]
Bug 17499: Define koha_object(s)_class where needed

This patch adds koha_object(s)_class for needed modules
to fix issues with tests.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 155367 [details] [review]
Bug 17499: Replace test subroutines with build_object

This patch replaces subroutines used to build test
objects with call to $builder->build_object.

To test prove that tests are still running with:
prove t/db_dependent/Koha/Patron/Message/*

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 155368 [details] [review]
Bug 17499: (QA follow-up) Fix Licence Statements

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 155369 [details] [review]
Bug 17499: (QA follow-up) Fix POD errors

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 155370 [details] [review]
Bug 17499: (follow-up) More explicit exceptions

Adds and throws more detailed exceptions.

This is useful for APIs and generating translatable errors in GUI.

In short, replaces Koha::Exceptions::BadParameter with:

Koha::Exceptions::Patron::NotFound
Koha::Exceptions::Patron::Category
Koha::Exceptions::Patron::Category::NotFound
Koha::Exceptions::Patron::Message::Preference
Koha::Exceptions::Patron::Message::Preference::AttributeNotFound
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceOutOfRange
Koha::Exceptions::Patron::Message::Preference::DaysInAdvanceNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestNotAvailable
Koha::Exceptions::Patron::Message::Preference::DigestRequired
Koha::Exceptions::Patron::Message::Preference::EmailAddressRequired
Koha::Exceptions::Patron::Message::Preference::NoTransportType
Koha::Exceptions::Patron::Message::Preference::SMSNumberRequired

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 155371 [details] [review]
Bug 17499: (follow-up) Fix warning

Fix warning

DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.
SQL that returns multiple rows is DEPRECATED for ->find and ->single.

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 155372 [details] [review]
Bug 17499: (follow-up) Validate phone and itiva transport types

Message transport types 'phone' and 'itiva' were not validated unlike
'email' and 'sms'. This patch adds equivalent checks to the missing mtts.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 155373 [details] [review]
Bug 17499: (follow-up) Contact information vs. mtt validation

This patch adds a subroutine that handles message transport type validation by
first checking related patron contact information.

As an example, if there is no email address, we shouldn't let email be selected
as a messaging transport type.

The reason to isolate logic into a separate subroutine is that C4/Reserves.pm
_koha_notify_reserve() also uses the same logic and we should not duplicate it.
C4::Reserves::_koha_notify_reserve() will be adjusted to use this new sub in
Bug 18595.

To test:
1. prove t/db_dependent/Koha/Patron/Message/Preferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 155374 [details] [review]
Bug 17499: (follow-up) Rename to Koha::Patron::MessagePreference

Koha::Patron::Message refers to "messages" table that is used for circulation
messages. MessagePreference now refers to a messaging preference that is used
for enqueued notifications.

To test:
1. prove t/db_dependent/Koha/Patron/MessagePreference/Attributes.t
2. prove t/db_dependent/Koha/Patron/MessagePreference/Transport/Preferences.t
3. prove t/db_dependent/Koha/Patron/MessagePreference/Transport/Types.t
4. prove t/db_dependent/Koha/Patron/MessagePreference/Transports.t
5. prove t/db_dependent/Koha/Patron/MessagePreferences.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 155375 [details] [review]
Bug 17499: (follow-up) Add $patron->messaging_preferences accessor

To test:
1. prove t/db_dependent/Koha/Patron.t

Sponsored-by: The National Library of Finland

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 155376 [details] [review]
Bug 17499: Replace Koha::MessageAttribute(s) with Koha::Patron::MessagePreference::Attribute(s)

To test:
1. prove t/db_dependent/Illrequests.t
2. grep -rn 'Koha::MessageAttribute', observe no results in Koha code

Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Overall looks very good to me. Great job. And with so much patience :)

A few minor remarks (for the record, no blockers)
+=head3 new_from_default
+
+NOTE: This subroutine initializes and STORES the object (in order to set
+message transport types for the preference), so no need to call ->store when
+preferences are initialized via this method.

=> This might be a bit confusing. Do not store immediately? No blocker (waited too long, etc)

+=head3 find_with_message_name
+
+Koha::Patron::Message::Preferences->find_with_message_name({
+    borrowernumber => 123,
+    message_name => 'Hold_Filled',
+});

This should be a search instead of a find imo. No blocker.
Note that there is a search_with.. too ?

+=head2 Internal methods
Recently someone failed my patches for something like this. I wont do that here :)

QA tools mentions a few untidy lines. Please ignore them now in view of time passed.

The big question is however: Who sets the next step and applies this stuff in favor of C4 Messaging routines?

(In reply to Marcel de Rooy from comment #205)
> The big question is however: Who sets the next step and applies this stuff
> in favor of C4 Messaging routines?

Note that 18595 does not apply.

Pushed to master for 23.11.

Nice work everyone, thanks!

Enhancement not pushed to 23.05.x