|
Description
Jonathan Druart
2014-09-30 14:53:58 UTC
Created attachment 31950 [details] [review] Bug 13014: Notify budget owner on new suggestion - UT When a suggestion is created and linked to a fund, a mail will be generated, using a cronjob, to notify the budget owner. A suggestion is considered as "can be treated" if its status is "ASKED". Created attachment 31951 [details] [review] Bug 13014: Notify budget owner on new suggestion - sample notices This patch add the new notice suggestion > TO_PROCESS Others will be added when the patch will be signed off. Created attachment 31952 [details] [review] Bug 13014: Notify budget owner on new suggestion - cronjob Test plan: 0/ Create a new notice suggestions > TO_PROCESS You can use the one defined in the other patch. 1/ Create a suggestion and link it to a fund 2/ Add a owner to this fund and make sure this patron has an email address (the email address used should be the one defined in the AutoEmailPrimaryAddress syspref). 3/ Execute the cronjob script with the -v and without the -c argument 4/ The output should tell you that an email will be sent 5/ Execute the cronjob script with the -v and with the -c argument 6/ Verify the notice is generated in the message_queue table and it is correctly formatted. Yes, it could be great to notify the budget owner, as you said.
Some remarks:
- A patch add a new TO_PROCESS notice to en/mandatory/sample_notices.sql. Why
no notice for other languages?
- notice_unprocessed_suggestion.pl script has no execution permisions like
other scripts in misc/cronjobs
- The script help should say that the script uses a notice named TO_PROCESS
- It would be a plus, if running the script without -c were sending the result
to STDOUT.
- Patron name & surname are in TO_PROCESS notice, but are not displayed. It
appears that, calling EnqueueLetter, your script doesn't provide borrowers
table info. You should have:
tables => {
suggestions => $suggestion->{suggestionid},
branches => $patron->{branchcode},
borrowers => $patron->{borrowernumber},
},
Created attachment 33247 [details] [review] Bug 13014: Notify budget owner on new suggestion - cronjob Test plan: 0/ Create a new notice suggestions > TO_PROCESS You can use the one defined in the other patch. 1/ Create a suggestion and link it to a fund 2/ Add a owner to this fund and make sure this patron has an email address (the email address used should be the one defined in the AutoEmailPrimaryAddress syspref). 3/ Execute the cronjob script with the -v and without the -c argument 4/ The output should tell you that an email will be sent 5/ Execute the cronjob script with the -v and with the -c argument 6/ Verify the notice is generated in the message_queue table and it is correctly formatted. (In reply to Frédéric Demians from comment #4) > - A patch add a new TO_PROCESS notice to en/mandatory/sample_notices.sql. Why > no notice for other languages? "Others will be added when the patch will be signed off." I will do :) > - notice_unprocessed_suggestion.pl script has no execution permisions like > other scripts in misc/cronjobs Done. > - The script help should say that the script uses a notice named TO_PROCESS > Done. > - It would be a plus, if running the script without -c were sending the > result > to STDOUT. Done. > - Patron name & surname are in TO_PROCESS notice, but are not displayed. It > appears that, calling EnqueueLetter, your script doesn't provide borrowers > table info. You should have: > > tables => { > suggestions => $suggestion->{suggestionid}, > branches => $patron->{branchcode}, > borrowers => $patron->{borrowernumber}, > }, Done. Thanks! Created attachment 33265 [details] [review] Bug 13014: Notify budget owner on new suggestion - UT When a suggestion is created and linked to a fund, a mail will be generated, using a cronjob, to notify the budget owner. A suggestion is considered as "can be treated" if its status is "ASKED". Signed-off-by: Frederic Demians <f.demians@tamil.fr> Created attachment 33266 [details] [review] Bug 13014: Notify budget owner on new suggestion - sample notices This patch add the new notice suggestion > TO_PROCESS Others will be added when the patch will be signed off. Signed-off-by: Frederic Demians <f.demians@tamil.fr> Followup expected :-) It would be nice also to have an updatabase.pl entry to insert the new TO_PROCESS notification. Created attachment 33267 [details] [review] Bug 13014: Notify budget owner on new suggestion - cronjob Test plan: 0/ Create a new notice suggestions > TO_PROCESS You can use the one defined in the other patch. 1/ Create a suggestion and link it to a fund 2/ Add a owner to this fund and make sure this patron has an email address (the email address used should be the one defined in the AutoEmailPrimaryAddress syspref). 3/ Execute the cronjob script with the -v and without the -c argument 4/ The output should tell you that an email will be sent 5/ Execute the cronjob script with the -v and with the -c argument 6/ Verify the notice is generated in the message_queue table and it is correctly formatted. Signed-off-by: Frederic Demians <f.demians@tamil.fr> Created attachment 33293 [details] [review] Bug 13014: (follow-up) Notify budget owner on new suggestion - sample notices This patch adds the new notice for all other languages. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 33294 [details] [review] Bug 13014: (follow-up 2) Notify budget owner on new suggestion - sample notices This patch updates the 2 optional sample_notices.sql files for ru-RU and uk-UA. Not sure if it is relevant but I don't understand why they are not up-to-date. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> (In reply to Frédéric Demians from comment #8) > Followup expected :-) It would be nice also to have an updatabase.pl entry to > insert the new TO_PROCESS notification. Done, thanks Frédéric for your tests! Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 13014: Notify budget owner on new suggestion - UT Using index info to reconstruct a base tree... M C4/Suggestions.pm M t/db_dependent/Suggestions.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Suggestions.t CONFLICT (content): Merge conflict in t/db_dependent/Suggestions.t Auto-merging C4/Suggestions.pm Failed to merge in the changes. Patch failed at 0001 Bug 13014: Notify budget owner on new suggestion - UT Created attachment 34792 [details] [review] Bug 13014: Notify budget owner on new suggestion - UT When a suggestion is created and linked to a fund, a mail will be generated, using a cronjob, to notify the budget owner. A suggestion is considered as "can be treated" if its status is "ASKED". Signed-off-by: Frederic Demians <f.demians@tamil.fr> Created attachment 34793 [details] [review] Bug 13014: Notify budget owner on new suggestion - sample notices This patch add the new notice suggestion > TO_PROCESS Others will be added when the patch will be signed off. Signed-off-by: Frederic Demians <f.demians@tamil.fr> Followup expected :-) It would be nice also to have an updatabase.pl entry to insert the new TO_PROCESS notification. Created attachment 34794 [details] [review] Bug 13014: Notify budget owner on new suggestion - cronjob Test plan: 0/ Create a new notice suggestions > TO_PROCESS You can use the one defined in the other patch. 1/ Create a suggestion and link it to a fund 2/ Add a owner to this fund and make sure this patron has an email address (the email address used should be the one defined in the AutoEmailPrimaryAddress syspref). 3/ Execute the cronjob script with the -v and without the -c argument 4/ The output should tell you that an email will be sent 5/ Execute the cronjob script with the -v and with the -c argument 6/ Verify the notice is generated in the message_queue table and it is correctly formatted. Signed-off-by: Frederic Demians <f.demians@tamil.fr> Created attachment 34795 [details] [review] Bug 13014: (follow-up) Notify budget owner on new suggestion - sample notices This patch adds the new notice for all other languages. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 34796 [details] [review] Bug 13014: (follow-up 2) Notify budget owner on new suggestion - sample notices This patch updates the 2 optional sample_notices.sql files for ru-RU and uk-UA. Not sure if it is relevant but I don't understand why they are not up-to-date. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Patches rebased. Kyle, the patches apply cleanly here. They depends on bug 13007. Created attachment 35603 [details] [review] Bug 13014: Notify budget owner on new suggestion - UT When a suggestion is created and linked to a fund, a mail will be generated, using a cronjob, to notify the budget owner. A suggestion is considered as "can be treated" if its status is "ASKED". Signed-off-by: Frederic Demians <f.demians@tamil.fr> Created attachment 35604 [details] [review] Bug 13014: Notify budget owner on new suggestion - sample notices This patch add the new notice suggestion > TO_PROCESS Others will be added when the patch will be signed off. Signed-off-by: Frederic Demians <f.demians@tamil.fr> Followup expected :-) It would be nice also to have an updatabase.pl entry to insert the new TO_PROCESS notification. Created attachment 35605 [details] [review] Bug 13014: Notify budget owner on new suggestion - cronjob Test plan: 0/ Create a new notice suggestions > TO_PROCESS You can use the one defined in the other patch. 1/ Create a suggestion and link it to a fund 2/ Add a owner to this fund and make sure this patron has an email address (the email address used should be the one defined in the AutoEmailPrimaryAddress syspref). 3/ Execute the cronjob script with the -v and without the -c argument 4/ The output should tell you that an email will be sent 5/ Execute the cronjob script with the -v and with the -c argument 6/ Verify the notice is generated in the message_queue table and it is correctly formatted. Signed-off-by: Frederic Demians <f.demians@tamil.fr> Created attachment 35606 [details] [review] Bug 13014: (follow-up) Notify budget owner on new suggestion - sample notices This patch adds the new notice for all other languages. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 35607 [details] [review] Bug 13014: (follow-up 2) Notify budget owner on new suggestion - sample notices This patch updates the 2 optional sample_notices.sql files for ru-RU and uk-UA. Not sure if it is relevant but I don't understand why they are not up-to-date. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> I moved the tests into a subtest to avoid conflicts later. Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 13014: Notify budget owner on new suggestion - UT fatal: sha1 information is lacking or useless (C4/Suggestions.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. :( Created attachment 36272 [details] [review] Bug 13014: Notify budget owner on new suggestion - UT When a suggestion is created and linked to a fund, a mail will be generated, using a cronjob, to notify the budget owner. A suggestion is considered as "can be treated" if its status is "ASKED". Signed-off-by: Frederic Demians <f.demians@tamil.fr> Created attachment 36273 [details] [review] Bug 13014: Notify budget owner on new suggestion - sample notices This patch add the new notice suggestion > TO_PROCESS Others will be added when the patch will be signed off. Signed-off-by: Frederic Demians <f.demians@tamil.fr> Followup expected :-) It would be nice also to have an updatabase.pl entry to insert the new TO_PROCESS notification. Created attachment 36275 [details] [review] Bug 13014: Notify budget owner on new suggestion - cronjob Test plan: 0/ Create a new notice suggestions > TO_PROCESS You can use the one defined in the other patch. 1/ Create a suggestion and link it to a fund 2/ Add a owner to this fund and make sure this patron has an email address (the email address used should be the one defined in the AutoEmailPrimaryAddress syspref). 3/ Execute the cronjob script with the -v and without the -c argument 4/ The output should tell you that an email will be sent 5/ Execute the cronjob script with the -v and with the -c argument 6/ Verify the notice is generated in the message_queue table and it is correctly formatted. Signed-off-by: Frederic Demians <f.demians@tamil.fr> Created attachment 36276 [details] [review] Bug 13014: (follow-up) Notify budget owner on new suggestion - sample notices This patch adds the new notice for all other languages. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 36277 [details] [review] Bug 13014: (follow-up 2) Notify budget owner on new suggestion - sample notices This patch updates the 2 optional sample_notices.sql files for ru-RU and uk-UA. Not sure if it is relevant but I don't understand why they are not up-to-date. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Hi Jonathan, this looks good so far in code review and the unit tests all pass. Just wondering: Why not send out the email immediately like the other emails, but via a cronjob? (In reply to Katrin Fischer from comment #33) > Just wondering: Why not send out the email immediately like the other > emails, but via a cronjob? Because we would like the ability to sent an email for the suggestions not processed for X days. (In reply to Jonathan Druart from comment #34) > (In reply to Katrin Fischer from comment #33) > > Just wondering: Why not send out the email immediately like the other > > emails, but via a cronjob? > > Because we would like the ability to sent an email for the suggestions not > processed for X days. Ok! Created attachment 38006 [details] [review] Bug 13014: (follow-up) Notify budget owner on new suggestion - sample notices This patch adds the new notice for all other languages. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 38007 [details] [review] Bug 13014: (follow-up 2) Notify budget owner on new suggestion - sample notices This patch updates the 2 optional sample_notices.sql files for ru-RU and uk-UA. Not sure if it is relevant but I don't understand why they are not up-to-date. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Rebased patch 4. Wondering about patch 5 here.. Created attachment 38009 [details] [review] Bug 13014: [QA Follow-up] Few typos in cronjob Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 38012 [details] [review] Bug 13014: Notify budget owner on new suggestion - UT When a suggestion is created and linked to a fund, a mail will be generated, using a cronjob, to notify the budget owner. A suggestion is considered as "can be treated" if its status is "ASKED". Signed-off-by: Frederic Demians <f.demians@tamil.fr> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 38013 [details] [review] Bug 13014: Notify budget owner on new suggestion - sample notices This patch add the new notice suggestion > TO_PROCESS Others will be added when the patch will be signed off. Signed-off-by: Frederic Demians <f.demians@tamil.fr> Followup expected :-) It would be nice also to have an updatabase.pl entry to insert the new TO_PROCESS notification. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 38014 [details] [review] Bug 13014: Notify budget owner on new suggestion - cronjob Test plan: 0/ Create a new notice suggestions > TO_PROCESS You can use the one defined in the other patch. 1/ Create a suggestion and link it to a fund 2/ Add a owner to this fund and make sure this patron has an email address (the email address used should be the one defined in the AutoEmailPrimaryAddress syspref). 3/ Execute the cronjob script with the -v and without the -c argument 4/ The output should tell you that an email will be sent 5/ Execute the cronjob script with the -v and with the -c argument 6/ Verify the notice is generated in the message_queue table and it is correctly formatted. Signed-off-by: Frederic Demians <f.demians@tamil.fr> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 38015 [details] [review] Bug 13014: (follow-up) Notify budget owner on new suggestion - sample notices This patch adds the new notice for all other languages. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 38016 [details] [review] Bug 13014: (follow-up 2) Notify budget owner on new suggestion - sample notices This patch updates the 2 optional sample_notices.sql files for ru-RU and uk-UA. Not sure if it is relevant but I don't understand why they are not up-to-date. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Checked by running them manually. English installs 27, Russian 27 and Ukrainian 26. Last language can be updated somewhere else. Created attachment 38017 [details] [review] Bug 13014: [QA Follow-up] Few typos in cronjob Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> QA Comment: Code looks good to me. Some small remarks: GetUnprocessedSuggestions misses pod section. notice_unprocessed_suggestions.pl documentation says: =item B<--days> This parameter is mandatory. It must contain an integer representing the number of days elapsed since the last modification of suggestions to process. A further explanation that it only selects suggestion that are exactly x days old should be added. The workflow of sending it after one day and reminding after 7 days with the parameters 1 7 could be clarifying. Passed QA Created attachment 40597 [details] [review] Bug 13014: (QA followup) have new warnings tested Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Enhancement pushed to master. Thanks Jonathan! The sample notice subject says "budget owner" - but I think it should be "fund owner"? *** Bug 19031 has been marked as a duplicate of this bug. *** |