Dnn.Platform icon indicating copy to clipboard operation
Dnn.Platform copied to clipboard

Add ISkinService for Abstractions Project

Open GerardSmit opened this issue 2 years ago • 7 comments
trafficstars

Fixes #5837

Summary

This PR adds ISkinService which is an abstraction layer for SkinController.

Things to validate

  1. I've renamed properties with ID to Id. Only DotNetNuke.Abstractions.Users.IUserInfo uses ID (AffiliateID, PortalID, UserID), the rest uses Id.
  2. I've moved 2 enums to the abstractions; I've added [TypeForwardTo] so extensions don't have to recompile.
  3. I didn't mark the static methods as [Obsolete] and I haven't modified the calls to SkinController.
  4. I've added the service as a scoped service; just in case the service will use scoped services in the future.

GerardSmit avatar Oct 10 '23 23:10 GerardSmit

I've changed the abstraction a little bit, in my eyes it makes more sense this way.

SkinPackageType

I've created a new enum named "SkinPackageType", this can be either Skin or Container. You can use this enum to get or update everything in the interface. For example:

Instead of having a property "RootSkin" or "RootContainer", you call: GetFolderName(SkinPackageType.Skin). Instead of having 4 methods (e.g. "GetDefaultPortalSkin"), you call: GetDefaultSkinSrc(SkinPackageType.Skin, SkinType.Edit) Instead of changing the skin with the folder name, you call: SetSkin(SkinPackageType.Skin, ...)

Skin in SkinPackageInfo

I also did see that there is a SkinInfo, but it's never used. I changed SkinPackageInfo.Skins to a list of SkinInfo. If we ever add more details in the table dbo.Skins, we can add these to this class. Before the Skins was a Dictionary with ID as key and SkinSrc as value. This means it's a breaking change, but it's already targeting v10. Is this OK?

Because of the abstractions, I cannot implement IList<ISkinInfo> because in the implementation this is a List<SkinInfo>. I've created a new wrapper class called AbstractionList<TInterface, TImplementation> which casts all the interfaces to the implementation.

In the abstractions I've added IObjectList<T>, which is used in SkinPackageInfo.Skins. This interface has an extra method called T AddNew() which creates a new instance of the implementation, adds it to the list and returns the instance. Because you don't have SkinInfo in the abstractions; you can never create it. Now you can do the following:

ISkinPackageInfo skinPackageInfo; // Instance of the Skin Package

ISkinInfo newSkin = skinPackageInfo.Skins.AddNew(); // New skin

Naming

I've also changed the naming a little bit, for example GetSkins returns an enumerable with the skins in a folder; not from the database. I've changed the name to GetSkinsInFolder.


This means the abstraction is quite different than the implementation now, but it's way clearer what every method does, just by the name and arguments. If there is any feedback I like to hear it.

GerardSmit avatar Oct 17 '23 20:10 GerardSmit

@dnnsoftware/approvers I've rebased on develop, should be good to merge.

bdukes avatar Oct 24 '23 20:10 bdukes

Just thinking about this PR, should we call the new abstraction IThemeService?

bdukes avatar Oct 26 '23 21:10 bdukes

Just thinking about this PR, should we call the new abstraction IThemeService?

I don't have a strong preference. I would love it but then a lot of other code uses Skin terminology so not sure if it would add or remove confusion :)

valadas avatar Oct 26 '23 22:10 valadas

I think we can merge this PR as-is. I opened a new PR to discuss the potential naming change.

bdukes avatar Nov 03 '23 14:11 bdukes

I propose we move this PR to a 9.14.0 milestone. @dnnsoftware/approvers ?

david-poindexter avatar Nov 28 '23 20:11 david-poindexter

Per Technology meeting today, we agreed on a 9.14.0 milestone for this PR. Thanks!

david-poindexter avatar Nov 28 '23 20:11 david-poindexter