Bug 17669 - Add purging temporary uploads to cleanup_database
Summary: Add purging temporary uploads to cleanup_database
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 17501
Blocks: 18300
  Show dependency treegraph
 
Reported: 2016-11-22 15:32 UTC by Marcel de Rooy
Modified: 2017-04-21 23:16 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17669: Introduce new preference for purging temp uploads (4.40 KB, patch)
2016-12-14 12:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: Add delete_temporary method with unit tests (4.68 KB, patch)
2016-12-14 12:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: Incorporate the changes in cleanup_database (3.92 KB, patch)
2016-12-14 12:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: Add new switch to crontab example and debian daily cron file (2.66 KB, patch)
2016-12-14 12:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: Add delete_temporary method with unit tests (4.67 KB, patch)
2017-01-11 13:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: Incorporate the changes in cleanup_database (3.92 KB, patch)
2017-01-11 13:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: Add new switch to crontab example and debian daily cron file (2.65 KB, patch)
2017-01-11 13:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: Introduce new preference for purging temp uploads (4.40 KB, patch)
2017-03-20 07:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: Small change to sub delete (1.32 KB, patch)
2017-03-20 07:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: Add delete_temporary method with unit tests (4.67 KB, patch)
2017-03-20 07:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: Incorporate the changes in cleanup_database (3.92 KB, patch)
2017-03-20 07:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: Add new switch to crontab example and debian daily cron file (2.65 KB, patch)
2017-03-20 07:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: Introduce new preference for purging temp uploads (4.41 KB, patch)
2017-03-22 15:20 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 17669: Small change to sub delete (1.34 KB, patch)
2017-03-22 15:21 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 17669: Add delete_temporary method with unit tests (4.68 KB, patch)
2017-03-22 15:21 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 17669: Incorporate the changes in cleanup_database (3.94 KB, patch)
2017-03-22 15:22 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 17669: Add new switch to crontab example and debian daily cron file (2.67 KB, patch)
2017-03-22 15:22 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 17669: Rename preference by removing underscores (6.54 KB, patch)
2017-03-30 06:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: [QA Follow-up] Rename preference by removing underscores (6.56 KB, patch)
2017-03-30 06:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: [QA Follow-up] Rename --temp-uploads-override parameter (2.73 KB, patch)
2017-03-30 06:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: [QA Follow-up] Allow zero in temp-uploads-days (4.20 KB, patch)
2017-03-31 06:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: [QA Follow-up] Allow zero in temp-uploads-days (5.20 KB, patch)
2017-03-31 06:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: [QA Follow-up] More consistency in return values of delete (4.14 KB, patch)
2017-04-03 17:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: [QA Follow-up] More consistency in return values of delete (4.11 KB, patch)
2017-04-03 19:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: [QA Follow-up] More consistency in return values of delete (7.75 KB, patch)
2017-04-04 12:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: [QA Follow-up] More consistency in return values of delete (7.66 KB, patch)
2017-04-04 12:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17669: Introduce new preference for purging temp uploads (4.49 KB, patch)
2017-04-20 18:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17669: Small change to sub delete (1.41 KB, patch)
2017-04-20 18:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17669: Add delete_temporary method with unit tests (4.76 KB, patch)
2017-04-20 18:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17669: Incorporate the changes in cleanup_database (4.01 KB, patch)
2017-04-20 18:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17669: Add new switch to crontab example and debian daily cron file (2.76 KB, patch)
2017-04-20 18:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17669: [QA Follow-up] Rename preference by removing underscores (6.59 KB, patch)
2017-04-20 18:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17669: [QA Follow-up] Rename --temp-uploads-override parameter (2.76 KB, patch)
2017-04-20 18:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17669: [QA Follow-up] Allow zero in temp-uploads-days (5.23 KB, patch)
2017-04-20 18:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17669: [QA Follow-up] More consistency in return values of delete (7.69 KB, patch)
2017-04-20 18:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17669: Remove warning 'variable $kohaobj masks earlier declaration in same scope' (1.11 KB, patch)
2017-04-20 18:28 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 Marcel de Rooy 2016-11-22 15:32:53 UTC
17501 will start a move to Koha::Objects.
On top of that we should add this one.
Comment 1 Marcel de Rooy 2016-12-14 12:54:15 UTC
Created attachment 58176 [details] [review]
Bug 17669: Introduce new preference for purging temp uploads

The new preference is: Upload_PurgeTemporaryFiles_Days.
It controls if the cron job will delete temporary uploaded files and if so,
how many days should have passed before doing so.

Since the default value is empty, it will not start deleting these uploads
until you change the pref. No behavior change. But the dbrev will print a
message if you already have temporary uploads.

Test plan:
[1] Check Administration, Preferences, Tools for the new pref.
[2] Run the db rev.
    If you have temporary uploads, check the reported count.
[3] Bonus: Run sysprefs.sql in a safe environment.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2016-12-14 12:54:18 UTC
Created attachment 58177 [details] [review]
Bug 17669: Add delete_temporary method with unit tests

Test plan:
Run t/db_dependent/Upload.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2016-12-14 12:54:22 UTC
Created attachment 58178 [details] [review]
Bug 17669: Incorporate the changes in cleanup_database

Two command line options are added:
[1] -temp-uploads to indicate that you want to purge these uploads,
[2] -temp-uploads-override DAYS to (optionally) tell that you want to
    override the corresponding pref value.

Test plan:
[1] Check the modified usage statement.
[2] If needed, backup your temporary uploads :)
    In case you do not have one, add a temporary one with Tools/Upload.
    Note: Do not choose an upload category.
[3] Set pref to 0, and run cleanup_database with only --temp-uploads.
    No files should be deleted.
[4] Check number of "old" temp uploads. Set pref to nonzero value.
    Verify that the oldest are gone (depending on the value chosen).
[5] Set pref to 0 again.
    If all uploads are gone now, add a new one with Tools/Upload.
    Run cleanup_database with --temp-uploads --temp-uploads-override -1
    All temporary files are gone.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2016-12-14 12:54:25 UTC
Created attachment 58179 [details] [review]
Bug 17669: Add new switch to crontab example and debian daily cron file

The new switch for deleting temporary uploads in cleanup_database can
be added to cron.

Note: Since the option --temp-uploads does only purge temporary uploads
when triggered by the preference Upload_PurgeTemporaryFiles_Days, it can
be safely added here.

Test plan:
There is actually nothing to test here if you followed the preceding test
plans. Just verify that the switch is inserted ocrrectly.
Comment 5 Marcel de Rooy 2016-12-14 12:57:45 UTC
Testers: Please apply 17501 first
Comment 6 Marcel de Rooy 2017-01-11 13:26:41 UTC
Created attachment 58810 [details] [review]
Bug 17669: Add delete_temporary method with unit tests

Test plan:
Run t/db_dependent/Upload.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2017-01-11 13:26:45 UTC
Created attachment 58811 [details] [review]
Bug 17669: Incorporate the changes in cleanup_database

Two command line options are added:
[1] -temp-uploads to indicate that you want to purge these uploads,
[2] -temp-uploads-override DAYS to (optionally) tell that you want to
    override the corresponding pref value.

Test plan:
[1] Check the modified usage statement.
[2] If needed, backup your temporary uploads :)
    In case you do not have one, add a temporary one with Tools/Upload.
    Note: Do not choose an upload category.
[3] Set pref to 0, and run cleanup_database with only --temp-uploads.
    No files should be deleted.
[4] Check number of "old" temp uploads. Set pref to nonzero value.
    Verify that the oldest are gone (depending on the value chosen).
[5] Set pref to 0 again.
    If all uploads are gone now, add a new one with Tools/Upload.
    Run cleanup_database with --temp-uploads --temp-uploads-override -1
    All temporary files are gone.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2017-01-11 13:26:48 UTC
Created attachment 58812 [details] [review]
Bug 17669: Add new switch to crontab example and debian daily cron file

The new switch for deleting temporary uploads in cleanup_database can
be added to cron.

Note: Since the option --temp-uploads does only purge temporary uploads
when triggered by the preference Upload_PurgeTemporaryFiles_Days, it can
be safely added here.

Test plan:
There is actually nothing to test here if you followed the preceding test
plans. Just verify that the switch is inserted ocrrectly.
Comment 9 Marcel de Rooy 2017-01-11 13:27:45 UTC
Rebased on top of changes to 17501
Comment 10 Marcel de Rooy 2017-03-20 07:53:31 UTC
Created attachment 61239 [details] [review]
Bug 17669: Introduce new preference for purging temp uploads

The new preference is: Upload_PurgeTemporaryFiles_Days.
It controls if the cron job will delete temporary uploaded files and if so,
how many days should have passed before doing so.

Since the default value is empty, it will not start deleting these uploads
until you change the pref. No behavior change. But the dbrev will print a
message if you already have temporary uploads.

Test plan:
[1] Check Administration, Preferences, Tools for the new pref.
[2] Run the db rev.
    If you have temporary uploads, check the reported count.
[3] Bonus: Run sysprefs.sql in a safe environment.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2017-03-20 07:53:35 UTC
Created attachment 61240 [details] [review]
Bug 17669: Small change to sub delete

Instead of looping through _resultset here and wrapping the results,
we should use a Koha::Objects instance. Since the method may be called
as a class method, we create an instance if needed.

Test plan:
Run t/db_dependent/Upload.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2017-03-20 07:53:39 UTC
Created attachment 61241 [details] [review]
Bug 17669: Add delete_temporary method with unit tests

Test plan:
Run t/db_dependent/Upload.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2017-03-20 07:53:43 UTC
Created attachment 61242 [details] [review]
Bug 17669: Incorporate the changes in cleanup_database

Two command line options are added:
[1] -temp-uploads to indicate that you want to purge these uploads,
[2] -temp-uploads-override DAYS to (optionally) tell that you want to
    override the corresponding pref value.

Test plan:
[1] Check the modified usage statement.
[2] If needed, backup your temporary uploads :)
    In case you do not have one, add a temporary one with Tools/Upload.
    Note: Do not choose an upload category.
[3] Set pref to 0, and run cleanup_database with only --temp-uploads.
    No files should be deleted.
[4] Check number of "old" temp uploads. Set pref to nonzero value.
    Verify that the oldest are gone (depending on the value chosen).
[5] Set pref to 0 again.
    If all uploads are gone now, add a new one with Tools/Upload.
    Run cleanup_database with --temp-uploads --temp-uploads-override -1
    All temporary files are gone.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2017-03-20 07:53:46 UTC
Created attachment 61243 [details] [review]
Bug 17669: Add new switch to crontab example and debian daily cron file

The new switch for deleting temporary uploads in cleanup_database can
be added to cron.

Note: Since the option --temp-uploads does only purge temporary uploads
when triggered by the preference Upload_PurgeTemporaryFiles_Days, it can
be safely added here.

Test plan:
There is actually nothing to test here if you followed the preceding test
plans. Just verify that the switch is inserted ocrrectly.
Comment 15 Marcel de Rooy 2017-03-20 07:58:32 UTC
Added the second patch. Ready to go again!
Comment 19 Mirko Tietgen 2017-03-22 15:21:46 UTC
Created attachment 61466 [details] [review]
Bug 17669: Small change to sub delete

Instead of looping through _resultset here and wrapping the results,
we should use a Koha::Objects instance. Since the method may be called
as a class method, we create an instance if needed.

Test plan:
Run t/db_dependent/Upload.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 20 Mirko Tietgen 2017-03-22 15:21:53 UTC
Created attachment 61467 [details] [review]
Bug 17669: Add delete_temporary method with unit tests

Test plan:
Run t/db_dependent/Upload.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 21 Mirko Tietgen 2017-03-22 15:22:05 UTC
Created attachment 61468 [details] [review]
Bug 17669: Incorporate the changes in cleanup_database

Two command line options are added:
[1] -temp-uploads to indicate that you want to purge these uploads,
[2] -temp-uploads-override DAYS to (optionally) tell that you want to
    override the corresponding pref value.

Test plan:
[1] Check the modified usage statement.
[2] If needed, backup your temporary uploads :)
    In case you do not have one, add a temporary one with Tools/Upload.
    Note: Do not choose an upload category.
[3] Set pref to 0, and run cleanup_database with only --temp-uploads.
    No files should be deleted.
[4] Check number of "old" temp uploads. Set pref to nonzero value.
    Verify that the oldest are gone (depending on the value chosen).
[5] Set pref to 0 again.
    If all uploads are gone now, add a new one with Tools/Upload.
    Run cleanup_database with --temp-uploads --temp-uploads-override -1
    All temporary files are gone.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 22 Mirko Tietgen 2017-03-22 15:22:21 UTC
Created attachment 61469 [details] [review]
Bug 17669: Add new switch to crontab example and debian daily cron file

The new switch for deleting temporary uploads in cleanup_database can
be added to cron.

Note: Since the option --temp-uploads does only purge temporary uploads
when triggered by the preference Upload_PurgeTemporaryFiles_Days, it can
be safely added here.

Test plan:
There is actually nothing to test here if you followed the preceding test
plans. Just verify that the switch is inserted ocrrectly.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 23 Jonathan Druart 2017-03-22 17:24:37 UTC
Why do we need to overwrite Koha::UploadedFiles->delete finally?
Comment 24 Marcel de Rooy 2017-03-22 20:25:03 UTC
(In reply to Jonathan Druart from comment #23)
> Why do we need to overwrite Koha::UploadedFiles->delete finally?

Because I want to call Koha::UploadedFile->delete and not DBIx::delete. Otherwise I only delete a bunch of records and not the files.
Comment 25 Jonathan Druart 2017-03-29 19:27:50 UTC
Right, I forgot that, makes sense.
It sounds weird to have ->delete behaving differently than Koha::Objects->delete, not blocker

The pref name contains '_', it's unusual. Not blocker

I'd rename the option's name of the script with --temp-uploads-days (currently --temp-uploads-override). What do you think?
Comment 26 Marcel de Rooy 2017-03-30 06:39:05 UTC
(In reply to Jonathan Druart from comment #25)
> Right, I forgot that, makes sense.
> It sounds weird to have ->delete behaving differently than
> Koha::Objects->delete, not blocker

Should be solved on a general level, not here.
Comment 27 Marcel de Rooy 2017-03-30 06:45:51 UTC Comment hidden (obsolete)
Comment 28 Marcel de Rooy 2017-03-30 06:53:21 UTC
Created attachment 61729 [details] [review]
Bug 17669: [QA Follow-up] Rename preference by removing underscores

Requested by QA on comment25.
Result of:
git grep -l Upload_PurgeTemporaryFiles_Days | xargs sed -i -e "s/Upload_PurgeTemporaryFiles_Days/UploadPurgeTemporaryFilesDays/g"

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2017-03-30 06:53:25 UTC
Created attachment 61730 [details] [review]
Bug 17669: [QA Follow-up] Rename --temp-uploads-override parameter

As requested by QA on comment25.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2017-03-30 06:54:08 UTC
(In reply to Jonathan Druart from comment #25)
> The pref name contains '_', it's unusual. Not blocker
> 
> I'd rename the option's name of the script with --temp-uploads-days
> (currently --temp-uploads-override). What do you think?

Always in for changing names or wording :)
Done
Comment 31 Jonathan Druart 2017-03-30 17:57:16 UTC
Marcel, just wondering: It seems that there is no way to delete *all* temporary uploads (the ones from today). If we handle differently 0 and NULL for UploadPurgeTemporaryFilesDays, it would be possible.

Does it make sense?
Comment 32 Marcel de Rooy 2017-03-30 18:55:16 UTC
(In reply to Jonathan Druart from comment #31)
> Marcel, just wondering: It seems that there is no way to delete *all*
> temporary uploads (the ones from today). If we handle differently 0 and NULL
> for UploadPurgeTemporaryFilesDays, it would be possible.
> 
> Does it make sense?

Step 5 from the test plan:
Run cleanup_database with --temp-uploads --temp-uploads-override -1
All temporary files are gone
Comment 33 Jonathan Druart 2017-03-30 19:48:27 UTC
I did not find where it is referenced, not in the syspref's description nor POD of the script nor tests.

To me today is 0, not -1
Comment 34 Marcel de Rooy 2017-03-31 06:34:42 UTC Comment hidden (obsolete)
Comment 35 Marcel de Rooy 2017-03-31 06:44:28 UTC
Created attachment 61743 [details] [review]
Bug 17669: [QA Follow-up] Allow zero in temp-uploads-days

As requested by QA on comment33.
If the pref is 0 or the overriding command line parameter is 0, all
temporary files will be deleted. But if the pref is NULL or empty string,
we will not delete files.

Also adjusted the description of the preference in this regard.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Jonathan Druart 2017-04-03 15:13:24 UTC
Very small thing again, delete_temporary returns true if no errors occur. I am wondering if we should not return true if the delete worked.

 92     } elsif( !defined($days) || $days eq '' ) { # allow 0, not NULL or ""
 93         return 1;
 94     }

would then be 'return;'

It's how delete and delete_missing work.
Comment 37 Marcel de Rooy 2017-04-03 17:26:05 UTC
(In reply to Jonathan Druart from comment #36)
> Very small thing again, delete_temporary returns true if no errors occur. I
> am wondering if we should not return true if the delete worked.
> 
>  92     } elsif( !defined($days) || $days eq '' ) { # allow 0, not NULL or ""
>  93         return 1;
>  94     }
> 
> would then be 'return;'
> 
> It's how delete and delete_missing work.

Hope we still make it before release :)
I agree that we should make this more consistent. But returning undef is perhaps not the best solution. I will add a follow-up that returns 0E0 just as DBIx does when DBI is the storage backend.
This affects UploadedFile too. Only a few small changes, but more than one line..
Comment 38 Marcel de Rooy 2017-04-03 17:29:06 UTC Comment hidden (obsolete)
Comment 39 Marcel de Rooy 2017-04-03 17:29:50 UTC Comment hidden (obsolete)
Comment 40 Marcel de Rooy 2017-04-03 19:26:01 UTC Comment hidden (obsolete)
Comment 41 Marcel de Rooy 2017-04-03 19:31:44 UTC Comment hidden (obsolete)
Comment 42 Marcel de Rooy 2017-04-03 19:33:00 UTC Comment hidden (obsolete)
Comment 43 Marcel de Rooy 2017-04-03 19:41:56 UTC Comment hidden (obsolete)
Comment 44 Marcel de Rooy 2017-04-04 12:32:40 UTC Comment hidden (obsolete)
Comment 45 Marcel de Rooy 2017-04-04 12:45:07 UTC
Created attachment 61819 [details] [review]
Bug 17669: [QA Follow-up] More consistency in return values of delete

See Bugzilla comment36 (QA request).

Koha::UploadedFile->delete
Returns 1, 0E0 or -1 (unknown).

Koha::UploadedFiles->delete and delete_temporary
Returns number of deleted records, 0E0 or -1.

POD lines are corrected accordingly. Unit tests adjusted too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Added a note that Koha::Object->delete itself is not completely consistent
with Koha::Objects->delete (DBIx). The singular may return 1 when it
gets 0E0 from DBIx, while the plural one may return 0E0. But should be
handled somewhere else.
Comment 46 Marcel de Rooy 2017-04-04 12:46:54 UTC
Back to Signed Off
Comment 47 Marcel de Rooy 2017-04-18 14:55:31 UTC
Joubu: hi; any chance to have another look at the qa changes on the upload reports 17669/18300 ?
Comment 48 Jonathan Druart 2017-04-20 18:27:58 UTC
Created attachment 62486 [details] [review]
Bug 17669: Introduce new preference for purging temp uploads

The new preference is: Upload_PurgeTemporaryFiles_Days.
It controls if the cron job will delete temporary uploaded files and if so,
how many days should have passed before doing so.

Since the default value is empty, it will not start deleting these uploads
until you change the pref. No behavior change. But the dbrev will print a
message if you already have temporary uploads.

Test plan:
[1] Check Administration, Preferences, Tools for the new pref.
[2] Run the db rev.
    If you have temporary uploads, check the reported count.
[3] Bonus: Run sysprefs.sql in a safe environment.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 49 Jonathan Druart 2017-04-20 18:28:03 UTC
Created attachment 62487 [details] [review]
Bug 17669: Small change to sub delete

Instead of looping through _resultset here and wrapping the results,
we should use a Koha::Objects instance. Since the method may be called
as a class method, we create an instance if needed.

Test plan:
Run t/db_dependent/Upload.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 50 Jonathan Druart 2017-04-20 18:28:08 UTC
Created attachment 62488 [details] [review]
Bug 17669: Add delete_temporary method with unit tests

Test plan:
Run t/db_dependent/Upload.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 51 Jonathan Druart 2017-04-20 18:28:13 UTC
Created attachment 62489 [details] [review]
Bug 17669: Incorporate the changes in cleanup_database

Two command line options are added:
[1] -temp-uploads to indicate that you want to purge these uploads,
[2] -temp-uploads-override DAYS to (optionally) tell that you want to
    override the corresponding pref value.

Test plan:
[1] Check the modified usage statement.
[2] If needed, backup your temporary uploads :)
    In case you do not have one, add a temporary one with Tools/Upload.
    Note: Do not choose an upload category.
