Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

berkarslan-xo
Copy link
Contributor

@berkarslan-xo berkarslan-xo commented Aug 2, 2019

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

Summary

A new overload which accepts an additional parameter named as "includeDeletedChildren" is added. (retargeted to "9.4.x" branch, you can reach the previous PR which was targeted to "development" branch from here: #2920 )

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

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

@berkarslan-xo
Copy link
Contributor Author

Dear approvers, any news on this PR?

@mitchelsellers
Copy link
Contributor

We are in the middle of the RC process. As soon as the next RC drops we will process.

mitchelsellers
mitchelsellers previously approved these changes Aug 10, 2019
Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @dnnsoftware/tag can I get a second approval on this, and merge it ASAP

valadas
valadas previously approved these changes Aug 10, 2019
@berkarslan-xo
Copy link
Contributor Author

CI seems broken (it got a Powershell script error after merging the latest 9.4.x branch) and I don't seem to have the permission to run the build again. Can someone trigger the build please? Btw, I saw some similar failure reasons in some other builds so that there might be an issue with it..

@mitchelsellers
Copy link
Contributor

@berkarslan-xo We are working on some issues with the build. For now I'll trigger a re-build which will at least build clean.

/azp run

@berkarslan-xo
Copy link
Contributor Author

@berkarslan-xo We are working on some issues with the build. For now I'll trigger a re-build which will at least build clean.

/azp run

Thanks, the build seems to be not retriggered yet as far as I can see though.

@mitchelsellers
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@berkarslan-xo
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2925 in repo dnnsoftware/Dnn.Platform

@bdukes
Copy link
Contributor

bdukes commented Aug 13, 2019

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).


// HasChildren should be true in case there is at least one not deleted child
tab.HasChildren &=
includeDeletedChildren || (
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if GetTabsByParent(tab.TabID, tab.PortalID) returns empty list, shouldn't tab.HasChildren be false at that time? I mean like in the following:

tab.HasChildren &= (
        includeDeletedChildren && GetTabsByParent(tab.TabID, tab.PortalID).Any()
    ) || (
        !includeDeletedChildren && GetTabsByParent(tab.TabID, tab.PortalID).Any(a => !a.IsDeleted));

or more cleanly:
GetTabsByParent(tab.TabID, tab.PortalID).Any(a => includeDeletedChildren || !a.IsDeleted));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If GetTabsByParent is empty, I would expect that tab.HasChildren is already false, so the rest of the conditions aren't considered. I believe it is fine as it is, but thanks for the review and double check 👍

@bdukes
Copy link
Contributor

bdukes commented Aug 13, 2019

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bdukes bdukes dismissed stale reviews from valadas and mitchelsellers via 2c00b6e August 13, 2019 19:56
@bdukes bdukes force-pushed the bug/DNN-31366-retargeted9.4.x branch from 0a098fb to 2c00b6e Compare August 13, 2019 19:56
bdukes
bdukes previously approved these changes Aug 13, 2019
@berkarslan-xo
Copy link
Contributor Author

berkarslan-xo commented Aug 14, 2019

Hi @valadas , could you give the second approval, please (approvals got reset since a small commit is made)?

@valadas valadas merged commit b2d5533 into dnnsoftware:release/9.4.x Aug 14, 2019
mitchelsellers pushed a commit that referenced this pull request Aug 15, 2019
* Fix error running unit tests (#2940)

When running tests, the following error occurs:
	VSTest: Could not locate executable.

cake-build/cake#1522 (comment)
indicates that there's an issue using VS 2019, and presents a workaround
to find the correct path

* DNN-31366 - Deleted page can still be selected as parent page (#2925)

* DNN-31366 - TabController HasChildren returns false if all children are deleted

* Simplify HasChildren condition
valadas added a commit that referenced this pull request Sep 1, 2019
* Fix error running unit tests (#2940)

When running tests, the following error occurs:
	VSTest: Could not locate executable.

cake-build/cake#1522 (comment)
indicates that there's an issue using VS 2019, and presents a workaround
to find the correct path

* DNN-31366 - Deleted page can still be selected as parent page (#2925)

* DNN-31366 - TabController HasChildren returns false if all children are deleted

* Simplify HasChildren condition

* deleting_themes_from_Extensions_doesnt_remove_it_from_themes (#2950)

* Returns the working types if some fail (#2953)
DnnSoftwarePersian pushed a commit to DnnSoftwarePersian/Dnn.Platform that referenced this pull request May 17, 2020
…e#2964)

* Fix error running unit tests (dnnsoftware#2940)

When running tests, the following error occurs:
	VSTest: Could not locate executable.

cake-build/cake#1522 (comment)
indicates that there's an issue using VS 2019, and presents a workaround
to find the correct path

* DNN-31366 - Deleted page can still be selected as parent page (dnnsoftware#2925)

* DNN-31366 - TabController HasChildren returns false if all children are deleted

* Simplify HasChildren condition

* deleting_themes_from_Extensions_doesnt_remove_it_from_themes (dnnsoftware#2950)

* Returns the working types if some fail (dnnsoftware#2953)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants