Bug 14723 - Additional delivery notes to messages
Summary: Additional delivery notes to messages
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 12426 18413
Blocks: 14767 14791 14843
  Show dependency treegraph
 
Reported: 2015-08-25 13:27 UTC by Lari Taskula
Modified: 2020-03-17 21:20 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14723 - Additional delivery notes to messages (11.88 KB, patch)
2015-08-28 09:53 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723: (follow-up) Example of handling custom SMS::Send Driver reports (13.58 KB, patch)
2015-08-28 12:09 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723 - Additional delivery notes to messages (15.75 KB, patch)
2015-09-01 14:15 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723 - Additional delivery notes to messages (15.61 KB, patch)
2015-09-01 14:56 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723 - Additional delivery notes to messages (15.09 KB, patch)
2015-09-02 14:57 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723 - Additional delivery notes to messages (14.98 KB, patch)
2015-09-02 15:00 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723 - Additional delivery notes to messages (15.21 KB, patch)
2015-09-04 09:20 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723 - Additional delivery notes to messages (17.78 KB, patch)
2015-09-08 11:38 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723 - Additional delivery notes to messages (17.71 KB, patch)
2015-09-08 12:12 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723 - Additional delivery notes to messages (17.71 KB, patch)
2015-09-09 14:01 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723 - Additional delivery notes to messages (17.88 KB, patch)
2015-09-14 07:53 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723 - Additional delivery notes to messages (17.63 KB, patch)
2015-09-15 10:10 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723 - Additional delivery notes to messages (17.99 KB, patch)
2015-09-16 09:40 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723 - Additional delivery notes to messages (18.23 KB, patch)
2015-09-17 13:46 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723 - Additional delivery notes to messages (18.59 KB, patch)
2015-09-17 13:50 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723 - Additional delivery notes to messages - fixed missing kohastructure (877 bytes, patch)
2015-11-04 12:58 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 14723 - Additional delivery notes to messages - fixed missing kohastructure (877 bytes, patch)
2015-11-04 15:17 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 14723: Additional delivery notes to messages (10.40 KB, patch)
2017-04-12 13:09 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723: Additional delivery notes to messages (11.60 KB, patch)
2017-04-13 09:16 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723: Additional delivery notes to messages (11.71 KB, patch)
2018-08-30 16:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14723: Additional delivery notes to messages (11.71 KB, patch)
2019-04-08 13:59 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 14723: Additional delivery notes to messages (11.78 KB, patch)
2019-04-08 15:27 UTC, Liz Rea
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2015-08-25 13:27:35 UTC
Currently at message_queue we are given only two delivery statuses for messages, "sent" and "failed". When the status becomes failed, we have no idea why it fails.

Some SMS gateway providers send the customer a report about the status of delivery. For example, in our local library we have a provider that does this. With the SMS HTTP request to their Gateway, we also provide them a "report" parameter that includes an URL. Once the SMS delivery has been finished, they will notify us with the status to the given report URL.

If they notify us that the delivery was failed, we will modify its status in the database accordingly. They also provide a short description of why the delivery was failed.

To be able to store more detailed information about the delivery, I propose adding a new column (e.g "delivery_notes") to message_queue table.

In our local library we sometimes have angry borrowers complaining that they have not received SMS messages, so this type of enhancement would be very useful to be able to find out why such has happened.
Comment 1 Lari Taskula 2015-08-28 09:53:08 UTC Comment hidden (obsolete)
Comment 2 Lari Taskula 2015-08-28 12:05:33 UTC
To test template modifications:

1. Find a borrower with notices at members/notices.pl
2. Observe that there is no column for Delivery notes
3. Apply patch and run the given database update
4. Repeat step 1.
5. Observe that there is now a column for Delivery notes
Comment 3 Lari Taskula 2015-08-28 12:09:13 UTC Comment hidden (obsolete)
Comment 4 Lari Taskula 2015-09-01 14:15:35 UTC Comment hidden (obsolete)
Comment 5 Lari Taskula 2015-09-01 14:56:23 UTC Comment hidden (obsolete)
Comment 6 Lari Taskula 2015-09-01 15:20:50 UTC
Selenium tests are using PageObjects and Authentication rewrite (therefore dependencey on Bug 7174).

