Description
Agustín Moyano
2019-09-24 16:07:30 UTC
Created attachment 94503 [details] [review] Bug 23673: change db structure 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 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. Created attachment 94954 [details] [review] Bug 23805: (RM follow-up) Remove CHECK constraints MariaDB and MySQL support different syntaxes for CHECK constraints at different versions. To remove complexities in the updatedatabase script I have opted to remove all CHECK constraints entirely. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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> Created attachment 96346 [details] [review] Bug 23673: change db structure 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> 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. Created attachment 96391 [details]
success
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> Created attachment 96451 [details] [review] Bug 23673: change db structure Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> 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> 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. 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. Created attachment 96643 [details] [review] Bug 23673: (follow-up) Change time_status_changed to updated_on (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. (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. (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 Created attachment 98090 [details] [review] Bug 23673: change db structure Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> 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> Not sure what should be the status of this bug report. 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> 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> Re-checked and re-signed-off. Thanks! Agustin, it seems that updated_on should be set to time_queued when the column is added. Created attachment 98431 [details] [review] Bug 23673: Avoid sleep in tests 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> 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> Created attachment 98434 [details] [review] Bug 23673: Avoid sleep in tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> 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> Not sure about the latch patch. Martin, please wait for Agustin approval before pushing it. Nice work everyone! Pushed to master for 20.05 Enhancement not pushed to 19.11.x |