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: Publishing single-type if config is true, and allow updating the drafts - api #14767

Merged
merged 1 commit into from
Jan 11, 2023

Conversation

iifawzi
Copy link
Contributor

@iifawzi iifawzi commented Nov 2, 2022

Signed-off-by: iifawzi iifawzie@gmail.com

What does it do?

Hi, this PR is addressing #14719, which mainly had two issues:

1- Creating a single type entry using API results in the type not being published (draft):
To handle this, I've added a check as we do in the collection types, that will publish based on the config

        if (hasDraftAndPublish(contentType)) {
          setPublishedAt(data);
        }

2- The second issue is that, if the type is draft, it can't be updated using the API:

This is caused because of the query filters

query.filters = ({ meta }) => {
if (publicationState === 'live' && has(PUBLISHED_AT_ATTRIBUTE, meta.attributes)) {
return { [PUBLISHED_AT_ATTRIBUTE]: { $notNull: true } };
}
};

I've fixed it with a onlyPublished parameter, that will be propagated to the function, to set the $notNull operator to false.
I feel like this's more of a hacky solution than a fix, let me know if you do suggest any better solutions, my only concern was that I didn't want to mess around with the transformParamsToQuery method, since it's used in a lot of places.

Related issue(s)/PR(s)

Fix #14719

@iifawzi iifawzi changed the title Fix: Publishing the Single-type if set to true, and allow updating draft types Fix: Publishing single-type if config is true, and allow updating the draft single types - api Nov 2, 2022
@iifawzi iifawzi changed the title Fix: Publishing single-type if config is true, and allow updating the draft single types - api Fix: Publishing single-type if config is true, and allow updating the drafts - api Nov 2, 2022
@codecov
Copy link

codecov bot commented Nov 2, 2022

Codecov Report

Base: 60.49% // Head: 60.50% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (3ae29ee) compared to base (b767d40).
Patch coverage: 71.42% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14767   +/-   ##
=======================================
  Coverage   60.49%   60.50%           
=======================================
  Files        1352     1352           
  Lines       33174    33180    +6     
  Branches     6337     6338    +1     
=======================================
+ Hits        20070    20074    +4     
- Misses      11271    11273    +2     
  Partials     1833     1833           
Flag Coverage Δ
back 50.36% <71.42%> (+<0.01%) ⬆️
front 65.05% <ø> (ø)
unit_back 50.36% <71.42%> (+<0.01%) ⬆️
unit_front 65.05% <ø> (ø)

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

Impacted Files Coverage Δ
...es/core/strapi/lib/core-api/service/single-type.js 88.00% <71.42%> (-6.74%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Convly Convly added source: core:strapi Source is core/strapi package pr: fix This PR is fixing a bug labels Nov 7, 2022
Copy link
Contributor

@petersg83 petersg83 left a comment

Choose a reason for hiding this comment

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

Hello, thank you for the PR! 😃
I think it is perfect for the first fix.
For the second fix, can you try to write:

const entity = await this.find({ ...params, publicationState: 'preview' });

line 40 of packages/core/strapi/lib/core-api/service/single-type.js?
I think it would fix the issue and would be cleaner :)

If you don't have time to handle this, just tell me and I'll take over, sorry for the delay :)
Also It would be nice to add a test to ensure both issues won't happen again (someting inside or similar to what is done in strapi/packages/core/strapi/tests/api/basic-dp.test.e2e.js)

@iifawzi
Copy link
Contributor Author

iifawzi commented Dec 20, 2022

Thank you for your time reviewing this @petersg83, I will make sure to work on the points you mentioned by the next week

@@ -65,6 +66,8 @@ describe('Content Manager single types', () => {
},
});

expect(res.body.data.attributes.publishedAt).toBeISODate();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I've pushed the changes and updated the tests.

The first issue, it's now covered by this line.

For the second issue, no need for additional tests, because the Update keeps the same data id is not failing after having draftAndPublish set to true ( Before the changes introduced in this pr, it would fail if draftAndPublish is true ).

@iifawzi
Copy link
Contributor Author

iifawzi commented Dec 24, 2022

Would appreciate if you can take another look @petersg83, and let me know if you think anything can be improved :)

Copy link
Contributor

@jhoward1994 jhoward1994 left a comment

Choose a reason for hiding this comment

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

Hi @iifawzi , apologies for the delay. Thank you for these changes the code looks great and is working well 👍🏻

Copy link
Contributor

@petersg83 petersg83 left a comment

Choose a reason for hiding this comment

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

Thank you @iifawzi! It's perfect :) Sorry for the delay, I was in holidays for 2 weeks :)
It should be in the next release!

@petersg83 petersg83 added this to the 4.5.6 milestone Jan 11, 2023
@petersg83 petersg83 merged commit 576ef90 into strapi:main Jan 11, 2023
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:strapi Source is core/strapi package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REST API Single Types do not publish
4 participants