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: Creating a new locale doesn't pre-fill the non-internationalized fields #18773

Merged
merged 2 commits into from Mar 27, 2024

Conversation

derrickmehaffy
Copy link
Member

@derrickmehaffy derrickmehaffy commented Nov 13, 2023

What does it do

fixes #17860
fixes #17700
Relates to #12538

Why is it needed

Creating a new locale doesn't pre-fill the non-internationalized fields. But when you refresh the page, they get filled.

How to test it

Recreation:

  1. Add a second locale in settings -> internationalization
  2. Create a collection type with the content-type builder with Internationalization enabled. (You can enable it in advanced settings)
  3. Add some fields
  4. Add some non-internationalized filled by unchecking the Enable localization for this field option in advanced settings
  5. Click Create a new entry on the collection-type just created.
  6. Fill in the fields and click save
  7. Click the locale dropdown and select another locale that doesn't have an entry yet.
  8. See error. All fields are empty, including the non-internationalized fields.
    a. See another error. Notice that if you open the locale dropdown again, the previous locale is marked as if it didn't have an entry, although it does.
    b. If you click the previous locale, it takes you to an empty new page with the locale that should be filled but it's not.
    c. If you reload, fields are also filled now. But the page is still Create an entry instead of the standard editing view.
  9. Reload the page
  10. The non-internationalized fields are magically filled now. And the locale dropdown displays the correct state for the translations.

https://www.loom.com/share/29deb7eca88840f6b829894313f18b18?t=12

Fix:

  1. Checkout to this branch fix/prefill-non-internationalized-fields and run the project
  2. try again by adding a new field or editing previous fields, you will see that the issue is resolved.

https://www.loom.com/share/737dede941974c209c4f8679ecc7ae1a?sid=21f65051-246d-4cd9-9af0-f5d03be6626d

How the fix works:

The error is happening in the try block of middlewares/addCommonFieldsToInitialDataMiddleware.js as the value for currentLayout is null, If we add await in getData() then it will be fixed.


This PR is a mirror of: #18386

Issues

  • resolves DX-1287

@derrickmehaffy derrickmehaffy added source: core:content-manager Source is core/content-manager package pr: fix This PR is fixing a bug labels Nov 13, 2023
Copy link
Contributor

github-actions bot commented Nov 13, 2023

Size Change: +1 B (0%)

Total Size: 666 kB

ℹ️ View Unchanged
Filename Size Change
examples/getstarted/build/bb4d0d527bdfb161bc5a.svg 2.33 kB 0 B
examples/getstarted/build/index.html 611 B +1 B (0%)
examples/getstarted/build/main.********.js 659 kB 0 B
examples/getstarted/build/runtime~main.********.js 4.27 kB 0 B

compressed-size-action

@alexandrebodin alexandrebodin added this to the 4.15.5 milestone Nov 13, 2023
@alexandrebodin
Copy link
Member

@derrickmehaffy can you move this to target main so we only address the 4.15.x issues in 4.15.5 1st. I'll clear the milestone for now

@alexandrebodin alexandrebodin removed this from the 4.15.5 milestone Nov 14, 2023
@derrickmehaffy derrickmehaffy changed the base branch from releases/4.15.5 to main November 14, 2023 18:12
@derrickmehaffy
Copy link
Member Author

Ill need to rebase this so don't merge yet

@derrickmehaffy derrickmehaffy added the flag: don't merge This PR should not be merged at the moment label Nov 14, 2023
@derrickmehaffy derrickmehaffy removed the flag: don't merge This PR should not be merged at the moment label Nov 20, 2023
@derrickmehaffy
Copy link
Member Author

Ready for review again, sorry for the delay

@witalijx
Copy link

witalijx commented Dec 6, 2023

When will this be merged, still waiting ^^

@joshuaellis joshuaellis changed the base branch from main to develop December 6, 2023 13:59
@joshuaellis
Copy link
Member

Now we have TS it's easier to understand this fix and I think i need more time to dig deep. Because dispatch calls aren't typed to return Promises which probably means something else is going on we perhaps should not be doing and i'd rather solve this properly.

@derrickmehaffy
Copy link
Member Author

Building an experimental to test with: https://github.com/strapi/strapi/actions/runs/7119256371

@pmknk

This comment has been minimized.

@gu-stav gu-stav removed their request for review December 13, 2023 15:26
@pmknk

This comment has been minimized.

@TithSambath

This comment was marked as spam.

@derrickmehaffy
Copy link
Member Author

@joshuaellis I understand the want/need to fix this properly but as it's impacting users and we know that so far this is working can we merge it and look at this later to figure out a more proper way to address it and in the meantime get it working for users?

If we need to validate if this will cause other problems I can generate an experimental and let users test with it for a bit

@joshuaellis
Copy link
Member

Yeah sorry, can you sort the conflicts out? The file is now in TS iirc, then we can do an experimental, I can write up a JIRA ticket and add a TODO and get the ball rolling 🎳

@derrickmehaffy
Copy link
Member Author

@joshuaellis I rebased this on main instead since we aren't using develop anymore (AFAIK) unless something is changing in 4.17 in which case I'll wait til after that release and rebase it on that instead.

@derrickmehaffy
Copy link
Member Author

New experimental being generated based on 4.16.2: https://github.com/strapi/strapi/actions/runs/7402447192

@joshuaellis
Copy link
Member

@joshuaellis I rebased this on main instead since we aren't using develop anymore (AFAIK) unless something is changing in 4.17 in which case I'll wait til after that release and rebase it on that instead.

I'm worried this will get lost in the re-merger because of the different file extensions. Lets wait to do 4.17.0 release & then merge 👍🏼

@derrickmehaffy
Copy link
Member Author

@joshuaellis I rebased this on main instead since we aren't using develop anymore (AFAIK) unless something is changing in 4.17 in which case I'll wait til after that release and rebase it on that instead.

I'm worried this will get lost in the re-merger because of the different file extensions. Lets wait to do 4.17.0 release & then merge 👍🏼

Sounds good!

@CyberSteelX
Copy link

Hi, sorry for bothering. May I ask when this issue will be fixed, because I'm not seeing any 'Milestone' for it (and this other issue that I was keeping track of for a similar problem has been closed 😅 #17837)? Thanks a lot.

@joshuaellis
Copy link
Member

@derrickmehaffy when you get a chance can you double check the fix please? Then I can approve and we can merge 😄

thank you for your patience.

@derrickmehaffy
Copy link
Member Author

@derrickmehaffy when you get a chance can you double check the fix please? Then I can approve and we can merge 😄

thank you for your patience.

Yup I'll do it this afternoon. Rebase and rebuild the experimental and confirm it fixes the problem.

@dabal

This comment was marked as spam.

Copy link

vercel bot commented Mar 26, 2024

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

Name Status Preview Comments Updated (UTC)
contributor-docs ❌ Failed (Inspect) Mar 26, 2024 5:32pm

@joshuaellis joshuaellis merged commit 2e80eaf into develop Mar 27, 2024
30 of 31 checks passed
@joshuaellis joshuaellis deleted the fix/17860 branch March 27, 2024 11:54
@SalahAdDin
Copy link

@joshuaellis Will this be for the new version?

@joshuaellis
Copy link
Member

Check the milestone.

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:content-manager Source is core/content-manager package
Projects
Status: Fixed/Shipped
9 participants