Bug 28855 - Purging suggestions test should not be on timestamp
Summary: Purging suggestions test should not be on timestamp
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial
Assignee: Marion Durand
QA Contact: Testopia
URL:
Keywords:
Depends on: 23590 28408
Blocks:
  Show dependency tree / graph
 
Reported: 2021-08-13 10:44 UTC by Didier Gautheron
Modified: 2022-12-12 21:24 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
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
Circulation function:

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
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

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 ﷐[U+1F984]﷑
Comment 12 Kyle M Hall (khall) 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.