Remove "set" config entries on disabling plugin

Close #241

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 Note: Breaking changes between versions are indicated by "💥".
 
+## Unreleased
+
+- [Improvement] Non-plugin settings added by "set" directives are now automatically removed when the plugin is disabled (#241)
+
 ## v10.2.2 (2020-09-05)
 
 - [Improvement] Add CORS basic configuration to LMS for subdomains of the LMS

tests/test_plugins.py

@@ -3,6 +3,7 @@ from unittest.mock import patch
 
 from tutor import config as tutor_config
 from tutor import exceptions
+from tutor import fmt
 from tutor import plugins
 
 
@@ -53,9 +54,30 @@ class PluginsTests(unittest.TestCase):
 
     def test_disable(self):
         config = {"PLUGINS": ["plugin1", "plugin2"]}
-        plugins.disable(config, "plugin1")
+        with patch.object(fmt, "STDOUT"):
+            plugins.disable(config, "plugin1")
         self.assertEqual(["plugin2"], config["PLUGINS"])
 
+    def test_disable_removes_set_config(self):
+        with patch.object(
+            plugins.Plugins,
+            "iter_enabled",
+            return_value=[
+                plugins.DictPlugin(
+                    {
+                        "name": "plugin1",
+                        "version": "1.0.0",
+                        "config": {"set": {"KEY": "value"}},
+                    }
+                )
+            ],
+        ):
+            config = {"PLUGINS": ["plugin1"], "KEY": "value"}
+            with patch.object(fmt, "STDOUT"):
+                plugins.disable(config, "plugin1")
+            self.assertEqual([], config["PLUGINS"])
+            self.assertNotIn("KEY", config)
+
     def test_patches(self):
         class plugin1:
             patches = {"patch1": "Hello {{ ID }}"}

tutor/commands/plugins.py

@@ -55,10 +55,9 @@ def enable(context, plugin_names):
 @click.pass_obj
 def disable(context, plugin_names):
     config = tutor_config.load_user(context.root)
-    for plugin in plugin_names:
-        plugins.disable(config, plugin)
-        delete_plugin(context.root, plugin)
-        fmt.echo_info("Plugin {} disabled".format(plugin))
+    for plugin_name in plugin_names:
+        plugins.disable(config, plugin_name)
+        delete_plugin(context.root, plugin_name)
 
     tutor_config.save_config_file(context.root, config)
     fmt.echo_info(

tutor/config.py

@@ -122,15 +122,16 @@ def load_plugins(config, defaults):
             if new_key not in config:
                 config[new_key] = env.render_unknown(config, value)
 
-        # Set existing config key/values: here, we do not override existing values
-        for key, value in plugin.config_set.items():
-            if key not in config:
-                config[key] = env.render_unknown(config, value)
-
         # Create new defaults
         for key, value in plugin.config_defaults.items():
             defaults[plugin.config_key(key)] = value
 
+        # Set existing config key/values: here, we do not override existing values
+        # This must come last, as overridden values may depend on plugin defaults
+        for key, value in plugin.config_set.items():
+            if key not in config:
+                config[key] = env.render_unknown(config, value)
+
 
 def is_service_activated(config, service):
     return config["ACTIVATE_" + service.upper()]

tutor/plugins.py

@@ -8,6 +8,7 @@ import pkg_resources
 import appdirs
 
 from . import exceptions
+from . import fmt
 from . import serialize
 
 
@@ -228,8 +229,10 @@ def get_callable_attr(plugin, attr_name, default=None):
 
 
 def is_installed(name):
-    plugin_names = [plugin.name for plugin in iter_installed()]
-    return name in plugin_names
+    for plugin in iter_installed():
+        if name == plugin.name:
+            return True
+    return False
 
 
 def iter_installed():
@@ -248,8 +251,17 @@ def enable(config, name):
 
 
 def disable(config, name):
+    fmt.echo_info("Disabling plugin {}...".format(name))
+    for plugin in Plugins.instance(config).iter_enabled():
+        if name == plugin.name:
+            # Remove "set" config entries
+            for key, value in plugin.config_set.items():
+                config.pop(key, None)
+                fmt.echo_info("    Removed config entry {}={}".format(key, value))
+    # Remove plugin from list
     while name in config[CONFIG_KEY]:
         config[CONFIG_KEY].remove(name)
+    fmt.echo_info("    Plugin disabled")
 
 
 def iter_enabled(config):