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(documentation): use correct localization schema #18580

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

ciriousjoker
Copy link
Contributor

@ciriousjoker ciriousjoker commented Oct 26, 2023

What does it do?

The generated full_documentation.json contains an error in the localizations property. The localizations property only shows up when strapi-plugin-populate-deep is also installed, but the issue is in the documentation plugin, not strapi-plugin-populate-deep.

The edited line is only hit once in the debugger and patching it seems to do exactly what I want and nothing else. It's used in production at our company.

Why is it needed?

Without this, the localization field in the schema doesn't match the actual api response. The api response contains an id & attributes, which then contain the actual data. The schema makes it seem like the localization field already contains the data.

Api response via Postman:
CleanShot 2023-10-26 at 07 54 51@2x

How to test it?

  1. Clone this: https://github.com/ciriousjoker/demo_strapi_localization_field
  2. Check out first, second and third commit in order
  3. Notice that by switching to the forked version that matches this PR, the schema now matches the api response

Related issue(s)/PR(s)

Context

We had a pretty scuffed package-lock.json at some point, which for whatever reason produced a correct full_documentation.json schema. However, I couldn't reproduce this correct behavior when deleting the package-lock.json & the node_modules folder. In the working full_documentation.json, it used this name: ArticleListResponseDataItemLocalized, so when I had the option of choosing the suffix, I chose the one that most closely resembled the (I assume) original, correct behavior.

@strapi-cla
Copy link

strapi-cla commented Oct 26, 2023

CLA assistant check
All committers have signed the CLA.

@Boegie19
Copy link
Collaborator

Maby a stupit question but should we not check if i18n plugin is installed and enabled before we add "ListResponseDataItemLocalized" or maby make this documentation come from the i18n package.

@Boegie19
Copy link
Collaborator

Also @ciriousjoker Mind adding fixes: #18577 to your initial message so that it properly links the issue and your PR.

@ciriousjoker
Copy link
Contributor Author

Do you mean adding it to the commit message or to the PR message?
I added it to the PR message now. Ping me if that's not enough.

@ciriousjoker
Copy link
Contributor Author

Should I bother to keep this in sync with the main branch or can I do it once right before it's (hopefully) being merged?

@Boegie19
Copy link
Collaborator

Idk don't work for strapi soo.

Copy link
Contributor

@markkaylor markkaylor left a comment

Choose a reason for hiding this comment

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

I think this looks good. I'll just want to test it when I have a second later. I just had one small request 🙏 .

Co-authored-by: markkaylor <mkaylor.guitar@gmail.com>
@ciriousjoker
Copy link
Contributor Author

Not sure how to fix the PR labels. @markkaylor Do I need to do anything?

@markkaylor markkaylor added pr: fix This PR is fixing a bug source: plugin:documentation Source is plugin/documentation package labels Nov 10, 2023
@markkaylor markkaylor self-requested a review November 17, 2023 14:38
@ciriousjoker
Copy link
Contributor Author

Thanks for reviewing it. Any eta for when this will be merged?

@markkaylor markkaylor added this to the 4.15.6 milestone Nov 27, 2023
@markkaylor markkaylor merged commit 4d35516 into strapi:main Nov 27, 2023
92 of 94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix This PR is fixing a bug source: plugin:documentation Source is plugin/documentation package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation plugin generates incorrect localization field
4 participants