Merge pull request #239 from dimagi/mk/improve-det-errors

Improve DET errors

commcare_export/cli.py

@@ -186,6 +186,12 @@ def main(argv):
 
     args = parser.parse_args(argv)
 
+    if args.output_format and args.output:
+        errors = []
+        errors.extend(validate_output_filename(args.output_format, args.output))
+        if errors:
+            raise Exception(f"Could not proceed. Following issues were found: {', '.join(errors)}.")
+
     if not args.no_logfile:
         exe_dir = os.path.dirname(sys.executable)
         log_file = os.path.join(exe_dir, "commcare_export.log")
@@ -254,6 +260,23 @@ def main(argv):
             stats.print_stats(100)
 
 
+def validate_output_filename(output_format, output_filename):
+    """
+    Validate file extensions for csv, xls and xlsx output formats.
+    Ensure extension unless using sql output_format.
+    """
+    errors = []
+    if output_format == 'csv' and not output_filename.endswith('.zip'):
+        errors.append("For output format as csv, output file name should have extension zip")
+    elif output_format == 'xls' and not output_filename.endswith('.xls'):
+        errors.append("For output format as xls, output file name should have extension xls")
+    elif output_format == 'xlsx' and not output_filename.endswith('.xlsx'):
+        errors.append("For output format as xlsx, output file name should have extension xlsx")
+    elif output_format != 'sql' and "." not in output_filename:
+        errors.append("Missing extension in output file name")
+    return errors
+
+
 def _get_query(args, writer, column_enforcer=None):
     return _get_query_from_file(
         args.query,

commcare_export/commcare_hq_client.py

@@ -164,7 +164,8 @@ def _get(resource, params=None):
                             logger.error(
                                 f"#{e}. Please ensure that your CommCare HQ credentials are correct and auth-mode "
                                 f"is passed as 'apikey' if using API Key to authenticate. Also, verify that your "
-                                f"account has the necessary permissions to use commcare-export."
+                                f"account has access to the project and the necessary permissions to use "
+                                f"commcare-export."
                             )
                         else:
                             logger.error(str(e))

tests/test_cli.py

@@ -16,7 +16,7 @@
     CheckpointManager,
     session_scope,
 )
-from commcare_export.cli import CLI_ARGS, main_with_args
+from commcare_export.cli import CLI_ARGS, main_with_args, validate_output_filename
 from commcare_export.commcare_hq_client import (
     CommCareHqClient,
     MockCommCareHqClient,
@@ -911,3 +911,71 @@ def _check_checkpoint(
         assert checkpoint.pagination_mode == pagination_mode
         assert checkpoint.since_param == since_param
         assert checkpoint.last_doc_id == doc_id
+
+
+class TestValidateOutputFilename(unittest.TestCase):
+    def _test_file_extension(self, output_format, expected_extension):
+        error_message = (f"For output format as {output_format}, "
+                         f"output file name should have extension {expected_extension}")
+
+        errors = validate_output_filename(
+            output_format=output_format,
+            output_filename=f'correct_file_extension.{expected_extension}'
+        )
+        self.assertEqual(len(errors), 0)
+
+        errors = validate_output_filename(
+            output_format=output_format,
+            output_filename=f'incorrect_file_extension.abc'
+        )
+        self.assertListEqual(
+            errors,
+            [error_message]
+        )
+
+        # incorrectly using sql output with non sql formats
+        errors = validate_output_filename(
+            output_format=output_format,
+            output_filename='postgresql+psycopg2://scott:tiger@localhost/mydatabase'
+        )
+        self.assertListEqual(
+            errors,
+            [error_message]
+        )
+
+    def test_for_csv_output(self):
+        self._test_file_extension(output_format='csv', expected_extension='zip')
+
+    def test_for_xls_output(self):
+        self._test_file_extension(output_format='xls', expected_extension='xls')
+
+    def test_for_xlsx_output(self):
+        self._test_file_extension(output_format='xlsx', expected_extension='xlsx')
+
+    def test_for_other_non_sql_output(self):
+        error_message = "Missing extension in output file name"
+
+        errors = validate_output_filename(
+            output_format='non_sql',
+            output_filename='correct_file.abc'
+        )
+        self.assertEqual(len(errors), 0)
+
+        errors = validate_output_filename(
+            output_format='non_sql',
+            output_filename='filename_without_extensionxls'
+        )
+        self.assertListEqual(
+            errors,
+            [error_message]
+        )
+
+        # incorrectly using sql output with non sql output formats
+        errors = validate_output_filename(
+            output_format='non_sql',
+            output_filename='postgresql+psycopg2://scott:tiger@localhost/mydatabase'
+        )
+        self.assertListEqual(
+            errors,
+            [error_message]
+        )

tests/test_commcare_hq_client.py

@@ -329,8 +329,8 @@ def test_get_with_forbidden_response_in_non_debug_mode(self, session_mock, logge
         logger_mock.error.assert_called_once_with(
             "#401 Client Error: None for url: None. "
             "Please ensure that your CommCare HQ credentials are correct and auth-mode is passed as 'apikey' "
-            "if using API Key to authenticate. Also, verify that your account has the necessary permissions "
-            "to use commcare-export.")
+            "if using API Key to authenticate. Also, verify that your account has access to the project "
+            "and the necessary permissions to use commcare-export.")
 
     @patch('commcare_export.commcare_hq_client.logger')
     @patch("commcare_export.commcare_hq_client.CommCareHqClient.session")