Only delete files that were successfully downloaded

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- fix: only delete files successfully downloaded
+
 ## 1.12.0 (2023-03-10)
 
 - experimental: add macos binary [#551](https://github.com/icloud-photos-downloader/icloud_photos_downloader/issues/551)

icloudpd/authentication.py

@@ -12,6 +12,7 @@ class TwoStepAuthRequiredError(Exception):
     and sends an email notification.
     """
 
+
 def authenticator(domain):
     """Wraping authentication with domain context"""
     def authenticate_(
@@ -33,7 +34,7 @@ def authenticate_(
                     username, password,
                     cookie_directory=cookie_directory,
                     client_id=client_id,
-                    )
+                )
                 break
             except pyicloud_ipd.exceptions.NoStoredPasswordAvailable:
                 # Prompt for password if not stored in PyiCloud's keyring

icloudpd/base.py

@@ -121,13 +121,12 @@
     + "(Does not download or delete any files.)",
     is_flag=True,
 )
-@click.option(
-    "--folder-structure",
-    help="Folder structure (default: {:%Y/%m/%d}). "
-    "If set to 'none' all photos will just be placed into the download directory",
-    metavar="<folder_structure>",
-    default="{:%Y/%m/%d}",
-)
+@click.option("--folder-structure",
+              help="Folder structure (default: {:%Y/%m/%d}). "
+              "If set to 'none' all photos will just be placed into the download directory",
+              metavar="<folder_structure>",
+              default="{:%Y/%m/%d}",
+              )
 @click.option(
     "--set-exif-datetime",
     help="Write the DateTimeOriginal exif tag from file creation date, " +
@@ -171,12 +170,11 @@
     "Default: SMTP username",
     metavar="<notification_email>",
 )
-@click.option(
-    "--notification-email-from",
-    help="Email address from which you would like to receive email notifications. "
-    "Default: SMTP username or notification-email",
-    metavar="<notification_email_from>",
-)
+@click.option("--notification-email-from",
+              help="Email address from which you would like to receive email notifications. "
+              "Default: SMTP username or notification-email",
+              metavar="<notification_email_from>",
+              )
 @click.option(
     "--notification-script",
     type=click.Path(),
@@ -194,12 +192,11 @@
               "(Progress bar is disabled by default if there is no tty attached)",
               is_flag=True,
               )
-@click.option(
-    "--threads-num",
-    help="Number of cpu threads -- deprecated. To be removed in future version",
-    type=click.IntRange(1),
-    default=1,
-)
+@click.option("--threads-num",
+              help="Number of cpu threads -- deprecated. To be removed in future version",
+              type=click.IntRange(1),
+              default=1,
+              )
 @click.option(
     "--delete-after-download",
     help='Delete the photo/video after download it.'
@@ -213,11 +210,10 @@
     type=click.Choice(["com", "cn"]),
     default="com",
 )
-@click.option(
-    "--watch-with-interval",
-    help="Run downloading in a infinite cycle, waiting specified seconds between runs",
-    type=click.IntRange(1),
-)
+@click.option("--watch-with-interval",
+              help="Run downloading in a infinite cycle, waiting specified seconds between runs",
+              type=click.IntRange(1),
+              )
 @click.version_option()
 # pylint: disable-msg=too-many-arguments,too-many-statements
 # pylint: disable-msg=too-many-branches,too-many-locals
@@ -279,7 +275,6 @@ def main(
         print('--auto-delete and --delete-after-download are mutually exclusive')
         sys.exit(2)
 
-
     if watch_with_interval and (list_albums or only_print_filenames):
         print('--watch_with_interval is not compatible with --list_albums, --only_print_filenames')
         sys.exit(2)
@@ -350,13 +345,12 @@ def download_photo_(counter, photo):
                 logger.set_tqdm_description(
                     f"Skipping {filename}, only downloading photos."
                 )
-                return
+                return False
             if photo.item_type not in ("image", "movie"):
                 logger.set_tqdm_description(
                     f"Skipping {filename}, only downloading photos and videos. "
-                    f"(Item type was: {photo.item_type})"
-                )
-                return
+                    f"(Item type was: {photo.item_type})")
+                return False
             try:
                 created_date = photo.created.astimezone(get_localzone())
             except (ValueError, OSError):
@@ -382,6 +376,7 @@ def download_photo_(counter, photo):
 
             download_dir = os.path.normpath(os.path.join(directory, date_path))
             download_size = size
+            success = False
 
             try:
                 versions = photo.versions
@@ -406,13 +401,13 @@ def download_photo_(counter, photo):
                 print(
                     "Include a link to the Gist in your issue, so that we can "
                     "see what went wrong.\n")
-                return
+                return False
 
             if size not in versions and size != "original":
                 if force_size:
                     logger.set_tqdm_description(
                         f"{size} size does not exist for {filename}. Skipping...", logging.ERROR, )
-                    return
+                    return False
                 download_size = "original"
 
             download_path = local_download_path(
@@ -462,6 +457,7 @@ def download_photo_(counter, photo):
                     download_result = download.download_media(
                         icloud, photo, download_path, download_size
                     )
+                    success = download_result
 
                     if download_result:
                         if set_exif_datetime and \
@@ -523,14 +519,17 @@ def download_photo_(counter, photo):
                                 lp_download_path, 96)
                             logger.set_tqdm_description(
                                 f"Downloading {truncated_path}")
-                            download.download_media(
+                            success = download.download_media(
                                 icloud, photo, lp_download_path, lp_size
-                            )
+                            ) and success
+            return success
         return download_photo_
     return state_
 
 # pylint: disable-msg=too-many-arguments,too-many-statements
 # pylint: disable-msg=too-many-branches,too-many-locals
+
+
 def core(
         downloader,
         directory,
@@ -676,7 +675,7 @@ def photos_exception_handler(ex, retries):
         # or if the progress bar is explicitly disabled,
         # or if this is not a terminal (e.g. cron or piping output to file)
         skip_bar = not os.environ.get("FORCE_TQDM") and (
-                only_print_filenames or no_progress_bar or not sys.stdout.isatty())
+            only_print_filenames or no_progress_bar or not sys.stdout.isatty())
         if skip_bar:
             photos_enumerator = photos
             logger.set_tqdm(None)
@@ -725,8 +724,9 @@ def should_break(counter):
                     )
                     break
                 item = next(photos_iterator)
-                download_photo(consecutive_files_found, item)
-                if delete_after_download:
+                if download_photo(
+                        consecutive_files_found,
+                        item) and delete_after_download:
                     delete_photo(item)
             except StopIteration:
                 break
@@ -742,7 +742,8 @@ def should_break(counter):
         if watch_interval:
             logger.info(f"Waiting for {watch_interval} sec...")
             interval = range(1, watch_interval)
-            for _ in interval if skip_bar else tqdm(interval, desc="Waiting...", ascii=True):
+            for _ in interval if skip_bar else tqdm(
+                    interval, desc="Waiting...", ascii=True):
                 time.sleep(1)
         else:
             break

icloudpd/download.py

@@ -27,16 +27,19 @@ def update_mtime(photo, download_path):
             return
         set_utime(download_path, created_date)
 
+
 def set_utime(download_path, created_date):
     """Set date & time of the file"""
     ctime = time.mktime(created_date.timetuple())
     os.utime(download_path, (ctime, ctime))
 
+
 def download_media(icloud, photo, download_path, size):
     """Download the photo to path, with retries and error handling"""
     logger = setup_logger()
 
-    # get back the directory for the file to be downloaded and create it if not there already
+    # get back the directory for the file to be downloaded and create it if
+    # not there already
     download_dir = os.path.dirname(download_path)
 
     if not os.path.exists(download_dir):

icloudpd/email_notifications.py

@@ -8,11 +8,17 @@
 
 
 def send_2sa_notification(
-        smtp_email, smtp_password, smtp_host, smtp_port, smtp_no_tls, to_addr, from_addr = None
-):
+        smtp_email,
+        smtp_password,
+        smtp_host,
+        smtp_port,
+        smtp_no_tls,
+        to_addr,
+        from_addr=None):
     """Send an email notification when 2SA is expired"""
     to_addr = to_addr if to_addr else smtp_email
-    from_addr = from_addr or (f"iCloud Photos Downloader <{smtp_email}>" if smtp_email else to_addr)
+    from_addr = from_addr or (
+        f"iCloud Photos Downloader <{smtp_email}>" if smtp_email else to_addr)
     logger = setup_logger()
     logger.info("Sending 'two-step expired' notification via email...")
     smtp = smtplib.SMTP(smtp_host, smtp_port)

tests/test_download_photos.py

@@ -1456,7 +1456,7 @@ def test_download_after_delete(self):
                 "icloudpd.exif_datetime.get_photo_exif"
             ) as get_exif_patched:
                 get_exif_patched.return_value = False
-                with vcr.use_cassette("tests/vcr_cassettes/listing_photos.yml"):
+                with vcr.use_cassette("tests/vcr_cassettes/listing_photos.yml") as cass:
                     # Pass fixed client ID via environment variable
                     runner = CliRunner(env={
                         "CLIENT_ID": "DE309E26-942E-11E8-92F5-14109FE0B321"
@@ -1497,6 +1497,7 @@ def test_download_after_delete(self):
                     self.assertIn(
                         "INFO     All photos have been downloaded!", self._caplog.text
                     )
+                    assert cass.all_played
                     assert result.exit_code == 0
 
         files_in_result = glob.glob(os.path.join(base_dir, "**/*.*"), recursive=True)
@@ -1506,6 +1507,60 @@ def test_download_after_delete(self):
         for file_name in files_to_download:
             assert os.path.exists(os.path.join(base_dir, os.path.normpath(file_name))), f"File {file_name} expected, but does not exist"
 
+    def test_download_after_delete_fail(self):
+        base_dir = os.path.normpath(f"tests/fixtures/Photos/{inspect.stack()[0][3]}")
+        if os.path.exists(base_dir):
+            shutil.rmtree(base_dir)
+        os.makedirs(base_dir)
+
+        with vcr.use_cassette("tests/vcr_cassettes/listing_photos_no_delete.yml") as cass:
+            # Pass fixed client ID via environment variable
+            runner = CliRunner(env={
+                "CLIENT_ID": "DE309E26-942E-11E8-92F5-14109FE0B321"
+            })
+            result = runner.invoke(
+                main,
+                [
+                    "--username",
+                    "[email protected]",
+                    "--password",
+                    "password1",
+                    "--recent",
+                    "1",
+                    "--skip-videos",
+                    "--skip-live-photos",
+                    "--no-progress-bar",
+                    "--threads-num",
+                    1,
+                    "--delete-after-download",
+                    "-d",
+                    base_dir,
+                ],
+            )
+            print_result_exception(result)
+
+            self.assertIn("DEBUG    Looking up all photos from album All Photos...", self._caplog.text)
+            self.assertIn(
+                f"INFO     Downloading the first original photo to {base_dir} ...",
+                self._caplog.text,
+            )
+            self.assertIn(
+                f"INFO     Downloading {os.path.join(base_dir, os.path.normpath('2018/07/31/IMG_7409.JPG'))}",
+                self._caplog.text,
+            )
+            self.assertNotIn(
+                "INFO     Deleting IMG_7409.JPG", self._caplog.text
+            )
+            self.assertIn(
+                "INFO     All photos have been downloaded!", self._caplog.text
+            )
+            assert cass.all_played
+            assert result.exit_code == 0
+
+        files_in_result = glob.glob(os.path.join(base_dir, "**/*.*"), recursive=True)
+
+        assert sum(1 for _ in files_in_result) == 0
+
     def test_download_over_old_original_photos(self):
         base_dir = os.path.normpath(f"tests/fixtures/Photos/{inspect.stack()[0][3]}")
         if os.path.exists(base_dir):