Bug 28855

Summary: Purging suggestions test should not be on timestamp
Product: Koha Reporter: Didier Gautheron <didier.gautheron>
Component: AcquisitionsAssignee: Marion Durand <marion.durand.lelion>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: trivial    
Priority: P5 - low CC: andrewfh, david, jonathan.druart, kyle, marion.durand.lelion, victor
Version: Main   
Hardware: All   
OS: All   
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
Bug Depends on: 23590, 28408    
Bug Blocks:    
Attachments: Bug 28855: Purging suggestions test should not be on timestamp
Bug 28855: (follow-up) adapt specific test
Bug 28855: Purging suggestions test should not be on timestamp
Bug 28855: (follow-up) adapt specific test
Bug 28855: Purging suggestions test should not be on timestamp
Bug 28855: (follow-up) adapt specific test

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.