Bug 18300

Summary: Delete missing upload records
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: katrin.fischer, kyle, mirko
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 17669    
Bug Blocks:    
Attachments: Bug 18300: Delete missing upload records
Bug 18300: Delete missing upload records
Bug 18300: Delete missing upload records
Bug 18300: [QA Follow-up] Fix return value inconsistency
Bug 18300: Delete missing upload records
Bug 18300: [QA Follow-up] Fix return value inconsistency
Bug 18300: Delete missing upload records
Bug 18300: [QA Follow-up] Fix return value inconsistency

Description Marcel de Rooy 2017-03-19 15:50:00 UTC
On top of bug 17669 (deleting temporary files), we should be able to remove records too for missing upload files.
Comment 1 Marcel de Rooy 2017-03-20 08:39:56 UTC
Created attachment 61250 [details] [review]
Bug 18300: Delete missing upload records

If you deleted files from the upload directories manually, or you rebooted
with files in the temporary upload folder, or for some other reason have
records without a file, you may want to cleanup your records in the
uploaded_files table.

This patch adds the method delete_missing to Koha::UploadedFiles. It also
supports a keep_record parameter to do a dry run (count the missing files
first).

Also, we add an option --uploads-missing to cleanup_database. If you add
the flag 1 after this option, you will delete missing files. If you add the
flag 0 or only use the option, you will count missing files.

A subtest is added to Upload.t for delete_missing tests.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Upload a file and delete the file manually.
[3] Run cleanup_database.pl --uploads-missing
    It should report at least one missing file now.
    Check that the record has not been deleted.
[4] Run cleanup_database.pl --uploads-missing 1
    It should report that it removed at least one file.
    Check that the record is gone.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Mirko Tietgen 2017-03-22 15:26:49 UTC
Created attachment 61470 [details] [review]
Bug 18300: Delete missing upload records

If you deleted files from the upload directories manually, or you rebooted
with files in the temporary upload folder, or for some other reason have
records without a file, you may want to cleanup your records in the
uploaded_files table.

This patch adds the method delete_missing to Koha::UploadedFiles. It also
supports a keep_record parameter to do a dry run (count the missing files
first).

Also, we add an option --uploads-missing to cleanup_database. If you add
the flag 1 after this option, you will delete missing files. If you add the
flag 0 or only use the option, you will count missing files.

A subtest is added to Upload.t for delete_missing tests.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Upload a file and delete the file manually.
[3] Run cleanup_database.pl --uploads-missing
    It should report at least one missing file now.
    Check that the record has not been deleted.
[4] Run cleanup_database.pl --uploads-missing 1
    It should report that it removed at least one file.
    Check that the record is gone.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 3 Marcel de Rooy 2017-04-03 08:26:36 UTC
Created attachment 61778 [details] [review]
Bug 18300: Delete missing upload records

If you deleted files from the upload directories manually, or you rebooted
with files in the temporary upload folder, or for some other reason have
records without a file, you may want to cleanup your records in the
uploaded_files table.

This patch adds the method delete_missing to Koha::UploadedFiles. It also
supports a keep_record parameter to do a dry run (count the missing files
first).

Also, we add an option --uploads-missing to cleanup_database. If you add
the flag 1 after this option, you will delete missing files. If you add the
flag 0 or only use the option, you will count missing files.

A subtest is added to Upload.t for delete_missing tests.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Upload a file and delete the file manually.
[3] Run cleanup_database.pl --uploads-missing
    It should report at least one missing file now.
    Check that the record has not been deleted.
[4] Run cleanup_database.pl --uploads-missing 1
    It should report that it removed at least one file.
    Check that the record is gone.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 4 Marcel de Rooy 2017-04-03 08:27:14 UTC
Rebased on top of changes in report 17669 (cleanup_database.pl)
Comment 5 Marcel de Rooy 2017-04-03 19:42:20 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2017-04-03 19:45:36 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2017-04-04 13:11:01 UTC
Created attachment 61821 [details] [review]
Bug 18300: Delete missing upload records

If you deleted files from the upload directories manually, or you rebooted
with files in the temporary upload folder, or for some other reason have
records without a file, you may want to cleanup your records in the
uploaded_files table.

This patch adds the method delete_missing to Koha::UploadedFiles. It also
supports a keep_record parameter to do a dry run (count the missing files
first).

Also, we add an option --uploads-missing to cleanup_database. If you add
the flag 1 after this option, you will delete missing files. If you add the
flag 0 or only use the option, you will count missing files.

A subtest is added to Upload.t for delete_missing tests.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Upload a file and delete the file manually.
[3] Run cleanup_database.pl --uploads-missing
    It should report at least one missing file now.
    Check that the record has not been deleted.
[4] Run cleanup_database.pl --uploads-missing 1
    It should report that it removed at least one file.
    Check that the record is gone.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 8 Marcel de Rooy 2017-04-04 13:11:05 UTC
Created attachment 61822 [details] [review]
Bug 18300: [QA Follow-up] Fix return value inconsistency

As noted on bug report 17669, the return values of delete (both singular
and plural), delete_missing and delete_temporary should be consistent.

Removed the if-construction around full_path. We do not need it; in the
very exceptional case that full_path would be empty, we should delete
the record since the file is missing.

Adjusted POD and unit tests accordingly.

Test plan:
Run t/db_dependent/Upload.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Jonathan Druart 2017-04-20 18:29:28 UTC
Created attachment 62496 [details] [review]
Bug 18300: Delete missing upload records

If you deleted files from the upload directories manually, or you rebooted
with files in the temporary upload folder, or for some other reason have
records without a file, you may want to cleanup your records in the
uploaded_files table.

This patch adds the method delete_missing to Koha::UploadedFiles. It also
supports a keep_record parameter to do a dry run (count the missing files
first).

Also, we add an option --uploads-missing to cleanup_database. If you add
the flag 1 after this option, you will delete missing files. If you add the
flag 0 or only use the option, you will count missing files.

A subtest is added to Upload.t for delete_missing tests.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Upload a file and delete the file manually.
[3] Run cleanup_database.pl --uploads-missing
    It should report at least one missing file now.
    Check that the record has not been deleted.
[4] Run cleanup_database.pl --uploads-missing 1
    It should report that it removed at least one file.
    Check that the record is 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 10 Jonathan Druart 2017-04-20 18:29:32 UTC
Created attachment 62497 [details] [review]
Bug 18300: [QA Follow-up] Fix return value inconsistency

As noted on bug report 17669, the return values of delete (both singular
and plural), delete_missing and delete_temporary should be consistent.

Removed the if-construction around full_path. We do not need it; in the
very exceptional case that full_path would be empty, we should delete
the record since the file is missing.

Adjusted POD and unit tests accordingly.

Test plan:
Run t/db_dependent/Upload.t

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Kyle M Hall 2017-04-21 18:12:44 UTC
Pushed to master for 17.05, thanks Marcel!
Comment 12 Katrin Fischer 2017-04-21 23:21:36 UTC
This depends on bug 17669 which is not in 16.11.x.

This won't get ported back to 16.11.x as it is an enhancement.