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.

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

Fix reset button in Try It Out form #9346

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aviral-badola9
Copy link

Description

The reset button needed the following two corrections:

  1. The oas3Actions.setRequestBodyValue expects a orderedMap/Map and was currently being passed a string. So a conversion from string to orderedMap was added.
  2. The UPDATE_REQUEST_BODY_VALUE action currently did not copy over the values if the value was a map assuming the user entered values will be string but that is not the case in reset, as the values will already have been converted to map. Therefore even the map values have been copied to correct key to reflect changes.
  3. Furthermore, the loop to copy over the values overwrote the previous iteration changes due to it using the same unchanged variable every loop. The variable is now updated and retained every iteration to preserve changes from previous iterations.

Motivation and Context

Fixes #9158

How Has This Been Tested?

I manually tested this by opening the try it out form and using the yaml file provided in the issue description.

Screenshots (if appropriate):

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

@mathis-m
Copy link
Contributor

LGTM thanks for your efforts and for advancing further the reset functionality.
Didn't noticed that when implementing #6837

Maybe you could add a test for regression

@aviral-badola9
Copy link
Author

LGTM thanks for your efforts and for advancing further the reset functionality. Didn't noticed that when implementing #6837

Maybe you could add a test for regression

Sure, I can add that. Could you point me in the right direction of what test I can write here. I see a test for this oas3-user-edit-request-body-flows.cy.js. Should that be edited or a new one be added.

@aviral-badola9
Copy link
Author

mathis-m

I have added a cypress test for regression. Kindly review the changes.

Copy link
Contributor

@glowcloud glowcloud left a comment

Choose a reason for hiding this comment

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

Thanks for this change.

I checked it out and it looks fine but I found more issues when using the specification from the issue itself.
It seems like it’s because the UserSource schema there has maxProperties set to 1 / lower count than each property of the schema. Despite this, each of the example values is filled and being sent. The reset button also stays visible even without making any changes. When resetting, only the first value is reset. It seems that not resetting other values is because we check if we already added the max amount of properties in sampleFromSchemaGeneric.

src/core/containers/OperationContainer.jsx Outdated Show resolved Hide resolved
@aviral-badola9
Copy link
Author

Thanks for this change.

I checked it out and it looks fine but I found more issues when using the specification from the issue itself. It seems like it’s because the UserSource schema there has maxProperties set to 1 / lower count than each property of the schema. Despite this, each of the example values is filled and being sent. The reset button also stays visible even without making any changes. When resetting, only the first value is reset. It seems that not resetting other values is because we check if we already added the max amount of properties in sampleFromSchemaGeneric.

Thanks for the review @glowcloud . I will take a look at this issue.

@glowcloud
Copy link
Contributor

I created a separate issue for maxProperties not being validated by Swagger UI #9675

The issue with only resetting some parameters seems related to this PR/issue, so leaving it here for now.

@aviral-badola9
Copy link
Author

I created a separate issue for maxProperties not being validated by Swagger UI #9675

The issue with only resetting some parameters seems related to this PR/issue, so leaving it here for now.

Have used the respectXML prop to bypass maxProperties check. The reset button now only appears if the body is actually edited and all fields are reset. Please review the changes. @glowcloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Reset" button creates invalid inputs in the Try It Out form
4 participants