dnnsoftware / Dnn.Platform

DNN (formerly DotNetNuke) is the leading open source web content management platform (CMS) in the Microsoft ecosystem.
https://dnncommunity.org/
MIT License
1.01k stars 745 forks source link

Fix resource manager permissions save issues #6046

Closed Adeoluwa-Simeon closed 1 week ago

Adeoluwa-Simeon commented 1 month ago

Fixes https://github.com/dnnsoftware/Dnn.Platform/issues/6045

Summary

Fix resource manager permissions save issues

Release notes

Resolved an issue where the permissions in the resource manager were not being saved and fixed some of the auto adjust enable and disable permission functions.

Testing steps

Re-run the steps in https://github.com/dnnsoftware/Dnn.Platform/issues/6045 and check that the issues are solved

Adeoluwa-Simeon commented 1 month ago

@microsoft-github-policy-service agree company="Trilogy"

mitchelsellers commented 1 month ago

@Adeoluwa-Simeon Thank you for this submission. Overall I think that this change resolves the reported issue, however, I am slightly concerned with the impact of changing the behavior of a Null Portal id on a view that is used in other locations. Upon a quick search that view is utilized by a number of other areas of the platform and I'm concerned that there would be unintended consequences with this change that are going to be hard, if not impossible to fully track.

@dnnsoftware/approvers thoughts?

david-poindexter commented 1 month ago

@Adeoluwa-Simeon Thank you for this submission. Overall I think that this change resolves the reported issue, however, I am slightly concerned with the impact of changing the behavior of a Null Portal id on a view that is used in other locations. Upon a quick search that view is utilized by a number of other areas of the platform and I'm concerned that there would be unintended consequences with this change that are going to be hard, if not impossible to fully track.

@dnnsoftware/approvers thoughts?

@mitchelsellers I tend to lean to your thought process here as well. I'd be interested to hear what @valadas thinks about this.

Adeoluwa-Simeon commented 1 month ago

An alternative solution would be to modify the procedure that calls the view from

CREATE PROCEDURE [dbo].[GetFolderPermissionsByPortalAndPath] @portalid int, @FolderPath nvarchar(300) AS BEGIN SET @portalid = IsNull(@portalid, -1)

SELECT * FROM dbo.[vw_FolderPermissions] WHERE PortalID = @PortalId AND (FolderPath = @FolderPath OR @FolderPath IS NULL) END

to cater to both portalId being null and -1, essentially solving the need for a default value

CREATE PROCEDURE [dbo].[GetFolderPermissionsByPortalAndPath] @portalid int, @FolderPath nvarchar(300) AS BEGIN

SELECT * FROM dbo.[vw_FolderPermissions] WHERE (PortalID = IsNull(@PortalId, -1) OR (@PortalId IS NULL AND PortalID IS NULL)) AND (FolderPath = @FolderPath OR @FolderPath IS NULL) END

leonhardholz commented 1 week ago

@valadas @mitchelsellers Is it possible to get this fix into the next release? We have customers complaining about the problem.

mitchelsellers commented 1 week ago

@valadas @david-poindexter Any thoughts on this? I have not had a chance to test/validate the actual change

Adeoluwa-Simeon commented 1 week ago

@valadas I have updated the file name. I took a quick look at the https://github.com/dnnsoftware/Dnn.Platform/issues/5857 issue, it would take some time for me to do a fix, it seems to be more than the null reference.

leonhardholz commented 1 week ago

Great, thank you. Can this be merged now?

valadas commented 1 week ago

Just needs a second approver

mitchelsellers commented 1 week ago

@valadas looks like we have a merge conflict? I'm on vacation but will try to review soon

valadas commented 1 week ago

Conflict resolved

leonhardholz commented 1 week ago

Thanks everyone! Is there an ETA for the 9.13.4 release?

valadas commented 1 week ago

It's not a promise but I think it may happen early next week.

leonhardholz commented 1 day ago

@valadas How's the release coming along?

valadas commented 14 hours ago

With the US holidays it won't be this week, we are meeting next week and will analyze the situation then