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

Handle non-localized content-types in discard-draft migration #20422

Merged
merged 5 commits into from
Jun 24, 2024

Conversation

Convly
Copy link
Member

@Convly Convly commented Jun 3, 2024

What does it do?

Adds a check to conditionally set locale in the discardDraft's params.

Why is it needed?

This can break app with no i18n
See #20225

How to test it?

See reproduction steps in #20225

Related issue(s)/PR(s)

fix #20225

Copy link

vercel bot commented Jun 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
contributor-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 24, 2024 7:53am

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

the PR looks good but sounds like we might want to fix this inside discardDraft (or both)

Copy link
Contributor

@innerdvations innerdvations left a comment

Choose a reason for hiding this comment

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

There's a build error from the discardDraft typings, but other than that LGTM

const isLocalized = strapi
.plugin('i18n')
.service('content-types')
.isLocalizedContentType(oldContentType);
Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure whether we wanted to target the old CT or the new one since I'm not sure about what we're trying to achieve exactly.

cc @Marc-Roig for confirmation

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should target the new one

@maccomaccomaccomacco
Copy link
Contributor

You would make a PM happy by resolving this PR, it's preventing me to have a data migration done

@Convly
Copy link
Member Author

Convly commented Jun 21, 2024

You would make a PM happy by resolving this PR, it's preventing me to have a data migration done

Even on the latest beta (14) ?

@ildfreelancer
Copy link

@Convly yes, the issue is still there while migrating from v4 to v5 (using beta(14))
image

@Convly
Copy link
Member Author

Convly commented Jun 24, 2024

@Convly yes, the issue is still there while migrating from v4 to v5 (using beta(14)) image

Wait, how is it possible to expect localized admin users, is it the wanted behavior in the new i18n implementation @Marc-Roig?

@Convly Convly changed the title Handle non-localized content-types in v5 discard-draft migration Handle non-localized content-types in discard-draft migration Jun 24, 2024
@Convly Convly merged commit bdee57a into v5/main Jun 24, 2024
7 of 9 checks passed
@Convly Convly deleted the fix/discard-draft-migration-non-localized-content branch June 24, 2024 07:41
@Marc-Roig
Copy link
Contributor

Marc-Roig commented Jun 25, 2024

Wait, how is it possible to expect localized admin users, is it the wanted behavior in the new i18n implementation

@Convly all content types are expected to have the locale column atm

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: core:core Source is core/core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants