https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 --- Comment #2 from Andreas Jonsson <andreas.jonsson@kreablo.se> --- Created attachment 73309 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=73309&action=edit Bug 20478: Refactor to remove code duplication. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de Status|NEW |Needs Signoff --- Comment #4 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Andreas Jonsson <andreas.jonsson@kreablo.se> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #73308|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Andreas Jonsson <andreas.jonsson@kreablo.se> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #73310|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 --- Comment #6 from Andreas Jonsson <andreas.jonsson@kreablo.se> --- Created attachment 73723 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=73723&action=edit Bug 20478: Refactor to remove code duplication. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Andreas Jonsson <andreas.jonsson@kreablo.se> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #8 from Andreas Jonsson <andreas.jonsson@kreablo.se> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 --- Comment #10 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- QA script passes now - leaving the sign-off to someone else, so I have the option to QA later. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |magnus@libriotech.no --- Comment #11 from Magnus Enger <magnus@libriotech.no> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 --- Comment #14 from Andreas Jonsson <andreas.jonsson@kreablo.se> --- Created attachment 80867 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80867&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #73722|0 |1 is obsolete| | --- Comment #15 from Magnus Enger <magnus@libriotech.no> --- Created attachment 80901 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80901&action=edit Bug 20478: Add unit test for advance_notices.pl cronjob. Signed-off-by: Magnus Enger <magnus@libriotech.no> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #80867|0 |1 is obsolete| | --- Comment #17 from Magnus Enger <magnus@libriotech.no> --- Created attachment 80903 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80903&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #80901|0 |1 is obsolete| | --- Comment #19 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 86381 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86381&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #80903|0 |1 is obsolete| | --- Comment #21 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 86383 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86383&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA CC| |martin.renvoize@ptfs-europe | |.com --- Comment #23 from 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com Status|Passed QA |Pushed to Master --- Comment #24 from Nick Clemens <nick@bywatersolutions.com> --- Awesome work all! Pushed to master for 19.05 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This enhancement add the release notes| |`--digest-per-branch` | |option to | |advanced_notices.pl to | |allow notices to be grouped | |by branch rather than | |grouped by borrower if so | |required. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=14837 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |24268 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24268 [Bug 24268] advance_notices.pl dies on undefined letter -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Advance notices: send |Advance notices: send |separate digest messages |separate digest messages |per branch |per library --- Comment #27 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |24326 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24326 [Bug 24326] Problem with advance_notices_digest.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 --- Comment #30 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- *** Bug 14837 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #31 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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? :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |33900 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33900 [Bug 33900] advance_notices.pl cronjob hangs -- You are receiving this mail because: You are watching all bug changes.