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.
Created attachment 47091 [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
Created attachment 47092 [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.
Created attachment 47093 [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
Created attachment 47094 [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
Created attachment 47095 [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.
Created attachment 47096 [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.
Created attachment 47097 [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.
I suspect Bug 13618 has caused a conflict with these patches now.
Created attachment 47515 [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.
Created attachment 47516 [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.
This is not working for me. When I try to add a message on the circulation page it does not get saved.
Created attachment 47525 [details] [review] Bug 15632: Koha::Patron::Messages - (follow-up) Remove AddMessage
(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.
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; )
Created attachment 47571 [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
Created attachment 47572 [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.
Created attachment 47573 [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.
Created attachment 47574 [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.
Created attachment 47575 [details] [review] Bug 15632: Koha::Patron::Messages - (follow-up) Remove AddMessage
Created attachment 47580 [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>
Created attachment 47581 [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>
Created attachment 47582 [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>
Created attachment 47583 [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>
Created attachment 47584 [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.
Created attachment 47585 [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>
Created attachment 47586 [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>
Created attachment 47587 [details] [review] Bug 15632: Koha::Patron::Messages - (follow-up) Remove AddMessage Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 48004 [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> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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>
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>
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>
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>
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.
Created attachment 48014 [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>
Created attachment 48015 [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>
Created attachment 48016 [details] [review] Bug 15632: Koha::Patron::Messages - (follow-up) Remove AddMessage Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 48017 [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
Created attachment 48018 [details] [review] Bug 15632 [QA Followup] - Undo changes needed for Bug 13618 which was reverted
Created attachment 48020 [details] [review] Bug 15632 [QA Followup] - Change method type to _type for bug 15446
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>
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>
Created attachment 48424 [details] [review] Bug 15632: Koha::Patron::Messages - (follow-up) Remove AddMessage Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 48425 [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
Created attachment 48426 [details] [review] Bug 15632 [QA Followup] - Undo changes needed for Bug 13618 which was reverted
Created attachment 48427 [details] [review] Bug 15632 [QA Followup] - Change method type to _type for bug 15446
Patches rebased.
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
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
Created attachment 48592 [details] [review] Bug 15632 [QA Followup] - Undo changes needed for Bug 13618 which was reverted
Created attachment 48593 [details] [review] Bug 15632 [QA Followup] - Change method type to _type for bug 15446
Pushed to Master - Should be in the May 2016 Release. Thanks!