Bug 15632

Summary: Move the messages related code to Koha::Patron::Messages
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: brendan, kyle, veron
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15548
Change sponsored?: --- Patch complexity: Medium patch
Who signed the patch off: Text to go in the release notes:
Version(s) released in:
Bug Depends on: 15446    
Bug Blocks: 15449, 16125, 16145    
Attachments: Bug 15632: Koha::Patron::Messages - Add new classes
Bug 15632: Koha::Patron::Messages - Be sure add and delete will log
Bug 15632: Koha::Patron::Messages - Remove AddMessage
Bug 15632: Koha::Patron::Messages - Remove DeleteMessage
Bug 15632: Koha::Patron::Messages - Remove GetMessagesCount
Bug 15632: Koha::Patron::Messages - Remove GetMessages
Bug 15632: Koha::Patron::Messages - Remove tests
Bug 15632: Koha::Patron::Messages - Remove GetMessages
Bug 15632: Koha::Patron::Messages - Remove tests
Bug 15632: Koha::Patron::Messages - (follow-up) Remove AddMessage
Bug 15632: Koha::Patron::Messages - Remove DeleteMessage
Bug 15632: Koha::Patron::Messages - Remove GetMessagesCount
Bug 15632: Koha::Patron::Messages - Remove GetMessages
Bug 15632: Koha::Patron::Messages - Remove tests
Bug 15632: Koha::Patron::Messages - (follow-up) Remove AddMessage
Bug 15632: Koha::Patron::Messages - Add new classes
Bug 15632: Koha::Patron::Messages - Be sure add and delete will log
Bug 15632: Koha::Patron::Messages - Remove AddMessage
Bug 15632: Koha::Patron::Messages - Remove DeleteMessage
Bug 15632: Koha::Patron::Messages - Remove GetMessagesCount
Bug 15632: Koha::Patron::Messages - Remove GetMessages
Bug 15632: Koha::Patron::Messages - Remove tests
Bug 15632: Koha::Patron::Messages - (follow-up) Remove AddMessage
Bug 15632: Koha::Patron::Messages - Add new classes
Bug 15632: Koha::Patron::Messages - Add new classes
Bug 15632: Koha::Patron::Messages - Be sure add and delete will log
Bug 15632: Koha::Patron::Messages - Remove AddMessage
Bug 15632: Koha::Patron::Messages - Remove DeleteMessage
Bug 15632: Koha::Patron::Messages - Remove GetMessagesCount
Bug 15632: Koha::Patron::Messages - Remove GetMessages
Bug 15632: Koha::Patron::Messages - Remove tests
Bug 15632: Koha::Patron::Messages - (follow-up) Remove AddMessage
Bug 15632 [QA Followup] - Get rid of use of uninitialized value errors for unit tests
Bug 15632 [QA Followup] - Undo changes needed for Bug 13618 which was reverted
Bug 15632 [QA Followup] - Change method type to _type for bug 15446
Bug 15632: Koha::Patron::Messages - Remove GetMessages
Bug 15632: Koha::Patron::Messages - Remove tests
Bug 15632: Koha::Patron::Messages - (follow-up) Remove AddMessage
Bug 15632 [QA Followup] - Get rid of use of uninitialized value errors for unit tests
Bug 15632 [QA Followup] - Undo changes needed for Bug 13618 which was reverted
Bug 15632 [QA Followup] - Change method type to _type for bug 15446
Bug 15632 [QA Followup] - Get rid of use of uninitialized value errors for unit tests
Bug 15632 [QA Followup] - Undo changes needed for Bug 13618 which was reverted
Bug 15632 [QA Followup] - Change method type to _type for bug 15446

Description Jonathan Druart 2016-01-20 18:40:45 UTC
There are 4 subroutines in C4::Messages which could be moved to their own package:
- AddMessage
- DeleteMessage
- GetMessagesCount
- GetMessages

