Skip to content

Conversation

jeff-matthews
Copy link
Contributor

@jeff-matthews jeff-matthews commented Jul 18, 2025

Purpose of this pull request

This pull request (PR) implements the GetCredential component for the Catalog Data Ingestion REST API. It enables Redocly's integrated API client in the reference documentation (relates to commerce-webapi#462).

Staging

https://developer-stage.adobe.com/commerce/services/reference/rest/

TODOs

  • Consult with PM/ENG to ensure alignment/expectations
  • Create project template for ACO in Adobe Developer Console (or add ACO API to existing template)
  • Replace templateId in get credential component (maybe; see previous TODO)
  • Add guidance for manually adding tenantId (get credentials component currently doesn't support retrieval)
  • Address OpenAPI spec errors (e.g., add x-summary to long response objects to fix page overflow on render)
  • Remove unnecessary Redocly config file
  • Deploy to staging for testing
  • Replace stage template ID & server URL with production (sandbox) as final step
    • We'll do this in the delivery to main. I created a new integration branch for this PR because it got a little messy.

@jeff-matthews jeff-matthews self-assigned this Jul 18, 2025
@github-project-automation github-project-automation bot moved this to 📋 Needs Review in Commerce - Pull Requests Jul 18, 2025
@jeff-matthews jeff-matthews added enhancement New feature or request major-update Significant original updates to existing content labels Jul 18, 2025
@jeff-matthews jeff-matthews changed the base branch from main to develop July 21, 2025 17:39
Base automatically changed from develop to main July 29, 2025 20:42
meker12 added 2 commits August 1, 2025 14:52
Remove unneeded file.  Data Ingestion API reference is pulled
@jeff-matthews
Copy link
Contributor Author

I'm not sure why, but the Try it console is displaying the server description instead of the url:

Screenshot 2025-08-04 at 1 46 36 PM

I don't know if this is a bug or expected behavior, but we need to show the URL here, even if that means removing the description.

@jeff-matthews
Copy link
Contributor Author

Another thing to note that isn't very intuitive: the Security section in the Try it console provides a dropdown menu that you must select for each of the three fields required to complete the request (e.g., API key, bearer token, and IMs org).

Screenshot 2025-08-04 at 1 50 07 PM

This might be why the AEP team chose not to use the securitySchemes OpenAPI object and used parameters instead. Parameters show as a flat list and don't require clicking through dropdown menus.

Screenshot 2025-08-04 at 1 55 00 PM

If we can't control the display of the Try it console, is it worth not adhering to the OpenAPI spec to improve UX?

@meker12
Copy link
Contributor

meker12 commented Aug 4, 2025

Another thing to note that isn't very intuitive: the Security section in the Try it console provides a dropdown menu that you must select for each of the three fields required to complete the request (e.g., API key, bearer token, and IMs org).

Screenshot 2025-08-04 at 1 50 07 PM This might be why the AEP team chose not to use the `securitySchemes` OpenAPI object and used parameters instead. Parameters show as a flat list and don't require clicking through dropdown menus. Screenshot 2025-08-04 at 1 55 00 PM If we can't control the display of the **Try it** console, is it worth not adhering to the OpenAPI spec to improve UX?

In my opinion yes --

@meker12
Copy link
Contributor

meker12 commented Aug 4, 2025

I'm not sure why, but the Try it console is displaying the server description instead of the url:

Screenshot 2025-08-04 at 1 46 36 PM I don't know if this is a bug or expected behavior, but we need to show the URL here, even if that means removing the description.

I notice that the description is not provided in any of the Target API specs. Another example of not following the spec to support better usability of the component, but it does seem like you should be able to have both.

@github-project-automation github-project-automation bot moved this from 📋 Needs Review to 🛠 Changes Requested in Commerce - Pull Requests Aug 6, 2025
@jeff-matthews jeff-matthews changed the base branch from main to redocly-try-it August 6, 2025 00:53
@jeff-matthews jeff-matthews marked this pull request as ready for review August 6, 2025 01:12
@jeff-matthews jeff-matthews requested a review from meker12 August 6, 2025 01:27
@jeff-matthews
Copy link
Contributor Author

@meker12

I create an integration branch and updated the base branch for this PR accordingly. I think that this Pr should be for our review. When we merge with the integration branch, I'll create a new PR for developer review.

Copy link
Contributor

@meker12 meker12 left a comment

Choose a reason for hiding this comment

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

Not sure if it works, but I think it makes more sense to include the instructions for adding the tenant ID info with the access token.

@jeff-matthews jeff-matthews requested a review from meker12 August 6, 2025 16:52
@jeff-matthews jeff-matthews merged commit e5696e5 into redocly-try-it Aug 6, 2025
11 checks passed
@jeff-matthews jeff-matthews deleted the test-playground branch August 6, 2025 22:18
@github-project-automation github-project-automation bot moved this from 🛠 Changes Requested to 🏁 Done in Commerce - Pull Requests Aug 6, 2025
@jeff-matthews jeff-matthews changed the title Data Ingestion REST API playground Data Ingestion REST API playground (staging) Aug 6, 2025
@jeff-matthews
Copy link
Contributor Author

See #284 for draft production PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request major-update Significant original updates to existing content
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants