From e43bb670a0fa03b2e8b1b3ca153621290e992bdd Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Thu, 10 Oct 2024 17:01:46 -0400 Subject: [PATCH] allow seed rules to refer to columns (#1254) This makes a small change to the UI for entering seed rules, so that a seed rule that refers to a column may be added. This is a bit niche, but occasionally handy when all tables have some common structure. --- app/client/aclui/AccessRules.ts | 28 +++++++++++- test/nbrowser/AccessRules3.ts | 76 +++++++++++++++++++++++++++++++++ test/nbrowser/aclTestUtils.ts | 8 +++- 3 files changed, 108 insertions(+), 4 deletions(-) diff --git a/app/client/aclui/AccessRules.ts b/app/client/aclui/AccessRules.ts index 167f27df26..edc1702c02 100644 --- a/app/client/aclui/AccessRules.ts +++ b/app/client/aclui/AccessRules.ts @@ -1084,6 +1084,16 @@ abstract class ObsRuleSet extends Disposable { public getCustomRules(): ObsRulePart[] { return this._body.get().filter(rule => !rule.isBuiltInOrEmpty()); } + + /** + * If the set applies to a special column, return its name. + */ + public getSpecialColumn(): string|undefined { + if (this._ruleSet?.tableId === SPECIAL_RULES_TABLE_ID && + this._ruleSet.colIds.length === 1) { + return this._ruleSet.colIds[0]; + } + } } class ColumnObsRuleSet extends ObsRuleSet { @@ -1635,6 +1645,14 @@ class ObsRulePart extends Disposable { !isEqual(use(this._permissions), this._rulePart?.permissions ?? emptyPerms) ); }); + // The formula may be invalid from the beginning. Make sure we show errors in this + // case. + const text = this._aclFormula.get(); + if (text) { + this._setAclFormula(text, true).catch(e => { + console.error(e); + }); + } } public getRulePart(): RuleRec { @@ -1790,8 +1808,8 @@ class ObsRulePart extends Disposable { return this.isBuiltIn() && this._ruleSet.getFirstBuiltIn() !== this; } - private async _setAclFormula(text: string) { - if (text === this._aclFormula.get()) { return; } + private async _setAclFormula(text: string, initial: boolean = false) { + if (text === this._aclFormula.get() && !initial) { return; } this._aclFormula.set(text); this._checkPending.set(true); this._formulaProperties.set({}); @@ -1809,6 +1827,12 @@ class ObsRulePart extends Disposable { private _warnInvalidColIds(colIds?: string[]) { if (!colIds || !colIds.length) { return false; } const allValid = new Set(this._ruleSet.getValidColIds()); + const specialColumn = this._ruleSet.getSpecialColumn(); + if (specialColumn === 'SeedRule') { + // We allow seed rules to refer to columns without checking + // them (until the seed rules are used). + return false; + } const invalid = colIds.filter(c => !allValid.has(c)); if (invalid.length > 0) { return `Invalid columns: ${invalid.join(', ')}`; diff --git a/test/nbrowser/AccessRules3.ts b/test/nbrowser/AccessRules3.ts index 678009ace6..3730ba2618 100644 --- a/test/nbrowser/AccessRules3.ts +++ b/test/nbrowser/AccessRules3.ts @@ -149,6 +149,82 @@ describe("AccessRules3", function() { await assertSaved(); }); + it('can have a SeedRule special that refers to columns', async function() { + // Open Access Rules page. + const mainSession = await gu.session().teamSite.user('user1').login(); + await mainSession.loadDoc(`/doc/${docId}`); + await driver.find('.test-tools-access-rules').click(); + await driver.findWait('.test-rule-set', 2000); + + // Check seed rule checkbox is unselected. + const seedRule = await driver.find('div.test-rule-special-SeedRule'); + const checkbox = seedRule.find('input[type=checkbox]'); + assert.equal(await checkbox.isSelected(), false); + + // Now check the box, and see we get the default rule we expect. + await checkbox.click(); + await assertChanged(); + await driver.find('.test-rule-special-SeedRule .test-rule-special-expand').click(); + assert.deepEqual(await getRules(seedRule), + [{ formula: 'user.Access in [OWNER]', perm: '+R+U+C+D' }]); + assert.equal(await hasExtraAdd(seedRule), true); + + // Tweak the seed rule to refer to a column. + await seedRule.find('.test-rule-part .test-rule-add').click(); + await enterRulePart(seedRule, 1, 'rec.Year == 1', 'Deny All', 'memo1'); + assert.equal(await checkbox.getAttribute('disabled'), 'true'); + + // New table rules should include the seed rule. + await driver.findContentWait('button', /Add Table Rules/, 2000).click(); + await driver.findContentWait('.grist-floating-menu li', /FinancialsTable/, 3000).click(); + let fin = findTable(/FinancialsTable/); + assert.deepEqual(await getRules(fin), + [{ formula: 'rec.Year == 1', perm: '-R-U-C-D', res: 'All', memo: 'memo1'}, + { formula: 'user.Access in [OWNER]', perm: '+R+U+C+D', res: 'All' }, + { formula: 'Everyone Else', perm: '', res: 'All' }]); + assert.equal(await hasExtraAdd(fin), false); + await removeTable(/FinancialsTable/); + + // Tweak the seed rule to refer to a column that won't exist. + await enterRulePart(seedRule, 1, 'rec.Unreal == 1', 'Deny All', 'memo1'); + assert.equal(await checkbox.getAttribute('disabled'), 'true'); + + // New table rules should include the seed rule, and show an error. + await driver.findContentWait('button', /Add Table Rules/, 2000).click(); + await driver.findContentWait('.grist-floating-menu li', /FinancialsTable/, 3000).click(); + fin = findTable(/FinancialsTable/); + assert.deepEqual(await getRules(fin), + [{ formula: 'rec.Unreal == 1', perm: '-R-U-C-D', res: 'All', memo: 'memo1', + error: 'Invalid columns: Unreal' }, + { formula: 'user.Access in [OWNER]', perm: '+R+U+C+D', res: 'All' }, + { formula: 'Everyone Else', perm: '', res: 'All' }]); + assert.equal(await hasExtraAdd(fin), false); + await removeTable(/FinancialsTable/); + + // Check that returning to the single OWNER rule gets us back to an uncomplicated + // selected checkbox. + await assertChanged(); + assert.equal(await checkbox.getAttribute('disabled'), 'true'); + assert.equal(await checkbox.isSelected(), false); + await seedRule.find('.test-rule-part .test-rule-remove').click(); + assert.equal(await checkbox.getAttribute('disabled'), null); + assert.equal(await checkbox.isSelected(), true); + + // Check that removing that rule deselected the checkbox and collapses rule list. + await seedRule.find('.test-rule-part .test-rule-remove').click(); + assert.equal(await checkbox.getAttribute('disabled'), null); + assert.equal(await checkbox.isSelected(), false); + await assertSaved(); + assert.lengthOf(await seedRule.findAll('.test-rule-set'), 0); + + // Expand again, and make sure we are back to default. + await driver.find('.test-rule-special-SeedRule .test-rule-special-expand').click(); + assert.lengthOf(await seedRule.findAll('.test-rule-set'), 1); + assert.deepEqual(await getRules(seedRule), + [{ formula: 'Everyone', perm: '' }]); + await assertSaved(); + }); + it('can save and reload SeedRule special', async function() { const mainSession = await gu.session().teamSite.user('user1').login(); await mainSession.loadDoc(`/doc/${docId}`); diff --git a/test/nbrowser/aclTestUtils.ts b/test/nbrowser/aclTestUtils.ts index 5d83d1aa76..e7d581e547 100644 --- a/test/nbrowser/aclTestUtils.ts +++ b/test/nbrowser/aclTestUtils.ts @@ -139,7 +139,8 @@ namespace gristUtils { export async function getRules(el: WebElement): Promise> { + memo?: string, + error?: string}>> { const ruleSets = await el.findAll('.test-rule-set'); const results: Array<{formula: string, perm: string, res?: string, @@ -162,9 +163,12 @@ namespace gristUtils { } const hasMemo = await part.find('.test-rule-memo').isPresent(); const memo = hasMemo ? await part.find('.test-rule-memo input').value() : undefined; + const hasError = await part.find('.test-rule-error').isPresent(); + const error = hasError ? await part.find('.test-rule-error').getText() : undefined; results.push({formula, perm: permParts.join(''), ...(memo ? {memo} : {}), - ...(res ? {res} : {}) + ...(res ? {res} : {}), + ...(error ? {error} : {}), }); } }