Bug 20478

Summary: Advance notices: send separate digest messages per library
Product: Koha Reporter: Andreas Jonsson <andreas.jonsson>
Component: NoticesAssignee: Andreas Jonsson <andreas.jonsson>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, katrin.fischer, m.de.rooy, magnus, margaret, martin.renvoize, nick
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14837
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This enhancement add the `--digest-per-branch` option to advanced_notices.pl to allow notices to be grouped by branch rather than grouped by borrower if so required.
Version(s) released in:
19.05
Bug Depends on:    
Bug Blocks: 24326, 24064, 24268, 33900, 34059    
Attachments: Bug 20478: Add unit test for advance_notices.pl cronjob.
Bug 20478: Refactor to remove code duplication.
Bug 20478: Have the cronjob script advance_notices.pl send digest messages per branch.
Bug 20478: Add unit test for advance_notices.pl cronjob.
Bug 20478: Refactor to remove code duplication.
Bug 20478: Have the cronjob script advance_notices.pl send digest messages per branch.
Bug 20478: Have the cronjob script advance_notices.pl send digest messages per branch.
Bug 20478: Add unit test for advance_notices.pl cronjob.
Bug 20478: Refactor to remove code duplication.
Bug 20478: Have the cronjob script advance_notices.pl send digest messages per branch.
Bug 20478: Add unit test for advance_notices.pl cronjob.
Bug 20478: Refactor to remove code duplication.
Bug 20478: Have the cronjob script advance_notices.pl send digest messages per branch.
Bug 20478: (QA follow-up) Make test more standard

Description Andreas Jonsson 2018-03-27 10:01:54 UTC
Current behavior for advance_notices.pl is to send one digest message for each borrower.  The sender for the message is then set to the branch where the borrower is registered, which can be different from the branch or branches on the issues in question.

Desired behavior is that the sender address on the notice message is that of the branch of the issues in question.  Thus, the solution is to generate digest messages per branch.
Comment 1 Andreas Jonsson 2018-03-27 10:06:57 UTC Comment hidden (obsolete)
Comment 2 Andreas Jonsson 2018-03-27 10:07:00 UTC Comment hidden (obsolete)
Comment 3 Andreas Jonsson 2018-03-27 10:07:03 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2018-04-01 14:13:15 UTC
Hi Andreas,

thx for your patches! Some things:

1) Once you submitted your patches it's important to switch the status to 'Needs Signoff' in order to get the attention of the testers, otherwise patches can get easily lost in the saved searches.

2) The QA test tools can help you to find problems with your code easily. For your patches the report is:

 FAIL	misc/cronjobs/advance_notices.pl
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 367)
		forbidden pattern: tab char (line 479)
		forbidden pattern: tab char (line 366)
		forbidden pattern: tab char (line 374)
		forbidden pattern: tab char (line 357)
		forbidden pattern: tab char (line 373)
		forbidden pattern: tab char (line 372)
		forbidden pattern: tab char (line 348)
		forbidden pattern: tab char (line 356)
		forbidden pattern: tab char (line 350)
		forbidden pattern: tab char (line 368)
		forbidden pattern: tab char (line 358)
		forbidden pattern: tab char (line 347)
		forbidden pattern: tab char (line 349)
		forbidden pattern: tab char (line 355)
		forbidden pattern: tab char (line 365)
		forbidden pattern: tab char (line 353)
		forbidden pattern: tab char (line 369)
		forbidden pattern: tab char (line 371)
		forbidden pattern: tab char (line 352)
		forbidden pattern: tab char (line 354)
		forbidden pattern: tab char (line 364)
		forbidden pattern: tab char (line 351)
		forbidden pattern: tab char (line 370)

 FAIL	t/db_dependent/cronjobs/advance_notices_digest.t
   FAIL	  critic
		Expression form of "eval" at line 167, column 1. See page 161 of PBP.
		Bareword file handle opened at line 158, column 1. See pages 202,204 of PBP.
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 67)


3) This can all be fixed, but I see another problem here: libraries are differently organized and we support a lot of different use cases with Koha. Some libraries want Koha to only send one email instead of many, others like yours don't want this behaviour. So we need to find a compromise here and a way to make this configurable. I think a system preference could work here as I don't see how to add it to the notices configuration in a good way.

