Add type checking when importing types from recipes #15979 #15980

hyzx86 · 2024-05-05T12:36:18Z

Summary by CodeRabbit

New Features
- Enhanced content type management by ensuring part names are not empty before adding, improving data integrity.
Bug Fixes
- Added a new test to block incorrect type definition imports during recipe execution, enhancing system reliability.
Tests
- Introduced a new JSON recipe file for testing type-checking functionality.
Documentation
- No visible changes to end-users.

hyzx86 · 2024-05-05T12:38:08Z

coderabbitai · 2024-05-05T12:39:56Z

The ContentDefinitionStep.cs file implements the ContentDefinitionStep class, which is part of the recipe steps in OrchardCore. This class is responsible for creating and updating content definitions based on the data provided in recipes. Here are some key observations and suggestions:

Error Handling: The class properly checks for null values and throws exceptions when necessary, such as when a part name is missing. This is crucial for preventing runtime errors and ensuring data integrity.
Type Checking: The methods UpdateContentTypeAsync and UpdateContentPartAsync use the builder pattern effectively to update content definitions. The type checking appears to be handled implicitly by the method signatures and the use of specific types like ContentTypeDefinition and ContentPartDefinition.
Code Quality: The code is well-structured and follows C# best practices, which enhances maintainability and readability. The use of async and await ensures that the operations are performed efficiently.
Documentation: The class and methods are well-documented with comments, which is helpful for understanding the purpose and functionality of the code.

Next, I will review the associated test files to ensure they cover the new functionalities adequately.

Analysis chain

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Fetch the list of files changed in the PR
gh pr view 15980 --json files --jq '.files[].path'

Length of output: 296

Script:

#!/bin/bash
# Display the contents of ContentDefinitionStep.cs for review
cat src/OrchardCore.Modules/OrchardCore.ContentTypes/RecipeSteps/ContentDefinitionStep.cs

Length of output: 3741

Actions Performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai · 2024-05-05T12:39:59Z

Walkthrough

The changes involve enhancing the robustness of content type imports in OrchardCore by introducing checks against incorrect or empty type definitions. This includes modifications to the ContentDefinitionStep.cs to prevent empty PartName entries and the addition of a new test in RecipeExecutorTests.cs to ensure that incorrect type definitions are blocked during recipe execution.

Changes

File Path	Change Summary
`src/OrchardCore.Modules/OrchardCore.ContentTypes/RecipeSteps/ContentDefinitionStep.cs`	Added check for empty `PartName` and exception handling.
`test/OrchardCore.Tests/OrchardCore.Tests.csproj`	Embedded resource addition: `recipe6.json`.
`test/OrchardCore.Tests/Recipes/RecipeExecutorTests.cs`	New test method to block incorrect type definition imports.
`test/OrchardCore.Tests/Recipes/RecipeFiles/recipe6.json`	New JSON file for testing type-checking functionality.

Assessment against linked issues

