export plugin: warning before overwriting files (#2783)

* Overwrite prompt as overlay

* Address review comment and fix tests

* Add test

* TST: TestExportPluginPlots get out of _jail
free and collect 200 dollars

* Throw exception by default from API

* Fix PEP 8

* Clear warning when change filename

jdaviz/configs/default/plugins/export/export.py

@@ -92,6 +92,8 @@ class Export(PluginTemplateMixin, ViewerSelectMixin, SubsetSelectMixin,
     movie_recording = Bool(False).tag(sync=True)
     movie_interrupt = Bool(False).tag(sync=True)
 
+    overwrite_warn = Bool(False).tag(sync=True)
+
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
 
@@ -231,6 +233,11 @@ def _disable_viewer_format_combo(self, event):
             msg = ""
         self.viewer_invalid_msg = msg
 
+    @observe('filename')
+    def _is_filename_changed(self, event):
+        # Clear overwrite warning when user changes filename
+        self.overwrite_warn = False
+
     def _set_subset_not_supported_msg(self, msg=None):
         """
         Check if selected subset is spectral or composite, and warn and
@@ -265,7 +272,7 @@ def _set_dataset_not_supported_msg(self, msg=None):
         else:
             self.data_invalid_msg = ''
 
-    def _normalize_filename(self, filename=None, filetype=None):
+    def _normalize_filename(self, filename=None, filetype=None, overwrite=False):
         # Make sure filename is valid and file does not end up in weird places in standalone mode.
         if not filename:
             raise ValueError("Invalid filename")
@@ -276,22 +283,40 @@ def _normalize_filename(self, filename=None, filetype=None):
             filename = Path(filename).expanduser()
 
         filepath = filename.parent
-        if filepath and not filepath.exists():
+        if filepath and not filepath.is_dir():
             raise ValueError(f"Invalid path={filepath}")
         elif ((not filepath or str(filepath).startswith(".")) and os.environ.get("JDAVIZ_START_DIR", "")):  # noqa: E501 # pragma: no cover
             filename = os.environ["JDAVIZ_START_DIR"] / filename
 
+        if filename.exists() and not overwrite:
+            self.overwrite_warn = True
+        else:
+            self.overwrite_warn = False
+
         return str(filename)
 
     @with_spinner()
-    def export(self, filename=None, show_dialog=None):
+    def export(self, filename=None, show_dialog=None, overwrite=False,
+               raise_error_for_overwrite=True):
         """
         Export selected item(s)
 
         Parameters
         ----------
         filename : str, optional
             If not provided, plugin value will be used.
+
+        show_dialog : bool or `None`
+            If `True`, prompts dialog to save PNG/SVG from browser.
+
+        overwrite : bool
+            If `True`, silently overwrite an existing file.
+
+        raise_error_for_overwrite : bool
+            If `True`, raise exception when ``overwrite=False`` but
+            output file already exists. Otherwise, a message will be sent
+            to application snackbar instead.
+
         """
 
         if self.multiselect:
@@ -309,7 +334,12 @@ def export(self, filename=None, show_dialog=None):
             viewer = self.viewer.selected_obj
 
             filetype = self.viewer_format.selected
-            filename = self._normalize_filename(filename, filetype)
+            filename = self._normalize_filename(filename, filetype, overwrite=overwrite)
+
+            if self.overwrite_warn and not overwrite:
+                if raise_error_for_overwrite:
+                    raise FileExistsError(f"{filename} exists but overwrite=False")
+                return
 
             if filetype == "mp4":
                 self.save_movie(viewer, filename, filetype)
@@ -336,21 +366,33 @@ def export(self, filename=None, show_dialog=None):
         elif len(self.plugin_table.selected):
             filetype = self.plugin_table_format.selected
             filename = self._normalize_filename(filename, filetype)
+            if self.overwrite_warn and not overwrite:
+                if raise_error_for_overwrite:
+                    raise FileExistsError(f"{filename} exists but overwrite=False")
+                return
             self.plugin_table.selected_obj.export_table(filename, overwrite=True)
 
         elif len(self.subset.selected):
             selected_subset_label = self.subset.selected
             filetype = self.subset_format.selected
-            filename = self._normalize_filename(filename, filetype)
+            filename = self._normalize_filename(filename, filetype, overwrite=overwrite)
             if self.subset_invalid_msg != '':
                 raise NotImplementedError(f'Subset can not be exported - {self.subset_invalid_msg}')
+            if self.overwrite_warn and not overwrite:
+                if raise_error_for_overwrite:
+                    raise FileExistsError(f"{filename} exists but overwrite=False")
+                return
             self.save_subset_as_region(selected_subset_label, filename)
 
         elif len(self.dataset.selected):
             filetype = self.dataset_format.selected
-            filename = self._normalize_filename(filename, filetype)
+            filename = self._normalize_filename(filename, filetype, overwrite=overwrite)
             if self.data_invalid_msg != "":
                 raise NotImplementedError(f"Data can not be exported - {self.data_invalid_msg}")
+            if self.overwrite_warn and not overwrite:
+                if raise_error_for_overwrite:
+                    raise FileExistsError(f"{filename} exists but overwrite=False")
+                return
             self.dataset.selected_obj.write(Path(filename), overwrite=True)
         else:
             raise ValueError("nothing selected for export")
@@ -359,7 +401,7 @@ def export(self, filename=None, show_dialog=None):
 
     def vue_export_from_ui(self, *args, **kwargs):
         try:
-            filename = self.export(show_dialog=True)
+            filename = self.export(show_dialog=True, raise_error_for_overwrite=False)
         except Exception as e:
             self.hub.broadcast(SnackbarMessage(
                 f"Export failed with: {e}", sender=self, color="error"))
@@ -368,6 +410,19 @@ def vue_export_from_ui(self, *args, **kwargs):
                 self.hub.broadcast(SnackbarMessage(
                     f"Exported to {filename}", sender=self, color="success"))
 
+    def vue_overwrite_from_ui(self, *args, **kwargs):
+        """Attempt to force writing the output if the user confirms the desire to overwrite."""
+        try:
+            filename = self.export(show_dialog=True, overwrite=True,
+                                   raise_error_for_overwrite=False)
+        except Exception as e:
+            self.hub.broadcast(SnackbarMessage(
+                f"Export with overwrite failed with: {e}", sender=self, color="error"))
+        else:
+            if filename is not None:
+                self.hub.broadcast(SnackbarMessage(
+                    f"Exported to {filename} (overwrite)", sender=self, color="success"))
+
     def save_figure(self, viewer, filename=None, filetype="png", show_dialog=False):
 
         if filetype == "png":

jdaviz/configs/default/plugins/export/export.vue

@@ -220,6 +220,8 @@
       </v-row>
     </div>
 
+    <div style="display: grid; position: relative"> <!-- overlay container -->
+    <div style="grid-area: 1/1">
 
     <v-row style="margin-top: 24px">
         <v-text-field
@@ -254,6 +256,35 @@
       >
         Export
       </plugin-action-button>
+    </div>
+
+      <v-overlay
+        absolute
+        opacity=1.0
+        :value="overwrite_warn"
+        :zIndex=3
+        style="grid-area: 1/1;
+               margin-left: -24px;
+               margin-right: -24px">
+
+      <v-card color="transparent" elevation=0 >
+        <v-card-text width="100%">
+          <div class="white--text">
+            A file with this name is already on disk. Overwrite?
+          </div>
+        </v-card-text>
+
+        <v-card-actions>
+          <v-row justify="end">
+            <v-btn tile small color="primary" class="mr-2" @click="overwrite_warn=false">Cancel</v-btn>
+            <v-btn tile small color="accent" class="mr-4" @click="overwrite_from_ui">Overwrite</v-btn>
+          </v-row>
+        </v-card-actions>
+      </v-card>
+
+      </v-overlay>
+    </div>
+
     </v-row>
 
   </j-tray-plugin>

jdaviz/configs/default/plugins/export/tests/test_export.py

@@ -1,24 +1,25 @@
-import numpy as np
 import os
-import pytest
 import re
 
+import numpy as np
+import pytest
+from astropy import units as u
 from astropy.io import fits
 from astropy.nddata import NDData
-import astropy.units as u
 from glue.core.edit_subset_mode import AndMode, NewMode
 from glue.core.roi import CircularROI, XRangeROI
 from regions import Regions, CircleSkyRegion
 from specutils import Spectrum1D
 
 
-class TestExportSubsets():
+@pytest.mark.usefixtures('_jail')
+class TestExportSubsets:
     """
     Tests for exporting subsets. Currently limited to non-composite spatial
     subsets.
     """
 
-    def test_basic_export_subsets_imviz(self, tmp_path, imviz_helper):
+    def test_basic_export_subsets_imviz(self, imviz_helper):
 
         data = NDData(np.ones((500, 500)) * u.nJy)
 
@@ -94,7 +95,7 @@ def test_not_implemented(self, cubeviz_helper, spectral_cube_wcs):
         cubeviz_helper.app.get_viewer("spectrum-viewer").apply_roi(XRangeROI(5, 15.5))
         assert 'Subset 2' not in export_plugin.subset.choices
 
-    def test_export_subsets_wcs(self, tmp_path, imviz_helper, spectral_cube_wcs):
+    def test_export_subsets_wcs(self, imviz_helper, spectral_cube_wcs):
 
         # using cube WCS instead of 2d imaging wcs for consistancy with
         # cubeviz test. accessing just the spatial part of this.
@@ -125,7 +126,7 @@ def test_export_subsets_wcs(self, tmp_path, imviz_helper, spectral_cube_wcs):
 
         assert isinstance(Regions.read('sky_region.reg')[0], CircleSkyRegion)
 
-    def test_basic_export_subsets_cubeviz(self, tmp_path, cubeviz_helper, spectral_cube_wcs):
+    def test_basic_export_subsets_cubeviz(self, cubeviz_helper, spectral_cube_wcs):
 
         data = Spectrum1D(flux=np.ones((128, 128, 256)) * u.nJy, wcs=spectral_cube_wcs)
 
@@ -168,6 +169,25 @@ def test_basic_export_subsets_cubeviz(self, tmp_path, cubeviz_helper, spectral_c
         export_plugin.export()
         assert os.path.isfile('test.reg')
 
+        # Overwrite not enable, so no-op with warning.
+        export_plugin.export(raise_error_for_overwrite=False)
+        assert export_plugin.overwrite_warn
+
+        # Changing filename should clear warning.
+        old_filename = export_plugin.filename
+        export_plugin.filename = "foo"
+        assert not export_plugin.overwrite_warn
+        export_plugin.filename = old_filename
+
+        # Overwrite not enable, but with exception from API by default.
+        with pytest.raises(FileExistsError, match=".* exists but overwrite=False"):
+            export_plugin.export()
+        assert export_plugin.overwrite_warn
+
+        # User forces overwrite.
+        export_plugin.export(overwrite=True)
+        assert not export_plugin.overwrite_warn
+
         # test that invalid file extension raises an error
         with pytest.raises(ValueError,
                            match=re.escape("x not one of ['fits', 'reg'], reverting selection to reg")):  # noqa
@@ -183,6 +203,7 @@ def test_basic_export_subsets_cubeviz(self, tmp_path, cubeviz_helper, spectral_c
             export_plugin.export()
 
 
+@pytest.mark.usefixtures('_jail')
 def test_export_data(cubeviz_helper, spectrum1d_cube):
     cubeviz_helper.load_data(spectrum1d_cube, data_label='test')
     mm = cubeviz_helper.plugins["Moment Maps"]
@@ -224,9 +245,9 @@ def test_disable_export_for_unsupported_units(specviz2d_helper):
     assert ep.data_invalid_msg == "Export Disabled: The unit DN / s could not be saved in native FITS format."  # noqa
 
 
-class TestExportPluginPlots():
+class TestExportPluginPlots:
 
-    def test_basic_export_plugin_plots(tmp_path, imviz_helper):
+    def test_basic_export_plugin_plots(self, imviz_helper):
         """
         Test basic funcionality of exporting plugin plots
         from the export plugin. Tests on the 'Plot Options: stretch_hist'
@@ -259,7 +280,7 @@ def test_basic_export_plugin_plots(tmp_path, imviz_helper):
         assert len(available_plots) == 1
         assert available_plots[0] == 'Plot Options: stretch_hist'
 
-    def test_ap_phot_plot_export(tmp_path, imviz_helper):
+    def test_ap_phot_plot_export(self, imviz_helper):
 
         """
         Test export functionality for plot from the aperture photometry

jdaviz/conftest.py

@@ -3,6 +3,7 @@
 # get picked up when running the tests inside an interpreter using
 # packagename.test
 
+import os
 import warnings
 
 import numpy as np
@@ -334,6 +335,18 @@ def roman_imagemodel():
         return create_wfi_image_model((20, 10))
 
 
+# Copied over from https://github.com/spacetelescope/ci_watson
+@pytest.fixture(scope='function')
+def _jail(tmp_path):
+    """Perform test in a pristine temporary working directory."""
+    old_dir = os.getcwd()
+    os.chdir(tmp_path)
+    try:
+        yield str(tmp_path)
+    finally:
+        os.chdir(old_dir)
+
+
 try:
     from pytest_astropy_header.display import PYTEST_HEADER_MODULES, TESTED_VERSIONS
 except ImportError: