feat: display launch warning on config change

To achieve that, we had to get rid of the seq_command_executed request
parameter. We replaced it by a cookie, which is cleared after the
warning is displayed.

Note that we had to remove a feature, which was to disable/enable all
inputs while commands were running. This was causing very weird
behaviour, where some disabled buttons were being re-enabled again.

We also had to get rid of the cookieStore, which is not compatible with
non-https access. Http access is required in the self-serve AMI, for
instance.

pyproject.toml

@@ -39,10 +39,10 @@ dynamic = ["version"]
 
 [project.optional-dependencies]
 dev = [
-  "tutor[dev]>=20.0.0,<21.0.0", 
+  "tutor[dev]>=20.0.0,<21.0.0",
   "types-aiofiles",
   "types-Markdown",
-  "pylint", 
+  "pylint",
   "black",
 ]
 

tutordeck/server/app.py

@@ -152,14 +152,16 @@ async def configuration_update() -> BaseResponse:
     """
     await process_config_update_request()
 
-    # Handle non-ajax call
+    response: BaseResponse
-    next_url = request.args.get("next", "")
+    if next_url := request.args.get("next", ""):
-    if next_url:
+        # Handle non-ajax call
-        return redirect(next_url)
+        response = redirect(next_url)
+    else:
+        # Handle ajax call
+        response = Response("", status=200, content_type="text/html")
+        response.headers["HX-Redirect"] = url_for("configuration")
 
-    # Handle ajax call
+    notify_run_sequential(response)
-    response = Response("", status=200, content_type="text/html")
-    response.headers["HX-Redirect"] = url_for("configuration")
     return response
 
 
@@ -260,9 +262,6 @@ async def plugin(name: str) -> Response:
     if not index_entry and name not in g.installed_plugins:
         return Response("Plugin not found", status=404)
 
-    # TODO this seq_command_executed argument is confusing and causing issues, for
-    # instance with the "unset" button. We need to get rid of it.
-    seq_command_executed = request.args.get("seq_command_executed")
     description = markdown(index_entry.description) if index_entry else ""
     rendered_template = await render_template(
         "plugin.html",
@@ -273,18 +272,15 @@ async def plugin(name: str) -> Response:
             tutorclient.Client.get_plugin_author(index_entry) if index_entry else ""
         ),
         plugin_description=description,
-        seq_command_executed=seq_command_executed,
         plugin_config_unique=tutorclient.Client.plugin_config_unique(name),
         plugin_config_defaults=tutorclient.Client.plugin_config_defaults(name),
         user_config=tutorclient.Project.get_user_config(),
     )
 
     # Redirect to plugin page
-    # TODO this is useful only after a POST to plugin/<name>/update. I don't think these two things should be handled in the same place.
     response = Response(rendered_template, status=200, content_type="text/html")
-    response.headers["HX-Redirect"] = url_for(
+
-        "plugin", name=name, seq_command_executed=seq_command_executed
+    response.headers["HX-Redirect"] = url_for("plugin", name=name)
-    )
     return response
 
 
@@ -305,16 +301,9 @@ async def plugin_toggle(name: str) -> Response:
 
     response = t.cast(
         Response,
-        await make_response(
+        await make_response(redirect(url_for("plugin", name=name))),
-            redirect(
-                url_for(
-                    "plugin",
-                    name=name,
-                    seq_command_executed=True,
-                )
-            )
-        ),
     )
+    notify_run_sequential(response)
     if enable_plugin:
         update_plugins_requiring_launch(response, add=name)
     else:
@@ -362,17 +351,10 @@ async def plugin_config_update(name: str) -> Response:
     await process_config_update_request()
     response = t.cast(
         Response,
-        await make_response(
+        await make_response(redirect(url_for("plugin", name=name))),
-            redirect(
-                url_for(
-                    "plugin",
-                    name=name,
-                    seq_command_executed=True,
-                )
-            )
-        ),
     )
     update_plugins_requiring_launch(response, add=name)
+    notify_run_sequential(response)
     return response
 
 
@@ -496,6 +478,13 @@ async def command() -> BaseResponse:
     return redirect(url_for("advanced"))
 
 
+def notify_run_sequential(response: BaseResponse) -> None:
+    """
+    Notify the frontend that a sequential command was run.
+    """
+    set_cookie(response, constants.COMMAND_EXECUTED_COOKIE_NAME, "1")
+
+
 def update_plugins_requiring_launch(
     response: Response, add: t.Optional[str] = None, remove: t.Optional[str] = None
 ) -> None:
@@ -527,8 +516,15 @@ def update_plugins_requiring_launch(
         names.discard(remove)
 
     # Update the response
-    response.set_cookie(
+    set_cookie(
+        response,
         constants.PLUGINS_REQUIRE_LAUNCH_COOKIE_NAME,
         separator.join(sorted(names)),
-        max_age=60 * 60 * 24 * 30,  # 1 month
     )
+
+
+def set_cookie(response: BaseResponse, name: str, value: str) -> None:
+    """
+    Set a cookie with a consistent expiry time.
+    """
+    response.set_cookie(name, value, max_age=60 * 60 * 24 * 30)  # 1 month

tutordeck/server/constants.py

@@ -1,3 +1,4 @@
 SHORT_SLEEP_SECONDS = 0.1
 PLUGINS_REQUIRE_LAUNCH_COOKIE_NAME = "plugins-require-launch"
+COMMAND_EXECUTED_COOKIE_NAME = "command-executed"
 ITEMS_PER_PAGE = 100

tutordeck/server/static/js/deck.js

@@ -1,9 +1,27 @@
+// Cookie utilities
+// We can't use the cookieStore because we might want to access tutor deck in http mode,
+// where it is not available.
+function getCookie(name) {
+	let nameEQ = name + "=";
+	return (
+		document.cookie
+			.split(";")
+			.map((cookie) => cookie.trim())
+			.find((cookie) => cookie.startsWith(nameEQ))
+			?.slice(nameEQ.length) || null
+	);
+}
+function eraseCookie(name) {
+	document.cookie =
+		name + "=; Path=/; Expires=Thu, 01 Jan 1970 00:00:01 GMT;";
+}
+
 // Handle plugins requiring launch based on the values in the corresponding cookie
 const pluginsRequireLaunchCookieName = "plugins-require-launch";
-async function displayPluginsRequireLaunchWarning() {
+function displayPluginsRequireLaunchWarning() {
-	const cookie = await cookieStore.get(pluginsRequireLaunchCookieName);
+	const cookie = getCookie(pluginsRequireLaunchCookieName);
-	if (cookie && cookie.value) {
+	if (cookie) {
-		const cookieValue = cookie.value.slice(1, -1); // remove quotes
+		const cookieValue = cookie.slice(1, -1); // remove quotes
 		cookieValue.split('+').map(s => s.trim()).forEach(plugin => {
 			document.querySelectorAll(`[data-plugin="${plugin}"] .warning-launch-required`).forEach(element => {
 				element.classList.add("visible");
@@ -41,7 +59,7 @@ function showLaunchSuccessfulToast() {
 	// TODO this is very brittle because it relies on static variables and string values.
 	if (toast) {
 		if (toastTitle === "Launch platform was successfully executed") {
-			cookieStore.delete(pluginsRequireLaunchCookieName);
+			eraseCookie(pluginsRequireLaunchCookieName);
 		}
 		toast.style.display = "flex";
 		setTimeout(() => {
@@ -59,35 +77,33 @@ function hideToast() {
 	}
 }
 
+const launchDescription = "To apply the changes, run Launch Platform. This will update your platform and may take a few minutes to complete.";
 const TOAST_CONFIGS = {
-	"tutor plugins enable": {
-		title: "Your plugin was successfully enabled",
-		description:
-			"To apply the changes, run Launch Platform. This will update your platform and may take a few minutes to complete.",
-		showFooter: true,
-	},
-	"tutor plugins upgrade": {
-		title: "Your plugin was successfully updated",
-		description:
-			"To apply the changes, run Launch Platform. This will update your platform and may take a few minutes to complete.",
-		showFooter: true,
-	},
-	"tutor plugins install": {
-		title: "Plugin Installed Successfully",
-		description: "Enable it now to start using its features",
-		showFooter: false,
-	},
 	"tutor config save": {
-		title: "You have successfully modified parameters",
+		title: "Configuration parameters were updated",
-		description:
+		description: launchDescription,
-			"To apply the changes, run Launch Platform. This will update your platform and may take a few minutes to complete.",
 		showFooter: true,
 	},
 	"tutor local launch": {
-		title: "Launch platform was successfully executed",
+		title: "Platform was launched",
 		description: "",
 		showFooter: false,
 	},
+	"tutor plugins install": {
+		title: "Plugin was installed",
+		description: "Enable it now to start using its features",
+		showFooter: false,
+	},
+	"tutor plugins enable": {
+		title: "Plugin was enabled",
+		description: launchDescription,
+		showFooter: true,
+	},
+	"tutor plugins upgrade": {
+		title: "Plugin was updated",
+		description: launchDescription,
+		showFooter: true,
+	},
 };
 let toastTitle = document.getElementById("toast-title");
 let toastDescription = document.getElementById("toast-description");
@@ -105,38 +121,7 @@ function setToastContent(cmd) {
 }
 
 // Each page defines its own relevant commands, we use them to check
-// if the currently running commands belong the currently opened page or not
+// if the currently running commands belong the currently opened page or not.
-let relevantCommands = [];
+// A "*" relevant command matches all possible commands.
-let onDeveloperPage = false;
+let tutorCommandsToWatch = [];
-function onRelevantPage(command) {
-	if (onDeveloperPage) {
-		// Developer page is relevant to all commands
-		return true;
-	}
-	return relevantCommands.some((prefix) => command.startsWith(prefix));
-}
 
-function activateInputs() {
-	document.querySelectorAll("button").forEach((button) => {
-		button.disabled = false;
-	});
-	document.querySelectorAll("input").forEach((input) => {
-		input.disabled = false;
-	});
-	document.querySelectorAll(".form-switch").forEach((formSwitch) => {
-		formSwitch.style.opacity = 1;
-	});
-	document.getElementById("warning-command-running").style.display = "none";
-}
-function deactivateInputs() {
-	document.querySelectorAll("button").forEach((button) => {
-		button.disabled = true;
-	});
-	document.querySelectorAll("input").forEach((input) => {
-		input.disabled = true;
-	});
-	document.querySelectorAll(".form-switch").forEach((formSwitch) => {
-		formSwitch.style.opacity = 0.5;
-	});
-	document.getElementById("warning-command-running").style.display = "flex";
-}

tutordeck/server/static/js/logs.js

@@ -7,53 +7,81 @@
 // Each page that uses logs defines its own command execution/cancellation toggle functions with the same signature
 // We can safely call these functions and their functionality will be handeled by the page specific js
 
+// Scrolling management
 let shouldAutoScroll = true;
 let isScrollingProgrammatically = false;
 // When user manually scrolls, update behaviour
-logsElement.addEventListener("scroll", function () {
+logsElement.addEventListener(
-	if (!isScrollingProgrammatically) {
+	"scroll",
-		shouldAutoScroll = false;
+	function () {
+		if (!isScrollingProgrammatically) {
+			shouldAutoScroll = false;
+		}
 	}
-});
+);
-let executedNewCommand = false;
+logsElement.addEventListener(
+	"wheel",
+	function () {
+		shouldAutoScroll = false;
+	},
+	{ passive: true }
+);
+logsElement.addEventListener(
+	"touchstart",
+	function () {
+		shouldAutoScroll = false;
+	},
+	{ passive: true }
+);
+
+let commandExecuted = false;
+function checkAndClearCommandExecuted() {
+	const commandExecutedCookieName = "command-executed";
+	if (getCookie(commandExecutedCookieName)) {
+		eraseCookie(commandExecutedCookieName);
+		commandExecuted = true;
+	}
+};
+checkAndClearCommandExecuted();
+
+let threadWasAlive = false;
 htmx.on("htmx:sseBeforeMessage", function (evt) {
 	// Don't swap content, we want to append
 	evt.preventDefault();
-
 	const data = JSON.parse(evt.detail.data);
-	const command = data.command;
+	evt.detail.elt.appendChild(document.createTextNode(data.stdout));
 
 	// This means a parallel command is executing
 	if (data.thread_alive) {
+		threadWasAlive = true;
 		// Check if we are on the same page on which the actual command was executed
-		// Each page defines its relevant commands which are sent to `onRelevantPage` function to check if we are on the relevant page
+		// Each page defines its relevant commands that are monitored and that will trigger a display of the log window.
-		if (onRelevantPage(command)) {
+		let shouldDisplayLogs = tutorCommandsToWatch.some(
+			(prefix) => prefix === "*" || data.command.startsWith(prefix)
+		);
+		if(shouldDisplayLogs) {
 			ShowCancelCommandButton();
 			logsElement.style.display = "block";
 		} else {
 			// If we are not on relevant page we don't show the cancel button and disable all inputs
-			deactivateInputs();
+			onCommandRunning();
 		}
-		executedNewCommand = true;
 	}
 
-	const parallelCommandCompleted = executedNewCommand && !data.thread_alive;
+	// A parallel command was running, and now it's completed
-
+	const parallelCommandCompleted = threadWasAlive && !data.thread_alive;
-	// TODO this is a very brittle way of checking that we are on a plugin page... Let's not use static variables. Same for sequentialCommandExecuted.
+	// TODO this is a very brittle way of checking that we are on a plugin page... Let's not use static variables.
 	const onPluginPage = typeof pluginName !== "undefined";
 	// Note that sequential commands are only executed on the plugins page
 	// Refreshing the page will run this block again
 	// Because there is no way to determine if its a newly executed sequential command or an old one
-	if (
+	if (parallelCommandCompleted || commandExecuted) {
-		parallelCommandCompleted ||
+		onCommandComplete();
-		(onPluginPage && sequentialCommandExecuted)
-	) {
-		activateInputs();
 		// There are certain commands for which we do not show the toast message
 		// Only show the toast if it was set in the `setToastContent` function and if the command ran successfully
 		// TODO this is brittle because it relies on a hard-coded "Success!" string that is sent from the backend.
 		if (data.stdout.includes("Success!")) {
-			setToastContent(command);
+			setToastContent(data.command);
 			if (toastTitle.textContent.trim()) {
 				showLaunchSuccessfulToast();
 			}
@@ -70,9 +98,10 @@ htmx.on("htmx:sseBeforeMessage", function (evt) {
 			ShowRunCommandButton();
 		}
 	}
-	evt.detail.elt.appendChild(document.createTextNode(data.stdout));
+
+	// Scrolling management
 	if (shouldAutoScroll) {
-		// Set flag so event listner knows we are scrolling programatically
+		// Set flag so event listener knows we are scrolling programmatically
 		isScrollingProgrammatically = true;
 		evt.detail.elt.scrollTop = evt.detail.elt.scrollHeight;
 
@@ -83,19 +112,32 @@ htmx.on("htmx:sseBeforeMessage", function (evt) {
 	}
 });
 
-// Additional handlers for scroll inputs
+// TODO we removed all code in these functions, which was too extensive. We should now clean this up.
-logsElement.addEventListener(
+function onCommandComplete() {
-	"wheel",
+	// // This is to enable "cancel" buttons. But as a side effect, it's causing all disable inputs to be
-	function () {
+	// // enabled, which is an error in some cases (e.g: "unset buttons")
-		shouldAutoScroll = false;
+	// document.querySelectorAll("button").forEach((button) => {
-	},
+	// 	button.disabled = false;
-	{ passive: true }
+	// });
-);
+	// document.querySelectorAll("input").forEach((input) => {
-
+	// 	input.disabled = false;
-logsElement.addEventListener(
+	// });
-	"touchstart",
+	// document.querySelectorAll(".form-switch").forEach((formSwitch) => {
-	function () {
+	// 	formSwitch.style.opacity = 1;
-		shouldAutoScroll = false;
+	// });
-	},
+	document.body.classList.remove("command-running");
-	{ passive: true }
+}
-);
+function onCommandRunning() {
+	// // This is to prevent running additional commands at the same time as a long-running
+	// // command. But it's way too extensive. We shouldn't disable ALL inputs.
+	// document.querySelectorAll("button").forEach((button) => {
+	// 	button.disabled = true;
+	// });
+	// document.querySelectorAll("input").forEach((input) => {
+	// 	input.disabled = true;
+	// });
+	// document.querySelectorAll(".form-switch").forEach((formSwitch) => {
+	// 	formSwitch.style.opacity = 0.5;
+	// });
+	document.body.classList.add("command-running");
+}

tutordeck/server/static/scss/deck.scss

@@ -364,10 +364,8 @@ main {
 				}
 			}
 			#warning-command-running {
-				display: flex;
 				justify-content: center;
 				align-items: center;
-				display: none;
 				border: 1px solid $gray-2;
 				border-radius: 0.5em;
 				align-items: center;
@@ -1009,3 +1007,13 @@ main {
 		}
 	}
 }
+
+// Show/Hide/Disable elements when commands are running or are complete
+.show-on-command-running {
+	display: none;
+}
+.command-running {
+	.show-on-command-running {
+		display: flex;
+	}
+}

tutordeck/server/templates/advanced.html

@@ -30,7 +30,7 @@ Search for any tutor command and execute it with a single click.
 
 {% block scripts %}
 <script>
-    onDeveloperPage = true;
+    tutorCommandsToWatch = ["*"]; // watch all commands
     logsElement.style.display = "block";
     runCommandButton = document.querySelector('.run-command-button')
     cancelCommandButton = document.querySelector('.cancel-command-button') 
@@ -61,7 +61,7 @@ Search for any tutor command and execute it with a single click.
                 },
                 body: JSON.stringify({ command })
             });
-            
+
             const suggestions = await response.json();
 
             // Display suggestions

tutordeck/server/templates/configuration.html

@@ -16,3 +16,13 @@
 
 <script src="{{ url_for('static', filename='js/config.js') }}"></script>
 {% endblock %}
+
+{% block scripts %}
+<script>
+    tutorCommandsToWatch = ["tutor config save"];
+    function ShowRunCommandButton() {
+        // TODO we shouldn't have to implement dummy functions. We need to get rid of
+        // these imperative function calls.
+    }
+</script>
+{% endblock %}

tutordeck/server/templates/index.html

@@ -54,7 +54,7 @@
 
         <section>
             <header>
-                <div id="warning-command-running">
+                <div id="warning-command-running" class="show-on-command-running">
                     <img src="{{ url_for('static', filename='img/Featured icon.svg')}}" alt="">
                     <span>Command execution in progress. <a href="{{ url_for('advanced') }}">Click here</a> to view details.</span>
                 </div>

tutordeck/server/templates/local_launch.html

@@ -24,7 +24,7 @@ This will run Launch Platform to apply all plugin changes. This may take a few m
 
 {% block scripts %}
 <script>
-    relevantCommands = ["tutor local launch --non-interactive"];
+    tutorCommandsToWatch = ["tutor local launch"];
     localLaunchButton = document.getElementById('local-launch-button')
     cancelLocalLaunchButton = document.getElementById('cancel-local-launch-button') 
     const toggleButtons = ({run = false, cancel = false} = {}) => {

tutordeck/server/templates/plugin.html

@@ -52,12 +52,11 @@
     pluginName = '{{ plugin_name }}';
     isPluginInstalled = '{{ is_installed }}' === 'True';
     isPluginEnabled = '{{ is_enabled }}' === 'True';
-    sequentialCommandExecuted = '{{ seq_command_executed }}' === 'True';
     pluginUpgradeButton = document.getElementById('plugin-upgrade-button');
     pluginInstallButton = document.getElementById('plugin-install-button');
     cancelCommandButton = document.getElementById('cancel-command-button');
 
-    relevantCommands = ["tutor plugins install", "tutor plugins upgrade"];
+    tutorCommandsToWatch = ["tutor plugins install", "tutor plugins upgrade"];
 
     const toggleButtons = ({install = false, upgrade = false, cancel = false} = {}) => {
         pluginInstallButton.style.display = install ? 'block' : 'none';