Objective	Addressed	Explanation
Add necessary checks for ContentType import (#15979)	✅
Ensure incorrect type definitions are blocked during recipe execution (#15979)	✅

The changes address the core objectives of the linked issue by implementing necessary checks and validations to prevent the import of incorrect or incomplete content types, aligning with the requirements to enhance data integrity during imports.

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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 as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 1

src/OrchardCore.Modules/OrchardCore.ContentTypes/RecipeSteps/ContentDefinitionStep.cs

Piedone

When addressing review feedback, please adhere to the following:

Please update your pull request according to feedback until it is approved by one of the core team members.
Apply suggested changes directly so the reviewer doesn't have to eyeball the changes. These resolve themselves after applying them, and that's fine.
Don't resolve other conversations so it's easier to track for the reviewer. Then, the reviewer will resolve them.
Feel free to mark conversations that you addressed to keep track of them with an emoji or otherwise, just don't resolve them.
Please keep conversations happening in line comments in those convos, otherwise, communication will be a mess. If you have trouble finding them, see this video.
When you're done addressing all feedback of a review, click "Re-request review" in the top-right corner for each reviewer when you're ready for another round of review, so they know that you're done.

src/OrchardCore.Modules/OrchardCore.ContentTypes/RecipeSteps/ContentDefinitionStep.cs

Piedone · 2024-05-05T22:21:26Z

test/OrchardCore.Tests/Recipes/RecipeExecutorTests.cs

+                catch (Exception ex)
+                {
+                    Assert.Equal("Failed to add part to ContentType 'Message', The PartName property is not allowed to be empty", ex.Message);
+                }


Use Assert.ThrowsAsync() instead.

Thanks, always learning something new from you!

I'm glad :).

test/OrchardCore.Tests/Recipes/RecipeFiles/recipe6.json

Piedone · 2024-05-05T22:23:27Z

test/OrchardCore.Tests/Recipes/RecipeFiles/recipe6.json

+          "Name": "Message",
+          "DisplayName": "Message",
+          "Settings": {
+            "ContentTypeSettings": {
+              "Creatable": true,
+              "Draftable": true,
+              "Versionable": true,
+              "Listable": true,
+              "Securable": true
+            }
+          },
+          "ContentTypePartDefinitionRecords": [
+            {
+              "PartName": "Message",
+              "Name": "Message",
+              "Settings": {
+                "ContentTypePartSettings": {
+                  "Position": "5"
+                }
+              }
+            },


Are these really needed?

I'll try it, I'm not sure, but I think this recipe is closer to the real scenario.

…ontentDefinitionStep.cs Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>

Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>

hyzx86 · 2024-05-06T03:32:16Z

@coderabbitai review

coderabbitai · 2024-05-06T03:32:22Z

Actions Performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

src/OrchardCore.Modules/OrchardCore.ContentTypes/RecipeSteps/ContentDefinitionStep.cs

test/OrchardCore.Tests/Recipes/RecipeExecutorTests.cs

test/OrchardCore.Tests/Recipes/RecipeFiles/recipe6.json

test/OrchardCore.Tests/Recipes/RecipeExecutorTests.cs

…ontentDefinitionStep.cs Co-authored-by: Mike Alhayek <mike@crestapps.com>

Co-authored-by: Mike Alhayek <mike@crestapps.com>

src/OrchardCore.Modules/OrchardCore.ContentTypes/RecipeSteps/ContentDefinitionStep.cs

hyzx86 · 2024-05-07T04:51:49Z

src/OrchardCore.Modules/OrchardCore.Deployment/Controllers/ImportController.cs

@@ -91,6 +92,10 @@ public async Task<IActionResult> Import(IFormFile importedPackage)

                    await _notifier.SuccessAsync(H["Deployment package imported."]);
                }
+                catch (Exception ex)
+                {
+                    await _notifier.ErrorAsync(H[ex.Message]);


Hi @MikeAlhayek ，Can you help me see what's wrong with my localization? Thank you!

you can't do that. the use of H and S must be a literal string since this has to be extracted.

But the notifier needs a HtmlLocalizedString ... What should I do?

Maybe I can send the notify in Step logic?

Added a layer of exception catching message alerts, as there is currently no exception handling when a recipe is imported.

you can't do that. the use of H and S must be a literal string since this has to be extracted.

Since the string is already processed internally, no additional processing should be required

I don't think you need to write the exception to the user using notifier.

I don't think you need to write the exception to the user using notifier.

I'm not sure I agree with this. Assuming that the person who has access to import recipes has full administrative access to the system, (I can't actually guess what other person roles have access to import recipes)
it makes sense to throw some underlying exceptions directly to the user, as most Windows applications do, so that the user can view the exceptions directly through the page rather than having to log in to the server to view the logs.
(I've developed a special feature to view log files online for this purpose~)

Currently our recipe import page does nothing, which would make the user feel that the system is very unreliable After all, we can't anticipate all exceptions

#15978

You can submit another PR to address another issue. I suggest keeping this PR simple and doing that type check without worrying about exception handling.

Okay, got it. Thanks.

Piedone · 2024-05-07T20:22:12Z

Guys, the level of activity under this PR is disproportionally big compared to the amount of proposed changes :). I'll check out from here, and if you think my input is also required, please just @ me.

MikeAlhayek · 2024-05-07T21:05:29Z

Guys, the level of activity under this PR is disproportionally big compared to the amount of proposed changes :). I'll check out from here, and if you think my input is also required, please just @ me.

Agreed. Same here. After I approved it there way many changes.

Many changes after the original approval

hishamco · 2024-05-07T21:21:57Z

@hyzx86 please revert the latest changes and keep the check that we agreed on, it seems everyone here are check out :)

… into recipetypecheck

hyzx86 · 2024-05-08T05:23:24Z

The changes have been rolled back, and now the import recipe looks like this after an error occurs

MikeAlhayek · 2024-05-08T05:26:03Z

src/OrchardCore.Modules/OrchardCore.ContentTypes/RecipeSteps/ContentDefinitionStep.cs


-        public ContentDefinitionStep(IContentDefinitionManager contentDefinitionManager)
+        public ContentDefinitionStep(IContentDefinitionManager contentDefinitionManager, IStringLocalizer<ContentDefinitionStep> s)


Is this still needed? If so, change s to stringLozalizer

src/OrchardCore.Modules/OrchardCore.ContentTypes/RecipeSteps/ContentDefinitionStep.cs

hyzx86 added 2 commits May 5, 2024 20:28

add typepart check OrchardCMS#15979

19889be

Update ContentDefinitionService.cs

34d55c0

Merge branch 'main' into recipetypecheck

bd2ff8b

coderabbitai bot reviewed May 5, 2024

View reviewed changes

src/OrchardCore.Modules/OrchardCore.ContentTypes/RecipeSteps/ContentDefinitionStep.cs Outdated Show resolved Hide resolved

Update ContentDefinitionStep.cs

0f14c07

Piedone requested changes May 5, 2024

View reviewed changes

hyzx86 and others added 5 commits May 6, 2024 11:06

Update src/OrchardCore.Modules/OrchardCore.ContentTypes/RecipeSteps/C…

5018c0f

…ontentDefinitionStep.cs Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>

Update test/OrchardCore.Tests/Recipes/RecipeFiles/recipe6.json

34063ce

Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>

Cleaning up recipes and removing unwanted elements

9f8809c

use Assert.ThrowsAsync

a3b7a3c

Merge branch 'main' into recipetypecheck

0c2c3f3

hyzx86 requested a review from Piedone May 6, 2024 07:24

MikeAlhayek requested changes May 6, 2024

View reviewed changes

hyzx86 and others added 7 commits May 6, 2024 22:55

Update src/OrchardCore.Modules/OrchardCore.ContentTypes/RecipeSteps/C…

3bc6cf2

…ontentDefinitionStep.cs Co-authored-by: Mike Alhayek <mike@crestapps.com>

Update test/OrchardCore.Tests/Recipes/RecipeExecutorTests.cs

d7d3851

Co-authored-by: Mike Alhayek <mike@crestapps.com>

Update test/OrchardCore.Tests/Recipes/RecipeFiles/recipe6.json

fcf0ee7

Co-authored-by: Mike Alhayek <mike@crestapps.com>

Update test/OrchardCore.Tests/Recipes/RecipeFiles/recipe6.json

6483f83

Co-authored-by: Mike Alhayek <mike@crestapps.com>

Update test/OrchardCore.Tests/Recipes/RecipeExecutorTests.cs

23aafad

Co-authored-by: Mike Alhayek <mike@crestapps.com>

Fix test

f252446

Merge branch 'main' into recipetypecheck

683b05a

MikeAlhayek previously approved these changes May 6, 2024

