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.
Created attachment 73308 [details] [review] Bug 20478: Add unit test for advance_notices.pl cronjob.
Created attachment 73309 [details] [review] Bug 20478: Refactor to remove code duplication.
Created attachment 73310 [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 - 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: The Bibliotek Mellansjö coallition consisting of: Gullspångs kommunbibliotek, Hjo stadsbibliotek, Karlsborgs bibliotek, Mariestads stadsbibliotek, Skövde stadsbibliotek, Tibro bibliotek, Tidaholms stadsbibliotek, Töreboda kommunbibliotek
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.
Created attachment 73722 [details] [review] Bug 20478: Add unit test for advance_notices.pl cronjob.
Created attachment 73723 [details] [review] Bug 20478: Refactor to remove code duplication.
Created attachment 73724 [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
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.
> 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.
QA script passes now - leaving the sign-off to someone else, so I have the option to QA later.
(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.
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 on attachment 73724 [details] [review] Bug 20478: Have the cronjob script advance_notices.pl send digest messages per branch. Thank you Magnus for reviewing this patch! Documentation has been added.
Created attachment 80867 [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>
Created attachment 80901 [details] [review] Bug 20478: Add unit test for advance_notices.pl cronjob. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 80902 [details] [review] Bug 20478: Refactor to remove code duplication. Signed-off-by: Magnus Enger <magnus@libriotech.no>
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.
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.
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>
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>
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>
Created attachment 86384 [details] [review] Bug 20478: (QA follow-up) Make test more standard Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
(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
Awesome work all! Pushed to master for 19.05
Enhancement will not be backported to 18.11.x series.
There is something terribly wrong with this test file, the cronjob script is run outside the transaction and so changes are effective.
(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?
Agreed, we should create a distinct bug for this..
(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
*** Bug 14837 has been marked as a duplicate of this bug. ***
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? :)
C4::Context->set_preference('EnhancedMessagingPreferences', 1); what about mocking, friends