Skip to content

Explorer subpath support #1368

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jan 13, 2022
Merged

Explorer subpath support #1368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5ad6e85
feat: add support for attribute expressions
aaron-steinfeld Jan 3, 2022
60a6bcd
Merge remote-tracking branch 'origin/main' into attribute-expressions
aaron-steinfeld Jan 12, 2022
775d4aa
test: update tests
aaron-steinfeld Jan 12, 2022
46648ea
Merge branch 'main' into attribute-expressions
aaron-steinfeld Jan 12, 2022
a7efe69
feat: add support for filtering with attribute expressions
aaron-steinfeld Jan 6, 2022
bb7ba1a
fix: caught a couple bugs while updating tests
aaron-steinfeld Jan 12, 2022
2751bcd
Merge remote-tracking branch 'origin/main' into explorer-subpath-support
aaron-steinfeld Jan 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions projects/common/src/utilities/types/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ export type DistributiveOmit<T, K extends UnionKeys<T>> = T extends any ? Omit<T

export type PartialBy<T, K extends keyof T> = Omit<T, K> & Partial<Pick<T, K>>;
export type RequireBy<T, K extends keyof T> = T & Required<Pick<T, K>>;
export type KeysWithType<T, V> = { [K in keyof T]-?: T[K] extends V ? K : never }[keyof T];

export interface Dictionary<T> {
[key: string]: T;
}
Expand Down
45 changes: 34 additions & 11 deletions projects/components/src/filtering/filter-bar/filter-bar.service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,9 @@ describe('Filter Bar service', () => {
),
new StringMapFilterBuilder().buildFilter(
getTestFilterAttribute(FilterAttributeType.StringMap),
FilterOperator.ContainsKeyValue,
['myKey', 'myValue']
FilterOperator.Equals,
'myValue',
'myKey'
)
];

Expand Down Expand Up @@ -184,18 +185,33 @@ describe('Filter Bar service', () => {
expect(testFilters).toEqual([stringFilter, inNumberFilter]);

/*
* Add a StringMap CONTAINS_KEY_VALUE that should not replace any existing filters
* Add a StringMap EQUALS should not replace any existing filters
*/

const ckvStringMapFilter = new StringMapFilterBuilder().buildFilter(
const firstStringMapFilter = new StringMapFilterBuilder().buildFilter(
getTestFilterAttribute(FilterAttributeType.StringMap),
FilterOperator.ContainsKeyValue,
['myKey', 'myValue']
FilterOperator.Equals,
'myValue',
'myKey'
);

testFilters = spectator.service.addFilter(testFilters, ckvStringMapFilter);
testFilters = spectator.service.addFilter(testFilters, firstStringMapFilter);

expect(testFilters).toEqual([stringFilter, inNumberFilter, firstStringMapFilter]);
/*
* Add a second StringMap EQUALS filters with a different key should not replace any existing filters
*/

expect(testFilters).toEqual([stringFilter, inNumberFilter, ckvStringMapFilter]);
const secondStringMapFilter = new StringMapFilterBuilder().buildFilter(
getTestFilterAttribute(FilterAttributeType.StringMap),
FilterOperator.Equals,
'myValue',
'mySecondKey'
);

testFilters = spectator.service.addFilter(testFilters, secondStringMapFilter);

expect(testFilters).toEqual([stringFilter, inNumberFilter, firstStringMapFilter, secondStringMapFilter]);

/*
* Add a StringMap CONTAINS_KEY that should not replace any existing filters
Expand All @@ -209,7 +225,13 @@ describe('Filter Bar service', () => {

testFilters = spectator.service.addFilter(testFilters, ckStringMapFilter);

expect(testFilters).toEqual([stringFilter, inNumberFilter, ckvStringMapFilter, ckStringMapFilter]);
expect(testFilters).toEqual([
stringFilter,
inNumberFilter,
firstStringMapFilter,
secondStringMapFilter,
ckStringMapFilter
]);
});

test('correctly updates filters', () => {
Expand All @@ -221,8 +243,9 @@ describe('Filter Bar service', () => {

const testStringMapFilter = new StringMapFilterBuilder().buildFilter(
getTestFilterAttribute(FilterAttributeType.StringMap),
FilterOperator.ContainsKeyValue,
['myTestKey', 'myTestValue']
FilterOperator.Equals,
'myTestValue',
'myTestKey'
);

const testNumberFilter = new NumberFilterBuilder().buildFilter(
Expand Down
9 changes: 5 additions & 4 deletions projects/components/src/filtering/filter-bar/filter-bar.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Injectable } from '@angular/core';
import { areCompatibleFilters, areEqualFilters, Filter } from '../filter/filter';
import { isEqual } from 'lodash-es';
import { areCompatibleFilters, Filter } from '../filter/filter';

@Injectable({
providedIn: 'root'
Expand All @@ -14,18 +15,18 @@ export class FilterBarService {
public updateFilter(filters: Filter[], oldFilter: Filter, newFilter: Filter): Filter[] {
const clonedFilters = [...filters];

const index = filters.findIndex(f => areEqualFilters(f, oldFilter));
const index = filters.findIndex(f => isEqual(f, oldFilter));

if (index < 0) {
throw new Error(`Unable to update filter. Filter for '${oldFilter.field}' not found.`);
}

clonedFilters.splice(index, 1, newFilter);

return clonedFilters.filter(f => areEqualFilters(f, newFilter) || areCompatibleFilters(f, newFilter));
return clonedFilters.filter(f => isEqual(f, newFilter) || areCompatibleFilters(f, newFilter));
}

public deleteFilter(filters: Filter[], filter: Filter): Filter[] {
return filters.filter(f => !areEqualFilters(f, filter));
return filters.filter(f => !isEqual(f, filter));
}
}
3 changes: 1 addition & 2 deletions projects/components/src/filtering/filter-bar/filter-chip/filter-chip.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,7 @@ export class FilterChipComponent implements OnInit, OnChanges {
private mapToComboBoxOption(filter: IncompleteFilter): ComboBoxOption<IncompleteFilter> {
return {
text: filter.userString,
value: filter,
tooltip: `${filter.userString} (${filter.field})`
value: filter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are tooltips now handled? Or at least, how do we let users know the underlying field backing the display name?

Copy link
Contributor Author

@aaron-steinfeld aaron-steinfeld Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually removed them, this was one of the things I wanted to ask. In the existing UI:

  • The underlying field isn't actually used for matching a user's string that I could tell
  • hard to find, since you have to hover on the popup
  • very strange to read, since this key is thrown after the user string

Example from existing ui:
image
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair points. Knowing the underlying keys was an ask early on, so we've always had the tooltip available. Not sure if users are even asking for that info. If they are, I'm surprised we haven't gotten feedback about the discoverability of it as you've noted.

I'm fine removing them as long as its a quick fix to add back if an issue arrises.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I remember the ask, but not sure why it was there given that we don't expose keys to users anywhere else. Easy enough to restore if the requirement comes back - hopefully can better understand the ask at that point.

};
}
}
69 changes: 63 additions & 6 deletions projects/components/src/filtering/filter-bar/filter-chip/filter-chip.service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ describe('Filter Chip service', () => {
case FilterOperator.In:
return new InFilterParser();
case FilterOperator.ContainsKey:
case FilterOperator.ContainsKeyValue:
return new ContainsFilterParser();
default:
assertUnreachable(operator);
Expand Down Expand Up @@ -246,21 +245,79 @@ describe('Filter Chip service', () => {
]);
});

test('correctly autocompletes operator filters for string map attribute once space key is entered', () => {
test('correctly autocompletes operator filters for string map attribute', () => {
const attribute = getTestFilterAttribute(FilterAttributeType.StringMap);

expect(spectator.service.autocompleteFilters(attributes, 'String Map Attribute.testKey ')).toEqual([
// Contains key or subpath operators if no subpath specified but could be
expect(spectator.service.autocompleteFilters(attributes, 'String Map Attribute')).toEqual([
{
metadata: attribute,
field: attribute.name,
operator: FilterOperator.ContainsKey,
userString: `${attribute.displayName} ${FilterOperator.ContainsKey} testKey`
userString: `${attribute.displayName} ${FilterOperator.ContainsKey}`
},
{
metadata: attribute,
field: attribute.name,
operator: FilterOperator.ContainsKeyValue,
userString: `${attribute.displayName}.testKey ${FilterOperator.ContainsKeyValue}`
operator: FilterOperator.Equals,
userString: `${attribute.displayName}.example ${FilterOperator.Equals}`
},
{
metadata: attribute,
field: attribute.name,
operator: FilterOperator.NotEquals,
userString: `${attribute.displayName}.example ${FilterOperator.NotEquals}`
},
{
metadata: attribute,
field: attribute.name,
operator: FilterOperator.In,
userString: `${attribute.displayName}.example ${FilterOperator.In}`
},
{
metadata: attribute,
field: attribute.name,
operator: FilterOperator.Like,
userString: `${attribute.displayName}.example ${FilterOperator.Like}`
}
]);

// Regular operators only once subpath included
expect(spectator.service.autocompleteFilters(attributes, 'String Map Attribute.testKey')).toEqual([
expect.objectContaining({
metadata: attribute,
field: attribute.name,
operator: FilterOperator.ContainsKey,
// This operator isn't actually eligible but filtering operators is done by the chip/combobox, so just make sure the string doesn't match
userString: expect.not.stringMatching(`${attribute.displayName}.testKey`)
}),
{
metadata: attribute,
field: attribute.name,
subpath: 'testKey',
operator: FilterOperator.Equals,
userString: `${attribute.displayName}.testKey ${FilterOperator.Equals}`
},
{
metadata: attribute,
field: attribute.name,
subpath: 'testKey',
operator: FilterOperator.NotEquals,
userString: `${attribute.displayName}.testKey ${FilterOperator.NotEquals}`
},
{
metadata: attribute,
field: attribute.name,
subpath: 'testKey',
operator: FilterOperator.In,
userString: `${attribute.displayName}.testKey ${FilterOperator.In}`
},
{
metadata: attribute,
field: attribute.name,
subpath: 'testKey',
operator: FilterOperator.Like,
userString: `${attribute.displayName}.testKey ${FilterOperator.Like}`
}
]);
});
Expand Down
Loading