From d2f98b40365d66a25286b7e96b92983445b759e2 Mon Sep 17 00:00:00 2001
From: Cyprien P <cyprien.pindat@gmail.com>
Date: Fri, 27 Jan 2023 10:33:15 +0100
Subject: [PATCH] (core) Fix the empty values last option in multicol sorting

Summary:
The "Empty values last" options was breaking sort when multiple
columns are involved in the sort spec. Problem comes from wrong
handling of equals.

Diff fixes that issue and update test.

Test Plan: Updates nbrowser test

Reviewers: jarek

Reviewed By: jarek

Differential Revision: https://phab.getgrist.com/D3774
---
 app/common/SortFunc.ts  | 12 ++++++++----
 test/common/SortFunc.ts | 41 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/app/common/SortFunc.ts b/app/common/SortFunc.ts
index 1bfc7098..36fb7598 100644
--- a/app/common/SortFunc.ts
+++ b/app/common/SortFunc.ts
@@ -38,11 +38,15 @@ function naturalCompare(val1: any, val2: any) {
 /**
  * Empty comparator will treat empty values as last.
  */
-const emptyCompare = (next: Comparator) => (val1: any, val2: any) => {
-  if (!val1 && typeof val1 !== 'number') {
-      return 1;
+export const emptyCompare = (next: Comparator) => (val1: any, val2: any) => {
+  const isEmptyValue1 = !val1 && typeof val1 !== 'number';
+  const isEmptyValue2 = !val2 && typeof val2 !== 'number';
+
+  // If both values are empty values, rely on next to compare.
+  if (isEmptyValue1 && !isEmptyValue2) {
+    return 1;
   }
-  if (!val2 && typeof val2 !== 'number') {
+  if (isEmptyValue2 && !isEmptyValue1) {
     return -1;
   }
   return next(val1, val2);
diff --git a/test/common/SortFunc.ts b/test/common/SortFunc.ts
index bb722de2..4f5ff4b5 100644
--- a/test/common/SortFunc.ts
+++ b/test/common/SortFunc.ts
@@ -1,4 +1,4 @@
-import {typedCompare} from 'app/common/SortFunc';
+import {emptyCompare, typedCompare} from 'app/common/SortFunc';
 import {assert} from 'chai';
 import {format} from 'util';
 
@@ -26,4 +26,43 @@ describe('SortFunc', function() {
       }
     }
   });
+
+  it('typedCompare should treat empty values as equal', function() {
+    assert.equal(typedCompare(null, null), 0);
+    assert.equal(typedCompare('', ''), 0);
+  });
+
+  describe('emptyCompare', function() {
+
+    it('should work correctly ', function() {
+      const comparator = emptyCompare(typedCompare);
+      assert.equal(comparator(null, null), 0);
+      assert.equal(comparator('', ''), 0);
+
+      assert.equal(comparator(null, 0), 1);
+      assert.equal(comparator(null, -1), 1);
+      assert.equal(comparator(null, 1), 1);
+      assert.equal(comparator(null, 'a'), 1);
+      assert.equal(comparator(null, 'z'), 1);
+
+      assert.equal(comparator(0, null), -1);
+      assert.equal(comparator(-1, null), -1);
+      assert.equal(comparator(1, null), -1);
+      assert.equal(comparator('a', null), -1);
+      assert.equal(comparator('z', null), -1);
+    });
+
+    it('should keep sorting order consistent amongst empty values', function() {
+      // values1 and values2 have same values but in different order. Sorting them with emptyCompare
+      // function should yield same results.
+      const values1 = ['', null, undefined, 2, 3, 4];
+      const values2 = [undefined, null, '', 2, 3, 4];
+      const comparator = emptyCompare(typedCompare);
+      values1.sort(comparator);
+      values2.sort(comparator);
+      assert.deepEqual(values1, values2);
+    });
+
+  });
+
 });