17501 will start a move to Koha::Objects. On top of that we should add this one.
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>
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>
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>
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.
Testers: Please apply 17501 first
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>
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>
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.
Rebased on top of changes to 17501
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>
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>
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>
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>
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.
Added the second patch. Ready to go again!
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>
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>
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>
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>
Why do we need to overwrite Koha::UploadedFiles->delete finally?
(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.
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?
(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.
Created attachment 61727 [details] [review] Bug 17669: 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>
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>
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>
(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
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?
(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
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
Created attachment 61742 [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. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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>
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.
(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..
Created attachment 61805 [details] [review] Bug 17669: [QA Follow-up] More consistency in return values of delete See Bugzilla comment36 (QA request). Koha::UploadedFile->delete Returns number of deleted records. POD corrected. Handles a possible or fictive undef from the storage backend. 0E0 means zero (like DBI), -1 means unknown. Koha::UploadedFile->delete Returns number of deleted records (just as above), and no longer true or false depending on errors encountered. POD corrected. Koha::UploadedFile->delete_temporary Returns number of deleted records (just as above). Does no longer return true when the associated pref contains NULL or empty string. POD corrected. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> As often in Koha, the return values of delete operations are ignored. The test t/db_dependent/Upload.t still passes.
If you insist now on further tests of the return value, I would rather move the last follow-up somewhere else.
Hmm Still a very small thing: if I come across an undef, pass -1 instead of 0E0 (two places?)
Created attachment 61809 [details] [review] Bug 17669: [QA Follow-up] More consistency in return values of delete See Bugzilla comment36 (QA request). Koha::UploadedFile->delete Returns number of deleted records. POD corrected. Handles a possible or fictive undef from the storage backend. 0E0 means zero (like DBI), -1 means unknown. Koha::UploadedFile->delete Returns number of deleted records (just as above), and no longer true or false depending on errors encountered. POD corrected. Koha::UploadedFile->delete_temporary Returns number of deleted records (just as above). Does no longer return true when the associated pref contains NULL or empty string. POD corrected. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> As often in Koha, the return values of delete operations are ignored. The test t/db_dependent/Upload.t still passes.
(In reply to Marcel de Rooy from comment #39) > If you insist now on further tests of the return value, I would rather move > the last follow-up somewhere else. Suppose I do that on 18300?
Temporarily blocked
Created attachment 61818 [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). Return value may depend on storage backend. 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.
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.
Back to Signed Off
Joubu: hi; any chance to have another look at the qa changes on the upload reports 17669/18300 ?
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Thx Jonathan
Pushed to master for 17.05, thanks Marcel, Jonathan!
This won't get ported back to 16.11.x as it is an enhancement.