Add IPortalSettingsManager for Abstractions Project #4071

SkyeHoefling · 2020-09-09T03:43:37Z

Related to #3985

Summary

Adds a new interface to Dependency Injection DotNetNuke.Abstractions.Portals.IPortalSettingsManager which is used for retrieving the current portal settings. The API structure in this PR has been simplified to use modern techniques in .NET such as generics and indexers. It still maintains the main functionality

Get
Update
Update Encrypted
Delete

Usage

This PR takes a few different interfaces and combines their implementations to work together. It will be consumed via dependency injection using the IPortalSettingsManager.

var portalSettingsManager = Globals.DependencyProvider.GetRequiredService<IPortalSettingsManager>();
bool mySetting = portalSettingsManager
    .GetPortalSettings(portalId, cultureCode)
    .Get<bool>("My true/false portal setting");

The example above uses the service locator pattern, it is always recommended to use constructor injection where possible.

Missing Items

Currently this PR is missing a few parts of the implementation. I submitted it early to get feedback prior on the general direction. Below is a list of missing features that I plan to implement after the initial review.

IConfigurationSetting provides a Key-Value-Pair object which is used throughout DNN
Get & Update dictionary. The old implementation allowed the user to get the entire dictionary or update the entire dictionary at once. Currently those APIs are not implemented

Required to merge

This PR was submitted prior to working on Unit Tests or validating the build works. I am doing this to get early feedback on architectural decisions being made with the changes proposed here.

Task	Status
Unit Tests	❌ Not Started
Test Installer	❌ Not Started

…PortalSettingsManager

…tation

SkyeHoefling · 2020-09-09T03:50:27Z

DNN Platform/Library/Entities/Portals/PortalSettingsManager.cs

+        public ISettingsService GetPortalSettings(int portalId, string cultureCode)
+        {
+            if (string.IsNullOrEmpty(cultureCode) && portalId > -1)
+            {
+                cultureCode = PortalController.GetActivePortalLanguage(portalId);
+            }
+
+            var cachedSettings = this.GetCachedSettings(portalId, cultureCode);
+            var saveService = new PortalSaveSettingsService(portalId, cultureCode);
+            var deleteService = new PortalDeleteSettingsService(portalId, cultureCode);
+
+            return new PortalSettingsService(() => this.GetCachedSettings(portalId, cultureCode), saveService, deleteService);
+        }


This may be one of the focal points of this code review and I would like to highlight the change as well as my reasoning behind the change.

I wanted to implement the PortalSettingsService as a very simple wrapper around an IDictionary<string, string>. Since DNN has static API calls everywhere I didn't want the PortalSettingsService to be tightly coupled to anything. In theory this implementation could live in a .NET Standard 2.0 project and doesn't need to exist in the DotNetNuke.Library project. To do this I needed to create an ISaveSettingsService and a IDeleteSettingsService The implementations of this interfaces are passed in at instantiation time. This allows the wrapper (PortalSettingsService) to easily invoke the functionality.

The other item here is the callback function that retrieves the latest settings GetCacheSettings(int, int). This is useful in the PortalSettingsService to reset the local IDictionary<string, string> whenever the cache is cleared. Due to the de-coupled structure we need a way for it to easily reload the data. Since the PortalSettingsService doesn't have direct knowledge how to construct the IDictionary<string, string> we are using the callback that stores the reference on how to load the data.

bdukes

Thanks @ahoefling!

bdukes · 2020-09-09T13:33:41Z

DNN Platform/DotNetNuke.Abstractions/Settings/ISaveSettingsService.cs

+    /// to be injected via Dependency Injection. Please use the
+    /// <see cref="ISettingsService"/>.
+    /// </summary>
+    public interface ISaveSettingsService


Should these interfaces be internal if they're not intended to be used via DI?

This is a really great question!

In my opinion these interfaces should remain public if we want to support making the PortalSettingsService extensible. If a developer wants to swap it out for their own implementation they should be able to use the following code.

public class CustomPortalSettingsManager : PortalSettingsManager { } public class CustomPortalSaveSettingsService : PortalSaveSettingsService { } public class CustomPortalDeleteSettingsService : PortalDeleteSettingsService { }

