Bug 23673 - Separate time sent from time created in message_queue table
Summary: Separate time sent from time created in message_queue table
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Agustín Moyano
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-24 16:07 UTC by Agustín Moyano
Modified: 2020-05-18 15:06 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
The time a message was created and the time it was sent are now separate columns in the message_queue table and will shown in the patron's account on the notice tab. Sponsored by: Northeast Kansas Library System (NEKLS)
Version(s) released in:
20.05.00


Attachments
Bug 23673: change db structure (1.93 KB, patch)
2019-10-21 23:29 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23673: Add "Time status changed" column to patron's notices (6.35 KB, patch)
2019-10-21 23:29 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23805: (RM follow-up) Remove CHECK constraints (2.48 KB, patch)
2019-11-01 15:06 UTC, Maxime Dufresne
Details | Diff | Splinter Review
Bug 23673: Add "Time status changed" column to patron's notices (6.42 KB, patch)
2019-12-12 19:26 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 23673: change db structure (1.94 KB, patch)
2019-12-16 17:08 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23673: Add "Time status changed" column to patron's notices (6.41 KB, patch)
2019-12-16 17:08 UTC, Agustín Moyano
Details | Diff | Splinter Review
success (36.34 KB, image/png)
2019-12-17 19:35 UTC, Kelly McElligott
Details
Bug 23673: Add "Time status changed" column to patron's notices (6.48 KB, patch)
2019-12-17 19:37 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 23673: change db structure (2.00 KB, patch)
2019-12-18 18:09 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23673: Add "Time status changed" column to patron's notices (6.47 KB, patch)
2019-12-18 18:09 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23673: (follow-up) Change time_status_changed to updated_on (5.44 KB, patch)
2019-12-26 18:52 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23673: change db structure (2.02 KB, patch)
2020-01-29 19:25 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23673: Add "Updated on" column to patron's notices (6.04 KB, patch)
2020-01-29 19:25 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23673: change db structure (2.08 KB, patch)
2020-02-05 01:14 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 23673: Add "Updated on" column to patron's notices (6.11 KB, patch)
2020-02-05 01:14 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 23673: Avoid sleep in tests (1.83 KB, patch)
2020-02-05 08:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23673: change db structure (2.15 KB, patch)
2020-02-05 08:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23673: Add "Updated on" column to patron's notices (6.18 KB, patch)
2020-02-05 08:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23673: Avoid sleep in tests (1.90 KB, patch)
2020-02-05 08:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23673: Set updated_on=time_queued on DB update (1.04 KB, patch)
2020-02-05 08:50 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Agustín Moyano 2019-09-24 16:07:30 UTC
Problem:

Currently in the message_queue table the only time/date field in the message_queue table is "time_queued".  This is set when the message is created and then updated when the message is sent.  Additionally, the time_queued data is displayed on the notices tab in a patron's account in a column simply called "Time."


Solution:

We need to create  separate fields for the time a message is created and for the time the message is sent.
The time_queued will be populated when added to the message queue table.
Populate the time_sent field with the time the message was sent and do not modify time_queued field.
Then, for the purposes of helping staff answer questions about notices for patrons, we need to rename the "Time" column on the patron's notices tab to "Time sent" and add a separate column for "Time created".
Comment 1 Agustín Moyano 2019-10-21 23:29:09 UTC
Created attachment 94503 [details] [review]
Bug 23673: change db structure
Comment 2 Agustín Moyano 2019-10-21 23:29:13 UTC
Created attachment 94504 [details] [review]
Bug 23673: Add "Time status changed" column to patron's notices

This patch adds "Time status changed" column to patron's notices tab. It also adds logic to C4::Letters to update time_status_changed column when status is changed, and leaves time_queued with message creation time.

To test:

1. Apply patches.

2. Restart plack.

3. Choose a patron and add a purchase suggestion.

4. Change suggestion status.

5. Open patron's notifications.
CHECK => Messages table has now "Time status changed" and "Time created" columns, and "Time" column is gone.
SUCCESS => There is a message with status pending, with a "time created" and no "time status changed"

6. Execute in the shell in Koha directory

   $ ./misc/cronjobs/process_message_queue.pl

7. Open patron's notifications one more time.
SUCCESS => The message changed status. Time created remained the same, and now "time status changed" has the current timestamp.

8. Resend the message and repeat sep 6.
SUCCESS => Every time you change the status, time created remains the same and time status changed updates.

9. prove t/db_dependant/Letters.t

10. Sign off
Comment 3 Agustín Moyano 2019-10-21 23:34:22 UTC
I decided to change "Time sent" column name for "Time status changed".

I thought it would be clearer in the case the message failed. The other way, a user may see that the message failed and a timestamp in "Time sent" column and may be confused thinking that the message was sent but failed afterwards.

If anyone comes up with a better name for the column I'll change it.
Comment 4 Maxime Dufresne 2019-11-01 15:06:20 UTC Comment hidden (obsolete)
Comment 5 ByWater Sandboxes 2019-12-12 19:26:05 UTC
Created attachment 96232 [details] [review]
Bug 23673: Add "Time status changed" column to patron's notices

This patch adds "Time status changed" column to patron's notices tab. It also adds logic to C4::Letters to update time_status_changed column when status is changed, and leaves time_queued with message creation time.

To test:

1. Apply patches.

2. Restart plack.

3. Choose a patron and add a purchase suggestion.

4. Change suggestion status.

5. Open patron's notifications.
CHECK => Messages table has now "Time status changed" and "Time created" columns, and "Time" column is gone.
SUCCESS => There is a message with status pending, with a "time created" and no "time status changed"

6. Execute in the shell in Koha directory

   $ ./misc/cronjobs/process_message_queue.pl

7. Open patron's notifications one more time.
SUCCESS => The message changed status. Time created remained the same, and now "time status changed" has the current timestamp.

8. Resend the message and repeat sep 6.
SUCCESS => Every time you change the status, time created remains the same and time status changed updates.

9. prove t/db_dependant/Letters.t

10. Sign off

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Comment 6 Agustín Moyano 2019-12-16 17:08:37 UTC
Created attachment 96346 [details] [review]
Bug 23673: change db structure
Comment 7 Agustín Moyano 2019-12-16 17:08:40 UTC
Created attachment 96347 [details] [review]
Bug 23673: Add "Time status changed" column to patron's notices

This patch adds "Time status changed" column to patron's notices tab. It also adds logic to C4::Letters to update time_status_changed column when status is changed, and leaves time_queued with message creation time.

To test:

1. Apply patches.

2. Restart plack.

3. Choose a patron and add a purchase suggestion.

4. Change suggestion status.

5. Open patron's notifications.
CHECK => Messages table has now "Time status changed" and "Time created" columns, and "Time" column is gone.
SUCCESS => There is a message with status pending, with a "time created" and no "time status changed"

6. Execute in the shell in Koha directory

   $ ./misc/cronjobs/process_message_queue.pl

7. Open patron's notifications one more time.
SUCCESS => The message changed status. Time created remained the same, and now "time status changed" has the current timestamp.

8. Resend the message and repeat sep 6.
SUCCESS => Every time you change the status, time created remains the same and time status changed updates.

9. prove t/db_dependant/Letters.t

10. Sign off

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Comment 8 Agustín Moyano 2019-12-16 17:11:53 UTC
Fixed atomic update file.

I missed in line 6 when it checked if column exists it said 'time_sent' instead of 'time_status_changed' as it should.
Comment 9 Kelly McElligott 2019-12-17 19:35:07 UTC
Created attachment 96391 [details]
success
Comment 10 ByWater Sandboxes 2019-12-17 19:37:26 UTC
Created attachment 96392 [details] [review]
Bug 23673: Add "Time status changed" column to patron's notices

This patch adds "Time status changed" column to patron's notices tab. It also adds logic to C4::Letters to update time_status_changed column when status is changed, and leaves time_queued with message creation time.

To test:

1. Apply patches.

2. Restart plack.

3. Choose a patron and add a purchase suggestion.

4. Change suggestion status.

5. Open patron's notifications.
CHECK => Messages table has now "Time status changed" and "Time created" columns, and "Time" column is gone.
SUCCESS => There is a message with status pending, with a "time created" and no "time status changed"

6. Execute in the shell in Koha directory

   $ ./misc/cronjobs/process_message_queue.pl

7. Open patron's notifications one more time.
SUCCESS => The message changed status. Time created remained the same, and now "time status changed" has the current timestamp.

8. Resend the message and repeat sep 6.
SUCCESS => Every time you change the status, time created remains the same and time status changed updates.

9. prove t/db_dependant/Letters.t

10. Sign off

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Comment 11 Agustín Moyano 2019-12-18 18:09:06 UTC
Created attachment 96451 [details] [review]
Bug 23673: change db structure

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Comment 12 Agustín Moyano 2019-12-18 18:09:10 UTC
Created attachment 96452 [details] [review]
Bug 23673: Add "Time status changed" column to patron's notices

This patch adds "Time status changed" column to patron's notices tab. It also adds logic to C4::Letters to update time_status_changed column when status is changed, and leaves time_queued with message creation time.

To test:

1. Apply patches.

2. Restart plack.

3. Choose a patron and add a purchase suggestion.

4. Change suggestion status.

5. Open patron's notifications.
CHECK => Messages table has now "Time status changed" and "Time created" columns, and "Time" column is gone.
SUCCESS => There is a message with status pending, with a "time created" and no "time status changed"

6. Execute in the shell in Koha directory

   $ ./misc/cronjobs/process_message_queue.pl

7. Open patron's notifications one more time.
SUCCESS => The message changed status. Time created remained the same, and now "time status changed" has the current timestamp.

8. Resend the message and repeat sep 6.
SUCCESS => Every time you change the status, time created remains the same and time status changed updates.

9. prove t/db_dependant/Letters.t

10. Sign off

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Comment 13 Jonathan Druart 2019-12-26 12:54:01 UTC
A couple of small things:

1. Should not we have the new column with the "default CURRENT_TIMESTAMP on update CURRENT_TIMESTAMP" clause in order to delegate the update of this value to the DBMS?

2. I think we almost agreed on a pattern for the timestamp/datetime columns (there is nothing in the coding guidelines however), to name them suffixed by _on
time_status_changed would be updated_on

I know that it's not exactly what the patch does, but in practice it will be the same I think. Once queued, we only update their status afaik.

Marked as Failed QA but could be considered as "In discussion" if you disagree or if I missed something.
Comment 14 Agustín Moyano 2019-12-26 18:29:57 UTC
Hi Jonathan
 
> 1. Should not we have the new column with the "default CURRENT_TIMESTAMP on
> update CURRENT_TIMESTAMP" clause in order to delegate the update of this
> value to the DBMS?

Mysql doesn't accept a table with 2 columns with "default CURRENT_TIMESTAMP".. there can be only one.
 
> 2. I think we almost agreed on a pattern for the timestamp/datetime columns
> (there is nothing in the coding guidelines however), to name them suffixed
> by _on
> time_status_changed would be updated_on

Ok, I didn't know about that rule.. I'll change that name in follow up.
 
Regards.
Comment 15 Agustín Moyano 2019-12-26 18:52:33 UTC
Created attachment 96643 [details] [review]
Bug 23673: (follow-up) Change time_status_changed to updated_on
Comment 16 Jonathan Druart 2019-12-27 10:37:44 UTC
(In reply to Agustín Moyano from comment #14)
> Hi Jonathan
>  
> > 1. Should not we have the new column with the "default CURRENT_TIMESTAMP on
> > update CURRENT_TIMESTAMP" clause in order to delegate the update of this
> > value to the DBMS?
> 
> Mysql doesn't accept a table with 2 columns with "default
> CURRENT_TIMESTAMP".. there can be only one.

Hola Agustín, Yes I know and that's why I suggested to have it on the new column :)

Take a look at library_groups or borrowers tables for instance. Note that article_requests does the reverse.
Comment 17 Tomás Cohen Arazi 2020-01-27 18:51:25 UTC
(In reply to Jonathan Druart from comment #16)
> (In reply to Agustín Moyano from comment #14)
> > Hi Jonathan
> >  
> > > 1. Should not we have the new column with the "default CURRENT_TIMESTAMP on
> > > update CURRENT_TIMESTAMP" clause in order to delegate the update of this
> > > value to the DBMS?
> > 
> > Mysql doesn't accept a table with 2 columns with "default
> > CURRENT_TIMESTAMP".. there can be only one.
> 
> Hola Agustín, Yes I know and that's why I suggested to have it on the new
> column :)
> 
> Take a look at library_groups or borrowers tables for instance. Note that
> article_requests does the reverse.

You mean making the 'time_queued' column TIMESTAMP DEFAULT NULL (and so making us set it manually), and making 'updated_on' be the one with the trigger? (so we don't do the update manually?). I'm fine with either. Just trying to clarify so we move this forward.
Comment 18 Jonathan Druart 2020-01-29 10:08:45 UTC
(In reply to Tomás Cohen Arazi from comment #17)
> (In reply to Jonathan Druart from comment #16)
> > (In reply to Agustín Moyano from comment #14)
> > > Hi Jonathan
> > >  
> > > > 1. Should not we have the new column with the "default CURRENT_TIMESTAMP on
> > > > update CURRENT_TIMESTAMP" clause in order to delegate the update of this
> > > > value to the DBMS?
> > > 
> > > Mysql doesn't accept a table with 2 columns with "default
> > > CURRENT_TIMESTAMP".. there can be only one.
> > 
> > Hola Agustín, Yes I know and that's why I suggested to have it on the new
> > column :)
> > 
> > Take a look at library_groups or borrowers tables for instance. Note that
> > article_requests does the reverse.
> 
> You mean making the 'time_queued' column TIMESTAMP DEFAULT NULL (and so
> making us set it manually), and making 'updated_on' be the one with the
> trigger? (so we don't do the update manually?). I'm fine with either. Just
> trying to clarify so we move this forward.

Yes, that is what I meant.

4252     created_on TIMESTAMP NULL,             -- Date and time of creation
4253     updated_on TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, -- Date and time of last
Comment 19 Agustín Moyano 2020-01-29 19:25:49 UTC
Created attachment 98090 [details] [review]
Bug 23673: change db structure

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Comment 20 Agustín Moyano 2020-01-29 19:25:53 UTC
Created attachment 98091 [details] [review]
Bug 23673: Add "Updated on" column to patron's notices

This patch adds "Updated on" column to patron's notices tab. It also adds logic to C4::Letters to retrieve updated_on column.

To test:

1. Apply patches.

2. Restart plack.

3. Choose a patron and add a purchase suggestion.

4. Change suggestion status.

5. Open patron's notifications.
CHECK => Messages table has now "Updated on" and "Time created" columns, and "Time" column is gone.
SUCCESS => There is a message with status pending, with a "time created" that equals "updated on"

6. Execute in the shell in Koha directory

   $ ./misc/cronjobs/process_message_queue.pl

7. Open patron's notifications one more time.
SUCCESS => The message changed status. Time created remained the same, and now "updated on" has the current timestamp.

8. Resend the message and repeat sep 6.
SUCCESS => Every time you change the status, time created remains the same and updated on updates.

9. prove t/db_dependant/Letters.t

10. Sign off

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Comment 21 Jonathan Druart 2020-02-04 10:09:55 UTC
Not sure what should be the status of this bug report.
Comment 22 ByWater Sandboxes 2020-02-05 01:14:41 UTC
Created attachment 98419 [details] [review]
Bug 23673: change db structure

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 23 ByWater Sandboxes 2020-02-05 01:14:44 UTC
Created attachment 98420 [details] [review]
Bug 23673: Add "Updated on" column to patron's notices

This patch adds "Updated on" column to patron's notices tab. It also adds logic to C4::Letters to retrieve updated_on column.

To test:

1. Apply patches.

2. Restart plack.

3. Choose a patron and add a purchase suggestion.

4. Change suggestion status.

5. Open patron's notifications.
CHECK => Messages table has now "Updated on" and "Time created" columns, and "Time" column is gone.
SUCCESS => There is a message with status pending, with a "time created" that equals "updated on"

6. Execute in the shell in Koha directory

   $ ./misc/cronjobs/process_message_queue.pl

7. Open patron's notifications one more time.
SUCCESS => The message changed status. Time created remained the same, and now "updated on" has the current timestamp.

8. Resend the message and repeat sep 6.
SUCCESS => Every time you change the status, time created remains the same and updated on updates.

9. prove t/db_dependant/Letters.t

10. Sign off

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 24 Andrew Fuerste-Henry 2020-02-05 01:15:24 UTC
Re-checked and re-signed-off. Thanks!
Comment 25 Jonathan Druart 2020-02-05 08:38:45 UTC
Agustin, it seems that updated_on should be set to time_queued when the column is added.
Comment 26 Jonathan Druart 2020-02-05 08:45:14 UTC
Created attachment 98431 [details] [review]
Bug 23673: Avoid sleep in tests
Comment 27 Jonathan Druart 2020-02-05 08:50:23 UTC
Created attachment 98432 [details] [review]
Bug 23673: change db structure

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2020-02-05 08:50:28 UTC
Created attachment 98433 [details] [review]
Bug 23673: Add "Updated on" column to patron's notices

This patch adds "Updated on" column to patron's notices tab. It also adds logic to C4::Letters to retrieve updated_on column.

To test:

1. Apply patches.

2. Restart plack.

3. Choose a patron and add a purchase suggestion.

4. Change suggestion status.

5. Open patron's notifications.
CHECK => Messages table has now "Updated on" and "Time created" columns, and "Time" column is gone.
SUCCESS => There is a message with status pending, with a "time created" that equals "updated on"

6. Execute in the shell in Koha directory

   $ ./misc/cronjobs/process_message_queue.pl

7. Open patron's notifications one more time.
SUCCESS => The message changed status. Time created remained the same, and now "updated on" has the current timestamp.

8. Resend the message and repeat sep 6.
SUCCESS => Every time you change the status, time created remains the same and updated on updates.

9. prove t/db_dependant/Letters.t

10. Sign off

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2020-02-05 08:50:32 UTC
Created attachment 98434 [details] [review]
Bug 23673: Avoid sleep in tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Jonathan Druart 2020-02-05 08:50:37 UTC
Created attachment 98435 [details] [review]
Bug 23673: Set updated_on=time_queued on DB update

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 31 Jonathan Druart 2020-02-05 08:51:14 UTC
Not sure about the latch patch. Martin, please wait for Agustin approval before pushing it.
Comment 32 Martin Renvoize 2020-02-05 14:28:23 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 33 Joy Nelson 2020-03-04 23:55:45 UTC
Enhancement not pushed to 19.11.x