fix(embed): defer the column check until metadata loads, report AND-ed pairs
Two review findings, both paths where a filter could look applied while producing nothing. - `unresolvedMappings` judged a mapping against an empty column list while the grid was still loading, so every entry was briefly reported as naming a column the table does not have. An empty or unset `cols` now means "not known yet" and returns nothing, so the banner appears only once metadata exists. - Two parameters mapped to one column are AND-ed, so different values yield no rows at all, and nothing said so. `sameColumnMappings()` now reports the contended column and both parameters. The clauses are deliberately left as an intersection: turning that AND into an OR is a filter-semantics decision, not a diagnostic, so it is tracked separately. Covered by unit tests for both (unloaded column list, one line per contended column) and e2e case 10, which drives two parameters onto one column and asserts the banner plus the empty grid.
This commit is contained in:
1 parent
388f418bf4
commit
2623a3fb4b
4 files changed
+173
-4
No files matched your search
@@ -252,6 +252,41 @@ context('embed=va (VA data-driven content) tests: ', function () {
|
||||
}
|
||||
)
|
||||
})
|
||||
|
||||
it('10 | warns when two parameters filter the same column (an empty grid)', () => {
|
||||
// Clauses are AND-ed, so two different values on one column produce no rows
|
||||
// at all - the last silent-empty-grid path. Reported rather than rewritten,
|
||||
// so the author decides whether to use one parameter carrying both values.
|
||||
openEditorInVaMode(
|
||||
'mpe_x_test',
|
||||
() => {
|
||||
pushVaMessage(
|
||||
{
|
||||
name: 'pr1',
|
||||
label: 'DROPDOWN_TAB1',
|
||||
dataType: 'string',
|
||||
value: 'Option 1'
|
||||
},
|
||||
{
|
||||
name: 'pr2',
|
||||
label: 'DROPDOWN_TAB2',
|
||||
dataType: 'string',
|
||||
value: 'Option 2'
|
||||
}
|
||||
)
|
||||
cy.get('.va-parmap-warning', { timeout: longerCommandTimeout }).should(
|
||||
'contain.text',
|
||||
'combined with AND'
|
||||
)
|
||||
cy.get('.va-parmap-warning').should('contain.text', 'DROPDOWN_TAB1')
|
||||
cy.get('.va-parmap-warning').should('contain.text', 'DROPDOWN_TAB2')
|
||||
// The consequence, deterministic on both SAS and the mock: rows must
|
||||
// match both values, so there are none.
|
||||
cy.get('h4').should('contain.text', '(0 rows')
|
||||
},
|
||||
'va_parmap=DROPDOWN_TAB1-SOME_DROPDOWN&va_parmap=DROPDOWN_TAB2-SOME_DROPDOWN'
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
/**
|
||||
@@ -289,8 +324,8 @@ const visitWithBufferedMessage = (
|
||||
if (callback) callback()
|
||||
}
|
||||
|
||||
/** Pushes one VA data-driven content message carrying a single parameter. */
|
||||
const pushVaMessage = (parameter: any) => {
|
||||
/** Pushes one VA data-driven content message carrying the given parameters. */
|
||||
const pushVaMessage = (...parameters: any[]) => {
|
||||
cy.window().then((win) => {
|
||||
win.postMessage(
|
||||
{
|
||||
@@ -300,7 +335,7 @@ const pushVaMessage = (parameter: any) => {
|
||||
availableRowCount: 0,
|
||||
data: [],
|
||||
columns: [],
|
||||
parameters: [parameter]
|
||||
parameters
|
||||
},
|
||||
'*'
|
||||
)
|
||||
|
||||
@@ -497,6 +497,8 @@ export class EditorComponent
|
||||
public vaUnresolvedMappings: string[] = []
|
||||
/** `va_parmap` keys that no VA message has named (a parameter-side typo). */
|
||||
public vaUnmatchedMappings: string[] = []
|
||||
/** Parameters in this message that filter the same column (an AND-ed pair). */
|
||||
public vaSameColumnMappings: string[] = []
|
||||
/** The warning text last written to the console, so repeated VA messages log once. */
|
||||
private vaWarningsReported = ''
|
||||
/**
|
||||
@@ -509,7 +511,8 @@ export class EditorComponent
|
||||
return [
|
||||
...globals.vaParmapProblems,
|
||||
...this.vaUnresolvedMappings,
|
||||
...this.vaUnmatchedMappings
|
||||
...this.vaUnmatchedMappings,
|
||||
...this.vaSameColumnMappings
|
||||
]
|
||||
}
|
||||
/**
|
||||
@@ -5091,6 +5094,13 @@ export class EditorComponent
|
||||
this.vaParamMap,
|
||||
this.vaMessaging.seenParamKeys
|
||||
)
|
||||
// Two parameters filtering one column are AND-ed, so differing values leave
|
||||
// an empty grid. Reported, not rewritten: turning that AND into an OR is a
|
||||
// filter-semantics decision, taken in buildClauses and not in a diagnostic.
|
||||
this.vaSameColumnMappings = this.vaFilter.sameColumnMappings(
|
||||
msg,
|
||||
this.vaParamMap
|
||||
)
|
||||
const warnings = this.vaMappingWarnings.join(' | ')
|
||||
if (warnings !== this.vaWarningsReported) {
|
||||
this.vaWarningsReported = warnings
|
||||
|
||||
@@ -382,6 +382,89 @@ describe('VaFilterService', () => {
|
||||
)
|
||||
expect(problems).toHaveSize(1)
|
||||
})
|
||||
|
||||
it('treats an unloaded column list as "not known yet", not as a bad mapping', () => {
|
||||
// While the grid is still loading there is no metadata to judge against, so
|
||||
// reporting would flash the banner on every load.
|
||||
expect(
|
||||
service.unresolvedMappings(
|
||||
msg([{ label: 'REGION_TAB1', value: 'x' }]),
|
||||
new Map([['region_tab1', 'NOPE']]),
|
||||
[],
|
||||
'DC_JSLIB.MPE_X_TEST'
|
||||
)
|
||||
).toEqual([])
|
||||
expect(
|
||||
service.unresolvedMappings(
|
||||
msg([{ label: 'REGION_TAB1', value: 'x' }]),
|
||||
new Map([['region_tab1', 'NOPE']]),
|
||||
undefined as any,
|
||||
'DC_JSLIB.MPE_X_TEST'
|
||||
)
|
||||
).toEqual([])
|
||||
})
|
||||
})
|
||||
|
||||
describe('sameColumnMappings (two parameters, one column)', () => {
|
||||
it('reports an AND-ed pair, naming both parameters and the column', () => {
|
||||
const map = new Map([
|
||||
['dropdown_tab1', 'SOME_DROPDOWN'],
|
||||
['dropdown_tab2', 'SOME_DROPDOWN']
|
||||
])
|
||||
const problems = service.sameColumnMappings(
|
||||
msg([
|
||||
{ label: 'DROPDOWN_TAB1', value: 'Option 1' },
|
||||
{ label: 'DROPDOWN_TAB2', value: 'Option 2' }
|
||||
]),
|
||||
map
|
||||
)
|
||||
expect(problems).toHaveSize(1)
|
||||
expect(problems[0]).toContain('"DROPDOWN_TAB1" and "DROPDOWN_TAB2"')
|
||||
expect(problems[0]).toContain('all filter SOME_DROPDOWN')
|
||||
expect(problems[0]).toContain('combined with AND')
|
||||
})
|
||||
|
||||
it('says nothing for one parameter per column', () => {
|
||||
const map = new Map([
|
||||
['dropdown_tab1', 'SOME_DROPDOWN'],
|
||||
['num_tab1', 'SOME_NUM']
|
||||
])
|
||||
expect(
|
||||
service.sameColumnMappings(
|
||||
msg([
|
||||
{ label: 'DROPDOWN_TAB1', value: 'Option 1' },
|
||||
{ label: 'NUM_TAB1', value: '1' }
|
||||
]),
|
||||
map
|
||||
)
|
||||
).toEqual([])
|
||||
expect(
|
||||
service.sameColumnMappings(
|
||||
msg([{ label: 'DROPDOWN_TAB1', value: 'Option 1' }]),
|
||||
map
|
||||
)
|
||||
).toEqual([])
|
||||
expect(service.sameColumnMappings(msg([]), map)).toEqual([])
|
||||
})
|
||||
|
||||
it('reports one line per contended column', () => {
|
||||
const map = new Map([
|
||||
['a', 'SOME_DROPDOWN'],
|
||||
['b', 'SOME_DROPDOWN'],
|
||||
['c', 'SOME_NUM'],
|
||||
['d', 'SOME_NUM']
|
||||
])
|
||||
const problems = service.sameColumnMappings(
|
||||
msg([
|
||||
{ label: 'A', value: '1' },
|
||||
{ label: 'B', value: '2' },
|
||||
{ label: 'C', value: '3' },
|
||||
{ label: 'D', value: '4' }
|
||||
]),
|
||||
map
|
||||
)
|
||||
expect(problems).toHaveSize(2)
|
||||
})
|
||||
})
|
||||
|
||||
describe('unmatchedMappings (parameter-side typo)', () => {
|
||||
|
||||
@@ -200,6 +200,10 @@ export class VaFilterService {
|
||||
): string[] {
|
||||
const problems = new Set<string>()
|
||||
if (!paramMap || paramMap.size === 0) return []
|
||||
// No metadata yet is not the same as "the table lacks this column": an empty
|
||||
// `cols` means the grid is still loading, so there is nothing to judge yet.
|
||||
// Reporting here would flash a misconfiguration banner during every load.
|
||||
if (!cols || cols.length === 0) return []
|
||||
for (const param of msg.parameters || []) {
|
||||
const mapped = this.mappedColumn(param, paramMap)
|
||||
if (!mapped || this.findCol(mapped, cols)) continue
|
||||
@@ -240,6 +244,43 @@ export class VaFilterService {
|
||||
return problems
|
||||
}
|
||||
|
||||
/**
|
||||
* Parameters in this message that filter the same column, one plain-English
|
||||
* line per column. Clauses are AND-ed, so two parameters on one column are an
|
||||
* intersection: with different values the grid comes back empty, which looks
|
||||
* like a working filter. Reported rather than rewritten - folding them into one
|
||||
* `IN (...)` would silently turn an AND into an OR, which is a semantics
|
||||
* decision, not a diagnostic.
|
||||
*/
|
||||
sameColumnMappings(
|
||||
msg: VaMessage,
|
||||
paramMap: Map<string, string> | undefined
|
||||
): string[] {
|
||||
if (!paramMap || paramMap.size === 0) return []
|
||||
const byColumn = new Map<string, string[]>()
|
||||
for (const param of msg.parameters || []) {
|
||||
const mapped = this.mappedColumn(param, paramMap)
|
||||
if (!mapped) continue
|
||||
const column = mapped.toUpperCase()
|
||||
const params = byColumn.get(column) || []
|
||||
const paramName = (param.label ?? param.name ?? '').toString().trim()
|
||||
if (paramName && !params.includes(paramName)) params.push(paramName)
|
||||
byColumn.set(column, params)
|
||||
}
|
||||
const problems: string[] = []
|
||||
for (const [column, params] of Array.from(byColumn.entries())) {
|
||||
if (params.length < 2) continue
|
||||
problems.push(
|
||||
`VA parameters ${params
|
||||
.map((p) => `"${p}"`)
|
||||
.join(
|
||||
' and '
|
||||
)} all filter ${column}. Parameters are combined with AND, so rows must match all of them - two different values will show no rows. Use one parameter carrying both values to match either.`
|
||||
)
|
||||
}
|
||||
return problems
|
||||
}
|
||||
|
||||
/**
|
||||
* The DC column an explicit URL mapping (`va_parmap`) assigns to this
|
||||
* parameter, or undefined when the parameter is not mapped. The key is matched
|
||||
|
||||
Reference in new issue
Block a user