View reviewed changes

hishamco reviewed May 6, 2024

View reviewed changes

src/OrchardCore.Modules/OrchardCore.ContentTypes/RecipeSteps/ContentDefinitionStep.cs Show resolved Hide resolved

add notify

f7cbf66

hyzx86 commented May 7, 2024

View reviewed changes

Merge branch 'main' into recipetypecheck

68d9b66

hyzx86 added 2 commits May 8, 2024 01:39

update notify

05387fd

ad log

e2e7a9e

hyzx86 added 3 commits May 8, 2024 11:28

Merge branch 'main' into recipetypecheck

174ec9c

restore Changes

bdd313d

Merge branch 'recipetypecheck' of https://github.com/hyzx86/OrchardCore…

75b42b6

… into recipetypecheck

hyzx86 requested review from MikeAlhayek and hishamco May 8, 2024 05:24

MikeAlhayek requested changes May 8, 2024

View reviewed changes

remove localize

13c32f1

hishamco reviewed May 8, 2024

View reviewed changes

src/OrchardCore.Modules/OrchardCore.ContentTypes/RecipeSteps/ContentDefinitionStep.cs Show resolved Hide resolved

MikeAlhayek approved these changes May 8, 2024

View reviewed changes

MikeAlhayek merged commit 548eaa1 into OrchardCMS:main May 9, 2024
30 checks passed

This was referenced Jun 1, 2024

Monthly community metrics report for 2024-05-01..2024-05-31 #16210

Closed

Monthly community metrics report for 2024-05-01..2024-05-31 Emmanjr/OrchardCore#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type checking when importing types from recipes #15979 #15980

Add type checking when importing types from recipes #15979 #15980

hyzx86 commented May 5, 2024 •

edited by coderabbitai bot

Loading

hyzx86 commented May 5, 2024

coderabbitai bot commented May 5, 2024

coderabbitai bot commented May 5, 2024 •

edited

Loading

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

coderabbitai bot left a comment

Piedone left a comment

Piedone May 5, 2024

hyzx86 May 6, 2024

hyzx86 May 6, 2024

Piedone May 6, 2024

Piedone May 5, 2024

hyzx86 May 6, 2024

hyzx86 May 6, 2024

hyzx86 commented May 6, 2024

coderabbitai bot commented May 6, 2024

hyzx86 May 7, 2024

MikeAlhayek May 7, 2024

hyzx86 May 7, 2024

hyzx86 May 7, 2024

This comment was marked as outdated.

hyzx86 May 7, 2024

MikeAlhayek May 7, 2024

hyzx86 May 8, 2024

MikeAlhayek May 8, 2024

hyzx86 May 8, 2024

Piedone commented May 7, 2024

MikeAlhayek commented May 7, 2024

hishamco commented May 7, 2024

hyzx86 commented May 8, 2024

MikeAlhayek May 8, 2024


		public ContentDefinitionStep(IContentDefinitionManager contentDefinitionManager)
		public ContentDefinitionStep(IContentDefinitionManager contentDefinitionManager, IStringLocalizer<ContentDefinitionStep> s)

Add type checking when importing types from recipes #15979 #15980

Add type checking when importing types from recipes #15979 #15980

Conversation

hyzx86 commented May 5, 2024 • edited by coderabbitai bot Loading

Summary by CodeRabbit

hyzx86 commented May 5, 2024

coderabbitai bot commented May 5, 2024

coderabbitai bot commented May 5, 2024 • edited Loading

Walkthrough

Changes

Assessment against linked issues

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

coderabbitai bot left a comment

Choose a reason for hiding this comment

Piedone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyzx86 commented May 6, 2024

coderabbitai bot commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piedone commented May 7, 2024

MikeAlhayek commented May 7, 2024

hishamco commented May 7, 2024

hyzx86 commented May 8, 2024

Choose a reason for hiding this comment

hyzx86 commented May 5, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented May 5, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)