Hi, DelSuggestionsOlderThan is on 'date', a timestamp, which is rather unreliable for that kind of stuff. eg: 'date' field is trashed by #23590 updatedatabase.pl , (which could use date=date in SET BTW) Maybe it could use lastmodificationdate ? Note: to some extend it's a RGPD issue. Regards Didier
Hi ! I'm not sure we can use lastmodificationdate. If we want to use lastmodificationdate, I think we have to initialise it with the suggestion date when creating a new suggestion It is currently initialised to NULL. So all suggestions that have never been modified will never be deleted. I'm not sure what to do for the existing suggestion that have not been modified yet (and so lastmodificationdate is still NULL) Another option would be to use suggesteddate that is always initialised with a date. Regards, Marion
Created attachment 127482 [details] [review] Bug 28855: Purging suggestions test should not be on timestamp DelSuggestionsOlderThan is on "date", a timestamp. It can be a problem because "date" is updated when we update the database, in consequences all the suggestions take the same date. This patch use 'manageddate' instead. In order to test this patch, you will need to have some suggestions where "Managed on" (suggestions.manageddate in the database) is older from "Last updated" (suggestions.date in the database). This can append during some specific update. If you don't have such data, you will have to add them via SQL request. Example of sql for insertion (ID is the patron id of a librarian which can manage suggestions): INSERT INTO suggestions (suggestedby, suggesteddate, managedby, manageddate, acceptedby, accepteddate, STATUS, title) VALUES (1, '2020-10-13', 1, '2020-10-26', 1, '2020-10-26', 'ACCEPTED', 'title'); Test plan: 1- Choose a date and find (or add) a suggestion where: - "Last updated" is not older than this date - "Managed on" is older than this date - "Status" is different from "Pending" ("ASKED" in database) Calculate number_days = today's date - chosen date Example: (Today's date 28/10/2021, chosen date : 28/10/2020, number_days=365) Suggestion | .. | Managed on | .. | Last updated | .. | Status title | .. | 10/26/2020 | .. | 05/15/2021 | .. | Accepted 2- Run cron job /home/koha/src/misc/cronjobs/purge_suggestions.pl --confirm --days number_days 3- Check that this suggestion has not been deleted 4- Apply the patch 5- Repeat steps 1 and 2 6- Check that this suggestion has been deleted
Created attachment 127483 [details] [review] Bug 28855: (follow-up) adapt specific test Adapt the test of function DelSuggestionsOlderThan to match the new behaviour of the function.
Created attachment 128411 [details] [review] Bug 28855: Purging suggestions test should not be on timestamp DelSuggestionsOlderThan is on "date", a timestamp. It can be a problem because "date" is updated when we update the database, in consequences all the suggestions take the same date. This patch use 'manageddate' instead. In order to test this patch, you will need to have some suggestions where "Managed on" (suggestions.manageddate in the database) is older from "Last updated" (suggestions.date in the database). This can append during some specific update. If you don't have such data, you will have to add them via SQL request. Example of sql for insertion (ID is the patron id of a librarian which can manage suggestions): INSERT INTO suggestions (suggestedby, suggesteddate, managedby, manageddate, acceptedby, accepteddate, STATUS, title) VALUES (1, '2020-10-13', 1, '2020-10-26', 1, '2020-10-26', 'ACCEPTED', 'title'); Test plan: 1- Choose a date and find (or add) a suggestion where: - "Last updated" is not older than this date - "Managed on" is older than this date - "Status" is different from "Pending" ("ASKED" in database) Calculate number_days = today's date - chosen date Example: (Today's date 28/10/2021, chosen date : 28/10/2020, number_days=365) Suggestion | .. | Managed on | .. | Last updated | .. | Status title | .. | 10/26/2020 | .. | 05/15/2021 | .. | Accepted 2- Run cron job /home/koha/src/misc/cronjobs/purge_suggestions.pl --confirm --days number_days 3- Check that this suggestion has not been deleted 4- Apply the patch 5- Repeat steps 1 and 2 6- Check that this suggestion has been deleted Signed-off-by: David Nind <david@davidnind.com>
Created attachment 128412 [details] [review] Bug 28855: (follow-up) adapt specific test Adapt the test of function DelSuggestionsOlderThan to match the new behaviour of the function. Signed-off-by: David Nind <david@davidnind.com>
I reviewed the manual to see whether any changes are required - no change as required as it doesn't mention how the date is calculated (see acquisitions > suggestions section, acquisition system preferences > PurgeSuggestionsOlderThan, and cron jobs > clean up old suggestions).
Andrew, does this make sense to you? It does to me but I would prefer to get your feedback on it before flagging it as PQA.
Makes sense to me! Thanks, all.
Created attachment 128464 [details] [review] Bug 28855: Purging suggestions test should not be on timestamp DelSuggestionsOlderThan is on "date", a timestamp. It can be a problem because "date" is updated when we update the database, in consequences all the suggestions take the same date. This patch use 'manageddate' instead. In order to test this patch, you will need to have some suggestions where "Managed on" (suggestions.manageddate in the database) is older from "Last updated" (suggestions.date in the database). This can append during some specific update. If you don't have such data, you will have to add them via SQL request. Example of sql for insertion (ID is the patron id of a librarian which can manage suggestions): INSERT INTO suggestions (suggestedby, suggesteddate, managedby, manageddate, acceptedby, accepteddate, STATUS, title) VALUES (1, '2020-10-13', 1, '2020-10-26', 1, '2020-10-26', 'ACCEPTED', 'title'); Test plan: 1- Choose a date and find (or add) a suggestion where: - "Last updated" is not older than this date - "Managed on" is older than this date - "Status" is different from "Pending" ("ASKED" in database) Calculate number_days = today's date - chosen date Example: (Today's date 28/10/2021, chosen date : 28/10/2020, number_days=365) Suggestion | .. | Managed on | .. | Last updated | .. | Status title | .. | 10/26/2020 | .. | 05/15/2021 | .. | Accepted 2- Run cron job /home/koha/src/misc/cronjobs/purge_suggestions.pl --confirm --days number_days 3- Check that this suggestion has not been deleted 4- Apply the patch 5- Repeat steps 1 and 2 6- Check that this suggestion has been deleted Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 128465 [details] [review] Bug 28855: (follow-up) adapt specific test Adapt the test of function DelSuggestionsOlderThan to match the new behaviour of the function. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Pushed to master for 22.05, thanks to everybody involved 🦄
Pushed to 21.11.x for 21.11.01
Pushed to 21.05.x for 21.05.08
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.