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

Fix resource manager permissions save issues #6046

Conversation

Adeoluwa-Simeon
Copy link

Fixes #6045

Summary

Fix resource manager permissions save issues

  • set default PortalID in vw_FolderPermissions to -1
  • include other selected permissions when 'Write to Folder' permisison is set
  • update javascript to fix allowAccess==0 permissions 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 #6045 and check that the issues are solved

- set default PortalID in vw_FolderPermissions to -1
- include other selected permissions when 'Write to Folder' permisison is set
- update javascript to fix allowAccess==0 permissions issues
@Adeoluwa-Simeon
Copy link
Author

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

@Adeoluwa-Simeon Adeoluwa-Simeon marked this pull request as draft May 24, 2024 23:14
@Adeoluwa-Simeon Adeoluwa-Simeon marked this pull request as ready for review May 24, 2024 23:14
@mitchelsellers
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Author

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

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.

I believe that this is an acceptable change, but I will need to review this to validate

@leonhardholz
Copy link

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This will need to be 09.13.04 at minimum since 9.13.03 has been released

@mitchelsellers
Copy link
Contributor

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

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

I downloaded this build and it does fix everything mentioned in the original issue.

Just need to rename that sql file for next version (09.13.04)
In a perfect world if while you are in this area if you could take a look at #5857 steps, it would be awesome if it could be fixed in the PR too as it is probably something similar for a fix.

@Adeoluwa-Simeon
Copy link
Author

@valadas I have updated the file name.
I took a quick look at the #5857 issue, it would take some time for me to do a fix, it seems to be more than the null reference.

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

Looks good to me

@leonhardholz
Copy link

Great, thank you. Can this be merged now?

@valadas
Copy link
Contributor

valadas commented Jun 21, 2024

Just needs a second approver

@mitchelsellers
Copy link
Contributor

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

@valadas
Copy link
Contributor

valadas commented Jun 22, 2024

Conflict resolved

@valadas valadas added this to the 9.13.4 milestone Jun 22, 2024
@valadas valadas merged commit 50bd25f into dnnsoftware:develop Jun 22, 2024
3 checks passed
@leonhardholz
Copy link

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

@valadas
Copy link
Contributor

valadas commented Jun 22, 2024

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

@leonhardholz
Copy link

@valadas How's the release coming along?

@valadas
Copy link
Contributor

valadas commented Jul 2, 2024

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

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.

[Bug]: Resource manager permissions not being saved properly
6 participants