Bug 37869

Created attachment 171217 [details] [review]
Bug 37869: Move plugin hook before_send_messages to SendQueuedMessages

Most messages in Koha are handled by process_message_queue.pl. This script calls before_send_messages which calls the plugin hook before_send_messages. This works for nearly all messages, but some messages like WELCOME notices are triggered immediately and do not way to for process_message_queue.pl to run. We should move the triggering of before_send_messages into SendQueuedMessages so it is always handled.

Test Plan:
1) Install version 2.5.1 of the kitchen sink plugin
2) Run process_messsage_queue.pl and also trigger a WELCOME notice
3) Note the "Plugin hook before_send_message called with the params" message in the logs for the former but not the latter
4) Apply this patch
5) Repeat step 2, note there the message also shows in the logs for the WELCOME notice!

Created attachment 171218 [details] [review]
Bug 37869: Tidy code

I had a go at testing, but I'm not seeing any messages in the logs. Any specific log to look in, or do I need to change the logging level?

Testing notes (using KTD):

1. Plugin installed from https://github.com/bywatersolutions/dev-koha-plugin-kitchen-sink/releases/tag/v2.5.1

2. Added an email address (mary@example.com) to Mary (for the welcome message), and added an SMTP server. (Not sure if this was necessary or not).

3. Before the patch, when running misc/cronjobs/process_message_queue.pl:

Plugin hook before_send_message called with the params: $VAR1 = {
          'letter_code' => [],
          'verbose' => 0,
          'where' => undef,
          'type' => [],
          'limit' => undef
        };

4. After the patch, when misc/cronjobs/process_message_queue.pl:

Plugin hook before_send_message called with the params: $VAR1 = {
          'exit_on_plugin_failure' => 0,
          'type' => [],
          'limit' => undef,
          'password' => undef,
          'letter_code' => [],
          'verbose' => 0,
          'where' => undef,
          'method' => 'LOGIN',
          'username' => undef
        };

(In reply to David Nind from comment #3)
> I had a go at testing, but I'm not seeing any messages in the logs. Any
> specific log to look in, or do I need to change the logging level?

I'm using KTD with all the defaults.
I set up Gmail as my SMTP server. 

When I create a new patron with a primary email address I see the following in
/var/log/koha/kohadev/plack-intranet-error.log

[2024/09/10 13:41:27] [WARN] Plugin hook before_send_message called with the params: $VAR1 = {
  'message_id' => 8
};

The only parameter will be the message_id and this will let the plugin know that it is not running from process_messages_queue.pl and instead is acting on a single message!

(In reply to Kyle M Hall (khall) from comment #4)
> (In reply to David Nind from comment #3)
> > I had a go at testing, but I'm not seeing any messages in the logs. Any
> > specific log to look in, or do I need to change the logging level?
> 
> I'm using KTD with all the defaults.
> I set up Gmail as my SMTP server. 
> 
> When I create a new patron with a primary email address I see the following
> in
> /var/log/koha/kohadev/plack-intranet-error.log
> 
> [2024/09/10 13:41:27] [WARN] Plugin hook before_send_message called with the
> params: $VAR1 = {
>   'message_id' => 8
> };
> 
> The only parameter will be the message_id and this will let the plugin know
> that it is not running from process_messages_queue.pl and instead is acting
> on a single message!

I had another go at testing using a default KTD.

I'm still not seeing any warning messages in /var/log/koha/kohadev/plack-intranet-error.log when running the message queue script.

8-(..

How I tested (using KTD):

1. Set up KTD to send email using a Gmail account.[1]

2. Install version 2.5.1 of the kitchen sink plugin (download from https://github.com/bywatersolutions/dev-koha-plugin-kitchen-sink/releases/tag/v2.5.1):
   2.1 Administration > Plugins > Manage plugins > Upload plugin
   2.2 No restart required (although I did do this as well)

3. Add a new patron with a primary email address.

4. Before the patch, run the process message queue script:

   misc/cronjobs/process_message_queue.pl 
   Plugin hook before_send_message called with the params: $VAR1 = {
             'verbose' => 0,
             'limit' => undef,
             'where' => undef,
             'letter_code' => [],
             'type' => []
           };

5. Check the /var/log/koha/kohadev/plack-intranet-error.log
   => There should be a warning (for me, there was no warning message)

   [2024/09/10 13:41:27] [WARN] Plugin hook before_send_message called with the params: $VAR1 = {
     'message_id' => 8
   };

6. Send the patron you created a welcome message and process the message queue as per step 4. There should be no warning message in the log. (This was the case for me.)

[1] To test sending emails using a Google account:
    - Set up an App password for your Google Account.
    - Edit /etc/koha/sites/kohadev/koha-conf.xml file and add this
      configuration near the end (where <user_name> = your Google email
      address; <password> = your APP password, not your Google account
      password):

      <smtp_server>
        <host>smtp.gmail.com</host>
        <port>587</port>
        <timeout>5</timeout>
        <ssl_mode>STARTTLS</ssl_mode>
        <user_name>GOOGLEACCOUNTUSER</user_name>
        <password>GOOGLEAPPPASSWORD</password>
        <debug>1</debug>
      </smtp_server>
   - Restart everything: restart_all
   - Test by:
     . Adding an email address to an existing patron
     . Sending that patron a welcome message (Patron account > More > Send welcome message)
     . Run the message queue processing script: misc/cronjobs/process_message_queue.pl

Now that we've moved this into a module method, any chance of getting a unit test for it Kyle?

I've not tested myself yet, I hope to take a look today.

(In reply to Martin Renvoize (ashimema) from comment #6)
> Now that we've moved this into a module method, any chance of getting a unit
> test for it Kyle?
> 
> I've not tested myself yet, I hope to take a look today.

Capital idea. I'll see what I can do!

Created attachment 172323 [details] [review]
Bug 37869: Move plugin hook before_send_messages to SendQueuedMessages

Most messages in Koha are handled by process_message_queue.pl. This script calls before_send_messages which calls the plugin hook before_send_messages. This works for nearly all messages, but some messages like WELCOME notices are triggered immediately and do not way to for process_message_queue.pl to run. We should move the triggering of before_send_messages into SendQueuedMessages so it is always handled.

Test Plan:
1) Install version 2.5.1 of the kitchen sink plugin
2) Run process_messsage_queue.pl and also trigger a WELCOME notice
3) Note the "Plugin hook before_send_message called with the params" message in the logs for the former but not the latter
4) Apply this patch
5) Repeat step 2, note there the message also shows in the logs for the WELCOME notice!

Created attachment 172324 [details] [review]
Bug 37869: Tidy code

Created attachment 172325 [details] [review]
Bug 37869: Add unit tests

As I have installed other plugins which are failing the given tests I cannot sign off now but I can give some remarks on how to adjust the test plan:

- add an smtp to deliver the mails, although I think that is not necessary to trigger the hook
- in system prefs AutoEmailNewUser must be enabled
- create a new patron with a primary mail address 
- submission of the new patron will trigger a warning 

==> /var/log/koha/kohadev/plack-intranet-error.log <==
[2024/10/18 08:23:07] [WARN] Plugin hook before_send_message called with the params: $VAR1 = {
  'message_id' => 6
};

(In reply to Jan Kissig from comment #11)
> As I have installed other plugins which are failing the given tests I cannot
> sign off now but I can give some remarks on how to adjust the test plan:
> 
> - add an smtp to deliver the mails, although I think that is not necessary
> to trigger the hook
> - in system prefs AutoEmailNewUser must be enabled
> - create a new patron with a primary mail address 
> - submission of the new patron will trigger a warning 
> 
> ==> /var/log/koha/kohadev/plack-intranet-error.log <==
> [2024/10/18 08:23:07] [WARN] Plugin hook before_send_message called with the
> params: $VAR1 = {
>   'message_id' => 6
> };

Thanks for these tips Jan!

I was still not able to generate the warning message in the log.

> I was still not able to generate the warning message in the log.

Does the unit test patch (t/db_dependent/Koha/Plugins/Message_hooks.t) fail without the other patches and pass with it? If that is the case, I believe that is also a sufficient test to prove it is working.

(In reply to Kyle M Hall (khall) from comment #13)
> > I was still not able to generate the warning message in the log.
> 
> Does the unit test patch (t/db_dependent/Koha/Plugins/Message_hooks.t) fail
> without the other patches and pass with it? If that is the case, I believe
> that is also a sufficient test to prove it is working.

It does! I'll use that as the basis to sign off.

Testing notes (using KTD):

1. Apply the unit tests patch (Add unit tests).
2. Run the tests - these should fail: prove t/db_dependent/Koha/Plugins/Message_hooks.t
3. Apply the remaining patches.
4. Run the tests again - these should now pass.

Created attachment 173072 [details] [review]
Bug 37869: Add unit tests

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

Created attachment 173073 [details] [review]
Bug 37869: Move plugin hook before_send_messages to SendQueuedMessages

Most messages in Koha are handled by process_message_queue.pl. This script calls before_send_messages which calls the plugin hook before_send_messages. This works for nearly all messages, but some messages like WELCOME notices are triggered immediately and do not way to for process_message_queue.pl to run. We should move the triggering of before_send_messages into SendQueuedMessages so it is always handled.

Test Plan:
1) Install version 2.5.1 of the kitchen sink plugin
2) Run process_messsage_queue.pl and also trigger a WELCOME notice
3) Note the "Plugin hook before_send_message called with the params" message in the logs for the former but not the latter
4) Apply this patch
5) Repeat step 2, note there the message also shows in the logs for the WELCOME notice!

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

Created attachment 173074 [details] [review]
Bug 37869: Tidy code

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

Created attachment 173429 [details] [review]
Bug 37869: Add unit tests

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

Created attachment 173430 [details] [review]
Bug 37869: Move plugin hook before_send_messages to SendQueuedMessages

Most messages in Koha are handled by process_message_queue.pl. This script calls before_send_messages which calls the plugin hook before_send_messages. This works for nearly all messages, but some messages like WELCOME notices are triggered immediately and do not way to for process_message_queue.pl to run. We should move the triggering of before_send_messages into SendQueuedMessages so it is always handled.

Test Plan:
1) Install version 2.5.1 of the kitchen sink plugin
2) Run process_messsage_queue.pl and also trigger a WELCOME notice
3) Note the "Plugin hook before_send_message called with the params" message in the logs for the former but not the latter
4) Apply this patch
5) Repeat step 2, note there the message also shows in the logs for the WELCOME notice!

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

Created attachment 173431 [details] [review]
Bug 37869: Tidy code

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

Adding a note for plugin developers about this change would be nice :)

Pushed for 24.11!

Well done everyone, thank you!

With these patches applied tests are failing in 24.05.x, no backport.