Merge pull request #1461 from quadratichq/fix-display_value-blank

fix bug with display_value and CellValue::Blank

quadratic-client/src/app/web-workers/quadraticCore/worker/core.ts

@@ -811,24 +811,16 @@ class Core {
   getCells(
     id: number,
     transactionId: string,
-    x0: number,
-    y0: number,
-    x1: number,
-    y1: number,
+    x: number,
+    y: number,
+    w: number,
+    h: number,
     sheet?: string,
     lineNumber?: number
   ) {
     if (!this.gridController) throw new Error('Expected gridController to be defined');
     try {
-      const cellsStringified = this.gridController.calculationGetCells(
-        transactionId,
-        x0,
-        y0,
-        x1,
-        y1,
-        sheet,
-        lineNumber
-      );
+      const cellsStringified = this.gridController.calculationGetCells(transactionId, x, y, w, h, sheet, lineNumber);
       const cells = cellsStringified ? (JSON.parse(cellsStringified) as JsGetCellResponse[]) : undefined;
       corePython.sendGetCells(id, cells);
     } catch (e) {

quadratic-core/src/controller/operations/import.rs

@@ -152,7 +152,7 @@ impl GridController {
             for (y, row) in range.rows().enumerate() {
                 for (x, col) in row.iter().enumerate() {
                     let cell_value = match col {
-                        ExcelData::Empty => CellValue::Blank,
+                        ExcelData::Empty => continue,
                         ExcelData::String(value) => CellValue::Text(value.to_string()),
                         ExcelData::DateTimeIso(ref value) => CellValue::Text(value.to_string()),
                         ExcelData::DurationIso(ref value) => CellValue::Text(value.to_string()),
@@ -178,7 +178,7 @@ impl GridController {
                         ExcelData::Int(ref value) => {
                             CellValue::unpack_str_float(&value.to_string(), CellValue::Blank)
                         }
-                        ExcelData::Error(_) => CellValue::Blank,
+                        ExcelData::Error(_) => continue,
                         ExcelData::Bool(value) => CellValue::Logical(*value),
                     };
 
@@ -191,6 +191,7 @@ impl GridController {
                     );
                 }
             }
+            // add new sheets
             ops.push(Operation::AddSheetSchema {
                 schema: export_sheet(&sheet),
             });
@@ -382,4 +383,37 @@ mod test {
             Some(&CellValue::Text("city0".into()))
         );
     }
+
+    #[test]
+    fn import_excel() {
+        let mut gc = GridController::test_blank();
+        let file = include_bytes!("../../../test-files/simple.xlsx");
+        gc.import_excel(file.to_vec(), "simple.xlsx").unwrap();
+
+        let sheet_id = gc.grid.sheets()[0].id;
+        let sheet = gc.sheet(sheet_id);
+
+        assert_eq!(
+            sheet.cell_value((0, 0).into()),
+            Some(CellValue::Number(1.into()))
+        );
+        assert_eq!(
+            sheet.cell_value((2, 9).into()),
+            Some(CellValue::Number(12.into()))
+        );
+        assert_eq!(sheet.cell_value((0, 5).into()), None);
+        assert_eq!(
+            sheet.cell_value((3, 1).into()),
+            Some(CellValue::Number(3.into()))
+        );
+        assert_eq!(sheet.cell_value((3, 0).into()), None);
+    }
+
+    #[test]
+    fn import_excel_invalid() {
+        let mut gc = GridController::test_blank();
+        let file = include_bytes!("../../../test-files/invalid.xlsx");
+        let result = gc.import_excel(file.to_vec(), "invalid.xlsx");
+        assert!(result.is_err());
+    }
 }

quadratic-core/src/grid/sheet.rs

@@ -15,7 +15,7 @@ use super::js_types::CellFormatSummary;
 use super::{CodeRun, NumericFormat, NumericFormatKind};
 use crate::grid::{borders, SheetBorders};
 use crate::sheet_offsets::SheetOffsets;
-use crate::{Array, CellValue, IsBlank, Pos, Rect};
+use crate::{Array, CellValue, Pos, Rect};
 
 pub mod bounds;
 pub mod cell_array;
@@ -122,7 +122,7 @@ impl Sheet {
     /// and did not previously exist (so no change is needed).
     pub fn set_cell_value(&mut self, pos: Pos, value: impl Into<CellValue>) -> Option<CellValue> {
         let value = value.into();
-        let is_empty = value.is_blank();
+        let is_empty = value.is_blank_or_empty_string();
         let value: Option<CellValue> = if is_empty { None } else { Some(value) };
 
         // if there's no value and the column doesn't exist, then nothing more needs to be done
@@ -202,6 +202,7 @@ impl Sheet {
                     .code_runs
                     .get(&pos)
                     .and_then(|run| run.cell_value_at(0, 0)),
+                CellValue::Blank => self.get_code_cell_value(pos),
                 _ => Some(cell_value.clone()),
             }
         } else {
@@ -660,4 +661,13 @@ mod test {
         let new_column = sheet.get_or_create_column(1);
         assert_eq!(new_column, &Column::new(new_column.x));
     }
+
+    #[test]
+    fn display_value_blanks() {
+        let mut sheet = Sheet::test();
+        let pos = Pos { x: 0, y: 0 };
+        assert_eq!(sheet.display_value(pos), None);
+        sheet.set_cell_value(pos, CellValue::Blank);
+        assert_eq!(sheet.display_value(pos), None);
+    }
 }

quadratic-core/src/grid/sheet/bounds.rs

@@ -311,7 +311,7 @@ mod test {
     use crate::{
         controller::GridController,
         grid::{CellAlign, CodeCellLanguage, GridBounds, Sheet},
-        CellValue, IsBlank, Pos, Rect, SheetPos, SheetRect,
+        CellValue, Pos, Rect, SheetPos, SheetRect,
     };
     use proptest::proptest;
     use std::collections::HashMap;
@@ -591,7 +591,7 @@ mod test {
 
         let nonempty_positions = hashmap_of_truth
             .iter()
-            .filter(|(_, value)| !value.is_blank())
+            .filter(|(_, value)| !value.is_blank_or_empty_string())
             .map(|(pos, _)| pos);
         let min_x = nonempty_positions.clone().map(|pos| pos.x).min();
         let min_y = nonempty_positions.clone().map(|pos| pos.y).min();
@@ -607,7 +607,7 @@ mod test {
 
         for (pos, expected) in hashmap_of_truth {
             let actual = sheet.display_value(pos);
-            if expected.is_blank() {
+            if expected.is_blank_or_empty_string() {
                 assert_eq!(None, actual);
             } else {
                 assert_eq!(Some(expected.clone()), actual);

quadratic-core/test-files/invalid.xlsx

@@ -0,0 +1 @@
+dummy