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

Make updateContentTypeConfiguration endpoint return full data #13676

Merged
merged 1 commit into from Jul 12, 2022

Conversation

benderillo
Copy link
Contributor

@benderillo benderillo commented Jul 2, 2022

This PR fixes the issue #13675

The Content-type controller returns the same response object as the "findConfiguration" now.

What does it do?

This PR changes the response of the PUT /content-types/:uid/configuration to be the same as GET /content-types/:uid/configuration.

In general it is a good practice for the REST APIs to have the same response object for GET and PUT for the same route with the PUT returning modified resource.

Why is it needed?

As described in the linked issue, attempting to change view configuration for a single-type entry will fail if the entry contains components.
What happens is that the PUT /content-types/:uid/configuration responds with only half of the data that the "Admin" UI package needs for correct behaviour. And the updateLayout method simply hardcodes the missing piece with an empty object components: {}.
The problem happens when later in the execution flow tree there is an attempt to dereference a null pointer:

if (type === 'component') {
const component = components[fieldSchema.component];
const mainFieldName = component.settings.mainField;
const mainFieldAttribute = component.attributes[mainFieldName];

Here the components is an empty object, hence the component is undefined. Which means component.settings expression is doomed to fail.

How to test it?

  1. Create a fresh new project with Strapi.
  2. Create a new component.
  3. Create a single-type model
  4. Go to Content Manager, and click the "Configure the view" button.
  5. Shuffle the fields around and click save button
  6. Confirm the action in the dialog

Related issue(s)/PR(s)

fixes #13675

Note: No tests have been added for this code change. I don't have capacity to look into and update/add tests, sorry.

@benderillo benderillo changed the title Make updateContentTypeConfiguration endpoint return full data (conten… Make updateContentTypeConfiguration endpoint return full data Jul 3, 2022
@alexandrebodin alexandrebodin added source: core:content-type-builder Source is core/content-type-builder package pr: fix This PR is fixing a bug labels Jul 4, 2022
@alexandrebodin alexandrebodin self-assigned this Jul 4, 2022
@alexandrebodin alexandrebodin self-requested a review July 4, 2022 09:47
@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

Merging #13676 (89a8f91) into master (8b719b1) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master   #13676      +/-   ##
==========================================
- Coverage   54.19%   54.18%   -0.01%     
==========================================
  Files        1197     1197              
  Lines       30588    30590       +2     
  Branches     5562     5562              
==========================================
  Hits        16576    16576              
- Misses      12191    12193       +2     
  Partials     1821     1821              
Flag Coverage Δ
front 56.51% <0.00%> (ø)
unit 48.55% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...t-manager/hooks/useFetchContentTypeLayout/index.js 79.06% <0.00%> (ø)
...ontent-manager/server/controllers/content-types.js 29.62% <0.00%> (-1.14%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b719b1...89a8f91. Read the comment docs.

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.

That looks fairly straight forward. Just an open question as maybe we can avoid refecthing everything

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.

LGTM

@alexandrebodin alexandrebodin added this to the 4.2.3 milestone Jul 12, 2022
@alexandrebodin alexandrebodin added source: core:content-manager Source is core/content-manager package and removed source: core:content-type-builder Source is core/content-type-builder package labels Jul 12, 2022
@alexandrebodin
Copy link
Member

Thanks for the fix 👍

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
None yet
Development

Successfully merging this pull request may close these issues.

Updating view configuration for a Single-type fails if it contains components
2 participants