Archives/gristlabs_grist-core
1
0
Fork 0
mirror of https://github.com/gristlabs/grist-core.git synced 2024-10-27 20:44:07 +00:00
Code Issues Projects Releases Wiki Activity

(core) Better Max and Min shortcut funtions in the new column menu.

Browse Source 
Summary: Using the lookup shortcut in the add column menu to find a max or min value is a buggy experience. MAX and MIN returns 0 for empty collections which can be interpreted as 1970.1.1 in date or date time columns. It is better to just return None (empty cell) when there are no records.

Test Plan: Updated

Reviewers: paulfitz

Reviewed By: paulfitz

Differential Revision: https://phab.getgrist.com/D4206
This commit is contained in:
Jarosław Sadziński 2024-03-08 17:29:26 +01:00
parent 14125b9b97
commit 0703c8527b
2 changed files with 39 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
app/client/ui/GridViewMenus.ts
View File

@ -426,13 +426,17 @@ function buildLookupSection(gridView: GridView, index?: number){
function formula() { function formula() {
switch(fun) { switch(fun) {
case 'list': return `${referenceToSource}.${col.colId()}`; case 'list': return `${referenceToSource}.${col.colId()}`;
case 'average': return `AVERAGE(${referenceToSource}.${col.colId()})`; case 'average': return `ref = ${referenceToSource}\n` +
case 'min': return `MIN(${referenceToSource}.${col.colId()})`; `AVERAGE(ref.${col.colId()}) if ref else None`;
case 'max': return `MAX(${referenceToSource}.${col.colId()})`; case 'min': return `ref = ${referenceToSource}\n` +
`MIN(ref.${col.colId()}) if ref else None`;
case 'max': return `ref = ${referenceToSource}\n` +
`MAX(ref.${col.colId()}) if ref else None`;
case 'count': case 'count':
case 'sum': return `SUM(${referenceToSource}.${col.colId()})`; case 'sum': return `SUM(${referenceToSource}.${col.colId()})`;
case 'percent': case 'percent':
return `AVERAGE(map(int, ${referenceToSource}.${col.colId()})) if ${referenceToSource} else None`; return `ref = ${referenceToSource}\n` +
`AVERAGE(map(int, ref.${col.colId()})) if ref else None`;
default: return `${referenceToSource}`; default: return `${referenceToSource}`;
} }
} }

61
test/nbrowser/GridViewNewColumnMenu.ts
View File

@ -150,7 +150,7 @@ describe('GridViewNewColumnMenu', function () {
describe('create column with type', function () { describe('create column with type', function () {
revertThis(); revertThis();
const columsThatShouldTriggerSideMenu = [ const columnsThatShouldTriggerSideMenu = [
"Reference", "Reference",
"Reference List" "Reference List"
]; ];
@ -176,7 +176,7 @@ describe('GridViewNewColumnMenu', function () {
it('should show "Add Column With type" option', async function () { it('should show "Add Column With type" option', async function () {
// open add new colum menu // open add new colum menu
await clickAddColumn(); await clickAddColumn();
// check if "Add Column With type" option is persent // check if "Add Column With type" option is present
const addWithType = await driver.findWait( const addWithType = await driver.findWait(
'.test-new-columns-menu-add-with-type', '.test-new-columns-menu-add-with-type',
100, 100,
@ -238,7 +238,7 @@ describe('GridViewNewColumnMenu', function () {
} }
for (const optionsTriggeringMenu of optionsToBeDisplayed.filter((option) => for (const optionsTriggeringMenu of optionsToBeDisplayed.filter((option) =>
columsThatShouldTriggerSideMenu.includes(option.type))) { columnsThatShouldTriggerSideMenu.includes(option.type))) {
it(`should open Right Menu on Column section after choosing ${optionsTriggeringMenu.type}`, async function(){ it(`should open Right Menu on Column section after choosing ${optionsTriggeringMenu.type}`, async function(){
await gu.enableTips(session.email); await gu.enableTips(session.email);
//close right panel just in case. //close right panel just in case.
@ -259,7 +259,7 @@ describe('GridViewNewColumnMenu', function () {
await gu.waitForServer(); await gu.waitForServer();
//discard rename menu //discard rename menu
await driver.findWait('.test-column-title-close', STANDARD_WAITING_TIME).click(); await driver.findWait('.test-column-title-close', STANDARD_WAITING_TIME).click();
// Wait for the sidepanel animation. // Wait for the side panel animation.
await gu.waitForSidePanel(); await gu.waitForSidePanel();
//check if right menu is opened on column section //check if right menu is opened on column section
assert.isTrue(await driver.findWait('.test-right-tab-field', 1000).isDisplayed()); assert.isTrue(await driver.findWait('.test-right-tab-field', 1000).isDisplayed());
@ -308,7 +308,7 @@ describe('GridViewNewColumnMenu', function () {
describe('on mobile', function () { describe('on mobile', function () {
gu.narrowScreen(); gu.narrowScreen();
for (const optionsTriggeringMenu of optionsToBeDisplayed.filter((option) => for (const optionsTriggeringMenu of optionsToBeDisplayed.filter((option) =>
columsThatShouldTriggerSideMenu.includes(option.type))) { columnsThatShouldTriggerSideMenu.includes(option.type))) {
it('should not show Right Menu when user is on the mobile/narrow screen', async function() { it('should not show Right Menu when user is on the mobile/narrow screen', async function() {
await gu.enableTips(session.email); await gu.enableTips(session.email);
//close right panel just in case. //close right panel just in case.
@ -378,7 +378,7 @@ describe('GridViewNewColumnMenu', function () {
await driver.findWait('.test-new-columns-menu-add-formula', STANDARD_WAITING_TIME).click(); await driver.findWait('.test-new-columns-menu-add-formula', STANDARD_WAITING_TIME).click();
//check if new column is present //check if new column is present
await gu.waitForServer(); await gu.waitForServer();
// there should not be a rename poup // there should not be a rename popup
assert.isFalse(await driver.find('test-column-title-popup').isPresent()); assert.isFalse(await driver.find('test-column-title-popup').isPresent());
// check if editor popup is opened // check if editor popup is opened
await driver.findWait('.test-floating-editor-popup', 200, 'Editor popup is not present'); await driver.findWait('.test-floating-editor-popup', 200, 'Editor popup is not present');
@ -797,15 +797,15 @@ describe('GridViewNewColumnMenu', function () {
// For this column test other aggregations as well. // For this column test other aggregations as well.
await gu.undo(); await gu.undo();
await addRefListLookup('Employees', column, 'average'); await addRefListLookup('Employees', column, 'average');
await checkTypeAndFormula('Numeric', `AVERAGE($Employees.${colId})`); await checkTypeAndFormula('Numeric', AVERAGE('$Employees', colId));
await gu.undo(); await gu.undo();
await addRefListLookup('Employees', column, 'min'); await addRefListLookup('Employees', column, 'min');
await checkTypeAndFormula('Numeric', `MIN($Employees.${colId})`); await checkTypeAndFormula('Numeric', MIN('$Employees', colId));
await gu.undo(); await gu.undo();
await addRefListLookup('Employees', column, 'max'); await addRefListLookup('Employees', column, 'max');
await checkTypeAndFormula('Numeric', `MAX($Employees.${colId})`); await checkTypeAndFormula('Numeric', MAX('$Employees', colId));
break; break;
case "Member": case "Member":
@ -813,7 +813,7 @@ describe('GridViewNewColumnMenu', function () {
// Here we also test that the formula is correct for percent. // Here we also test that the formula is correct for percent.
await gu.undo(); await gu.undo();
await addRefListLookup('Employees', column, 'percent'); await addRefListLookup('Employees', column, 'percent');
await checkTypeAndFormula('Numeric', `AVERAGE(map(int, $Employees.Member)) if $Employees else None`); await checkTypeAndFormula('Numeric', PERCENT('$Employees', colId));
assert.isTrue( assert.isTrue(
await driver.findContent('.test-numeric-mode .test-select-button', /%/).matches('[class*=-selected]')); await driver.findContent('.test-numeric-mode .test-select-button', /%/).matches('[class*=-selected]'));
break; break;
@ -821,12 +821,12 @@ describe('GridViewNewColumnMenu', function () {
await checkTypeAndFormula('Any', `$Employees.${colId}`); await checkTypeAndFormula('Any', `$Employees.${colId}`);
await gu.undo(); await gu.undo();
await addRefListLookup('Employees', column, 'min'); await addRefListLookup('Employees', column, 'min');
await checkTypeAndFormula('DateTime', `MIN($Employees.${colId})`); await checkTypeAndFormula('DateTime', MIN('$Employees', colId));
assert.equal(await driver.find(".test-tz-autocomplete input").value(), 'UTC'); assert.equal(await driver.find(".test-tz-autocomplete input").value(), 'UTC');
await gu.undo(); await gu.undo();
await addRefListLookup('Employees', column, 'max'); await addRefListLookup('Employees', column, 'max');
await checkTypeAndFormula('DateTime', `MAX($Employees.${colId})`); await checkTypeAndFormula('DateTime', MAX('$Employees', colId));
assert.equal(await driver.find(".test-tz-autocomplete input").value(), 'UTC'); assert.equal(await driver.find(".test-tz-autocomplete input").value(), 'UTC');
break; break;
default: default:
@ -959,15 +959,15 @@ describe('GridViewNewColumnMenu', function () {
await gu.undo(); await gu.undo();
await addRevLookup('average'); await addRevLookup('average');
await checkTypeAndFormula('Numeric', `AVERAGE(Person.lookupRecords(Item=$id).Age)`); await checkTypeAndFormula('Numeric', AVERAGE(`Person.lookupRecords(Item=$id)`, 'Age'));
await gu.undo(); await gu.undo();
await addRevLookup('min'); await addRevLookup('min');
await checkTypeAndFormula('Numeric', `MIN(Person.lookupRecords(Item=$id).Age)`); await checkTypeAndFormula('Numeric', MIN(`Person.lookupRecords(Item=$id)`, 'Age'));
await gu.undo(); await gu.undo();
await addRevLookup('max'); await addRevLookup('max');
await checkTypeAndFormula('Numeric', `MAX(Person.lookupRecords(Item=$id).Age)`); await checkTypeAndFormula('Numeric', MAX(`Person.lookupRecords(Item=$id)`, 'Age'));
break; break;
case "Member": case "Member":
await addRevLookup('count'); await addRevLookup('count');
@ -975,9 +975,7 @@ describe('GridViewNewColumnMenu', function () {
await gu.undo(); await gu.undo();
await addRevLookup('percent'); await addRevLookup('percent');
await checkTypeAndFormula('Numeric', await checkTypeAndFormula('Numeric', PERCENT(`Person.lookupRecords(Item=$id)`, column));
`AVERAGE(map(int, Person.lookupRecords(Item=$id).Member))` +
` if Person.lookupRecords(Item=$id) else None`);
break; break;
case "Birthday date": case "Birthday date":
await addRevLookup('list'); await addRevLookup('list');
@ -985,11 +983,11 @@ describe('GridViewNewColumnMenu', function () {
await gu.undo(); await gu.undo();
await addRevLookup('min'); await addRevLookup('min');
await checkTypeAndFormula('Date', `MIN(Person.lookupRecords(Item=$id).Birthday_date)`); await checkTypeAndFormula('Date', MIN('Person.lookupRecords(Item=$id)', 'Birthday_date'));
await gu.undo(); await gu.undo();
await addRevLookup('max'); await addRevLookup('max');
await checkTypeAndFormula('Date', `MAX(Person.lookupRecords(Item=$id).Birthday_date)`); await checkTypeAndFormula('Date', MAX('Person.lookupRecords(Item=$id)', 'Birthday_date'));
assert.deepEqual(await gu.getColumnNames(), assert.deepEqual(await gu.getColumnNames(),
['A', 'B', 'C', 'Person_Birthday date']); ['A', 'B', 'C', 'Person_Birthday date']);
@ -997,7 +995,7 @@ describe('GridViewNewColumnMenu', function () {
break; break;
case "SeenAt": case "SeenAt":
await addRevLookup('max'); await addRevLookup('max');
await checkTypeAndFormula('DateTime', `MAX(Person.lookupRecords(Item=$id).SeenAt)`); await checkTypeAndFormula('DateTime', MAX('Person.lookupRecords(Item=$id)', 'SeenAt'));
// Here check the timezone. // Here check the timezone.
assert.equal(await driver.find(".test-tz-autocomplete input").value(), 'UTC'); assert.equal(await driver.find(".test-tz-autocomplete input").value(), 'UTC');
break; break;
@ -1076,15 +1074,15 @@ describe('GridViewNewColumnMenu', function () {
await gu.undo(); await gu.undo();
await addRevLookup('average'); await addRevLookup('average');
await checkTypeAndFormula('Numeric', `AVERAGE(Person.lookupRecords(Items=CONTAINS($id)).Age)`); await checkTypeAndFormula('Numeric', AVERAGE(`Person.lookupRecords(Items=CONTAINS($id))`, 'Age'));
await gu.undo(); await gu.undo();
await addRevLookup('min'); await addRevLookup('min');
await checkTypeAndFormula('Numeric', `MIN(Person.lookupRecords(Items=CONTAINS($id)).Age)`); await checkTypeAndFormula('Numeric', MIN(`Person.lookupRecords(Items=CONTAINS($id))`, 'Age'));
await gu.undo(); await gu.undo();
await addRevLookup('max'); await addRevLookup('max');
await checkTypeAndFormula('Numeric', `MAX(Person.lookupRecords(Items=CONTAINS($id)).Age)`); await checkTypeAndFormula('Numeric', MAX(`Person.lookupRecords(Items=CONTAINS($id))`, 'Age'));
break; break;
case "Member": case "Member":
await addRevLookup('count'); await addRevLookup('count');
@ -1092,9 +1090,7 @@ describe('GridViewNewColumnMenu', function () {
await gu.undo(); await gu.undo();
await addRevLookup('percent'); await addRevLookup('percent');
await checkTypeAndFormula('Numeric', await checkTypeAndFormula('Numeric', PERCENT(`Person.lookupRecords(Items=CONTAINS($id))`, column));
`AVERAGE(map(int, Person.lookupRecords(Items=CONTAINS($id)).Member))` +
` if Person.lookupRecords(Items=CONTAINS($id)) else None`);
break; break;
case "Birthday date": case "Birthday date":
await addRevLookup('list'); await addRevLookup('list');
@ -1102,15 +1098,15 @@ describe('GridViewNewColumnMenu', function () {
await gu.undo(); await gu.undo();
await addRevLookup('min'); await addRevLookup('min');
await checkTypeAndFormula('Date', `MIN(Person.lookupRecords(Items=CONTAINS($id)).Birthday_date)`); await checkTypeAndFormula('Date', MIN('Person.lookupRecords(Items=CONTAINS($id))', 'Birthday_date'));
await gu.undo(); await gu.undo();
await addRevLookup('max'); await addRevLookup('max');
await checkTypeAndFormula('Date', `MAX(Person.lookupRecords(Items=CONTAINS($id)).Birthday_date)`); await checkTypeAndFormula('Date', MAX('Person.lookupRecords(Items=CONTAINS($id))', 'Birthday_date'));
break; break;
case "SeenAt": case "SeenAt":
await addRevLookup('max'); await addRevLookup('max');
await checkTypeAndFormula('DateTime', `MAX(Person.lookupRecords(Items=CONTAINS($id)).SeenAt)`); await checkTypeAndFormula('DateTime', MAX('Person.lookupRecords(Items=CONTAINS($id))', 'SeenAt'));
// Here check the timezone. // Here check the timezone.
assert.equal(await driver.find(".test-tz-autocomplete input").value(), 'UTC'); assert.equal(await driver.find(".test-tz-autocomplete input").value(), 'UTC');
break; break;
@ -1518,3 +1514,8 @@ describe('GridViewNewColumnMenu', function () {
await gu.sendKeys(Key.ESCAPE); await gu.sendKeys(Key.ESCAPE);
} }
}); });
const PERCENT = (ref: string, col: string) => `ref = ${ref}\nAVERAGE(map(int, ref.${col})) if ref else None`;
const AVERAGE = (ref: string, col: string) => `ref = ${ref}\nAVERAGE(ref.${col}) if ref else None`;
const MIN = (ref: string, col: string) => `ref = ${ref}\nMIN(ref.${col}) if ref else None`;
const MAX = (ref: string, col: string) => `ref = ${ref}\nMAX(ref.${col}) if ref else None`;