Add SelectNode and Remove method to JsonDynamicObject class #15509

hyzx86 · 2024-03-14T08:32:33Z

try to fix #15505

hyzx86 · 2024-03-14T08:39:48Z

Since the Content property could be of type JsonDynamicObject or JsonObject, do we need to migrate all functions that can be called from JsonObject here?

OrchardCore/src/OrchardCore/OrchardCore.ContentManagement.Abstractions/ContentElement.cs

Lines 26 to 31 in b83e2dc

    
           [JsonIgnore] 
        
           public dynamic Content => _dynamicObject ??= Data; 
        
           [JsonIgnore] 
        
           internal JsonObject Data { get; set; }

src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicObject.cs

…micObject.cs Co-authored-by: Hisham Bin Ateya <[email protected]>

…tentActions

hyzx86 · 2024-03-14T15:14:48Z

This PR can't to fix #15497 , because there was a logical error in the last change

test/OrchardCore.Tests/Data/ContentItemTests.cs

sebastienros · 2024-03-14T17:33:17Z

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


            // Couldn't find targeted menu item.
            if (menuItem == null)
            {
                return NotFound();
            }

-            menu.Content.Remove(menuItemId);


Do we have to change the code if Remove is added in this class?

The problem here is not because of the Remove function, because menu.Content is a document object, so we can not directly remove a MenuItem from the document object based on the menuId
The code on the main branch here is wrong

sebastienros · 2024-03-14T17:35:14Z

Fixes #15497 too?

hyzx86 · 2024-03-14T17:46:50Z

Fixes #15497 too?

Yes , I have merged @hishamco 's PR

github-actions · 2024-03-21T16:47:55Z

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

hishamco · 2024-03-21T16:54:20Z

Please resolve the conflict

github-actions · 2024-03-21T18:21:38Z

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

hyzx86 · 2024-03-22T02:17:40Z

@hyzx86 please resolve the conflict so we can merge this after @sarahelsaig approval

Resolved

sarahelsaig

Please only request a review when the checks are successful. You have these errors:

Also you didn't address this: https://github.com/OrchardCMS/OrchardCore/pull/15509/files/2e877408abb79e7f6613ce5a091cf918bb99d758#r1534515122

hyzx86 · 2024-03-22T09:32:06Z

@sarahelsaig updated .

Piedone · 2024-03-22T22:25:58Z

Given that #15524 was merged, is this still needed?

hyzx86 · 2024-03-23T15:30:02Z

Given that #15524 was merged, is this still needed?

Yes, the PR just modify JsonObject. SelectNode function implementation, here will repair some direct call ContentItem.Content.Remove and ContentItem.Content.SelectNode

src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicObject.cs

sarahelsaig · 2024-03-24T00:18:06Z

src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicArray.cs

@@ -41,6 +41,10 @@ public class JsonDynamicArray : DynamicObject, IEnumerable<JsonNode?>
        }
    }

+    public bool Remove(JsonNode? item) => _jsonArray.Remove(item);


As I mentioned Please update _dictionary in JsonDynamicArray.Remove as well.

…micObject.cs Co-authored-by: Sára El-Saig <[email protected]>

src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicObject.cs

…micObject.cs Co-authored-by: Hisham Bin Ateya <[email protected]>

test/OrchardCore.Tests/Data/ContentItemTests.cs

hishamco · 2024-03-24T17:46:14Z

@sarahelsaig waiting for your approval if everything looks good for you

sarahelsaig

LGTM

hishamco · 2024-03-24T18:19:41Z

@MikeAlhayek is there anything to add or shall we merge this

MikeAlhayek · 2024-03-24T18:27:44Z

Merge it

hishamco and others added 2 commits March 13, 2024 18:47

Fix menu item deletion

f9bfc19

try fix OrchardCMS#15505

4ecdcbe

hyzx86 changed the title ~~try fix #15505 and #15497~~ add SelectNode and Remove method on JsonDynamicObject class Mar 14, 2024

hyzx86 changed the title ~~add SelectNode and Remove method on JsonDynamicObject class~~ Add SelectNode and Remove method on JsonDynamicObject class Mar 14, 2024

hyzx86 changed the title ~~Add SelectNode and Remove method on JsonDynamicObject class~~ Add SelectNode and Remove method to JsonDynamicObject class Mar 14, 2024

hishamco requested changes Mar 14, 2024

View reviewed changes

src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicObject.cs Outdated Show resolved Hide resolved

src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicObject.cs Outdated Show resolved Hide resolved

hyzx86 and others added 3 commits March 14, 2024 21:59

Update src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDyna…

d363844

…micObject.cs Co-authored-by: Hisham Bin Ateya <[email protected]>

Update src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDyna…

55de8af

…micObject.cs Co-authored-by: Hisham Bin Ateya <[email protected]>

Merge branch 'main' into fix_ContentActions

c0e7045

This comment was marked as off-topic.

Sign in to view

hyzx86 requested a review from hishamco March 14, 2024 14:21

add remove and remove at to JsonDynamicArray

02b412b

hyzx86 mentioned this pull request Mar 14, 2024

Fix menu item deletion #15500

Merged

Merge remote-tracking branch 'origin/hishamco/menu-item' into fix_Con…

7aab0f0

…tentActions