If the extension is just changing the save/delete rules it doesn't necessarily need to create another class for the PortalSettingsService.This approach gives us the highest degree of extensibility, but also makes it more complicated.

For many modules and implementations this will never be used. I see this being valuable for something like Evoq or custom installations that want to execute special logic during the save or delete sequence.

bdukes · 2020-09-09T13:34:58Z

DNN Platform/DotNetNuke.Abstractions/Settings/ISettingsService.cs

+        /// </summary>
+        /// <param name="key">The setting key to update.</param>
+        /// <param name="value">The value to set.</param>
+        void Update(string key, string value);


Do we want generic Update APIs to match the generic Get APIs?

Should the update API support full C# objects that get serialized into json or should it just support primitives such as

string

int

double

float

long

short

bool

The SerializationController exposes an overload that takes a custom serializer. otherwise it checks for DateTime and TimeSpan and finally falls back to Convert.ToString.

WOW! That sounds really powerful, I will certainly swap out this implementation for that. Should we migrate that to the abstractions project as part of this PR. That sounds like something really useful to be available via Dependency Injection

If you're up for it, that would make sense to me. We can also do that as a separate PR if it makes sense to reduce the scope of this one.

I'll submit a new PR for the SerializationController

Summited PR #4087

bdukes · 2020-09-09T13:36:25Z

DNN Platform/Library/Entities/Portals/PortalDeleteSettingsService.cs

+
+        /// <inheritdoc />
+        public void DeleteAll() =>
+            this.DeleteAll(false);


I would expect clearCache to default to true

bdukes · 2020-09-09T13:38:45Z

DNN Platform/Library/Entities/Portals/PortalSaveSettingsService.cs

+        /// <inheritdoc />
+        public void UpdateEncrypted(string key, string value, bool clearCache)
+        {
+            this.Update(key, value, true, clearCache);


Where does the encryption happen when calling this overload?

bdukes · 2020-09-09T13:39:27Z

DNN Platform/Library/Entities/Portals/PortalSettingsManager.cs

+                cultureCode = PortalController.GetActivePortalLanguage(portalId);
+            }
+
+            var cachedSettings = this.GetCachedSettings(portalId, cultureCode);


This instance isn't getting used, right?

Suggested change

var cachedSettings = this.GetCachedSettings(portalId, cultureCode);

bdukes · 2020-09-09T13:44:21Z

DNN Platform/Library/Entities/Portals/PortalSettingsService.cs

+
+        /// <inheritdoc />
+        public void Update(string key, string value) => 
+            this.SaveService.Update(key, value, false);


As above, I would expect clearCache to default to true

bdukes · 2020-09-09T13:45:44Z

DNN Platform/Library/Entities/Portals/PortalSettingsService.cs

