Bug 28855 - Purging suggestions test should not be on timestamp
Summary: Purging suggestions test should not be on timestamp
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Assignee: marion.durand
QA Contact: Testopia
URL:
Keywords:
Depends on: 23590 28408
Blocks:
  Show dependency treegraph
 
Reported: 2021-08-13 10:44 UTC by Didier Gautheron
Modified: 2021-12-22 22:04 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
This changes the date field that cronjob misc/cronjobs/purge_suggestions.pl uses to calculate the number of days for deleting accepted or rejected suggestions. It now uses the managed on date, as the last updated date that was used can be changed by other database updates.
Version(s) released in:
22.05.00,21.11.01,21.05.08


Attachments
Bug 28855: Purging suggestions test should not be on timestamp (2.22 KB, patch)
2021-11-09 13:08 UTC, marion.durand
Details | Diff | Splinter Review
Bug 28855: (follow-up) adapt specific test (1.91 KB, patch)
2021-11-09 13:14 UTC, marion.durand
Details | Diff | Splinter Review
Bug 28855: Purging suggestions test should not be on timestamp (2.27 KB, patch)
2021-12-11 17:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 28855: (follow-up) adapt specific test (1.96 KB, patch)
2021-12-11 17:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 28855: Purging suggestions test should not be on timestamp (2.34 KB, patch)
2021-12-13 14:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28855: (follow-up) adapt specific test (2.03 KB, patch)
2021-12-13 14:06 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 Didier Gautheron 2021-08-13 10:44:06 UTC
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
Comment 1 marion.durand 2021-10-19 08:20:31 UTC Comment hidden (obsolete)
Comment 2 marion.durand 2021-11-09 13:08:44 UTC
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
Comment 3 marion.durand 2021-11-09 13:14:29 UTC
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.
Comment 4 David Nind 2021-12-11 17:37:07 UTC
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>
Comment 5 David Nind 2021-12-11 17:37:13 UTC
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>
Comment 6 David Nind 2021-12-11 18:10:54 UTC
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).
Comment 7 Jonathan Druart 2021-12-13 11:35:55 UTC
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.
Comment 8 Andrew Fuerste-Henry 2021-12-13 13:57:20 UTC
Makes sense to me! Thanks, all.
Comment 9 Jonathan Druart 2021-12-13 14:06:04 UTC
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>
Comment 10 Jonathan Druart 2021-12-13 14:06:08 UTC
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>
Comment 11 Fridolin Somers 2021-12-15 21:50:43 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 12 Kyle M Hall 2021-12-17 14:49:52 UTC
Pushed to 21.11.x for 21.11.01
Comment 13 Andrew Fuerste-Henry 2021-12-17 21:07:16 UTC
Pushed to 21.05.x for 21.05.08
Comment 14 Victor Grousset/tuxayo 2021-12-22 22:04:44 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.