From e877580d5405fef1756083d637daa5c38dc82c92 Mon Sep 17 00:00:00 2001 From: Mahtis Michel Date: Mon, 25 Jan 2021 20:16:07 +0100 Subject: [PATCH] feat(ux): enhance media-type switching experience in RequestBodyEditor (#6837) * feat(ux): enhance media-type switching experience in RequestBodyEditor 1. When canceling the try-out mode the request body will be reset to its initial state. 2. When the user switches the media-type in the try-out mode, the experience is as follows: - If the user did edit the request body the body wont be touched and only media type is updated. This is to ensure that user content is NEVER accidentally overwritten with a default value. - If the user did not edit the request body it is safe to be replaced by the default value of the target media-type. Multiple example needed some care in order to allow the retain example value to function properly * fix(test): workaround cypress issue that can't be reproduced manually * test: added new feature to ensure enhanced user editing flow Signed-off-by: mathis-m --- .../examples-select-value-retainer.jsx | 51 ++++++++--- src/core/components/parameters/parameters.jsx | 30 ++++--- src/core/components/try-it-out-button.jsx | 15 +++- src/core/plugins/oas3/actions.js | 9 ++ .../oas3/components/request-body-editor.jsx | 5 +- .../plugins/oas3/components/request-body.jsx | 9 +- src/core/plugins/oas3/reducers.js | 6 +- src/core/plugins/oas3/selectors.js | 41 ++++++++- src/core/plugins/spec/selectors.js | 2 +- src/style/_layout.scss | 6 +- .../features/oas3-multiple-media-type.js | 6 +- .../oas3-user-edit-request-body-flows.js | 87 +++++++++++++++++++ 12 files changed, 235 insertions(+), 32 deletions(-) create mode 100644 test/e2e-cypress/tests/features/oas3-user-edit-request-body-flows.js diff --git a/src/core/components/examples-select-value-retainer.jsx b/src/core/components/examples-select-value-retainer.jsx index e959105c..9f038624 100644 --- a/src/core/components/examples-select-value-retainer.jsx +++ b/src/core/components/examples-select-value-retainer.jsx @@ -36,16 +36,22 @@ export default class ExamplesSelectValueRetainer extends React.PureComponent { examples: ImPropTypes.map, onSelect: PropTypes.func, updateValue: PropTypes.func, // mechanism to update upstream value + userHasEditedBody: PropTypes.bool, getComponent: PropTypes.func.isRequired, currentUserInputValue: PropTypes.any, currentKey: PropTypes.string, currentNamespace: PropTypes.string, + setRetainRequestBodyValueFlag: PropTypes.func.isRequired, // (also proxies props for Examples) } static defaultProps = { + userHasEditedBody: false, examples: Map({}), currentNamespace: "__DEFAULT__NAMESPACE__", + setRetainRequestBodyValueFlag: () => { + // NOOP + }, onSelect: (...args) => console.log( // eslint-disable-line no-console "ExamplesSelectValueRetainer: no `onSelect` function was provided", @@ -72,11 +78,16 @@ export default class ExamplesSelectValueRetainer extends React.PureComponent { lastDownstreamValue: valueFromExample, isModifiedValueSelected: // valueFromExample !== undefined && + this.props.userHasEditedBody || this.props.currentUserInputValue !== valueFromExample, }), } } + componentWillUnmount() { + this.props.setRetainRequestBodyValueFlag(false) + } + _getStateForCurrentNamespace = () => { const { currentNamespace } = this.props @@ -122,7 +133,12 @@ export default class ExamplesSelectValueRetainer extends React.PureComponent { } _onExamplesSelect = (key, { isSyntheticChange } = {}, ...otherArgs) => { - const { onSelect, updateValue, currentUserInputValue } = this.props + const { + onSelect, + updateValue, + currentUserInputValue, + userHasEditedBody, + } = this.props const { lastUserEditedValue } = this._getStateForCurrentNamespace() const valueFromExample = this._getValueForExample(key) @@ -141,9 +157,8 @@ export default class ExamplesSelectValueRetainer extends React.PureComponent { this._setStateForCurrentNamespace({ lastDownstreamValue: valueFromExample, isModifiedValueSelected: - isSyntheticChange && - !!currentUserInputValue && - currentUserInputValue !== valueFromExample, + (isSyntheticChange && userHasEditedBody) || + (!!currentUserInputValue && currentUserInputValue !== valueFromExample), }) // we never want to send up value updates from synthetic changes @@ -157,7 +172,12 @@ export default class ExamplesSelectValueRetainer extends React.PureComponent { componentWillReceiveProps(nextProps) { // update `lastUserEditedValue` as new currentUserInput values come in - const { currentUserInputValue: newValue, examples, onSelect } = nextProps + const { + currentUserInputValue: newValue, + examples, + onSelect, + userHasEditedBody, + } = nextProps const { lastUserEditedValue, @@ -170,7 +190,7 @@ export default class ExamplesSelectValueRetainer extends React.PureComponent { ) const exampleMatchingNewValue = examples.find( - example => + (example) => example.get("value") === newValue || // sometimes data is stored as a string (e.g. in Request Bodies), so // let's check against a stringified version of our example too @@ -186,15 +206,23 @@ export default class ExamplesSelectValueRetainer extends React.PureComponent { newValue !== lastUserEditedValue && // value isn't already tracked newValue !== lastDownstreamValue // value isn't what we've seen on the other side ) { + this.props.setRetainRequestBodyValueFlag(true) this._setStateForNamespace(nextProps.currentNamespace, { lastUserEditedValue: nextProps.currentUserInputValue, - isModifiedValueSelected: newValue !== valueFromCurrentExample, + isModifiedValueSelected: + userHasEditedBody || newValue !== valueFromCurrentExample, }) } } render() { - const { currentUserInputValue, examples, currentKey, getComponent } = this.props + const { + currentUserInputValue, + examples, + currentKey, + getComponent, + userHasEditedBody, + } = this.props const { lastDownstreamValue, lastUserEditedValue, @@ -212,9 +240,10 @@ export default class ExamplesSelectValueRetainer extends React.PureComponent { !!lastUserEditedValue && lastUserEditedValue !== lastDownstreamValue } isValueModified={ - currentUserInputValue !== undefined && - isModifiedValueSelected && - currentUserInputValue !== this._getCurrentExampleValue() + (currentUserInputValue !== undefined && + isModifiedValueSelected && + currentUserInputValue !== this._getCurrentExampleValue()) || + userHasEditedBody } /> ) diff --git a/src/core/components/parameters/parameters.jsx b/src/core/components/parameters/parameters.jsx index d09700dd..a33cc508 100644 --- a/src/core/components/parameters/parameters.jsx +++ b/src/core/components/parameters/parameters.jsx @@ -75,30 +75,29 @@ export default class Parameters extends Component { } onChangeMediaType = ({ value, pathMethod }) => { - let { specSelectors, specActions, oas3Selectors, oas3Actions } = this.props - let targetMediaType = value - let currentMediaType = oas3Selectors.requestContentType(...pathMethod) - let schemaPropertiesMatch = specSelectors.isMediaTypeSchemaPropertiesEqual(pathMethod, currentMediaType, targetMediaType) - if (!schemaPropertiesMatch) { - oas3Actions.clearRequestBodyValue({ pathMethod }) + let { specActions, oas3Selectors, oas3Actions } = this.props + const userHasEditedBody = oas3Selectors.hasUserEditedBody(...pathMethod) + const shouldRetainRequestBodyValue = oas3Selectors.shouldRetainRequestBodyValue(...pathMethod) + oas3Actions.setRequestContentType({ value, pathMethod }) + oas3Actions.initRequestBodyValidateError({ pathMethod }) + if (!userHasEditedBody) { + if(!shouldRetainRequestBodyValue) { + oas3Actions.setRequestBodyValue({ value: undefined, pathMethod }) + } specActions.clearResponse(...pathMethod) specActions.clearRequest(...pathMethod) specActions.clearValidateParams(pathMethod) } - oas3Actions.setRequestContentType({ value, pathMethod }) - oas3Actions.initRequestBodyValidateError({ pathMethod }) } render() { let { onTryoutClick, - onCancelClick, parameters, allowTryItOut, tryItOutEnabled, specPath, - fn, getComponent, getConfigs, @@ -131,6 +130,7 @@ export default class Parameters extends Component { }, {})) .reduce((acc, x) => acc.concat(x), []) + const retainRequestBodyValueFlagForOperation = (f) => oas3Actions.setRetainRequestBodyValueFlag({ value: f, pathMethod }) return (
@@ -155,7 +155,13 @@ export default class Parameters extends Component {
)} {allowTryItOut ? ( - + oas3Actions.setRequestBodyValue({ value: undefined, pathMethod })}/> ) : null}
{this.state.parametersVisible ?
@@ -219,6 +225,8 @@ export default class Parameters extends Component {
+
{ enabled ? : + + } + { + showReset && }
) diff --git a/src/core/plugins/oas3/actions.js b/src/core/plugins/oas3/actions.js index e0f3c256..a45ae41f 100644 --- a/src/core/plugins/oas3/actions.js +++ b/src/core/plugins/oas3/actions.js @@ -3,6 +3,7 @@ export const UPDATE_SELECTED_SERVER = "oas3_set_servers" export const UPDATE_REQUEST_BODY_VALUE = "oas3_set_request_body_value" +export const UPDATE_REQUEST_BODY_VALUE_RETAIN_FLAG = "oas3_set_request_body_retain_flag" export const UPDATE_REQUEST_BODY_INCLUSION = "oas3_set_request_body_inclusion" export const UPDATE_ACTIVE_EXAMPLES_MEMBER = "oas3_set_active_examples_member" export const UPDATE_REQUEST_CONTENT_TYPE = "oas3_set_request_content_type" @@ -26,6 +27,14 @@ export function setRequestBodyValue ({ value, pathMethod }) { } } +export const setRetainRequestBodyValueFlag = ({ value, pathMethod }) => { + return { + type: UPDATE_REQUEST_BODY_VALUE_RETAIN_FLAG, + payload: { value, pathMethod } + } +} + + export function setRequestBodyInclusion ({ value, pathMethod, name }) { return { type: UPDATE_REQUEST_BODY_INCLUSION, diff --git a/src/core/plugins/oas3/components/request-body-editor.jsx b/src/core/plugins/oas3/components/request-body-editor.jsx index ea729ba5..216fd8f3 100644 --- a/src/core/plugins/oas3/components/request-body-editor.jsx +++ b/src/core/plugins/oas3/components/request-body-editor.jsx @@ -17,6 +17,7 @@ export default class RequestBodyEditor extends PureComponent { static defaultProps = { onChange: NOOP, + userHasEditedBody: false, }; constructor(props, context) { @@ -65,7 +66,7 @@ export default class RequestBodyEditor extends PureComponent { }) } - + if(!nextProps.value && nextProps.defaultValue && !!this.state.value) { // if new value is falsy, we have a default, AND the falsy value didn't @@ -77,7 +78,7 @@ export default class RequestBodyEditor extends PureComponent { render() { let { getComponent, - errors + errors, } = this.props let { diff --git a/src/core/plugins/oas3/components/request-body.jsx b/src/core/plugins/oas3/components/request-body.jsx index 0affb8ac..24dcde3c 100644 --- a/src/core/plugins/oas3/components/request-body.jsx +++ b/src/core/plugins/oas3/components/request-body.jsx @@ -4,7 +4,7 @@ import ImPropTypes from "react-immutable-proptypes" import { Map, OrderedMap, List } from "immutable" import { getCommonExtensions, getSampleSchema, stringify, isEmptyValue } from "core/utils" -function getDefaultRequestBodyValue(requestBody, mediaType, activeExamplesKey) { +export const getDefaultRequestBodyValue = (requestBody, mediaType, activeExamplesKey) => { const mediaTypeValue = requestBody.getIn(["content", mediaType]) const schema = mediaTypeValue.get("schema").toJS() @@ -32,6 +32,7 @@ function getDefaultRequestBodyValue(requestBody, mediaType, activeExamplesKey) { const RequestBody = ({ + userHasEditedBody, requestBody, requestBodyValue, requestBodyInclusionSetting, @@ -47,6 +48,7 @@ const RequestBody = ({ onChangeIncludeEmpty, activeExamplesKey, updateActiveExamplesKey, + setRetainRequestBodyValueFlag }) => { const handleFile = (e) => { onChange(e.target.files[0]) @@ -222,6 +224,7 @@ const RequestBody = ({ { examplesForMediaType ? ( ) : null } @@ -276,6 +280,7 @@ const RequestBody = ({ } RequestBody.propTypes = { + userHasEditedBody: PropTypes.bool.isRequired, requestBody: ImPropTypes.orderedMap.isRequired, requestBodyValue: ImPropTypes.orderedMap.isRequired, requestBodyInclusionSetting: ImPropTypes.Map.isRequired, @@ -291,6 +296,8 @@ RequestBody.propTypes = { specPath: PropTypes.array.isRequired, activeExamplesKey: PropTypes.string, updateActiveExamplesKey: PropTypes.func, + setRetainRequestBodyValueFlag: PropTypes.func, + oas3Actions: PropTypes.object.isRequired } export default RequestBody diff --git a/src/core/plugins/oas3/reducers.js b/src/core/plugins/oas3/reducers.js index e317bbdc..9efdb267 100644 --- a/src/core/plugins/oas3/reducers.js +++ b/src/core/plugins/oas3/reducers.js @@ -10,7 +10,7 @@ import { UPDATE_RESPONSE_CONTENT_TYPE, SET_REQUEST_BODY_VALIDATE_ERROR, CLEAR_REQUEST_BODY_VALIDATE_ERROR, - CLEAR_REQUEST_BODY_VALUE, + CLEAR_REQUEST_BODY_VALUE, UPDATE_REQUEST_BODY_VALUE_RETAIN_FLAG, } from "./actions" export default { @@ -42,6 +42,10 @@ export default { }) return state.setIn(["requestData", path, method, "bodyValue"], newVal) }, + [UPDATE_REQUEST_BODY_VALUE_RETAIN_FLAG]: (state, { payload: { value, pathMethod } } ) =>{ + let [path, method] = pathMethod + return state.setIn(["requestData", path, method, "retainBodyValue"], value) + }, [UPDATE_REQUEST_BODY_INCLUSION]: (state, { payload: { value, pathMethod, name } } ) =>{ let [path, method] = pathMethod return state.setIn( [ "requestData", path, method, "bodyInclusion", name ], value) diff --git a/src/core/plugins/oas3/selectors.js b/src/core/plugins/oas3/selectors.js index 2176e2e5..72c86f01 100644 --- a/src/core/plugins/oas3/selectors.js +++ b/src/core/plugins/oas3/selectors.js @@ -1,5 +1,7 @@ -import { OrderedMap, Map } from "immutable" +import { OrderedMap, Map, List } from "immutable" import { isOAS3 as isOAS3Helper } from "./helpers" +import { getDefaultRequestBodyValue } from "./components/request-body" +import { stringify } from "../../utils" // Helpers @@ -54,6 +56,43 @@ export const requestBodyValue = onlyOAS3((state, path, method) => { } ) +export const shouldRetainRequestBodyValue = onlyOAS3((state, path, method) => { + return state.getIn(["requestData", path, method, "retainBodyValue"]) || false + } +) + +export const hasUserEditedBody = (state, path, method) => (system) => { + const {oas3Selectors, specSelectors} = system.getSystem() + const spec = specSelectors.specJson() + if(isOAS3Helper(spec)) { + let userHasEditedBody = false + const currentMediaType = oas3Selectors.requestContentType(path, method) + let userEditedRequestBody = oas3Selectors.requestBodyValue(path, method) + if (Map.isMap(userEditedRequestBody)) { + // context is not application/json media-type + userEditedRequestBody = stringify(userEditedRequestBody.mapEntries((kv) => Map.isMap(kv[1]) ? [kv[0], kv[1].get("value")] : kv).toJS()) + } + if(List.isList(userEditedRequestBody)) { + userEditedRequestBody = stringify(userEditedRequestBody) + } + if (currentMediaType) { + const currentMediaTypeDefaultBodyValue = getDefaultRequestBodyValue( + specSelectors.specResolvedSubtree(["paths", path, method, "requestBody"]), + currentMediaType, + oas3Selectors.activeExamplesMember( + path, method, + "requestBody", + "requestBody", + ) + ) + userHasEditedBody = !!userEditedRequestBody && userEditedRequestBody !== currentMediaTypeDefaultBodyValue + } + return userHasEditedBody + } else { + return null + } +} + export const requestBodyInclusionSetting = onlyOAS3((state, path, method) => { return state.getIn(["requestData", path, method, "bodyInclusion"]) || Map() } diff --git a/src/core/plugins/spec/selectors.js b/src/core/plugins/spec/selectors.js index d216326d..89ea09ed 100644 --- a/src/core/plugins/spec/selectors.js +++ b/src/core/plugins/spec/selectors.js @@ -525,7 +525,7 @@ export const isMediaTypeSchemaPropertiesEqual = ( state, pathMethod, currentMedi } let currentMediaTypeSchemaProperties = requestBodyContent.getIn([currentMediaType, "schema", "properties"], fromJS([])) let targetMediaTypeSchemaProperties = requestBodyContent.getIn([targetMediaType, "schema", "properties"], fromJS([])) - return currentMediaTypeSchemaProperties.equals(targetMediaTypeSchemaProperties) ? true: false + return !!currentMediaTypeSchemaProperties.equals(targetMediaTypeSchemaProperties) } function returnSelfOrNewMap(obj) { diff --git a/src/style/_layout.scss b/src/style/_layout.scss index 2192712e..40f62c94 100644 --- a/src/style/_layout.scss +++ b/src/style/_layout.scss @@ -13,6 +13,10 @@ flex-direction: column; } +.try-out.btn-group { + padding: 0; +} + .opblock-tag { display: flex; @@ -668,7 +672,7 @@ overflow-y: auto; max-height: 400px; min-height: 6em; - + code { white-space: pre-wrap !important; word-break: break-all; diff --git a/test/e2e-cypress/tests/features/oas3-multiple-media-type.js b/test/e2e-cypress/tests/features/oas3-multiple-media-type.js index 13e8cde6..18505e9c 100644 --- a/test/e2e-cypress/tests/features/oas3-multiple-media-type.js +++ b/test/e2e-cypress/tests/features/oas3-multiple-media-type.js @@ -22,13 +22,17 @@ describe("OpenAPI 3.0 Multiple Media Types with different schemas", () => { cy.get(".opblock-section-request-body .content-type").as("selectMediaType") }) - // In all cases, + // In all cases, // - assume that examples are populated based on schema (not explicitly tested) // - assume validation passes based on successful "execute" // - expect final cURL command result doees not contain unexpected artifacts from other content-type schemas describe("multipart/form-data (only 'bar')", () => { it("should execute multipart/form-data", () => { cy.get("@selectMediaType") + .select(mediaTypeUrlencoded) + .get("@executeBtn") + .click() + .get("@selectMediaType") .select(mediaTypeFormData) .get("@executeBtn") .click() diff --git a/test/e2e-cypress/tests/features/oas3-user-edit-request-body-flows.js b/test/e2e-cypress/tests/features/oas3-user-edit-request-body-flows.js new file mode 100644 index 00000000..ec66f647 --- /dev/null +++ b/test/e2e-cypress/tests/features/oas3-user-edit-request-body-flows.js @@ -0,0 +1,87 @@ +const getRequestBodyFromCY = (page = null) => + (page || cy.visit( + "/?url=/documents/features/petstore-only-pet.openapi.yaml", + )) + .get("#operations-pet-addPet") + .click() + // Expand Try It Out + .get(".try-out__btn") + .click() + // get textarea + .get(".opblock-body .opblock-section .opblock-section-request-body .body-param textarea") + +const xmlIndicator = "\n" +const userEditXmlSample = xmlIndicator + + "\n" + + "\t420\n" + + "\tdoggie<3\n" + + "\t\n" + + "\t\t99999999999\n" + + "\t\tDogiiiiiiiieeee\n" + + "\t\n" + + "\t\n" + + "\t\tstring\n" + + "\t\n" + + "\t\n" + + "\t\t\n" + + "\t\t\t0\n" + + "\t\t\tstring\n" + + "\t\t\n" + + "\t\n" + + "\tavailable\n" + + "" + +describe("OAS3 Request Body user edit flows", () => { + // Case: Copy xml from email, paste into request body editor, change media-type to xml + it("it should never overwrite user edited value in case of media-type change", () => { + getRequestBodyFromCY() + // replace default sample with xml edited value + .type(`{selectall}${userEditXmlSample}`) + // change media type to xml, because I have forgotten it + .get(".opblock-section .opblock-section-request-body .body-param-content-type > select") + .select("application/xml") + // Ensure user edited body is not overwritten + .get(".opblock-section-request-body") + .within(() => { + cy + .get("textarea") + .should(($div) => { + expect($div.get(0).textContent).to.eq(userEditXmlSample) + }) + }) + }) + // Case: User really wants to try out the brand new xml content-type + it("it should overwrite default value in case of content-type change, even within request body editor(#6836)", () => { + getRequestBodyFromCY() + // change media type to xml, because I have forgotten it (sry really wanted to try out the new xml content-type) + .get(".opblock-section .opblock-section-request-body .body-param-content-type > select") + .select("application/xml") + // Ensure default value is xml after content type change + .get(".opblock-section-request-body") + .within(() => { + cy + .get("textarea") + .should(($div) => { + expect($div.get(0).textContent).to.contain(xmlIndicator) + }) + }) + }) + // Case: User wants to get the default value back + it("it reset the user edited value and render the default value in case of try out reset. (#6517)", () => { + getRequestBodyFromCY() + // replace default sample with bad value + .type("{selectall}ups that should not have happened") + // Cancel Try It Out + .get(".try-out__btn.reset") + .click() + // Ensure default value is xml after content type change + .get(".opblock-section-request-body") + .within(() => { + cy + .get("textarea") + .should(($div) => { + expect($div.get(0).textContent).to.not.contain("ups that should not have happened") + }) + }) + }) +})