4) We usually don't test .pl files yet... but not opposed to more tsts :) Tests are failing for me right now:
t/db_dependent/cronjobs/advance_notices_digest.t .. 1/5 
#   Failed test 'There are two messages in the queue'
#   at t/db_dependent/cronjobs/advance_notices_digest.t line 175.
#          got: '0'
#     expected: '2'
# Looks like you planned 5 tests but ran 1.
# Looks like you failed 1 test of 1 run.
t/db_dependent/cronjobs/advance_notices_digest.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 5/5 subtests 

5) If you want, you could also add an individual Sponsord-By line for each sponsoring library. It depends on how you want the output in the release notes later on.
Comment 5 Andreas Jonsson 2018-04-05 20:54:09 UTC Comment hidden (obsolete)
Comment 6 Andreas Jonsson 2018-04-05 20:54:29 UTC Comment hidden (obsolete)
Comment 7 Andreas Jonsson 2018-04-05 20:54:50 UTC Comment hidden (obsolete)
Comment 8 Andreas Jonsson 2018-04-05 21:07:02 UTC
Katrin, thank you for the feedback!

I have updated the patches after running the qa tool.

Also, I've added the command line parameter '-digest-per-branch' while preserving the original behavior as default.  But isn't the expected behavior to receive the message from the issuing library rather from the home library?  Maybe '-digest-per-branch' should be default?

The unit test has been fixed.  I think it failed when running on a database already containing a message attribute for Advance_Notice.
Comment 9 Katrin Fischer 2018-04-07 09:53:01 UTC
> Also, I've added the command line parameter '-digest-per-branch' while
> preserving the original behavior as default.  But isn't the expected
> behavior to receive the message from the issuing library rather from the
> home library?  Maybe '-digest-per-branch' should be default?

Some libraries might prefer for the user to get only one notice instead of multiples. I think that might appply in a library system where it doesn't matter where an item is checked out or returend to - you just want to inform the user. Emails could all use the same formatting.
Comment 10 Katrin Fischer 2018-04-07 09:59:32 UTC
QA script passes now - leaving the sign-off to someone else, so I have the option to QA later.
Comment 11 Magnus Enger 2018-10-18 13:39:02 UTC
(In reply to Andreas Jonsson from comment #8)
> Also, I've added the command line parameter '-digest-per-branch' while
> preserving the original behavior as default.  But isn't the expected
> behavior to receive the message from the issuing library rather from the
> home library?  Maybe '-digest-per-branch' should be default?

Changing the default behaviour would probably come as a surprise to a lot of libraries. ;-) For cronjobs I think it is preferable to keep the old behaviour as default and make new behaviour available by adding command line options.
Comment 12 Magnus Enger 2018-10-18 14:05:25 UTC
I followed the test plan, and the script works as it should. But the new --digest-per-branch parameter needs to be added to the POD in the script, along witht the other options. If you provide a followup for that I will sign off! :)
Comment 13 Andreas Jonsson 2018-10-18 17:20:59 UTC Comment hidden (obsolete)
Comment 14 Andreas Jonsson 2018-10-18 17:21:52 UTC Comment hidden (obsolete)
Comment 15 Magnus Enger 2018-10-19 07:57:26 UTC
Created attachment 80901 [details] [review]
Bug 20478: Add unit test for advance_notices.pl cronjob.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 16 Magnus Enger 2018-10-19 07:57:31 UTC
Created attachment 80902 [details] [review]
Bug 20478: Refactor to remove code duplication.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 17 Magnus Enger 2018-10-19 07:57:35 UTC
Created attachment 80903 [details] [review]
Bug 20478: Have the cronjob script advance_notices.pl send digest messages per branch.

Desired behavior of the script advance_notices.pl is that the sender
address on the notice message is that of the branch of the issues in
question.  Thus, the solution is to generate digest messages per
branch.

