Bug 13014 - Sent an email to the fund owner when a suggestion can be treated
Summary: Sent an email to the fund owner when a suggestion can be treated
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 13007
Blocks: 15177
  Show dependency treegraph
 
Reported: 2014-09-30 14:53 UTC by Jonathan Druart
Modified: 2016-06-21 21:39 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13014: Notify budget owner on new suggestion - UT (6.33 KB, patch)
2014-09-30 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - sample notices (3.94 KB, patch)
2014-09-30 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - cronjob (5.15 KB, patch)
2014-09-30 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - cronjob (5.30 KB, patch)
2014-11-05 14:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - UT (6.38 KB, patch)
2014-11-05 16:58 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - sample notices (4.11 KB, patch)
2014-11-05 16:59 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - cronjob (5.38 KB, patch)
2014-11-05 17:00 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13014: (follow-up) Notify budget owner on new suggestion - sample notices (30.48 KB, patch)
2014-11-06 09:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: (follow-up 2) Notify budget owner on new suggestion - sample notices (26.71 KB, patch)
2014-11-06 09:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - UT (6.37 KB, patch)
2014-12-29 10:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - sample notices (4.11 KB, patch)
2014-12-29 10:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - cronjob (5.38 KB, patch)
2014-12-29 10:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: (follow-up) Notify budget owner on new suggestion - sample notices (30.01 KB, patch)
2014-12-29 10:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: (follow-up 2) Notify budget owner on new suggestion - sample notices (26.71 KB, patch)
2014-12-29 10:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - UT (4.72 KB, patch)
2015-01-29 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - sample notices (4.11 KB, patch)
2015-01-29 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - cronjob (5.38 KB, patch)
2015-01-29 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: (follow-up) Notify budget owner on new suggestion - sample notices (30.08 KB, patch)
2015-01-29 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: (follow-up 2) Notify budget owner on new suggestion - sample notices (26.71 KB, patch)
2015-01-29 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - UT (4.73 KB, patch)
2015-03-02 10:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - sample notices (4.11 KB, patch)
2015-03-02 10:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - cronjob (5.38 KB, patch)
2015-03-02 10:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: (follow-up) Notify budget owner on new suggestion - sample notices (30.08 KB, patch)
2015-03-02 10:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: (follow-up 2) Notify budget owner on new suggestion - sample notices (26.71 KB, patch)
2015-03-02 10:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13014: (follow-up) Notify budget owner on new suggestion - sample notices (30.24 KB, patch)
2015-04-17 08:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13014: (follow-up 2) Notify budget owner on new suggestion - sample notices (26.75 KB, patch)
2015-04-17 08:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13014: [QA Follow-up] Few typos in cronjob (1.20 KB, patch)
2015-04-17 09:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - UT (4.84 KB, patch)
2015-04-17 10:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - sample notices (4.21 KB, patch)
2015-04-17 10:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13014: Notify budget owner on new suggestion - cronjob (5.48 KB, patch)
2015-04-17 10:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13014: (follow-up) Notify budget owner on new suggestion - sample notices (30.30 KB, patch)
2015-04-17 10:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13014: (follow-up 2) Notify budget owner on new suggestion - sample notices (26.93 KB, patch)
2015-04-17 10:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13014: [QA Follow-up] Few typos in cronjob (1.20 KB, patch)
2015-04-17 10:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13014: (QA followup) have new warnings tested (2.61 KB, patch)
2015-06-24 17:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2014-09-30 14:53:58 UTC
When a suggestion is created and linked to a fund, it could be great to notify the budget owner.

A cronjob will be developed in order to generate an email going to the budget owner.
Comment 1 Jonathan Druart 2014-09-30 15:06:12 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-09-30 15:06:19 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-09-30 15:06:23 UTC Comment hidden (obsolete)
Comment 4 Frédéric Demians 2014-10-26 16:30:21 UTC
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},
   },
Comment 5 Jonathan Druart 2014-11-05 14:53:14 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2014-11-05 14:54:14 UTC
(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!
Comment 7 Frédéric Demians 2014-11-05 16:58:01 UTC Comment hidden (obsolete)
Comment 8 Frédéric Demians 2014-11-05 16:59:46 UTC Comment hidden (obsolete)
Comment 9 Frédéric Demians 2014-11-05 17:00:07 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2014-11-06 09:23:47 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2014-11-06 09:23:51 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2014-11-06 09:24:48 UTC
(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!
Comment 13 Katrin Fischer 2014-12-27 19:46:31 UTC
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
Comment 14 Jonathan Druart 2014-12-29 10:32:16 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2014-12-29 10:32:19 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2014-12-29 10:32:22 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2014-12-29 10:32:25 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2014-12-29 10:32:29 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2014-12-29 10:32:48 UTC
Patches rebased.
Comment 20 Jonathan Druart 2015-01-09 14:27:02 UTC
Kyle, the patches apply cleanly here.
They depends on bug 13007.
Comment 21 Jonathan Druart 2015-01-29 09:25:09 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2015-01-29 09:25:15 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2015-01-29 09:25:20 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2015-01-29 09:25:25 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2015-01-29 09:25:29 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2015-01-29 09:26:29 UTC
I moved the tests into a subtest to avoid conflicts later.
Comment 27 Katrin Fischer 2015-02-28 22:47:59 UTC
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.

:(
Comment 28 Jonathan Druart 2015-03-02 10:58:40 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2015-03-02 10:58:48 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2015-03-02 10:59:21 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2015-03-02 10:59:34 UTC Comment hidden (obsolete)
Comment 32 Jonathan Druart 2015-03-02 10:59:45 UTC Comment hidden (obsolete)
Comment 33 Katrin Fischer 2015-03-04 14:04:50 UTC
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?
Comment 34 Jonathan Druart 2015-03-04 15:03:21 UTC
(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.
Comment 35 Katrin Fischer 2015-04-17 08:31:39 UTC
(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!
Comment 36 Marcel de Rooy 2015-04-17 08:32:13 UTC Comment hidden (obsolete)
Comment 37 Marcel de Rooy 2015-04-17 08:32:19 UTC Comment hidden (obsolete)
Comment 38 Marcel de Rooy 2015-04-17 08:35:14 UTC
Rebased patch 4. Wondering about patch 5 here..
Comment 39 Marcel de Rooy 2015-04-17 09:49:44 UTC Comment hidden (obsolete)
Comment 40 Marcel de Rooy 2015-04-17 10:02:16 UTC
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>
Comment 41 Marcel de Rooy 2015-04-17 10:02:20 UTC
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>
Comment 42 Marcel de Rooy 2015-04-17 10:02:25 UTC
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>
Comment 43 Marcel de Rooy 2015-04-17 10:02:29 UTC
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>
Comment 44 Marcel de Rooy 2015-04-17 10:02:33 UTC
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.
Comment 45 Marcel de Rooy 2015-04-17 10:02:37 UTC
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>
Comment 46 Marcel de Rooy 2015-04-17 10:07:38 UTC
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
Comment 47 Tomás Cohen Arazi 2015-06-24 17:57:50 UTC
Created attachment 40597 [details] [review]
Bug 13014: (QA followup) have new warnings tested

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 48 Tomás Cohen Arazi 2015-06-24 17:58:44 UTC
Enhancement pushed to master.

Thanks Jonathan!
Comment 49 Katrin Fischer 2015-11-09 14:11:06 UTC
The sample notice subject says "budget owner" - but I think it should be "fund owner"?