Fix UpdateModuleSetting in ModuleController

[engineering87]

Fix the 'UpdateModuleSetting' method in ModuleController refactoring the update module conditions.
Issue #2868

Summary

Modified the update conditions, removed the 'settingExist' variable and check directly the settings value.

[bdukes]

This change seems fine, but I don't see how it would fix #2868. That issue is that updating an existing settings doesn't have an effect, but this code does the exact same existValue != settingValue check in that scenario, right? I'm probably missing a piece to understanding this change. Do you have a sample module you've been testing with which demonstrates the fix works?

[engineering87]

Hi Brian,
I tested the change with a simple test module defined in this way:

protected void Page_Load(object sender, EventArgs e)
        {
            try
            {
                const string key = "TestProperty";
                ModuleController.Instance.UpdateModuleSetting(ModuleContext.ModuleId, key, "False");
                var moduleInfo = ModuleController.Instance.GetModule(ModuleContext.ModuleId, ModuleContext.TabId, true);
                var value = moduleInfo.ModuleSettings[key];
                if (value != null && value.ToString() == "False")
                {
                    ModuleController.Instance.UpdateModuleSetting(ModuleContext.ModuleId, key, "True");
                    moduleInfo = ModuleController.Instance.GetModule(ModuleContext.ModuleId, ModuleContext.TabId, true);
                    value = moduleInfo.ModuleSettings[key];
                    if (value == null || value.ToString() == "False")
                    {
                        throw new ModuleLoadException("Second update failed");
                    }
                }
                else
                {
                    throw new ModuleLoadException("First insert/update failed");
                }
            }
            catch (Exception exc) //Module failed to load
            {
                Exceptions.ProcessModuleLoadException(this, exc);
            }
        }

the TestProperty is correctly set in the ModuleSettings table.

Actually I haven't tried the same code with the unpatched version yet but reading the issue it seems that the problem is consistent. Regarding the original code, the second condition in 'else if' was executed only when the 'settingValue' was null.

[bdukes]

I tried your test code and it appears to work correctly on 9.4.3 (without this patch). @mitchelsellers were you testing a different scenario in #2868?

[mitchelsellers]

@bdukes This appears to have fixed itself from when I initially posted it as well, as I just validated that I have modules working with 9.4.1 and 9.4.2 that do not experience the issue in #2868

[bdukes]

@engineering87 had you run into this issue in practice, or were you just tackling it based on @mitchelsellers' issue #2868? It would be great if we could isolate and reproduce the real issue to make sure any change we make is truly helpful.

[engineering87]

Actually just tackling it based on the #2868 issue but I will make further checks and tests to try to replicate what has been reported.

[mitchelsellers]

From what I can tell. It appears that this is no longer an issue. It was recreatable when I submitted the issue, but I cannot now.

[donker]

I actually find this code proposal better than what was there. It's more logical: first check if the value was previously set and only then check for a changed value, not the other way around.

[valadas]

Looks good to me