From e59dcc142df09d54d0a292f7e639069f3e21ed62 Mon Sep 17 00:00:00 2001
From: Dmitry S <dsagal+git@gmail.com>
Date: Tue, 26 Apr 2022 01:50:57 -0400
Subject: [PATCH] (core) Show proper message on empty Excel import, rather than
 a code error

Summary:
- Previously showed "UnboundLocalError". Now will show:
    Import failed: Failed to parse Excel file.
    Error: No tables found (1 empty tables skipped)
- Also fix logging for import code

Test Plan: Added a test case

Reviewers: georgegevoian

Reviewed By: georgegevoian

Differential Revision: https://phab.getgrist.com/D3396
---
 app/client/components/PluginScreen.ts |  1 +
 app/server/lib/DocPluginManager.ts    |  2 +-
 sandbox/grist/imports/import_xls.py   | 10 ++++++++--
 sandbox/grist/main.py                 |  4 ++++
 4 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/app/client/components/PluginScreen.ts b/app/client/components/PluginScreen.ts
index 3aad4073..5a0f9dd6 100644
--- a/app/client/components/PluginScreen.ts
+++ b/app/client/components/PluginScreen.ts
@@ -111,6 +111,7 @@ const cssModalBody = styled('div', `
   padding: 16px 0;
   overflow-y: auto;
   max-width: 470px;
+  white-space: pre-line;
 `);
 
 const cssModalHeader = styled('div', `
diff --git a/app/server/lib/DocPluginManager.ts b/app/server/lib/DocPluginManager.ts
index dc70c96f..e02933d9 100644
--- a/app/server/lib/DocPluginManager.ts
+++ b/app/server/lib/DocPluginManager.ts
@@ -141,7 +141,7 @@ export class DocPluginManager {
         '.csv' : 'CSV',
       };
       const fileType = extToType[path.extname(fileName)] || path.extname(fileName);
-      throw new Error(`Failed to parse ${fileType} file. Error: ${messages.join("; ")}`);
+      throw new Error(`Failed to parse ${fileType} file.\nError: ${messages.join("; ")}`);
     }
     throw new Error(`File format is not supported.`);
   }
diff --git a/sandbox/grist/imports/import_xls.py b/sandbox/grist/imports/import_xls.py
index f7b62635..93013e25 100644
--- a/sandbox/grist/imports/import_xls.py
+++ b/sandbox/grist/imports/import_xls.py
@@ -40,7 +40,7 @@ def parse_file(file_path, orig_name, parse_options=None, table_name_hint=None, n
       return parse_open_file(f, orig_name, table_name_hint=table_name_hint)
     except Exception as e:
       # Log the full error, but simplify the thrown error to omit the unhelpful extra args.
-      log.info("import_xls parse_file failed: %s", e)
+      log.warn("import_xls parse_file failed: %s", e)
       if six.PY2 and e.args and isinstance(e.args[0], six.string_types):
         raise Exception(e.args[0])
       raise
@@ -58,6 +58,7 @@ def parse_open_file(file_obj, orig_name, table_name_hint=None):
     if table_set.encoding == 'ascii':
       table_set.encoding = 'utf8'
 
+  skipped_tables = 0
   export_list = []
   # A table set is a collection of tables:
   for row_set in table_set.tables:
@@ -101,6 +102,7 @@ def parse_open_file(file_obj, orig_name, table_name_hint=None):
 
     if not table_data:
       # Don't add tables with no columns.
+      skipped_tables += 1
       continue
 
     log.info("Output table %r with %d columns", table_name, len(column_metadata))
@@ -112,8 +114,12 @@ def parse_open_file(file_obj, orig_name, table_name_hint=None):
       "table_data": table_data
     })
 
-    parse_options = {}
+  if not export_list:
+    if skipped_tables:
+      raise Exception("No tables found ({} empty tables skipped)".format(skipped_tables))
+    raise Exception("No tables found")
 
+  parse_options = {}
   return parse_options, export_list
 
 
diff --git a/sandbox/grist/main.py b/sandbox/grist/main.py
index 0f75a514..b0604b91 100644
--- a/sandbox/grist/main.py
+++ b/sandbox/grist/main.py
@@ -7,6 +7,7 @@ import sys
 sys.path.append('thirdparty')
 # pylint: disable=wrong-import-position
 
+import logging
 import marshal
 import functools
 
@@ -25,6 +26,9 @@ from imports.register import register_import_parsers
 import logger
 log = logger.Logger(__name__, logger.INFO)
 
+# Configure logging module to behave similarly to logger. (It may be OK to get rid of logger.)
+logging.basicConfig(format="[%(levelname)s] [%(name)s] %(message)s")
+
 def table_data_from_db(table_name, table_data_repr):
   if table_data_repr is None:
     return actions.TableData(table_name, [], {})