From 6f79688e2c3361339c698d7686554e762876ea9a Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 12 Apr 2024 12:35:33 +0200 Subject: [PATCH] feat: display removed context props in the UI (#6844) This PR shows context warnings in the UI if they exist. That means that if you provide invalid properties on the top level, you'll get the result back, but we'll also tell you that we didn't take these properties into account. Screenie: ![image](https://github.com/Unleash/unleash/assets/17786332/06c31b15-a52f-4a22-a1ac-4f326cc34173) --- .../Playground/AdvancedPlayground.test.tsx | 143 +++++++++++++++++- .../Playground/AdvancedPlayground.tsx | 68 ++++++++- 2 files changed, 205 insertions(+), 6 deletions(-) diff --git a/frontend/src/component/playground/Playground/AdvancedPlayground.test.tsx b/frontend/src/component/playground/Playground/AdvancedPlayground.test.tsx index ee51b31dcd..bf2cc564ea 100644 --- a/frontend/src/component/playground/Playground/AdvancedPlayground.test.tsx +++ b/frontend/src/component/playground/Playground/AdvancedPlayground.test.tsx @@ -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); 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(); + }); +}); diff --git a/frontend/src/component/playground/Playground/AdvancedPlayground.tsx b/frontend/src/component/playground/Playground/AdvancedPlayground.tsx index 39b34b7c52..31702dcc9a 100644 --- a/frontend/src/component/playground/Playground/AdvancedPlayground.tsx +++ b/frontend/src/component/playground/Playground/AdvancedPlayground.tsx @@ -28,6 +28,59 @@ const StyledAlert = styled(Alert)(({ theme }) => ({ marginBottom: theme.spacing(3), })); +const GenerateWarningMessages: React.FC<{ + 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 ( + + + {summary} +

+ 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: +

+
    + {invalidContextProperties.map((prop) => ( +
  • + {prop} +
    • + ))} +
+ +

+ Remember that context fields (with the exception of the{' '} + properties object) must be strings. +

+

+ Because we didn't take these properties into account + during the feature flag evaluation, they will not appear + in the results table. +

+ + + ); + } else { + return null; + } +}; + export const AdvancedPlayground: VFC<{ FormComponent?: typeof PlaygroundForm; }> = ({ FormComponent = PlaygroundForm }) => { @@ -304,11 +357,16 @@ export const AdvancedPlayground: VFC<{ Object.values(errors).length === 0 } show={ - + <> + + + } />