sarahelsaig reviewed Mar 14, 2024

View reviewed changes

test/OrchardCore.Tests/Data/ContentItemTests.cs Outdated Show resolved Hide resolved

sebastienros reviewed Mar 14, 2024

View reviewed changes

sebastienros added this to the 1.9 milestone Mar 14, 2024

Merge branch 'main' into fix_ContentActions

6da518b

MikeAlhayek added the needs triage label Mar 21, 2024

github-actions bot added the merge conflict label Mar 21, 2024

Merge branch 'main' into fix_ContentActions

a1af2dc

github-actions bot added merge conflict and removed merge conflict labels Mar 21, 2024

hyzx86 requested review from sebastienros and sarahelsaig March 22, 2024 02:18

sarahelsaig suggested changes Mar 22, 2024

View reviewed changes

hyzx86 added 2 commits March 22, 2024 16:20

fix build

4f8d779

add remove key from dictionary

d868bf1

Merge branch 'main' into fix_ContentActions

f7c9083

sarahelsaig suggested changes Mar 24, 2024

View reviewed changes

MikeAlhayek and others added 3 commits March 23, 2024 17:54

Update src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDyna…

9f92a6c

…micObject.cs Co-authored-by: Sára El-Saig <[email protected]>

Merge branch 'main' into fix_ContentActions

ae76b89

update remove methods

178841e

hyzx86 requested a review from sarahelsaig March 24, 2024 14:23

hishamco requested changes Mar 24, 2024

View reviewed changes

src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicObject.cs Show resolved Hide resolved

src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDynamicObject.cs Show resolved Hide resolved

hyzx86 and others added 2 commits March 25, 2024 00:08

Update src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDyna…

646eb83

…micObject.cs Co-authored-by: Hisham Bin Ateya <[email protected]>

Update src/OrchardCore/OrchardCore.Abstractions/Json/Dynamic/JsonDyna…

981cc0c

…micObject.cs Co-authored-by: Hisham Bin Ateya <[email protected]>

hyzx86 requested a review from hishamco March 24, 2024 16:15

hishamco reviewed Mar 24, 2024

View reviewed changes

test/OrchardCore.Tests/Data/ContentItemTests.cs Show resolved Hide resolved

hishamco approved these changes Mar 24, 2024

View reviewed changes

sarahelsaig approved these changes Mar 24, 2024

View reviewed changes

hishamco merged commit ba718ab into OrchardCMS:main Mar 24, 2024
4 checks passed

hishamco mentioned this pull request Mar 27, 2024

Taxonomy Terms Management is broken #15590

Closed

This was referenced Apr 1, 2024

Monthly community metrics report for 2024-03-01..2024-03-31 #15634

Closed

Monthly community metrics report for 2024-03-01..2024-03-31 Emmanjr/OrchardCore#1

Open

hyzx86 deleted the fix_ContentActions branch June 13, 2024 06:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SelectNode and Remove method to JsonDynamicObject class #15509

Add SelectNode and Remove method to JsonDynamicObject class #15509

hyzx86 commented Mar 14, 2024 •

edited

Loading

hyzx86 commented Mar 14, 2024

This comment was marked as off-topic.

hyzx86 commented Mar 14, 2024

sebastienros Mar 14, 2024

hyzx86 Mar 14, 2024

sebastienros commented Mar 14, 2024

hyzx86 commented Mar 14, 2024

github-actions bot commented Mar 21, 2024

hishamco commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

hyzx86 commented Mar 22, 2024

sarahelsaig left a comment

hyzx86 commented Mar 22, 2024

Piedone commented Mar 22, 2024

hyzx86 commented Mar 23, 2024 •

edited

Loading

sarahelsaig Mar 24, 2024

hyzx86 Mar 24, 2024

hishamco commented Mar 24, 2024

sarahelsaig left a comment

hishamco commented Mar 24, 2024

MikeAlhayek commented Mar 24, 2024

Add SelectNode and Remove method to JsonDynamicObject class #15509

Add SelectNode and Remove method to JsonDynamicObject class #15509

Conversation

hyzx86 commented Mar 14, 2024 • edited Loading

hyzx86 commented Mar 14, 2024

This comment was marked as off-topic.

hyzx86 commented Mar 14, 2024

sebastienros Mar 14, 2024

Choose a reason for hiding this comment

hyzx86 Mar 14, 2024

Choose a reason for hiding this comment

sebastienros commented Mar 14, 2024

hyzx86 commented Mar 14, 2024

github-actions bot commented Mar 21, 2024

hishamco commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

hyzx86 commented Mar 22, 2024

sarahelsaig left a comment

Choose a reason for hiding this comment

hyzx86 commented Mar 22, 2024

Piedone commented Mar 22, 2024

hyzx86 commented Mar 23, 2024 • edited Loading

sarahelsaig Mar 24, 2024

Choose a reason for hiding this comment

hyzx86 Mar 24, 2024

Choose a reason for hiding this comment

hishamco commented Mar 24, 2024

sarahelsaig left a comment

Choose a reason for hiding this comment

hishamco commented Mar 24, 2024

MikeAlhayek commented Mar 24, 2024

hyzx86 commented Mar 14, 2024 •

edited

Loading

hyzx86 commented Mar 23, 2024 •

edited

Loading