Fix GetAsync not work when option is AllVersion #11433

hyzx86 · 2022-03-25T15:14:51Z

jtkech · 2022-03-25T22:07:24Z

src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs

-                        x.ContentItemId == contentItemId &&
-                        x.Published == false &&
-                        x.Latest == true)
+                        x.ContentItemId == contentItemId)


Hmm, I think better to not take into account soft deleted items having both Published = Latest = false.

x.ContentItemId == contentItemId && (x.Latest || x.Published)

Not really, if you select AllVersion but it is still restricted. It's confusing

Hmm, I think better to not take into account soft deleted items having both Published = Latest = false.

@jtkech Yes, you are right, if you want to get different versions, you need to specify the version number.

We need to have a DeletedVersion option then too I think.

x.ContentItemId == contentItemId && (!x.Latest && !x.Published)

But maybe not necessary for now.

But the current content state is a little bit too complicated. For example, as a new person, you need to look up code or documentation to fully understand what the state means

sebastienros · 2022-03-29T19:19:28Z

Can you describe what "doesn't work" so we can recommend the best approach? Maybe open an issue?

Skrypt · 2022-03-29T19:31:43Z

Retrieving AllVersions all at once with the ContentManager.

jtkech · 2022-03-29T21:28:02Z

Or as I understood, return any active version, maybe in priority the published version if it exists, or a draft version if it exists, that' why here the suggested code uses contentItemId && (x.Latest || x.Published).

But currently it then uses FirstOrDefaultAsync(), so it may return a draft whereas a published version exists.

Maybe it should use ListAsync() allowing to return a published version in priority.

hyzx86 · 2022-03-30T04:34:31Z

Yes, it is the same problem as this one,
#11447

Because my program uses SPA mode, much of the data manipulation is done through the Api
Initially I thought the draft state would be saved as the new ContentItemId,
It turns out there may be something wrong with the data I'm transferring, so the new ContentItemId is generated because the POST API/content doesn't seem to have any validation.
It might need to exclude two status fields (latest, published)(I can't remember),
For simplicity, I've implemented an API similar to the Graphql mutation myself
It can directly convert Graphql query results to Contentitem and then update the content

https://github.com/EasyOC/EasyOC/blob/eb1dc57f37491296823b633c30dc30806613e573/src/Modules/EasyOC.OrchardCore.ContentExtentions/AppServices/ContentManagementAppService.cs#L67

hyzx86 · 2022-03-30T04:48:40Z

Can you describe what "doesn't work" so we can recommend the best approach? Maybe open an issue?

mean is this filter option is never used in this method, But now it seems that just changing this doesn't make much sense, because if no filtering conditions are attached, a ContentItem will be obtained randomly (or with a minimum ID), so at least one ContentVersionId needs to be specified here

hyzx86 · 2022-04-01T04:59:04Z

Since this option doesn't serve any purpose, can we remove it?
It only has one reference in the Graphql status filter
This option does not return any data

OrchardCore/src/OrchardCore/OrchardCore.ContentManagement.Abstractions/IContentManager.cs

Line 265 in 5d3cdca

    
           public static VersionOptions AllVersions { get { return new VersionOptions { IsAllVersions = true }; } }

jtkech · 2022-04-01T22:46:11Z

Maybe we could have used AllVersions for the GetAsync() that returns a collection, currently we use a bool latest = false parameter, false meaning return all published, true all latest regardless there are published or not. So yes here we could have used AlVersions but we would need a new method to not break the existing one.

For the GetAsync that returns only one item, hmm maybe we could have used an AnyVersion with the behavior I described here #11433 (comment), but was just an idea, maybe too confusing ;)

So yes, if we keep the existing code I agree with you, maybe better to just get rid of the AllVersions option, or as said above and if we think it is worth, maybe use it for a GetAsync that returns a collection of items.

Note: Here we are talking about active versions (Published or Latest), Published and Latest are not versions in themselves but properties of a given version (related to a given contentItemVersionId), a content item having some versions but no active versions is a soft removed item.

hyzx86 · 2022-09-08T14:36:15Z

#11447

hyzx86 · 2022-09-08T15:04:05Z

@jtkech
This is a very embarrassing question for me, if I want to rewrite GetAsync(id,option) , I need call BuildNewVersionAsync
But since it is not public, I have to copy this method to my ContentManager

OrchardCore/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs

Line 239 in 4093d2e

contentItem = await BuildNewVersionAsync(contentItem);

But even if I copied it, I found ContentElement.Data is not open either

OrchardCore/src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs

Line 416 in 4093d2e

buildingContentItem.Data = new JObject(existingContentItem.Data);

Finally, I wrote l index query directly by referring to the code in AdminController
If so, what is the meaning of ContentManager

OrchardCore/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs

Line 310 in 4093d2e

    
           var checkedContentItems = await _session.Query<ContentItem, ContentItemIndex>().Where(x => x.DocumentId.IsIn(itemIds) && x.Latest).ListAsync(_contentManager);

jtkech · 2022-09-08T16:01:54Z

Finally, I wrote l index query directly by referring to the code in AdminController
If so, what is the meaning of ContentManager

It allows to call the contentManager LoadAsync() on each contentItem which call handlers to activate correctly items if not yet done by looking at the contentManagerSession cache.

hyzx86 · 2022-09-08T16:11:22Z

Yes, I see. I mean, the current implementation of content manager is not complete enough

jtkech · 2022-09-08T16:31:29Z

As additional info, as it is done when using content manager GetAsync(), activating item meaning activating item parts/fields

github-actions · 2024-01-29T13:27:39Z

This pull request has merge conflicts. Please resolve those before requesting a review.

Piedone · 2024-03-13T17:39:55Z

src/OrchardCore/OrchardCore.ContentManagement.Abstractions/IContentManager.cs

@@ -262,12 +262,12 @@ public class VersionOptions
        /// <summary>
        /// Gets all versions.
        /// </summary>
-        public static VersionOptions AllVersions { get { return new VersionOptions { IsAllVersions = true }; } }
+        public static VersionOptions AnyVersion { get { return new VersionOptions { IsAnyVersions = true }; } }


Keep the property but with [Obsolete], telling people to use this, so they'll know what's up instead of wondering after a build error.

More importantly though, I think the intent here is indeed to return all the versions of a content item. So, while this might not make sense for GetAsync(IEnumerable<string> contentItemIds, VersionOptions options), it does for GetAsync(string id, VersionOptions options). So, I don't think this should be changed.

Piedone · 2024-03-13T18:52:56Z

src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs

Please provide unit tests.

Piedone · 2024-05-16T17:42:15Z

We should rather remove VersionOptions.AllVersions, see comments from #16027 (comment)

Fix GetAsync notwork when option is AllVersion

3a710dc

jtkech reviewed Mar 25, 2022

View reviewed changes

add && (x.Latest || x.Published) limit

fdd602c

hyzx86 closed this Mar 30, 2022

hyzx86 reopened this Apr 1, 2022

Merge branch 'origin_main' into fixGetContentItemAllVersion

97d3a9e

hyzx86 added 2 commits September 8, 2022 23:06

update AllVersion to AnyVersion

96cea90

fix build error

11653c2

github-actions bot added the merge conflict label Jan 29, 2024

Piedone requested changes Mar 13, 2024

View reviewed changes

Merge branch 'main' into fixGetContentItemAllVersion

e5a1402

github-actions bot removed the merge conflict label Mar 13, 2024

Merge branch 'main' into fixGetContentItemAllVersion

1656945

Piedone mentioned this pull request May 12, 2024

Remove VersionOptions.AllVersions #16027

Closed

Piedone closed this May 16, 2024

hyzx86 deleted the fixGetContentItemAllVersion branch June 13, 2024 06:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GetAsync not work when option is AllVersion #11433

Fix GetAsync not work when option is AllVersion #11433

hyzx86 commented Mar 25, 2022 •

edited by Piedone

Loading

jtkech Mar 25, 2022

hyzx86 Mar 26, 2022

hyzx86 Mar 26, 2022

Skrypt Mar 29, 2022 •

edited

Loading

hyzx86 Mar 29, 2022

sebastienros commented Mar 29, 2022

Skrypt commented Mar 29, 2022

jtkech commented Mar 29, 2022

hyzx86 commented Mar 30, 2022 •

edited

Loading

hyzx86 commented Mar 30, 2022

hyzx86 commented Apr 1, 2022 •

edited

Loading

jtkech commented Apr 1, 2022

hyzx86 commented Sep 8, 2022

hyzx86 commented Sep 8, 2022 •

edited

Loading

jtkech commented Sep 8, 2022

hyzx86 commented Sep 8, 2022

jtkech commented Sep 8, 2022

github-actions bot commented Jan 29, 2024

Piedone Mar 13, 2024

Piedone Mar 13, 2024

Piedone commented May 16, 2024 •

edited

Loading

Fix GetAsync not work when option is AllVersion #11433

Fix GetAsync not work when option is AllVersion #11433

Conversation

hyzx86 commented Mar 25, 2022 • edited by Piedone Loading

jtkech Mar 25, 2022

Choose a reason for hiding this comment

hyzx86 Mar 26, 2022

Choose a reason for hiding this comment

hyzx86 Mar 26, 2022

Choose a reason for hiding this comment

Skrypt Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

hyzx86 Mar 29, 2022

Choose a reason for hiding this comment

sebastienros commented Mar 29, 2022

Skrypt commented Mar 29, 2022

jtkech commented Mar 29, 2022

hyzx86 commented Mar 30, 2022 • edited Loading

hyzx86 commented Mar 30, 2022

hyzx86 commented Apr 1, 2022 • edited Loading

jtkech commented Apr 1, 2022

hyzx86 commented Sep 8, 2022

hyzx86 commented Sep 8, 2022 • edited Loading

jtkech commented Sep 8, 2022

hyzx86 commented Sep 8, 2022

jtkech commented Sep 8, 2022

github-actions bot commented Jan 29, 2024

Piedone Mar 13, 2024

Choose a reason for hiding this comment

Piedone Mar 13, 2024

Choose a reason for hiding this comment

Piedone commented May 16, 2024 • edited Loading

hyzx86 commented Mar 25, 2022 •

edited by Piedone

Loading

Skrypt Mar 29, 2022 •

edited

Loading

hyzx86 commented Mar 30, 2022 •

edited

Loading

hyzx86 commented Apr 1, 2022 •

edited

Loading

hyzx86 commented Sep 8, 2022 •

edited

Loading

Piedone commented May 16, 2024 •

edited

Loading