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

VP-1558: Merge settings between base and current themes #383

Merged
merged 4 commits into from
Mar 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions VirtoCommerce.LiquidThemeEngine/LiquidThemeEngineOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,31 @@ public class LiquidThemeEngineOptions
public IList<string> TemplatesDiscoveryFolders { get; set; } = new List<string>() { "templates", "snippets", "layout", "assets" };
public string ThemesAssetsRelativeUrl { get; set; } = "~/themes/assets";
public bool RethrowLiquidRenderErrors { get; set; } = false;

/// <summary>
/// The path to the base theme that will be used to discover the theme resources not found by the path of theme for current store.
/// This parameter can be used for theme inheritance logic.
/// Example values:
/// Electronics/default -> wwwroot/cms-content/Themes/Electronics/default
/// default-> wwwroot/cms-content/Themes/default
/// </summary>
public string BaseThemePath { get; set; }

/// <summary>
/// Original description:
/// The name of the base theme that will be used to discover the theme resources not found by the path of theme for current store.
/// This parameter can be used for theme inheritance logic.
/// Example values: default_theme -> wwwroot/cms-content/default_theme
///
/// How it actually worked:
/// Storefront used this parameter as a store name, i.e. Electronics -> wwwroot/cms-content/Themes/Electronics/default
/// </summary>
[Obsolete("Obsolete. Use BaseThemePath instead.")]
public string BaseThemeName { get; set; }

/// <summary>
/// Set to true if you want to merge current theme settings with base theme settings instead of placement
/// </summary>
public bool MergeBaseSettings { get; set; }
}
}
90 changes: 55 additions & 35 deletions VirtoCommerce.LiquidThemeEngine/ShopifyLiquidThemeEngine.cs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,12 @@ public ShopifyLiquidThemeEngine(IStorefrontMemoryCache memoryCache, IWorkContext
private string CurrentThemePath => Path.Combine("Themes", WorkContext.CurrentStore.Id, CurrentThemeName);

//Relative path to the discovery of theme resources that weren't found by the current path.
private string BaseThemePath => !string.IsNullOrEmpty(_options.BaseThemeName) ? Path.Combine("Themes", _options.BaseThemeName, "default") : null;
private string BaseThemePath =>
!string.IsNullOrEmpty(_options.BaseThemePath) ? Path.Combine("Themes", _options.BaseThemePath) :
#pragma warning disable 618
// We need to use obsolete value here for backward compatibility.
!string.IsNullOrEmpty(_options.BaseThemeName) ? Path.Combine("Themes", _options.BaseThemeName, "default") : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

MAJOR Extract this nested ternary operation into an independent statement. rule

#pragma warning restore 618
private string BaseThemeSettingPath => BaseThemePath != null ? Path.Combine(BaseThemePath, "config", "settings_data.json") : null;
public string BaseThemeLocalePath => BaseThemePath != null ? Path.Combine(BaseThemePath, "locales") : null;

Expand Down Expand Up @@ -347,44 +352,28 @@ public ValueTask<string> RenderTemplateAsync(string templateContent, string temp
public IDictionary<string, object> GetSettings(string defaultValue = null)
{
var cacheKey = CacheKey.With(GetType(), "GetSettings", CurrentThemeSettingPath, defaultValue);
return _memoryCache.GetOrCreateExclusive(cacheKey, (cacheItem) =>
return _memoryCache.GetOrCreateExclusive(cacheKey, cacheItem =>
{
cacheItem.AddExpirationToken(new CompositeChangeToken(new[] { ThemeEngineCacheRegion.CreateChangeToken(), _themeBlobProvider.Watch(CurrentThemeSettingPath) }));
var retVal = new Dictionary<string, object>().WithDefaultValue(defaultValue);
//Load all data from current theme config
var resultSettings = InnerGetAllSettings(_themeBlobProvider, CurrentThemeSettingPath);
if (resultSettings == null && BaseThemeSettingPath != null)

JObject result;
var baseThemeSettings = new JObject();
var currentThemeSettings = result = GetCurrentSettingsPreset(InnerGetAllSettings(_themeBlobProvider, CurrentThemeSettingPath));

//Try to load settings from base theme path and merge them with resources for local theme
if ((_options.MergeBaseSettings || currentThemeSettings == null) && !string.IsNullOrEmpty(BaseThemeSettingPath))
{
resultSettings = InnerGetAllSettings(_themeBlobProvider, BaseThemeSettingPath);
cacheItem.AddExpirationToken(new CompositeChangeToken(new[] { ThemeEngineCacheRegion.CreateChangeToken(), _themeBlobProvider.Watch(BaseThemeSettingPath) }));
result = baseThemeSettings = GetCurrentSettingsPreset(InnerGetAllSettings(_themeBlobProvider, BaseThemeSettingPath));
}
if (resultSettings != null)
{
//Get actual preset from merged config
var currentPreset = resultSettings.GetValue("current");
if (currentPreset is JValue)
{
var currentPresetName = ((JValue)currentPreset).Value.ToString();
if (!(resultSettings.GetValue("presets") is JObject presets) || !presets.Children().Any())
{
throw new StorefrontException("Setting presets not defined");
}

IList<JProperty> allPresets = presets.Children().Cast<JProperty>().ToList();
resultSettings = allPresets.FirstOrDefault(p => p.Name == currentPresetName).Value as JObject;
if (resultSettings == null)
{
throw new StorefrontException($"Setting preset with name '{currentPresetName}' not found");
}
}
if (currentPreset is JObject)
{
resultSettings = (JObject)currentPreset;
}

retVal = resultSettings.ToObject<Dictionary<string, object>>().ToDictionary(x => x.Key, x => x.Value).WithDefaultValue(defaultValue);
if (_options.MergeBaseSettings)
{
result = baseThemeSettings;
result.Merge(currentThemeSettings ?? new JObject(), new JsonMergeSettings { MergeArrayHandling = MergeArrayHandling.Merge });
}

return retVal;
return result.ToObject<Dictionary<string, object>>().ToDictionary(x => x.Key, x => x.Value).WithDefaultValue(defaultValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

MAJOR 'result' is null on at least one execution path. rule

});
}

Expand Down Expand Up @@ -475,16 +464,47 @@ private static JObject InnerGetAllSettings(IContentBlobProvider themeBlobProvide
throw new ArgumentNullException(nameof(settingsPath));
}

JObject retVal = null;
var result = new JObject();

if (themeBlobProvider.PathExists(settingsPath))
{
using (var stream = themeBlobProvider.OpenRead(settingsPath))
{
retVal = JsonConvert.DeserializeObject<JObject>(stream.ReadToString());
result = JsonConvert.DeserializeObject<JObject>(stream.ReadToString());
}
}
return retVal;
return result;
}

/// <summary>
/// Get actual preset from config
/// </summary>
/// <returns></returns>
private static JObject GetCurrentSettingsPreset(JObject allSettings)
{
var result = allSettings;
var currentPreset = allSettings.GetValue("current");
if (currentPreset is JValue currentPresetValue)
{
var currentPresetName = currentPresetValue.Value.ToString();
if (!(allSettings.GetValue("presets") is JObject presets) || !presets.Children().Any())
{
throw new StorefrontException("Setting presets not defined");
}

IList<JProperty> allPresets = presets.Children().Cast<JProperty>().ToList();
result = allPresets.FirstOrDefault(p => p.Name == currentPresetName)?.Value as JObject;
if (result == null)
{
throw new StorefrontException($"Setting preset with name '{currentPresetName}' not found");
}
}
if (currentPreset is JObject preset)
{
result = preset;
}

return result;
}

private string ReadTemplateByPath(string templatePath)
Expand Down
10 changes: 6 additions & 4 deletions VirtoCommerce.Storefront/appsettings.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,12 @@
},
"LiquidThemeEngine": {
"RethrowLiquidRenderErrors": false,
//The name of the base theme that will be used to discover the theme resources not found by the path of theme for current store.
//This parameter can be used for theme inheritance logic.
// Example values: 'default_theme' will map to this path 'wwwroot/cms-content/default_theme'
"BaseThemeName": ""
// The path to the base theme that will be used to discover the theme resources not found by the path of theme for current store.
// This parameter can be used for theme inheritance logic.
// Example values: Electronics/default_theme -> wwwroot/cms-content/Themes/Electronics/default_theme
"BaseThemePath": "",
// Set to true if you want to merge current theme settings with base theme settings instead of placement
"MergeBaseSettings": false
},
"RequireHttps": {
"Enabled": false,
Expand Down