[3] Set pref to 0, and run cleanup_database with only --temp-uploads.
    No files should be deleted.
[4] Check number of "old" temp uploads. Set pref to nonzero value.
    Verify that the oldest are gone (depending on the value chosen).
[5] Set pref to 0 again.
    If all uploads are gone now, add a new one with Tools/Upload.
    Run cleanup_database with --temp-uploads --temp-uploads-override -1
    All temporary files are gone.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 52 Jonathan Druart 2017-04-20 18:28:18 UTC
Created attachment 62490 [details] [review]
Bug 17669: Add new switch to crontab example and debian daily cron file

The new switch for deleting temporary uploads in cleanup_database can
be added to cron.

Note: Since the option --temp-uploads does only purge temporary uploads
when triggered by the preference Upload_PurgeTemporaryFiles_Days, it can
be safely added here.

Test plan:
There is actually nothing to test here if you followed the preceding test
plans. Just verify that the switch is inserted ocrrectly.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 53 Jonathan Druart 2017-04-20 18:28:22 UTC
Created attachment 62491 [details] [review]
Bug 17669: [QA Follow-up] Rename preference by removing underscores

Requested by QA on comment25.
Result of:
git grep -l Upload_PurgeTemporaryFiles_Days | xargs sed -i -e "s/Upload_PurgeTemporaryFiles_Days/UploadPurgeTemporaryFilesDays/g"

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 54 Jonathan Druart 2017-04-20 18:28:27 UTC
Created attachment 62492 [details] [review]
Bug 17669: [QA Follow-up] Rename --temp-uploads-override parameter

