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

Added IPermissionDefinitionService for Abstractions Project #5841

Merged
merged 5 commits into from
Oct 26, 2023

Conversation

GerardSmit
Copy link
Contributor

@GerardSmit GerardSmit commented Oct 11, 2023

Fixes #5840

Summary

This PR adds IPermissionDefinitionService which is an abstraction layer for PermissionController.

Instead of returning an ArrayList in the GetPermissions*-methods, I've made the return type IEnumerable<IPermissionInfo>; this reduces allocations (ArrayList instance and two arrays). For all these methods I've created an Enumerable method, that's being used by the current implementation and interface implementation; this is so we don't have duplicated code.

I've only created an interface for this class. All calls to the PermissionController haven't been refactored.

TODO

  • Merge IPermissionInfoBase and IPermissionInfo into a single interface IPermissionInfo

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.

Nothing sticks out to me. Just no in love with IPermissionInfoBase name which makes it look like a base class. What about we use IPermission and IPermissionInfo instead of IPermissionInfo and IPermissionInfoBase ? Just a thought...

@valadas valadas added this to the 10.0.0 milestone Oct 11, 2023
@GerardSmit
Copy link
Contributor Author

The only downside to IPermission is that it's an already existing interface in .NET Framework: System.Security.IPermission.

We can also merge the two interfaces to a single interface IPermissionInfo, since the current implementations of *PermissionInfo all extends PermissionInfoBase and PermissionInfo:

image

@valadas
Copy link
Contributor

valadas commented Oct 11, 2023

Sounds good. Hey what tool you use for that graph ?

@GerardSmit
Copy link
Contributor Author

I used ReSharper to create that diagram.

What I did:

  1. Left click the interface name and click "Derrived Symbols":
    image
  2. Click on the first icon "Show on Diagram":
    image
  3. Then I changed the view to "Business Logic":
    image

@GerardSmit GerardSmit marked this pull request as draft October 13, 2023 11:00
Copy link
Contributor

@bdukes bdukes left a comment

Choose a reason for hiding this comment

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

Thanks!

@bdukes
Copy link
Contributor

bdukes commented Oct 13, 2023

I agree, let's merge IPermissionInfo and IPermissionBase

@GerardSmit
Copy link
Contributor Author

Sadly, I couldn't merge IPermissionInfo and IPermissionInfoBase, since I misunderstood PermissionInfo.

PermissionInfo is from dbo.Permissions, which are the definitions.
PermissionInfoBase is the base for dbo.TabPermission, dbo.ModulePermission etc. they are the actual permission to the object they belong to (e.g. Tab or Module) with their definition.

I've renamed IPermissionInfo to IPermissionDefinition to prevent confusion, and IPermissionInfoBase to IPermissionInfo. This only applies to the abstractions, the actual implementation still have the same name (class PortalInfo and PortalInfoBase).

Is this OK?

@bdukes
Copy link
Contributor

bdukes commented Oct 13, 2023

Sounds great to me, thanks!

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.

Nice, given this is the resource manager and not part of any distributed nuget package I am not worried about the breaking changes in this.

@GerardSmit
Copy link
Contributor Author

I've checked /Admin/File-Management if the permissions are being loaded and saved. Everything works as expected 🎉

@GerardSmit GerardSmit marked this pull request as ready for review October 17, 2023 18:29
@GerardSmit
Copy link
Contributor Author

GerardSmit commented Oct 17, 2023

I'm still thinking about the interface name. If we're gonna make abstractions for PermissionProvider, should this be done in the IPermissionService as well?

Another solution is to rename this interface to IPermissionDefinitionService, and when we make abstractions for PermissionProvider we can call the interface name IPermissionService.

@bdukes
Copy link
Contributor

bdukes commented Oct 17, 2023

I'm still thinking about the interface name. If we're gonna make abstractions for PermissionProvider, should this be done in the IPermissionService as well?

Another solution is to rename this interface to IPermissionDefinitionService, and when we make abstractions for PermissionProvider we can call the interface name IPermissionService.

I think IPermissionDefinitionService is a better name, since every member deals with IPermissionDefinitionInfo.

@GerardSmit
Copy link
Contributor Author

There are still 2 comments open: #5841 (comment), #5841 (comment); other than that everything is done.

@bdukes
Copy link
Contributor

bdukes commented Oct 17, 2023

Thanks @GerardSmit, this is looking great. The @dnnsoftware/approvers will meet next Tuesday, so we may not get a definitive answer on those open questions until then. We appreciate your contribution and your patience.

@bdukes bdukes changed the base branch from release/10.0.0 to develop October 24, 2023 20:25
@bdukes bdukes requested a review from valadas October 24, 2023 20:25
@bdukes
Copy link
Contributor

bdukes commented Oct 24, 2023

@dnnsoftware/approvers I've rebased on develop, updated the deprecations to 9.13.0, added deprecations for the properties which we're changing casing on, and also extracted IFolderPermissionInfo as part of adjusting usages of the deprecated properties. This should be good to merge.

@bdukes bdukes modified the milestones: 10.0.0, 9.13.1 Oct 24, 2023
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.

Awesome, great work!

@valadas valadas merged commit 5430f97 into dnnsoftware:develop Oct 26, 2023
3 checks passed
@GerardSmit GerardSmit changed the title Add IPermissionService for Abstractions Project Add IPermissionDefinitionService for Abstractions Project Oct 27, 2023
@valadas valadas changed the title Add IPermissionDefinitionService for Abstractions Project Added IPermissionDefinitionService for Abstractions Project Nov 28, 2023
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.

[Enhancement]: Add Dependency Injection Support for PermissionController
4 participants