From f5f501dad06cb61091b43d729cdbd83e7b21b859 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Behmo?= <regis@behmo.com>
Date: Fri, 18 Sep 2020 12:39:22 +0200
Subject: [PATCH] Remove "set" config entries on disabling plugin

Close #241
---
 CHANGELOG.md              |  4 ++++
 tests/test_plugins.py     | 24 +++++++++++++++++++++++-
 tutor/commands/plugins.py |  7 +++----
 tutor/config.py           | 11 ++++++-----
 tutor/plugins.py          | 16 ++++++++++++++--
 5 files changed, 50 insertions(+), 12 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8c1e1ec..5d9745a 100644
--- a/CHANGELOG.md
+++ b/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
diff --git a/tests/test_plugins.py b/tests/test_plugins.py
index e6e167c..c874fe1 100644
--- a/tests/test_plugins.py
+++ b/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 }}"}
diff --git a/tutor/commands/plugins.py b/tutor/commands/plugins.py
index 7fc9362..be2ee1d 100644
--- a/tutor/commands/plugins.py
+++ b/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(
diff --git a/tutor/config.py b/tutor/config.py
index a50c2e8..1d6fc82 100644
--- a/tutor/config.py
+++ b/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()]
diff --git a/tutor/plugins.py b/tutor/plugins.py
index 51adef9..f2ba978 100644
--- a/tutor/plugins.py
+++ b/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):