+            try
+            {
+                // DNN uses special logic for boolean. It accepts Y and TRUE. We should make sure that is convertible
+                return (T)Convert.ChangeType(this[key], typeof(T));


This is where I would like to see the logic from SerializationController reused (which will handle the special Boolean logic, for example).

bdukes · 2020-09-09T13:48:21Z

DNN Platform/Library/Entities/Portals/PortalController.cs

+            var portalSettingsManager = Globals.DependencyProvider.GetRequiredService<IPortalSettingsManager>();
+            return portalSettingsManager
+                .GetPortalSettings(portalID)
+                .Get<string>(settingName) ?? defaultValue;


The previous logic is to return the defaultValue when the value is an empty string, is this an intentional change to only return defaultValue when the value is null?

bdukes · 2020-09-09T13:52:59Z

DNN Platform/Library/Entities/Portals/PortalController.cs

-        {
-            string currentSetting = GetPortalSetting(settingName, portalID, string.Empty, cultureCode);
-
-            if (currentSetting != settingValue)


Is this check for the new value being equal to the old value still in the new implementation? I don't remember seeing something like that.

bdukes · 2020-09-09T13:53:29Z

DNN Platform/Library/Entities/Portals/PortalController.cs

-                    }
-                }
-
-                EventManager.Instance.OnPortalSettingUpdated(new PortalSettingUpdatedEventArgs


Is this event still being called?

SkyeHoefling · 2020-10-14T17:31:13Z

Since #4087 and #4159 have been merged this PR can be updated to reflect the latest and greatest code changes. Here is my list of things to do

Update eventlogger to use IEventLogger via DI
Update serialization to use DI
Update PR based on review feedback

I am not 100% sure, but I think I am going to flatten the structure here as the PR is as written is kind of complicated and unnecessary

stale · 2021-03-05T17:14:40Z

We have detected this issue has not had any activity during the last 90 days. That could mean this issue is no longer relevant and/or nobody has found the necessary time to address the issue. We are trying to keep the list of open issues limited to those issues that are relevant to the majority and to close the ones that have become 'stale' (inactive). If no further activity is detected within the next 14 days, the issue will be closed automatically.
If new comments are are posted and/or a solution (pull request) is submitted for review that references this issue, the issue will not be closed. Closed issues can be reopened at any time in the future. Please remember those participating in this open source project are volunteers trying to help others and creating a better DNN Platform for all. Thank you for your continued involvement and contributions!

mitchelsellers · 2023-04-25T19:18:52Z

Thanks for this contribution, given the time and other shifts these proposed changes have been noted, but a new pull request will be created if necessary to merge elements into the new 10.0 release plan.

SkyeHoefling added 16 commits September 5, 2020 21:29

Created IPortalSettingsManager and ISettingsService

46ae867

Added ISaveSettingsService to abstractions project

a446852

Implemented PortalSettingsManager and PortalSettingsService

b0135cd

Migrated API specific to HostSettings to specific IHostSettingsService

0a1f2d4

Added missing xml comments

753d467

Added deprecation notices to PortalController

66221e7

Added delete all settings implementations

b647cb0

Updated get portal setting and get encrypted portal settings to use I…

8be8c88

…PortalSettingsManager

Implemented delete key for PortalSettings

bfad523

Refeactored ISettingsService to use generics

286fdf8

Updated PortalController to use IPortalSettingsManager

e4f999c

Updated portal settings to reload the settings if the clearcache is true

ab934e8

Removed stale code

6e2740f

Updated XML comments

1280297

Cleaned up IDeleteSettingsService API structure and added XML documen…

64aef2d

…tation

Remvoed default constructor

80f8a4d

SkyeHoefling commented Sep 9, 2020

View reviewed changes

bdukes reviewed Sep 9, 2020

View reviewed changes

valadas added the Status: On Hold label Sep 10, 2020

SkyeHoefling mentioned this pull request Sep 15, 2020

Add ISerializationManager for Dependency Injection #4087

Merged

stale bot added the stale label Mar 5, 2021

bdukes added Alert: Pinned Used to prevent this from becoming stale and removed stale labels Mar 5, 2021

bdukes added this to the Future: Major milestone Jul 27, 2021

valadas modified the milestones: Future: Major, 10.0.0 Jan 19, 2023

mitchelsellers closed this Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IPortalSettingsManager for Abstractions Project #4071

Add IPortalSettingsManager for Abstractions Project #4071

SkyeHoefling commented Sep 9, 2020 •

edited

Loading

SkyeHoefling Sep 9, 2020

bdukes left a comment

bdukes Sep 9, 2020

SkyeHoefling Sep 12, 2020

bdukes Sep 9, 2020

SkyeHoefling Sep 12, 2020

bdukes Sep 14, 2020

SkyeHoefling Sep 14, 2020

bdukes Sep 14, 2020

SkyeHoefling Sep 15, 2020

SkyeHoefling Sep 15, 2020

bdukes Sep 9, 2020

bdukes Sep 9, 2020

bdukes Sep 9, 2020

bdukes Sep 9, 2020

bdukes Sep 9, 2020

bdukes Sep 9, 2020

bdukes Sep 9, 2020

bdukes Sep 9, 2020

SkyeHoefling commented Oct 14, 2020

stale bot commented Mar 5, 2021

mitchelsellers commented Apr 25, 2023

Add IPortalSettingsManager for Abstractions Project #4071

Add IPortalSettingsManager for Abstractions Project #4071

Conversation

SkyeHoefling commented Sep 9, 2020 • edited Loading

Summary

Usage

Missing Items

Required to merge

Choose a reason for hiding this comment

bdukes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SkyeHoefling commented Oct 14, 2020

stale bot commented Mar 5, 2021

mitchelsellers commented Apr 25, 2023

SkyeHoefling commented Sep 9, 2020 •

edited

Loading