With Koha::Patron::Message[s] based on Koha::Object[s], it will be easy to move them.
Comment 1 Jonathan Druart 2016-01-21 09:35:06 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-01-21 09:35:16 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2016-01-21 09:35:22 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-01-21 09:35:26 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2016-01-21 09:35:32 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2016-01-21 09:35:37 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2016-01-21 09:35:44 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2016-02-01 15:12:51 UTC
I suspect Bug 13618 has caused a conflict with these patches now.
Comment 9 Jonathan Druart 2016-02-01 16:21:16 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2016-02-01 16:21:34 UTC Comment hidden (obsolete)
Comment 11 Owen Leonard 2016-02-01 16:58:43 UTC
This is not working for me. When I try to add a message on the circulation page it does not get saved.
Comment 12 Jonathan Druart 2016-02-01 17:20:45 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2016-02-01 17:22:05 UTC
(In reply to Owen Leonard from comment #11)
> This is not working for me. When I try to add a message on the circulation
> page it does not get saved.

Hum, indeed. Sorry about that.
Comment 14 Marc Véron 2016-02-02 18:27:45 UTC
I get an error in circ/del_message.pl line 26:

Can't locate Koha/Patrons/Messages.pm in @INC (@INC contains: /usr/share/kohaclone /etc/perl /usr/local/lib/perl/5.14.2 /usr/local/share/perl/5.14.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.14 /usr/share/perl/5.14 /usr/local/lib/site_perl .) at /usr/share/kohaclone/circ/del_message.pl line 26.
BEGIN failed--compilation aborted at /usr/share/kohaclone/circ/del_message.pl line 26.

The line should read 
use Koha::Patron::Messages; ( instead of use Koha::Patrons::Messages; )
Comment 15 Jonathan Druart 2016-02-03 09:24:57 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2016-02-03 09:25:03 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-02-03 09:25:09 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2016-02-03 09:25:15 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2016-02-03 09:25:19 UTC Comment hidden (obsolete)
Comment 20 Marc Véron 2016-02-03 12:42:44 UTC Comment hidden (obsolete)
Comment 21 Marc Véron 2016-02-03 12:43:51 UTC Comment hidden (obsolete)
Comment 22 Marc Véron 2016-02-03 12:44:53 UTC Comment hidden (obsolete)
Comment 23 Marc Véron 2016-02-03 12:45:47 UTC Comment hidden (obsolete)
Comment 24 Marc Véron 2016-02-03 12:46:27 UTC Comment hidden (obsolete)
Comment 25 Marc Véron 2016-02-03 12:47:17 UTC Comment hidden (obsolete)
Comment 26 Marc Véron 2016-02-03 12:48:15 UTC Comment hidden (obsolete)
Comment 27 Marc Véron 2016-02-03 12:49:13 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2016-02-12 18:54:28 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2016-02-12 19:05:10 UTC
Created attachment 48009 [details] [review]
Bug 15632: Koha::Patron::Messages - Add new classes

The following 4 CRUD subroutines in C4::Members:
- AddMessage
- DeleteMessage
- GetMessagesCount
- GetMessages

could be replaced with a new package based on Koha::Objets.
This patchset will add the 2 Koha::Patron::Message[s] classes, then use
it to replace the different calls to these subroutine.
It will slightly reduce the size of C4::Members

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 30 Kyle M Hall 2016-02-12 19:05:17 UTC
Created attachment 48010 [details] [review]
Bug 15632: Koha::Patron::Messages - Be sure add and delete will log

If the pref BorrowersLog is on, the store and delete method should log
into the action_logs table.
Easy to do by overloading them.

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 31 Kyle M Hall 2016-02-12 19:05:19 UTC
Created attachment 48011 [details] [review]
Bug 15632: Koha::Patron::Messages - Remove AddMessage

The AddMessage subroutine just
1. checked if all the parameters were passed
2. inserted the message in the DB
3. logged the action if needed

These 3 jobs are now done by the overloaded store method of
Koha::Patron::Message.

Test plan:
1/ Go on the "check out" page of a patron
2/ Add different new messages

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 32 Kyle M Hall 2016-02-12 19:05:22 UTC
Created attachment 48012 [details] [review]
Bug 15632: Koha::Patron::Messages - Remove DeleteMessage

The DeleteMessage just deleted and logged if needed.
This is now be done by Koha::Patron::Message->delete.

Test plan:
1/ Go on the "check out" page of a patron
2/ Delete some messages

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 33 Kyle M Hall 2016-02-12 19:05:24 UTC
Created attachment 48013 [details] [review]
Bug 15632: Koha::Patron::Messages - Remove GetMessagesCount

The GetMessageCount subroutine was only used once, in opac-user.pl, to
know if some messages will be displayed.

Test plan:
1/ Create messages to display at the OPAC for a patron
2/ Logged this patron in at the OPAC, you should see the messages
displayed.
Comment 34 Kyle M Hall 2016-02-12 19:05:27 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2016-02-12 19:05:30 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2016-02-12 19:05:32 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2016-02-12 19:05:35 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2016-02-12 19:05:37 UTC Comment hidden (obsolete)
Comment 39 Kyle M Hall 2016-02-12 19:14:02 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2016-02-29 07:17:22 UTC
Created attachment 48422 [details] [review]
Bug 15632: Koha::Patron::Messages - Remove GetMessages

This subroutine just retrieved the messages given some parameters.
Some job should not have been done in this subroutine.
It was called only 3 times, in circ/circulation.pl and opac-user.pl.
Basically it was used to retrieved the message to displaye for a given
patron ($borrowernumber) at the OPAC (B) or Staff (L).

For the 3 calls, the 2 parameters $borrowernumber and $type
(message_type) were passed, the "%" trick at the beginning of the
subroutine was useless.
Moreover, the date formatting should be done on the TT side, not in
subroutine.
The can_delete flag was set if the branchcode given in parameter was the
same as the one of the message. This has been delegated to the template.
Indeed the can_delete was not valid, since it must depend on the
AllowAllMessageDeletion pref.
The test is now:
  IF message.branchcode == branch OR
  Koha.Preference('AllowAllMessageDeletion'')

There is not specific test plan for this patch, the changes have already
been tested in previous patches.

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 41 Jonathan Druart 2016-02-29 07:17:27 UTC
Created attachment 48423 [details] [review]
Bug 15632: Koha::Patron::Messages - Remove tests

The messages related tests in t/db_dependent/Members.t are not
deprecated and can be removed.

Test plan:
  git grep AddMessage
  git grep DeleteMessage
  git grep GetMessagesCount
  git grep GetMessages
should not return any result.

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 42 Jonathan Druart 2016-02-29 07:17:32 UTC
Created attachment 48424 [details] [review]
Bug 15632: Koha::Patron::Messages - (follow-up) Remove AddMessage

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 43 Jonathan Druart 2016-02-29 07:17:38 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2016-02-29 07:17:44 UTC Comment hidden (obsolete)
Comment 45 Jonathan Druart 2016-02-29 07:17:49 UTC Comment hidden (obsolete)
Comment 46 Jonathan Druart 2016-02-29 07:18:21 UTC
Patches rebased.
Comment 47 Brendan Gallagher 2016-03-02 22:43:28 UTC
Didn't apply cleanly - I know because of the moving target that master is.  Please rebase.  Once it's done - leave me an @later in #koha and I'll jump right on it.  Thank you so much for the time and bother of rebasing.

Brendan
Comment 48 Jonathan Druart 2016-03-03 08:29:12 UTC
Created attachment 48591 [details] [review]
Bug 15632 [QA Followup] - Get rid of use of uninitialized value errors for unit tests

* Set $user to "" if not passed
* Tidy sub
Comment 49 Jonathan Druart 2016-03-03 08:29:33 UTC
Created attachment 48592 [details] [review]
Bug 15632 [QA Followup] - Undo changes needed for Bug 13618 which was reverted
Comment 50 Jonathan Druart 2016-03-03 08:29:37 UTC
Created attachment 48593 [details] [review]
Bug 15632 [QA Followup] - Change method type to _type for bug 15446
Comment 51 Brendan Gallagher 2016-03-03 21:24:11 UTC
Pushed to Master - Should be in the May 2016 Release.  Thanks!