#1880 Issue with new SettingService

[leigh-pointer]

Needed to update the SettingService to apply the IsPublic field when updating settings for the ModuleDefinition.

[sbwalker]

Thank you @leigh-pointer ... to provide a bit more background, the SettingService relies on a standard Dictionary type (ie. name/value pairs) to make the API as simple as possible for developers. However this makes it difficult to support additional metadata for settings without breaking the API. So the IsPublic property is "supported" by embedding the metadata into the Dictionary value and then parsing/interpreting the value during load/save. This makes the internal code of the SettingService less intuitive and more complex - but it does shield the consumer of the API from these internal complexities.

The bug you ran into is caused by logic which is trying to be very specific about the specific types of settings which support the IsPublic property. Originally it was introduced for Site Settings - and the use case was that the SMTP details for a site are stored in Site Settings - however that information is sensitive and should not be disclosed to non-Administrators - so there needed to be a mechanism to be able to filter Site Settings for standard users. The IsPublic concept accomplishes this... however there is currently a drawback to the implementation:

The IsPublic concept is currently only enabled for some setting types and not others. This can cause issues if a developer tries to use the SetSetting(...IsPublic) overload method for types other than Site - which is the scenario you ran into for ModuleDefinition. The problem is that the supported types are static - but the SettingService is intended to be dynamic.

One possible solution is to make the IsPublic concept applicable to ALL setting types. This would eliminate the hard-coded logic for checking if a specific type supports it or not. I do not really see a downside to this approach as the concept could be valuable for all types. Basically the service would ensure the user is authorized to access the settings for the specific setting type and entityid - and if so, it would then filter the results.

What are your thoughts?

[leigh-pointer]

@sbwalker I think the approach you are outlining sounds more intuitive and that is always a good thing.