Fix Publish Content Task #16962
Fix Publish Content Task #16962
Conversation
When a content item is created, it is the latest version of the content item by default.
The condition for the Noop behavior is related to the properties of the content item, not the workflow. The workflow should publish content when the workflow is scheduled, restarted or triggered by a user event.
|
Thank you for submitting your first pull request, awesome! 🚀 If you haven't already, please take a moment to review our contribution guide. This guide provides helpful information to ensure your contribution aligns with our standards. A core team member will review your pull request. |
|
Results of manual testing are documented here - #16939 (comment) I will work on submitting a functional test as I have time. |
|
@AmyBlaine please accept the Contributor License Agreement |
I'm waiting on permission from my employer |
|
@AmyBlaine i hope this will be ready soon as we are getting ready to release 2.1. This is a personal contribution and not company, maybe you don't need to use your company's name here and just use your own name instead. |
|
If that's blocking then we should create a different PR. There is not much IP in here, including the fact we came up with the solution during a joint meeting. |
|
@coderabbitai review |
📝 WalkthroughWalkthroughThe changes in this pull request primarily involve modifications to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/OrchardCore.Modules/OrchardCore.Contents/Workflows/Activities/PublishContentTask.cs (1)
Line range hint
41-49: Consider enhancing error messages with more context.The error message could be more helpful for debugging by including the content item ID when the retrieval fails.
- throw new InvalidOperationException($"The '{nameof(PublishContentTask)}' failed to retrieve the content item."); + throw new InvalidOperationException($"The '{nameof(PublishContentTask)}' failed to retrieve content item with ID '{content.ContentItem.ContentItemId}'.");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/OrchardCore.Modules/OrchardCore.Contents/Workflows/Activities/PublishContentTask.cs(1 hunks)src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs(1 hunks)
🔇 Additional comments (2)
src/OrchardCore.Modules/OrchardCore.Contents/Workflows/Activities/PublishContentTask.cs (1)
36-39: LGTM! The HasDraft() check fixes the event-triggered publishing issue.
The change correctly determines when to publish content by checking for draft status instead of comparing content IDs. This aligns with the workflow requirements and fixes the bug reported in #16939.
Let's verify the HasDraft() implementation:
✅ Verification successful
The verification is complete. I can now provide the final response based on the shell script results:
The HasDraft() implementation is correct and consistently used across the codebase
The HasDraft() extension method is properly implemented in ContentExtensions.cs and returns true when either:
- The content item is not published, or
- The content item is not the latest version
This implementation aligns perfectly with the workflow task's requirements, as it correctly identifies when content needs to be published. The method is also consistently used across the codebase in similar scenarios (preview buttons, content actions, etc.).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the HasDraft() implementation and its usage
# Expected: HasDraft() should be an extension method on IContent that checks draft status
# Find the HasDraft implementation
ast-grep --pattern 'public static bool HasDraft($content) {
$$$
}'
# Find other usages of HasDraft for consistency
rg 'HasDraft\(' -A 3
Length of output: 6489
src/OrchardCore/OrchardCore.ContentManagement/DefaultContentManager.cs (1)
570-570: LGTM: Setting Latest flag for draft items
This change ensures that draft content items are properly marked as the latest version, which is essential for the content versioning system to work correctly. This aligns with fixing the issue where the Publish Content task was failing to publish content when triggered by an event.
Let's verify the content versioning behavior:
✅ Verification successful
Based on the extensive search results, I can now provide a definitive response about the Latest flag usage in the codebase:
Setting Latest flag for draft items is correct and consistent with codebase patterns
The change to set contentItem.Latest = true in CreateAsync aligns with the established content versioning patterns throughout the codebase:
- The Latest flag is used to track the most recent version of a content item, whether published or draft
- The codebase consistently sets Latest=true when creating new draft versions
- The flag is used extensively in queries and indexes to retrieve the most recent versions
- The DefaultContentManager properly manages the Latest flag lifecycle by:
- Setting Latest=false on previous versions when creating new ones
- Using Latest in conjunction with Published to determine content state
- Maintaining Latest flag during content operations like publish/unpublish
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other places where Latest flag is set/used to ensure consistency
# Look for patterns where Latest flag is set
ast-grep --pattern 'contentItem.Latest = $_'
# Look for conditions checking Latest flag
ast-grep --pattern 'if ($_.Latest)'
Length of output: 84
Script:
#!/bin/bash
# Let's try a broader search with ripgrep to find Latest property usage
rg -l "Latest" --type cs
# Then look for specific patterns in those files
rg "\.Latest\s*=|\.Latest\b" --type cs -A 2 -B 2
Length of output: 149619
I escalated this to my manager because I did this work on company time with company resources in the course of my work for my employer. There are active conversations about the company policy and procedure, but the resolution may not occur within your desired timeframe.
That's fine with me (personally). I will let my team know. |
|
This PR was replaced by #17033 |
Fixes #16939
Updated the condition in the Publish Content Task. The condition for the Noop behavior is related to the properties of the content item, not the workflow. The workflow should publish content when the workflow is scheduled, restarted or triggered by a user event. To publish a content item, it must have a draft
Updated the default properties for a content item when it is created. When a content item is created, it is the latest version of the content item by default.