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

[Bug] Fix createPostmanCollection method where a copy of openapiData is generated #183

Merged
merged 2 commits into from
Aug 1, 2022

Conversation

johnnyswan
Copy link
Contributor

Description

This PR introduces the following changes:

  1. Fix createPostmanCollection method where a copy of openapiData is generated
  2. Add groupId to Tabs component for Authentication section in generated *.info.mdx file

Motivation and Context

  1. The motivation is to fix the problem when some paths can have their own server object. For example:
openapi: 3.0.3
info:
  title: Burger Example
  version: 1.0.0
  description: Sample description.
servers:
  - url: https://api.example.com/v1
paths:
  /user:
    get:
      servers:
        - url: https://users.example.com/v1
      responses:
        200:
          description: OK
  /flavors:
    get:
      responses:
        200:
          description: OK

The previous realization with Object.assign does not copy nested objects, and the next operation with deleting 'servers' attributes removes them into the parent object too.

  1. Fix problem when changing of Authentication method in *.info.mdx file does not save user choice to storage and does not change security scheme all over the documentation

How Has This Been Tested?

Environment: Local

Changes were tested by viewing specific API docs to ensure functionality for API docs were intact and ensuring that the app builds successfully upon each change.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes if appropriate.
  • All new and existing tests passed.

@welcome-to-palo-alto-networks

🎉 Thanks for opening this pull request! We really appreciate contributors like you! 🙌

@github-actions
Copy link

github-actions bot commented Jul 31, 2022

Visit the preview URL for this PR (updated for commit 57b57b5):

https://docusaurus-openapi-36b86--pr183-tz6wbim6.web.app

(expires Wed, 31 Aug 2022 19:44:35 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@sserrata
Copy link
Member

sserrata commented Aug 1, 2022

Hi @johnnyswan, thanks for your contribution! The cloneDeep implementation looks good but it looks like we missed an opportunity to "tree shake" the kebabCase import. Can you refactor as follows?

Change this:

import { kebabCase, cloneDeep } from "lodash";

To this:

import { kebabCase } from "lodash/kebabCase";
import { cloneDeep } from "lodash/cloneDeep";

As for the groupId change for the authentication tabs, I was wondering what the expected behavior/use-case should be. Based on the change alone, I would only expect the tab choice to be persisted in localStorage. Is the idea to eventually support reading the tab choice from localStorage and using it somewhere else? I ask because sometimes the authentication tab choices are meant to be used in combination, like when multiple authentication headers are required for example - just something to keep in mind.

@johnnyswan
Copy link
Contributor Author

I plan to use groupId in SecuritySchemes-like component

@sserrata
Copy link
Member

sserrata commented Aug 1, 2022

Gotcha - definitely excited to see what you have in mind!

In that case, I think it's best to leave the groupId change out of this PR with the intent to reintroduce it with the new SecuritySchemes-like component.

Copy link
Member

@sserrata sserrata left a comment

Choose a reason for hiding this comment

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

Remove groupId from createAuthentication until new SecurityScheme component is introduced. Thanks!

Copy link
Member

@sserrata sserrata left a comment

Choose a reason for hiding this comment

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

🚀

@sserrata sserrata merged commit 5c623f5 into PaloAltoNetworks:main Aug 1, 2022
@welcome-to-palo-alto-networks
Copy link

🎉 Congrats on getting your first pull request merged! We here at Palo Alto Networks are so grateful! ❤️

@TarasMelnyk
Copy link

Can you please say when will be the release of this fix? I have the same issue.

@sserrata
Copy link
Member

sserrata commented Aug 2, 2022

Hi @TarasMelnyk, we should have a new production release out later this week. If you need it sooner, you can always upgrade to a canary release, which auto publishes whenever we merge to main.

image

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

Successfully merging this pull request may close these issues.

None yet

3 participants