The patch only covers delivery notes at failure cases in C4::Letters. If you wish to see how delivery notes can be taken further with custom SMS::Send driver and SMS Gateway reports, please see Bug 14767.
Comment 7 Lari Taskula 2015-09-02 09:03:20 UTC
(In reply to Lari Taskula from comment #3)
> Created attachment 42074 [details] [review] [review]
> Bug 14723: (follow-up) Example of handling custom SMS::Send Driver reports
> 
Patch moved to bug 14767.
Comment 8 Lari Taskula 2015-09-02 14:57:29 UTC Comment hidden (obsolete)
Comment 9 Lari Taskula 2015-09-02 15:00:14 UTC Comment hidden (obsolete)
Comment 10 Lari Taskula 2015-09-04 09:20:37 UTC Comment hidden (obsolete)
Comment 11 Lari Taskula 2015-09-08 11:38:38 UTC Comment hidden (obsolete)
Comment 12 Lari Taskula 2015-09-08 12:12:04 UTC Comment hidden (obsolete)
Comment 13 Lari Taskula 2015-09-09 14:01:53 UTC Comment hidden (obsolete)
Comment 14 Lari Taskula 2015-09-14 07:53:00 UTC Comment hidden (obsolete)
Comment 15 Lari Taskula 2015-09-15 10:10:24 UTC Comment hidden (obsolete)
Comment 16 Lari Taskula 2015-09-16 09:40:52 UTC Comment hidden (obsolete)
Comment 17 Lari Taskula 2015-09-17 13:46:22 UTC Comment hidden (obsolete)
Comment 18 Lari Taskula 2015-09-17 13:50:06 UTC
Created attachment 42669 [details] [review]
Bug 14723 - Additional delivery notes to messages
Comment 19 Olli-Antti Kivilahti 2015-11-04 12:58:28 UTC Comment hidden (obsolete)
Comment 20 Olli-Antti Kivilahti 2015-11-04 15:17:06 UTC
Created attachment 44413 [details] [review]
Bug 14723 - Additional delivery notes to messages - fixed missing kohastructure
Comment 21 Frédéric Demians 2015-11-06 13:42:16 UTC
Get this:

fatal: sha1 information is lacking or useless (C4/Letters.pm).
error: could not build fake ancestor
Comment 22 Lari Taskula 2017-04-12 13:09:59 UTC
Created attachment 62080 [details] [review]
Bug 14723: Additional delivery notes to messages

This patch adds additional delivery notes to messages in message queue as there
can be multiple reasons for a delivery to fail.

Currently in message_queue we are given only two delivery statuses for messages,
"sent" and "failed". When the status becomes failed, we have no idea why it fails.

This feature can be useful with SMS gateway providers. Many SMS gateways inform
the application the reason of SMS delivery failure. With this feature, this
information can now be stored. As well as for emails, instead of simply logging
failures, we can now store the reason of failure directly into the message row
of message_queue.

Test plan:

1. Enable EnhancedMessagingPreferences syspref
2. Find a borrower with notices at members/notices.pl
3. Observe that there is no column for Delivery notes
4. Apply patch and run the given database update
5. Repeat step 1.
6. Observe that there is now a column for Delivery notes
Comment 23 Lari Taskula 2017-04-12 13:11:43 UTC
(In reply to Frédéric Demians from comment #21)
> Get this:
> 
> fatal: sha1 information is lacking or useless (C4/Letters.pm).
> error: could not build fake ancestor

I removed some dependencies and rebased this on master. Should now apply.
Comment 24 Lari Taskula 2017-04-13 09:12:14 UTC
Depends on Bug 18413 (otherwise Letters.t fails)
Comment 25 Lari Taskula 2017-04-13 09:16:34 UTC
Created attachment 62138 [details] [review]
Bug 14723: Additional delivery notes to messages

This patch adds additional delivery notes to messages in message queue as there
can be multiple reasons for a delivery to fail.

Currently in message_queue we are given only two delivery statuses for messages,
"sent" and "failed". When the status becomes failed, we have no idea why it fails.

This feature can be useful with SMS gateway providers. Many SMS gateways inform
the application the reason of SMS delivery failure. With this feature, this
information can now be stored. As well as for emails, instead of simply logging
failures, we can now store the reason of failure directly into the message row
of message_queue.

Test plan:

1. Enable EnhancedMessagingPreferences syspref
2. Find a borrower with notices at members/notices.pl
3. Observe that there is no column for Delivery notes
4. Apply patch and run the given database update
5. Repeat step 1.
6. Observe that there is now a column for Delivery notes
Comment 26 Alex Buckley 2017-10-04 04:23:20 UTC
Hi Lari  

When applying your patch I got the following merge error on Letters.t file, (note: as I am using an up to date master branch patch 18413 is already applied (I double checked to make sure)): 

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 14723: Additional delivery notes to messages
Using index info to reconstruct a base tree...
M	C4/Letters.pm
M	installer/data/mysql/kohastructure.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt
M	t/db_dependent/Letters.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Letters.t
CONFLICT (content): Merge conflict in t/db_dependent/Letters.t
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Letters.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 14723: Additional delivery notes to messages
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-14723-Additional-delivery-notes-to-messages-Zqwu8p.patch
Comment 27 Nick Clemens 2018-08-30 16:29:50 UTC
Created attachment 78313 [details] [review]
Bug 14723: Additional delivery notes to messages

This patch adds additional delivery notes to messages in message queue as there
can be multiple reasons for a delivery to fail.

Currently in message_queue we are given only two delivery statuses for messages,
"sent" and "failed". When the status becomes failed, we have no idea why it fails.

This feature can be useful with SMS gateway providers. Many SMS gateways inform
the application the reason of SMS delivery failure. With this feature, this
information can now be stored. As well as for emails, instead of simply logging
failures, we can now store the reason of failure directly into the message row
of message_queue.

Test plan:

1. Enable EnhancedMessagingPreferences syspref
2. Find a borrower with notices at members/notices.pl
3. Observe that there is no column for Delivery notes
4. Apply patch and run the given database update
5. Repeat step 1.
6. Observe that there is now a column for Delivery notes
Comment 28 José-Mario Monteiro-Santos 2019-01-18 19:53:57 UTC
Failed to apply patch 

Bug 14723 - Additional delivery notes to messages

78313 - Bug 14723: Additional delivery notes to messages

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 14723: Additional delivery notes to messages
Using index info to reconstruct a base tree...
M	C4/Letters.pm
M	installer/data/mysql/kohastructure.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt
M	t/db_dependent/Letters.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Letters.t
CONFLICT (content): Merge conflict in t/db_dependent/Letters.t
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Letters.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 14723: Additional delivery notes to messages
The copy of the patch that failed is found in: .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-14723-Additional-delivery-notes-to-messages-vyvgwK.patch
Comment 29 Lari Taskula 2019-04-08 13:59:33 UTC
Created attachment 87525 [details] [review]
Bug 14723: Additional delivery notes to messages

This patch adds additional delivery notes to messages in message queue as there
can be multiple reasons for a delivery to fail.

Currently in message_queue we are given only two delivery statuses for messages,
"sent" and "failed". When the status becomes failed, we have no idea why it fails.

This feature can be useful with SMS gateway providers. Many SMS gateways inform
the application the reason of SMS delivery failure. With this feature, this
information can now be stored. As well as for emails, instead of simply logging
failures, we can now store the reason of failure directly into the message row
of message_queue.

Test plan:

1. Enable EnhancedMessagingPreferences syspref
2. Find a borrower with notices at members/notices.pl
3. Observe that there is no column for Delivery notes
4. Apply patch and run the given database update
5. Repeat step 1.
6. Observe that there is now a column for Delivery notes

Sponsored-by: Hypernova Oy
Comment 30 Liz Rea 2019-04-08 15:27:56 UTC
Created attachment 87531 [details] [review]
Bug 14723: Additional delivery notes to messages

This patch adds additional delivery notes to messages in message queue as there
can be multiple reasons for a delivery to fail.

Currently in message_queue we are given only two delivery statuses for messages,
"sent" and "failed". When the status becomes failed, we have no idea why it fails.

This feature can be useful with SMS gateway providers. Many SMS gateways inform
the application the reason of SMS delivery failure. With this feature, this
information can now be stored. As well as for emails, instead of simply logging
failures, we can now store the reason of failure directly into the message row
of message_queue.

Test plan:

1. Enable EnhancedMessagingPreferences syspref
2. Find a borrower with notices at members/notices.pl
3. Observe that there is no column for Delivery notes
4. Apply patch and run the given database update
5. Repeat step 1.
6. Observe that there is now a column for Delivery notes

Sponsored-by: Hypernova Oy

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 31 Josef Moravec 2019-04-17 13:23:09 UTC
Comment on attachment 87531 [details] [review]
Bug 14723: Additional delivery notes to messages

Review of attachment 87531 [details] [review]:
-----------------------------------------------------------------

::: C4/Letters.pm
@@ +1272,5 @@
>          unless ($patron) {
>              warn "FAIL: No 'to_address' and INVALID borrowernumber ($message->{borrowernumber})";
>              _set_message_status( { message_id => $message->{'message_id'},
> +                                   status     => 'failed',
> +                                   delivery_note => 'Invalid borrowernumber '.$message->{borrowernumber} } );

These notes are untranslatable.
Comment 32 Katrin Fischer 2019-05-11 13:23:00 UTC
Comment on attachment 87531 [details] [review]
Bug 14723: Additional delivery notes to messages

Review of attachment 87531 [details] [review]:
-----------------------------------------------------------------

I took another look at the code and Joseph is right. Made some additional notes too.

::: C4/Letters.pm
@@ +912,2 @@
>  VALUES
> +( ?,              ?,       ?,       ?,        ?,           ?,                      ?,      NOW(),       ?,          ?,            ?,            ? )

I think NOW() might be a MySQLism from my research.

@@ +1281,5 @@
>              # warn "FAIL: No 'to_address' and no email for " . ($member->{surname} ||'') . ", borrowernumber ($message->{borrowernumber})";
>              # warning too verbose for this more common case?
>              _set_message_status( { message_id => $message->{'message_id'},
> +                                   status     => 'failed',
> +                                   delivery_note => 'Unable to find an email address for this borrower' } );

See above, these need to be dealt with in the template.

::: installer/data/mysql/atomicupdate/Bug-14723_-_Additional_delivery_notes_to_messages.perl
@@ +1,3 @@
> +$DBversion = 'XXX';  # will be replaced by the RM
> +if( CheckVersion( $DBversion ) ) {
> +    $dbh->do("ALTER TABLE message_queue ADD delivery_note TEXT");

Please use BEFORE or AFTER column to specify where the new column will be added exactly. This will help to keep things the same between new and old installations.
Comment 33 Lari Taskula 2020-03-17 21:20:11 UTC
I'm no longer able to work on this, so I'm setting assignee to default. Feel free to continue this work.