To test:
1) Inspect unit test in t/db_dependent/cronjobs/advance_notices_digest.t and note that:
   - There are three libraries
   - There is a borrower
   - The borrower is registered at library1
   - The borrower has message preference wants_digest set to 1
   - The borrower has message preference days_in_advance set to 1
   - The content of the letter PREDUEDGST is '<<count>> <<branches.branchname>>'
   - There are three items
   - There is one issue per item
   - There is one issues at library2
   - There are two issues at library3
   - The date_due of the issues are set to tomorrow
   - For the default case (no -digest-per-message)
      - It is asserted that there is one message in the message queue after running the script
      - It is asserted that there are three items in the message.
      - It is asserted that the branchname is that of the borrower's home library.
   - For the case where -digest-per-message is enabled
      - It is asserted that there are two messages in the message queue after running the script
      - It is asserted that the item count of the message corresponding to library2 is 1
      - It is asserted that the item count of the message corresponding to library3 is 2
      - It is asserted that the branchnames are correct.
2) Run unit test: prove t/db_dependent/cronjobs/advance_notices_digest.t

Sponsored-By: Bibliotek Mellansjö, which is a cooperation between
Sponsored-By: Gullspångs kommunbibliotek
Sponsored-By: Hjo stadsbibliotek
Sponsored-By: Karlsborgs bibliotek
Sponsored-By: Mariestads stadsbibliotek
Sponsored-By: Skövde stadsbibliotek
Sponsored-By: Tibro bibliotek
Sponsored-By: Tidaholms stadsbibliotek
Sponsored-By: Töreboda kommunbibliotek

Signed-off-by: Andreas Jonsson <andreas.jonsson@kreablo.se>
Signed-off-by: Magnus Enger <magnus@libriotech.no>
Adding the --digest-per-branch switch turns the digest into one digest per
library. I think it makes perfect sense to keep the default behaviour
and hide this new functionality behind a command line switch.
Comment 18 Jonathan Druart 2018-12-17 20:41:07 UTC
Great to see tests for this code, even if done in an unusual way.

Some quick remarks:
1. Transactions must be done using txn_begin (also note that you are setting AutoCommit off in the sub and rollback at the end => no sense)

2. $ENV{"OVERRIDE_SYSPREF_dateformat"} = 'metric';
Why not set_preference? Note that for test we usually use mock_preference (from t::lib::Mocks) to avoid to mess with the cache.

3. You could use build_sample_item from bug 21971 to simplify the objects creation (not pushed yet)

4. use Koha::DateUtils instead of DateTime directly

5. You could use File::Slurp::read_file to simplify a bit the code

I would have preferred to see the code moved to a module. It would have eased the write of the tests and made the code reusable.

Keeping the SO status to get other QA opinions.
Comment 19 Martin Renvoize 2019-03-08 13:01:25 UTC
Created attachment 86381 [details] [review]
Bug 20478: Add unit test for advance_notices.pl cronjob.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2019-03-08 13:01:28 UTC
Created attachment 86382 [details] [review]
Bug 20478: Refactor to remove code duplication.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2019-03-08 13:01:31 UTC
Created attachment 86383 [details] [review]
Bug 20478: Have the cronjob script advance_notices.pl send digest messages per branch.

Desired behavior of the script advance_notices.pl is that the sender
address on the notice message is that of the branch of the issues in
question.  Thus, the solution is to generate digest messages per
branch.

To test:
1) Inspect unit test in t/db_dependent/cronjobs/advance_notices_digest.t and note that:
   - There are three libraries
   - There is a borrower
   - The borrower is registered at library1
   - The borrower has message preference wants_digest set to 1
   - The borrower has message preference days_in_advance set to 1
   - The content of the letter PREDUEDGST is '<<count>> <<branches.branchname>>'
   - There are three items
   - There is one issue per item
   - There is one issues at library2
   - There are two issues at library3
   - The date_due of the issues are set to tomorrow
   - For the default case (no -digest-per-message)
      - It is asserted that there is one message in the message queue after running the script
      - It is asserted that there are three items in the message.
      - It is asserted that the branchname is that of the borrower's home library.
   - For the case where -digest-per-message is enabled
      - It is asserted that there are two messages in the message queue after running the script
      - It is asserted that the item count of the message corresponding to library2 is 1
      - It is asserted that the item count of the message corresponding to library3 is 2
      - It is asserted that the branchnames are correct.
2) Run unit test: prove t/db_dependent/cronjobs/advance_notices_digest.t

Sponsored-By: Bibliotek Mellansjö, which is a cooperation between
Sponsored-By: Gullspångs kommunbibliotek
Sponsored-By: Hjo stadsbibliotek
Sponsored-By: Karlsborgs bibliotek
Sponsored-By: Mariestads stadsbibliotek
Sponsored-By: Skövde stadsbibliotek
Sponsored-By: Tibro bibliotek
Sponsored-By: Tidaholms stadsbibliotek
Sponsored-By: Töreboda kommunbibliotek

Signed-off-by: Andreas Jonsson <andreas.jonsson@kreablo.se>
Signed-off-by: Magnus Enger <magnus@libriotech.no>
Adding the --digest-per-branch switch turns the digest into one digest per
library. I think it makes perfect sense to keep the default behaviour
and hide this new functionality behind a command line switch.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2019-03-08 13:01:35 UTC
Created attachment 86384 [details] [review]
Bug 20478: (QA follow-up) Make test more standard

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2019-03-08 13:04:46 UTC
(In reply to Jonathan Druart from comment #18)
> Great to see tests for this code, even if done in an unusual way.
> 
> Some quick remarks:
> 1. Transactions must be done using txn_begin (also note that you are setting
> AutoCommit off in the sub and rollback at the end => no sense)
> 
> 2. $ENV{"OVERRIDE_SYSPREF_dateformat"} = 'metric';
> Why not set_preference? Note that for test we usually use mock_preference
> (from t::lib::Mocks) to avoid to mess with the cache.
> 
> 3. You could use build_sample_item from bug 21971 to simplify the objects
> creation (not pushed yet)
> 
> 4. use Koha::DateUtils instead of DateTime directly
> 
> 5. You could use File::Slurp::read_file to simplify a bit the code
> 
> I would have preferred to see the code moved to a module. It would have
> eased the write of the tests and made the code reusable.
> 
> Keeping the SO status to get other QA opinions.

I've cleaned up the test a little to make it a bit more 'koha', but I've not gone as far as Jonathan suggests above.. I feel these additional clean ups could be handled separately, like the factoring out of code into a module for example.

Code works and passes all tests.. It's also great to see a cronscript with tests.

Passing QA
Comment 24 Nick Clemens (kidclamp) 2019-03-22 20:36:57 UTC
Awesome work all!

Pushed to master for 19.05
Comment 25 Martin Renvoize 2019-04-01 10:32:44 UTC
Enhancement will not be backported to 18.11.x series.
Comment 26 Jonathan Druart 2019-12-19 06:53:28 UTC
There is something terribly wrong with this test file, the cronjob script is run outside the transaction and so changes are effective.
Comment 27 Katrin Fischer 2019-12-22 13:11:58 UTC
(In reply to Jonathan Druart from comment #26)
> There is something terribly wrong with this test file, the cronjob script is
> run outside the transaction and so changes are effective.

Should this be a separate bug?
Comment 28 Martin Renvoize 2019-12-23 08:20:18 UTC
Agreed, we should create a distinct bug for this..
Comment 29 Katrin Fischer 2020-01-02 07:11:30 UTC
(In reply to Martin Renvoize from comment #28)
> Agreed, we should create a distinct bug for this..

I've created a bug, but not sure I got it right:
Bug 24326 - Problem with advance_notices_digest.t
Comment 30 Katrin Fischer 2020-01-08 06:59:27 UTC
*** Bug 14837 has been marked as a duplicate of this bug. ***
Comment 31 Marcel de Rooy 2021-10-06 13:16:45 UTC
This is really ugly.

my $scriptFile = "$scriptDir/../../../misc/cronjobs/advance_notices.pl";
open my $scriptfh, "<", $scriptFile or die "Failed to open $scriptFile: $!";
while (<$scriptfh>) {
    $scriptContent .= $_;
}
etc
eval $script; ## no critic (StringyEval)

What about moving stuff from scripts into a module, and test it properly?

imho we should never pqa/push this kind of stuff. Surely, we dont need a rule for all bad coding practices? :)
Comment 32 Marcel de Rooy 2021-10-06 13:19:15 UTC
C4::Context->set_preference('EnhancedMessagingPreferences', 1);

what about mocking, friends