Repository navigation
Conversation
![Xu, David [AU-AU]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
01eb405 to
a189087
Compare
andrewkfiedler
left a comment
andrewkfiedler
left a comment
There was a problem hiding this comment.
When I tested this locally, the app crashed on load.
metacard-definitions.tsx now imports extension-points, which loops back to startup through user-instance.ts. Since index.tsx loads startup first, the app throws `Cannot read properties of undefined (reading 'data').
Instead of the model reading extension points, I'd give it a slot the same way OverridableSaveFile and the other Overridables work. Export an Overridable<AttributeMapType> from metacard-definitions.tsx, merge it into attributeMap on 'fetched' and on 'override', and have Entry.tsx call .override(extraFilters.typeDefinitions) right after the Object.assign. getSearchOnlyAttributes can use its keys. That also gives these properties a definition in the map, which fixes the grouped lookup crash I mentioned in query-basic.view.tsx. I tried this locally (forcing hidden: true on those definitions and letting the server win on a name clash) and it made it so an OR'd group of extra filters no longer throws.
| ) | ||
| } | ||
|
|
||
| if (filter.property in (Extensions.extraFilters?.filterPropertyNames ?? [])) { |
There was a problem hiding this comment.
in on an array checks indexes ('0', '1', 'length'), not values, so this is false for any real property name and the custom input never renders.
| if (filter.property in (Extensions.extraFilters?.filterPropertyNames ?? [])) { | |
| if (Extensions.extraFilters?.filterPropertyNames.includes(filter.property)) { |
Also, getFilterInput only gets the property name, so the input can't show or change the filter's value. Passing filter and setFilter like the other inputs here would fix that (plus the type in extension-points.tsx).
| </Grid> | ||
| ) : null} | ||
| </div> | ||
| {Extensions.extraFilters?.queryBasicCheckboxFilters && ( |
There was a problem hiding this comment.
These get passed as true, not the actual values.
| {Extensions.extraFilters?.queryBasicCheckboxFilters && ( | |
| model={model} | |
| basicFilter={basicFilter} |
| value: new BasicDatatypeFilter({ | ||
| value: [], | ||
| }), | ||
| ...Extensions.extraFilters?.extraPropertyValueMaps, |
There was a problem hiding this comment.
This lands inside the basic datatype entry instead of the top level of the map. And since extraPropertyValueMaps is typed any[], spreading an array here adds keys "0", "1", and so on, not property names. Probably should be moved up a level and be typed as an object in extension-points.tsx.
| [key: string]: ValidationResult | undefined | ||
| }) => void | ||
| Extensions?: React.FunctionComponent | ||
| QueryBasicPropsExtensions?: React.FunctionComponent |
There was a problem hiding this comment.
Renaming this prop breaks query-add.tsx, which still passes Extensions. That's the type error failing CI. Importing the module on line 46 as ExtensionPoints instead avoids the clash, and this can stay Extensions.
| StartupDataStore.MetacardDefinitions.getAttributeMap()[ | ||
| stripQuotes(filter.property) | ||
| ].type === 'DATE' | ||
| ]?.type === 'DATE' |
There was a problem hiding this comment.
This covers a single extra filter, but the grouped lookup at line 85 doesn't have it, so an OR'd group of extra filters still throws. Basically, nothing adds typeDefinitions to the attribute map, so these properties have no definition at runtime. Feeding typeDefinitions into the map through the Overridable fixes both lookups (see summary comment).
| isInjected: false, | ||
| hidden: true, | ||
| }, | ||
| ...Extensions.extraFilters?.typeDefinitions, |
There was a problem hiding this comment.
extraFilters is undefined in tests, so this doesn't end up exercising the new code.
This branch has not been deployed
4 participants
…heckbox filters…heckbox filters