DNN-31366 - Deleted page can still be selected as parent page

[berkarslan-xo]

Partially Fixes #2919 (There are total 2 PRs; for the other PR, you can check: dnnsoftware/Dnn.AdminExperience#1082 ).

Summary

A new overload which accepts an additional parameter named as "includeDeletedChildren" is added.

Demo: https://drive.google.com/open?id=1yg6OuVuLMbwCbBlFHXDqQa8Abd3WPIn_

Note: This PR should be merged BEFORE dnnsoftware/Dnn.AdminExperience#1082

[dnfclas]

All CLA requirements met.

[sleupold]

This might be a breaking change, if the property is used not just for navigation, but even for determining exportable pages. IMO it would be safer to add a property "active pages"

[valadas]

I agree with @sleupold this would be a breaking change. I would prefer a PR that handles this situation in the frontend only (AdminExperience) to not show deleted pages in that dropdown.

[sleupold]

IMO this could be handled well by an additional (computed) property in the API (where computation IMO belongs)

[valadas]

Yes but active and not deleted does not have the same meaning, specially here, we do want to be able to add sub-pages to inactive pages, but not to deleted pages. The IsDeleted property already exists, the frontend just needs to ignore those pages that are deleted. Or adding an api that has a additional parameter to inclue or explude deleted pages could be ok.

The problem here is that there is a modification of a current public api, adding a new overload that takes an additional boot to included deleted pages or not and then redirecting the other one to this new overload and mark it deprecated would be more acceptable. But as it is, this has the potential to break anything that expected the old signature without any previous warning.

[berkarslan-xo]

The problem here was that I did this change because in the tree display (which appears when you click on the combobox of Parent Page), child pages are only loaded when you click on a parent page through AJAX call but that little arrows on the lefthand side of the pages appear only when HasChildren property is true (if there are no children, arrows don't appear as you might guess). I changed the part that I needed to change in AdminExperience repo about not listing the deleted children (I fixed it in the place that serves the AJAX call) but in order to not show that little arrows when there are no children, I made this change.

As @sleupold suggested, another solution might be adding a new property into TabInfo class named as "HasNonDeletedChildren" which will come from vw_Tabs view. I then can bind to this property in UI side instead of HasChildren OR I can do "HasChildren = HasNonDeletedChildren" in Controller action of Dnn.AdminExperience side so that I won't have to touch react side. Is this solution ok?

[valadas]

Would have to dig into the previous PR and check what needs to be done here, but there are multiple ways to skin this cat. What is really important is to not change the behaviour of an existing api or a common component because it may be used elsewhere for different purposes and needs.

I did not take the time to go check the details of all this but we either:
🅰️ add an api that returns what we want without modifying existing ones (overload)
🅱️ we handle that special need in that specific UI part by skipping nodes that have isDeleted true

The problem with this PR is that it changes what is returned for people already expecting the current behaviour.

[berkarslan-xo]

I have added a new overload which accepts an additional parameter named as "includeDeletedChildren".

[mitchelsellers]

@berkarslan-xo Can we get you to submit this against the 9.4.x branch rather than Development?

[berkarslan-xo]

@berkarslan-xo Can we get you to submit this against the 9.4.x branch rather than Development?

I have created #2925 now. What about my second PR (which is dependent on this PR): dnnsoftware/Dnn.AdminExperience#1082 ? Should it stay as is or should I change it's target branch too?

[berkarslan-xo]

Dear approvers, I have created the requested PR for 9.4.x (#2925) but it's not picked up yet for a review, do you need anything on my side?

And could you please let me know what I need to do about dnnsoftware/Dnn.AdminExperience#1082 ? Should I retarget it to another branch or is it ok as it is?

[mitchelsellers]

Closing this as it will be processed with the other PR against the release branch.