As requested by QA on comment25.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 55 Jonathan Druart 2017-04-20 18:28:33 UTC
Created attachment 62493 [details] [review]
Bug 17669: [QA Follow-up] Allow zero in temp-uploads-days

As requested by QA on comment33.
If the pref is 0 or the overriding command line parameter is 0, all
temporary files will be deleted. But if the pref is NULL or empty string,
we will not delete files.

Also adjusted the description of the preference in this regard.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 56 Jonathan Druart 2017-04-20 18:28:39 UTC
Created attachment 62494 [details] [review]
Bug 17669: [QA Follow-up] More consistency in return values of delete

See Bugzilla comment36 (QA request).

Koha::UploadedFile->delete
Returns 1, 0E0 or -1 (unknown).

Koha::UploadedFiles->delete and delete_temporary
Returns number of deleted records, 0E0 or -1.

POD lines are corrected accordingly. Unit tests adjusted too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Added a note that Koha::Object->delete itself is not completely consistent
with Koha::Objects->delete (DBIx). The singular may return 1 when it
gets 0E0 from DBIx, while the plural one may return 0E0. But should be
handled somewhere else.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 57 Jonathan Druart 2017-04-20 18:28:46 UTC
Created attachment 62495 [details] [review]
Bug 17669: Remove warning 'variable $kohaobj masks earlier declaration in same scope'

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 58 Marcel de Rooy 2017-04-21 14:06:40 UTC
Thx Jonathan
Comment 59 Kyle M Hall 2017-04-21 18:00:34 UTC
Pushed to master for 17.05, thanks Marcel, Jonathan!
Comment 60 Katrin Fischer 2017-04-21 23:16:23 UTC
This won't get ported back to 16.11.x as it is an enhancement.