Skip to content
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

feat: display removed context props in the UI #6844

Merged
merged 14 commits into from
Apr 12, 2024
+205 −6
Merged

feat: display removed context props in the UI #6844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
76719fe
feat: display removed context props in the UI
thomasheartman Apr 12, 2024
d90a5ba
test: quick scaffold
thomasheartman Apr 12, 2024
c002a1d
feat: remove b
thomasheartman Apr 12, 2024
e2a0497
feat: make test pass
thomasheartman Apr 12, 2024
98f5378
feat: update test test negative outcome
thomasheartman Apr 12, 2024
164c578
feat: add missing await
thomasheartman Apr 12, 2024
be31ae2
Chore: fix some minor issues
thomasheartman Apr 12, 2024
81a87b8
Chore: remove unused data
thomasheartman Apr 12, 2024
21ea47c
Make check a little more robust
thomasheartman Apr 12, 2024
7f7cd71
Chore: update with new schemas
thomasheartman Apr 12, 2024
59397de
Chore: update warning summary text
thomasheartman Apr 12, 2024
35d8eca
feat: improve styling a little
thomasheartman Apr 12, 2024
7402691
Chore: test sorting
thomasheartman Apr 12, 2024
0dd24c4
feat: remove redundant code
thomasheartman Apr 12, 2024
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
143 changes: 142 additions & 1 deletion frontend/src/component/playground/Playground/AdvancedPlayground.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,148 @@ test('should display error on submit', async () => {

const user = userEvent.setup();
const submitButton = screen.getByText('Submit');
user.click(submitButton);
await user.click(submitButton);
thomasheartman marked this conversation as resolved.
Show resolved Hide resolved

await screen.findByText('some error about too many items');
});

describe('context warnings on successful evaluation', () => {
const warningSummaryText =
'Some context properties were not taken into account during evaluation';

test('should show context warnings if they exist in the response', async () => {
const response = {
features: [],
input: {
environments: [],
projects: [],
context: {},
},
warnings: {
invalidContextProperties: [
'empty array',
'true',
'false',
'number',
'null',
'accountId',
'object',
],
},
};
testServerRoute(
server,
'/api/admin/playground/advanced',
response,
'post',
200,
);

render(testEvaluateComponent);

const user = userEvent.setup();
const submitButton = screen.getByText('Submit');
await user.click(submitButton);

await screen.findByText(warningSummaryText, { exact: false });
for (const prop of response.warnings.invalidContextProperties) {
await screen.findByText(prop);
}
});

test('sorts context warnings alphabetically', async () => {
const response = {
features: [],
input: {
environments: [],
projects: [],
context: {},
},
warnings: {
invalidContextProperties: ['b', 'a', 'z'],
},
};
testServerRoute(
server,
'/api/admin/playground/advanced',
response,
'post',
200,
);

render(testEvaluateComponent);

const user = userEvent.setup();
const submitButton = screen.getByText('Submit');
await user.click(submitButton);

const warnings = screen.getAllByTestId('context-warning-list-element');

expect(warnings[0]).toHaveTextContent('a');
expect(warnings[1]).toHaveTextContent('b');
expect(warnings[2]).toHaveTextContent('z');
});

test('does not render context warnings if the list of properties is empty', async () => {
const response = {
features: [],
input: {
environments: [],
projects: [],
context: {},
},
warnings: {
invalidContextProperties: [],
},
};
testServerRoute(
server,
'/api/admin/playground/advanced',
response,
'post',
200,
);

render(testEvaluateComponent);

const user = userEvent.setup();
const submitButton = screen.getByText('Submit');
await user.click(submitButton);

const warningSummary = screen.queryByText(warningSummaryText, {
exact: false,
});

expect(warningSummary).toBeNull();
});

test("should not show context warnings if they don't exist in the response", async () => {
testServerRoute(
server,
'/api/admin/playground/advanced',
{
features: [],
input: {
environments: [],
projects: [],
context: {},
},
warnings: {},
},
'post',
200,
);

render(testEvaluateComponent);

const user = userEvent.setup();
const submitButton = screen.getByText('Submit');
await user.click(submitButton);

const warningSummary = screen.queryByText(warningSummaryText, {
exact: false,
});

expect(warningSummary).toBeNull();
});
});
68 changes: 63 additions & 5 deletions frontend/src/component/playground/Playground/AdvancedPlayground.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,59 @@ const StyledAlert = styled(Alert)(({ theme }) => ({
marginBottom: theme.spacing(3),
}));

const GenerateWarningMessages: React.FC<{
Copy link

Choose a reason for hiding this comment

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

ℹ Getting worse: Overall Code Complexity
The mean cyclomatic complexity increases from 4.33 to 4.40, threshold = 4

response?: AdvancedPlaygroundResponseSchema;
}> = ({ response }) => {
const invalidContextProperties =
response?.warnings?.invalidContextProperties;

if (invalidContextProperties && invalidContextProperties.length > 0) {
invalidContextProperties.sort();
const summary =
'Some context properties were not taken into account during evaluation';

const StyledDetails = styled('details')(({ theme }) => ({
'* + *': { marginBlockStart: theme.spacing(1) },
}));

return (
<StyledAlert severity='warning'>
<StyledDetails>
<summary>{summary}</summary>
<p>
The context you provided for this query contained
top-level properties with invalid values. These
properties were not taken into consideration when
evaluating your query. The properties are:
</p>
<ul>
{invalidContextProperties.map((prop) => (
<li
key={prop}
data-testid='context-warning-list-element'
>
<code>{prop}</code>
</li>
))}
</ul>

<p>
Remember that context fields (with the exception of the{' '}
<code>properties</code> object) must be strings.
</p>
<p>
Because we didn't take these properties into account
during the feature flag evaluation, they will not appear
in the results table.
</p>
</StyledDetails>
</StyledAlert>
);
} else {
return null;
}
};

export const AdvancedPlayground: VFC<{
FormComponent?: typeof PlaygroundForm;
}> = ({ FormComponent = PlaygroundForm }) => {
Expand Down Expand Up @@ -304,11 +357,16 @@ export const AdvancedPlayground: VFC<{
Object.values(errors).length === 0
}
show={
<AdvancedPlaygroundResultsTable
loading={loading}
features={results?.features}
input={results?.input}
/>
<>
<GenerateWarningMessages
response={results}
/>
Comment on lines +361 to +363
Copy link
Contributor

Choose a reason for hiding this comment

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

This component will be null when no warning, I'm just wondering if this is ok (i.e. react knows how to handle null components). Even if react supports this, I remember we were using conditional rendering rather than returning null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure that react knows how to handle nulls, yeah. I think it's even preferable to, say, a, fragment. ConditionallyRender also returns null if it's not rendering anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And because in this case, the decision whether to render something or not can be a little complex (though I guess condition={results.warnings?.invalidContextProperties?.length > 0} would work), I think it makes sense to relegate it to the function instead.

Does that sound reasonable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable! I kind of like this approach because it requires less boilerplate code (I mean, tags are boilerplate in a way). So I think it's good and something for my TIL list :)

<AdvancedPlaygroundResultsTable
loading={loading}
features={results?.features}
input={results?.input}
/>
</>
}
/>
<ConditionallyRender
Expand Down
Loading