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

Add ISkinService for Abstractions Project #5838

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

GerardSmit
Copy link
Contributor

@GerardSmit GerardSmit commented Oct 10, 2023

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.

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 have 2 small concernes but I am not sure if relevant, just something to double-check...

DNN Platform/Library/UI/Skins/SkinScope.cs Outdated Show resolved Hide resolved
DNN Platform/Library/UI/Skins/SkinType.cs Outdated Show resolved Hide resolved
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! We appreciate your work, this is a nice and clean PR. Hopefully we can quickly resolve our couple of questions and get it included.

DNN Platform/Library/UI/Skins/SkinScope.cs Outdated Show resolved Hide resolved
DNN Platform/Library/UI/Skins/SkinController.cs Outdated Show resolved Hide resolved
DNN Platform/Library/UI/Skins/SkinController.cs Outdated Show resolved Hide resolved
DNN Platform/DotNetNuke.Abstractions/Skins/SkinType.cs Outdated Show resolved Hide resolved
DNN Platform/Library/UI/Skins/SkinScope.cs Outdated Show resolved Hide resolved
@GerardSmit
Copy link
Contributor Author

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 GerardSmit marked this pull request as ready for review October 18, 2023 19:42
GerardSmit and others added 2 commits October 24, 2023 15:35
Deprecate SkinPackageInfo.Skins, rename list to SkinPackageInfo.SkinsList
@bdukes bdukes force-pushed the feature/skin-service branch from 8b38df8 to c70a8ba Compare October 24, 2023 20:48
@bdukes bdukes changed the base branch from release/10.0.0 to develop October 24, 2023 20:49
@bdukes bdukes modified the milestones: 10.0.0, 9.13.1 Oct 24, 2023
@bdukes
Copy link
Contributor

bdukes commented Oct 24, 2023

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

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 so much!

@bdukes
Copy link
Contributor

bdukes commented Oct 26, 2023

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

@valadas
Copy link
Contributor

valadas commented Oct 26, 2023

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 :)

@bdukes
Copy link
Contributor

bdukes commented Nov 3, 2023

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

[JsonIgnore]
[Obsolete($"Deprecated in DotNetNuke 9.13.1. Use {nameof(ISkinPackageInfo)}.{nameof(ISkinPackageInfo.Skins)} instead. Scheduled for removal in v11.0.0.")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, the SkinController only updates the SkinsList (skinPackage is ISkinPackageInfo here):

/// <inheritdoc cref="ISkinService.UpdateSkinPackage" />
private static void UpdateSkinPackage(ISkinPackageInfo skinPackage)
{
DataProvider.Instance().UpdateSkinPackage(
skinPackage.SkinPackageId,
skinPackage.PackageId,
skinPackage.PortalId,
skinPackage.SkinName,
SkinUtils.ToDatabaseName(skinPackage.SkinType),
UserController.Instance.GetCurrentUserInfo().UserID);
EventLogController.Instance.AddLog(skinPackage, PortalController.Instance.GetCurrentPortalSettings(), UserController.Instance.GetCurrentUserInfo().UserID, string.Empty, EventLogController.EventLogType.SKINPACKAGE_UPDATED);
foreach (var skin in skinPackage.Skins)
{
UpdateSkin(skin.SkinId, skin.SkinSrc);
}
}

Even though the property is deprecated, it's read-only now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand. Are you saying that UpdateSkinPackage is wrong to use skinPackage.Skins? Or that the ISkinPackageInfo.Skins property is fundamentally broken? Or that we're missing an update to keep the two collection in sync?

Copy link
Contributor Author

@GerardSmit GerardSmit Nov 6, 2023

Choose a reason for hiding this comment

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

We currently have two properties:

Skins = Dictionary<int, string>
SkinsList = List<ISkinInfo>

If you change the SkinsList and call UpdateSkinPackage the skins will be updated.
However, if you change the Skins and call UpdateSkinPackage the skins will not be updated, which does work in the current version.

Or that we're missing an update to keep the two collection in sync?

Correct.

@david-poindexter
Copy link
Contributor

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

@david-poindexter david-poindexter modified the milestones: 9.13.1, 9.14.0 Nov 28, 2023
@david-poindexter
Copy link
Contributor

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

@donker donker modified the milestones: 9.14.0, 10.0.0 May 7, 2024
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 SkinController
6 participants