Description
Nicole C. Engard
2016-03-28 13:40:37 UTC
*** Bug 15507 has been marked as a duplicate of this bug. *** Created attachment 62217 [details] [review] Bug 16149 - Generate and send custom notices based on report output Ths patch add an EmailReport function to C4::Reports::Guided It accepts a notice (module, code, branch) and a report and attempts to email notices to patron, generating content using report content. Notice must be in template toolkit syntax, only columns in report are available for notice. To test: 1 - Specify various options 2 - Ensure errors are returned if options are incomplete or incorrect 3 - Pass a report containing 'from' and 'email' and 'borrowernumber' columns and ensure message queue populated as expected Created attachment 63454 [details] [review] Bug 16149 - Generate and send custom notices based on report output Ths patch add an EmailReport function to C4::Reports::Guided It accepts a notice (module, code, branch) and a report and attempts to email notices to patron, generating content using report content. Notice must be in template toolkit syntax, only columns in report are available for notice. To test: 1 - Specify various options 2 - Ensure errors are returned if options are incomplete or incorrect 3 - Pass a report containing 'from' and 'email' and 'borrowernumber' columns and ensure message queue populated as expected Created attachment 63455 [details] [review] Bug 16149 - Unit tests To test: prove -v t/db_dependent/Reports/Guided.t Bug 16149 - Generate and send custom notices based on report output 63454 - Bug 16149 - Generate and send custom notices based on report output 63455 - Bug 16149 - Unit tests Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 16149 - Generate and send custom notices based on report output Using index info to reconstruct a base tree... M C4/Reports/Guided.pm Falling back to patching base and 3-way merge... Auto-merging C4/Reports/Guided.pm Applying: Bug 16149 - Unit tests Using index info to reconstruct a base tree... M t/db_dependent/Reports/Guided.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Reports/Guided.t CONFLICT (content): Merge conflict in t/db_dependent/Reports/Guided.t 6d20h 1.30 2x2.8GHz 3.7G85% 2017-08-23 08:09:18 Failed to merge in the changes. Patch failed at 0001 Bug 16149 - Unit tests The copy of the patch that failed is found in: /home/vagrant/kohaclone/.git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-16149---Unit-tests-IoVGvi.patch Created attachment 66398 [details] [review] Bug 16149 - Generate and send custom notices based on report output Ths patch add an EmailReport function to C4::Reports::Guided It accepts a notice (module, code, branch) and a report and attempts to email notices to patron, generating content using report content. Notice must be in template toolkit syntax, only columns in report are available for notice. To test: 1 - Specify various options 2 - Ensure errors are returned if options are incomplete or incorrect 3 - Pass a report containing 'from' and 'email' and 'borrowernumber' columns and ensure message queue populated as expected Created attachment 66399 [details] [review] Bug 16149 - Unit tests To test: prove -v t/db_dependent/Reports/Guided.t Hi, I have been able to apply the patch but I am not able to send any mail I have enabled koha mail and I have install postfix... may you help me on this, please? kohadev-koha@kohadevbox:/home/vagrant/kohaclone/misc/cronjobs$ perl patron_emailer.pl -report=1 -notice=ODUE -module=circulation -library=orex -from=info@orex.es -email=email -verbose LETTER_FOUND => 1 REPORT_FOUND => 1 REPORT_SUCCEED => 1 SENDING_SUCCEED => 0 kohadev-koha@kohadevbox:/home/vagrant/kohaclone/misc/cronjobs$ perl patron_emailer.pl -report=1 -notice=CHECKIN -module=circulation -library=orex -from=info@orex.es -email=email -verbose LETTER_FOUND => 1 REPORT_FOUND => 1 REPORT_SUCCEED => 1 SENDING_SUCCEED => 0 kohadev-koha@kohadevbox:/home/vagrant/kohaclone/misc/cronjobs$ perl patron_emailer.pl -report=1 -notice=DUE -module=circulation -library=orex -from=info@orex.es -email=email -verbose LETTER_FOUND => 1 REPORT_FOUND => 1 REPORT_SUCCEED => 1 SENDING_SUCCEED => 0 kohadev-koha@kohadevbox:/home/vagrant/kohaclone/misc/cronjobs$ perl patron_emailer.pl -report=1 -notice=PREDUE -module=circulation -library=orex -from=info@orex.es -email=email -verbose LETTER_FOUND => 1 REPORT_FOUND => 1 REPORT_SUCCEED => 1 SENDING_SUCCEED => 0 kohadev-koha@kohadevbox:/home/vagrant/kohaclone/misc/cronjobs$ exit Created attachment 67392 [details] [review] Bug 16149 - Generate and send custom notices based on report output Ths patch add an EmailReport function to C4::Reports::Guided It accepts a notice (module, code, branch) and a report and attempts to email notices to patron, generating content using report content. Notice must be in template toolkit syntax, only columns in report are available for notice. To test: 1 - Specify various options 2 - Ensure errors are returned if options are incomplete or incorrect 3 - Pass a report containing 'from' and 'email' and 'borrowernumber' columns and ensure message queue populated as expected Created attachment 67393 [details] [review] Bug 16149 - Unit tests To test: prove -v t/db_dependent/Reports/Guided.t (In reply to Hugo Agud from comment #8) > SENDING_SUCCEED => 0 Heh, the line above is reporting the success of row #0, not a failure. Poor design. I altered the patch to count up from 1. Additionally you need the '-commit' option to actually populate the message queue. Please retest and let me know if that works :-) Created attachment 78890 [details] [review] Bug 16149 - Generate and send custom notices based on report output Ths patch add an EmailReport function to C4::Reports::Guided It accepts a notice (module, code, branch) and a report and attempts to email notices to patron, generating content using report content. Notice must be in template toolkit syntax, only columns in report are available for notice. To test: 1 - Specify various options 2 - Ensure errors are returned if options are incomplete or incorrect 3 - Pass a report containing 'from' and 'email' and 'borrowernumber' columns and ensure message queue populated as expected Created attachment 78891 [details] [review] Bug 16149 - Unit tests To test: prove -v t/db_dependent/Reports/Guided.t Created attachment 78896 [details] [review] [SIGNED-OFF] Bug 16149 - Generate and send custom notices based on report output Ths patch add an EmailReport function to C4::Reports::Guided It accepts a notice (module, code, branch) and a report and attempts to email notices to patron, generating content using report content. Notice must be in template toolkit syntax, only columns in report are available for notice. To test: 1 - Specify various options 2 - Ensure errors are returned if options are incomplete or incorrect 3 - Pass a report containing 'from' and 'email' and 'borrowernumber' columns and ensure message queue populated as expected Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de> Created attachment 78897 [details] [review] [SIGNED-OFF] Bug 16149 - Unit tests To test: prove -v t/db_dependent/Reports/Guided.t Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de> 1. FAIL C4/Reports/Guided.pm FAIL valid "my" variable $report masks earlier declaration in same scope FAIL misc/cronjobs/patron_emailer.pl FAIL spelling ommited ==> omitted sucesses ==> successes FAIL valid "my" variable $report masks earlier declaration in same scope * Commit title does not start with 'Bug XXXXX: ' - 2380dc8 * Commit title does not start with 'Bug XXXXX: ' - 579e02c 2. I think we display something more useful than the error codes 3. Please correct indentation 4. Reading the tests: is( $result[0]{NO_EMAIL_COL}, 2, "Warning only for patron with no email"); Why 2? 5. The cronjob script has too many use statements 6. I do not think the subroutine should take "commit", maybe it should return the notice instead? Same for verbose actually. 7. Subroutine takes "branch" whereas the script pass "branchcode" (!) 8. POD says "--report Specify a saved SQL report in the Koha system to user for the emails." It's actually the *id* (saved_sql.id), maybe it should be more explicit 9. --email, should not we use Koha::Patron->notice_email_address and remove this option? 10. --from specified email for 'from' address, report column 'from' used if not specified Should not we use branches.branchemail instead? Created attachment 79328 [details] [review] Bug 16149: Generate and send custom notices based on report output Ths patch add an EmailReport function to C4::Reports::Guided It accepts a notice (module, code, branch) and a report and attempts to email notices to patron, generating content using report content. Notice must be in template toolkit syntax, only columns in report are available for notice. To test: 1 - Specify various options 2 - Ensure errors are returned if options are incomplete or incorrect 3 - Pass a report containing 'from' and 'email' and 'borrowernumber' columns and ensure message queue populated as expected Created attachment 79329 [details] [review] Bug 16149: Unit tests To test: prove -v t/db_dependent/Reports/Guided.t Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de> Created attachment 79330 [details] [review] Bug 16149: Generate and send custom notices based on report output Ths patch add an EmailReport function to C4::Reports::Guided It accepts a notice (module, code, branch) and a report and attempts to email notices to patron, generating content using report content. Notice must be in template toolkit syntax, only columns in report are available for notice. To test: 1 - Specify various options 2 - Ensure errors are returned if options are incomplete or incorrect 3 - Pass a report containing 'from' and 'email' and 'borrowernumber' columns and ensure message queue populated as expected (In reply to Jonathan Druart from comment #16) > 1. QA tool complaints fixed > 2. I think we display something more useful than the error codes Okay, we now have readable errors > 3. Please correct indentation Ah, I think i fixed it > 4. Reading the tests: > is( $result[0]{NO_EMAIL_COL}, 2, "Warning only for patron with no email"); > Why 2? 2 is the row with the error - the returned has includes the error and referenced row (unless borrowernumber not found, then it is the wrong borrowernumber) > 5. The cronjob script has too many use statements Yes, CP error, cleaned > 6. I do not think the subroutine should take "commit", maybe it should > return the notice instead? > Same for verbose actually. Okay, moved those and the sending into the cronjob > 7. Subroutine takes "branch" whereas the script pass "branchcode" (!) Oops, fixed > 8. POD says "--report Specify a saved SQL report in the Koha system to user > for the emails." > It's actually the *id* (saved_sql.id), maybe it should be more explicit Fixed > 9. --email, should not we use Koha::Patron->notice_email_address and remove > this option? This is designed to be more flexible and let the report determine the content - perhaps the library w3ants to check secondary emails or something? > 10. --from specified email for 'from' address, report > column 'from' used if not specified > Should not we use branches.branchemail instead? Again, designed for flexibility in allowing report to set this - perhaps we want to send emails for patron clubs and they have their own email? This was designed to so that it is a cronjob now, but also so it could be used in a script in the GUI. This was a major refactoring, back to NSO Created attachment 83890 [details] [review] Bug 16149: Unit tests To test: prove -v t/db_dependent/Reports/Guided.t Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de> Created attachment 83891 [details] [review] Bug 16149: Generate and send custom notices based on report output Ths patch add an EmailReport function to C4::Reports::Guided It accepts a notice (module, code, branch) and a report and attempts to email notices to patron, generating content using report content. Notice must be in template toolkit syntax, only columns in report are available for notice. To test: 1 - Specify various options 2 - Ensure errors are returned if options are incomplete or incorrect 3 - Pass a report containing 'from' and 'email' and 'borrowernumber' columns and ensure message queue populated as expected Created attachment 85116 [details] [review] Bug 16149: Unit tests To test: prove -v t/db_dependent/Reports/Guided.t Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de> Signed-off-by: Jessica Ofsa <jofsa@vt.edu> Created attachment 85117 [details] [review] Bug 16149: Generate and send custom notices based on report output Ths patch add an EmailReport function to C4::Reports::Guided It accepts a notice (module, code, branch) and a report and attempts to email notices to patron, generating content using report content. Notice must be in template toolkit syntax, only columns in report are available for notice. To test: 1 - Specify various options 2 - Ensure errors are returned if options are incomplete or incorrect 3 - Pass a report containing 'from' and 'email' and 'borrowernumber' columns and ensure message queue populated as expected Signed-off-by: Jessica Ofsa <jofsa@vt.edu> Created attachment 88231 [details] [review] Bug 16149: Unit tests To test: prove -v t/db_dependent/Reports/Guided.t Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de> Signed-off-by: Jessica Ofsa <jofsa@vt.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88232 [details] [review] Bug 16149: Generate and send custom notices based on report output Ths patch add an EmailReport function to C4::Reports::Guided It accepts a notice (module, code, branch) and a report and attempts to email notices to patron, generating content using report content. Notice must be in template toolkit syntax, only columns in report are available for notice. To test: 1 - Specify various options 2 - Ensure errors are returned if options are incomplete or incorrect 3 - Pass a report containing 'from' and 'email' and 'borrowernumber' columns and ensure message queue populated as expected Signed-off-by: Jessica Ofsa <jofsa@vt.edu> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Awesome work all! Pushed to master for 19.05 Enhancement will not be backported to 18.11.x series. |