J5: test the Ansible scope history, and stop showing unrecorded values as null

crates/private-server/src/fns/ansible_variables.rs

@@ -161,6 +161,10 @@ pub struct HistoryEntry {
 	/// The plain value it set or removed.
 	#[schema(value_type = Option<serde_json::Value>)]
 	pub value: Option<Value>,
+	/// Whether its entry holds the plain value it set or removed, so a value of
+	/// null is told from one the entry never recorded, as an entry written before
+	/// entries held values does not.
+	pub value_recorded: bool,
 	/// The plain value a set replaced.
 	#[schema(value_type = Option<serde_json::Value>)]
 	pub replaced: Option<Value>,
@@ -555,7 +559,9 @@ async fn history_entry(
 	let secret = flag("secret");
 	let replaced = plain("replaced");
 	let replaced_secret = flag("replaced_secret");
-	let value = plain("value");
+	// A plain value is the entry's own, null included, where the entry holds one.
+	let value = (!secret).then(|| detail.get("value").cloned()).flatten();
+	let value_recorded = value.is_some();
 	let put_away_secret = match change {
 		Change::Set => replaced_secret,
 		Change::Removed => secret,
@@ -565,7 +571,7 @@ async fn history_entry(
 	} else {
 		match change {
 			Change::Set => replaced.is_some(),
-			Change::Removed => value.is_some(),
+			Change::Removed => value_recorded,
 		}
 	};
 	Ok(HistoryEntry {
@@ -575,6 +581,7 @@ async fn history_entry(
 		change,
 		secret,
 		value,
+		value_recorded,
 		replaced,
 		replaced_secret,
 		restored_from: detail
@@ -811,10 +818,16 @@ pub async fn restore(
 				Change::Removed => change.value,
 			};
 			let value = put_away.ok_or_else(|| {
-				AppError::BadRequest(format!(
-					"change {} replaced nothing, so there is nothing to put back",
-					args.entry_id
-				))
+				AppError::BadRequest(match change.change {
+					Change::Set => format!(
+						"change {} replaced nothing, so there is nothing to put back",
+						args.entry_id
+					),
+					Change::Removed => format!(
+						"change {} does not record the value it removed, so there is nothing to put back",
+						args.entry_id
+					),
+				})
 			})?;
 			(value, false)
 		}

crates/private-server/tests/it/ansible_variables.rs

@@ -1273,18 +1273,36 @@ async fn a_secret_change_is_undone_while_its_value_is_held() {
 }
 
 /// A scope's history holds every change to a variable set there, removed ones
-/// among them and however long ago within the log, and nothing of another
-/// scope's, though the group's record names its environments' too.
+/// among them: one set again since, and one removed long enough ago to be off
+/// the list of recent removals. It holds nothing of another scope's, though the
+/// group's record names its environments' variables too.
 // spec: ANSVAR#presentation
 #[tokio::test(flavor = "multi_thread")]
 async fn a_scopes_history_holds_its_removed_variables_and_no_other_scopes() {
 	commons_tests::server::run(async move |mut conn, _public, private| {
 		let group = insert_group(&mut conn, "kamaka").await;
-		insert_application(&mut conn, group, "kamaka-central", "tamanu-central", None).await;
+		let (_, machine) = insert_application_on_its_own_machine(
+			&mut conn,
+			group,
+			"kamaka-central",
+			"tamanu-central",
+			None,
+			None,
+		)
+		.await;
 		let environment = json!({ "server_group_id": group, "rank": "dev" });
 		let group_scope = json!({ "server_group_id": group });
+		let machine_scope = json!({ "machine_id": machine });
 		set_var(&private, environment.clone(), "retries", json!(3), false).await;
 		set_var(&private, environment.clone(), "kept", json!(true), false).await;
+		set_var(&private, machine_scope.clone(), "mine", json!(1), false).await;
+		post(
+			&private,
+			"/api/ansible_variables/remove",
+			named(&environment, "retries"),
+		)
+		.await;
+		set_var(&private, environment.clone(), "retries", json!(4), false).await;
 		set_var(
 			&private,
 			group_scope.clone(),
@@ -1293,26 +1311,38 @@ async fn a_scopes_history_holds_its_removed_variables_and_no_other_scopes() {
 			false,
 		)
 		.await;
-		post(
-			&private,
-			"/api/ansible_variables/remove",
-			named(&environment, "retries"),
-		)
-		.await;
-		// Removed long enough ago to be off the list of recent removals.
+
+		// A removal a month old, in a week laid out as provisioning lays them
+		// out: the log is never updated, so it is written there in the first place.
 		conn.batch_execute(
-			"UPDATE audit_entries SET recorded_at = recorded_at - interval '30 days'
-			 WHERE action = 'inventory_variable_removed'",
+			"CREATE TABLE audit_entries_old PARTITION OF audit_entries \
+			   FOR VALUES FROM (now() - interval '40 days') TO (now() - interval '20 days') \
+			   PARTITION BY LIST (retention); \
+			 CREATE TABLE audit_entries_old_standard PARTITION OF audit_entries_old FOR VALUES IN ('standard'); \
+			 CREATE TABLE audit_entries_old_year PARTITION OF audit_entries_old FOR VALUES IN ('year'); \
+			 INSERT INTO audit_entries (recorded_at, retention, actor_kind, actor_id, actor_name, \
+			   action, records, detail, outcome, settled_at) \
+			 SELECT recorded_at - interval '30 days', retention, actor_kind, actor_id, actor_name, action, \
+			   (SELECT jsonb_agg(CASE WHEN r->>'name' = 'retries' \
+			      THEN r || jsonb_build_object('id', replace(r->>'id', ':retries', ':ancient'), 'name', 'ancient') \
+			      ELSE r END) FROM jsonb_array_elements(records) AS r), \
+			   detail || '{\"value\": \"old\"}', outcome, settled_at - interval '30 days' \
+			 FROM audit_entries WHERE action = 'inventory_variable_removed'",
 		)
 		.await
 		.unwrap();
+
 		let recent = post(
 			&private,
 			"/api/ansible_variables/removed",
 			environment.clone(),
 		)
 		.await;
-		assert_eq!(recent, json!([]));
+		assert_eq!(
+			recent,
+			json!([]),
+			"one is set again, the other removed too long ago"
+		);
 
 		let history = post(
 			&private,
@@ -1333,18 +1363,26 @@ async fn a_scopes_history_holds_its_removed_variables_and_no_other_scopes() {
 			.collect();
 		assert_eq!(
 			changes,
-			[("kept", "set"), ("retries", "set"), ("retries", "removed")],
+			[
+				("retries", "set"),
+				("retries", "removed"),
+				("kept", "set"),
+				("retries", "set"),
+				("ancient", "removed"),
+			],
 			"newest first, the group's own changes left out"
 		);
-		let removal = &history[2];
-		assert_eq!(removal["value"], 3);
-		assert_eq!(removal["undoable"], true);
+		assert_eq!(history[1]["value"], 3);
+		let ancient = &history[4];
+		assert_eq!(ancient["value"], "old");
+		assert_eq!(ancient["undoable"], true);
 
-		let mut restore = named(&environment, "retries");
-		restore["entry_id"] = removal["entry_id"].clone();
+		let mut restore = named(&environment, "ancient");
+		restore["entry_id"] = ancient["entry_id"].clone();
 		post(&private, "/api/ansible_variables/restore", restore).await;
 		let body = read_served(&private, json!({ "server_group_id": group })).await;
-		assert_eq!(body["vars"]["retries"], 3);
+		assert_eq!(body["vars"]["ancient"], "old");
+		assert_eq!(body["vars"]["retries"], 4);
 
 		let at_group = post(
 			&private,
@@ -1354,6 +1392,129 @@ async fn a_scopes_history_holds_its_removed_variables_and_no_other_scopes() {
 		.await;
 		assert_eq!(at_group.as_array().unwrap().len(), 1);
 		assert_eq!(at_group[0]["name"], "timezone");
+		let at_machine = post(
+			&private,
+			"/api/ansible_variables/scope_history",
+			machine_scope,
+		)
+		.await;
+		assert_eq!(at_machine.as_array().unwrap().len(), 1);
+		assert_eq!(at_machine[0]["name"], "mine");
+	})
+	.await
+}
+
+/// A group's recent removals are found among its own entries, so a busy
+/// environment's removals, which name the group too, do not crowd them out.
+// spec: ANSVAR#presentation
+#[tokio::test(flavor = "multi_thread")]
+async fn a_busy_environment_does_not_crowd_out_its_groups_removals() {
+	commons_tests::server::run(async move |mut conn, _public, private| {
+		let group = insert_group(&mut conn, "kamaka").await;
+		insert_application(&mut conn, group, "kamaka-central", "tamanu-central", None).await;
+		let environment = json!({ "server_group_id": group, "rank": "dev" });
+		let group_scope = json!({ "server_group_id": group });
+		set_var(
+			&private,
+			group_scope.clone(),
+			"timezone",
+			json!("UTC"),
+			false,
+		)
+		.await;
+		post(
+			&private,
+			"/api/ansible_variables/remove",
+			named(&group_scope, "timezone"),
+		)
+		.await;
+		set_var(&private, environment.clone(), "retries", json!(3), false).await;
+		post(
+			&private,
+			"/api/ansible_variables/remove",
+			named(&environment, "retries"),
+		)
+		.await;
+		// More of the environment's removals since than a page of the group's
+		// entries holds.
+		conn.batch_execute(
+			"INSERT INTO audit_entries (recorded_at, retention, actor_kind, actor_id, actor_name, \
+			   action, records, detail, outcome, settled_at) \
+			 SELECT recorded_at + n * interval '1 millisecond', retention, actor_kind, actor_id, \
+			   actor_name, action, records, detail, outcome, settled_at \
+			 FROM audit_entries, generate_series(1, 600) AS n \
+			 WHERE action = 'inventory_variable_removed' \
+			   AND records @> '[{\"name\": \"retries\"}]'",
+		)
+		.await
+		.unwrap();
+
+		let at_group = post(&private, "/api/ansible_variables/removed", group_scope).await;
+		assert_eq!(at_group.as_array().unwrap().len(), 1);
+		assert_eq!(at_group[0]["name"], "timezone");
+		let at_environment = post(&private, "/api/ansible_variables/removed", environment).await;
+		assert_eq!(at_environment.as_array().unwrap().len(), 1);
+		assert_eq!(at_environment[0]["name"], "retries");
+	})
+	.await
+}
+
+/// A change whose entry never recorded its value is told from one that
+/// recorded a value of null: the first has nothing to put back, the second puts
+/// back null.
+// spec: ANSVAR#history
+#[tokio::test(flavor = "multi_thread")]
+async fn a_change_without_its_value_is_told_from_one_of_null() {
+	commons_tests::server::run(async move |mut conn, _public, private| {
+		let group = insert_group(&mut conn, "kamaka").await;
+		insert_application(&mut conn, group, "kamaka-central", "tamanu-central", None).await;
+		let environment = json!({ "server_group_id": group, "rank": "dev" });
+		set_var(&private, environment.clone(), "proxy", json!(null), false).await;
+		post(
+			&private,
+			"/api/ansible_variables/remove",
+			named(&environment, "proxy"),
+		)
+		.await;
+		// The same removal as an entry holding no value, as one written before
+		// entries held them.
+		conn.batch_execute(
+			"INSERT INTO audit_entries (recorded_at, retention, actor_kind, actor_id, actor_name, \
+			   action, records, detail, outcome, settled_at) \
+			 SELECT recorded_at + interval '1 millisecond', retention, actor_kind, actor_id, \
+			   actor_name, action, records, detail - 'value', outcome, settled_at \
+			 FROM audit_entries WHERE action = 'inventory_variable_removed'",
+		)
+		.await
+		.unwrap();
+
+		let history = post(
+			&private,
+			"/api/ansible_variables/history",
+			named(&environment, "proxy"),
+		)
+		.await;
+		let [unrecorded, of_null, _set] = history.as_array().unwrap().as_slice() else {
+			panic!("three changes: {history}");
+		};
+		assert_eq!(unrecorded["value_recorded"], false);
+		assert_eq!(unrecorded["undoable"], false);
+		assert_eq!(of_null["value_recorded"], true);
+		assert_eq!(of_null["value"], Value::Null);
+		assert_eq!(of_null["undoable"], true);
+
+		let mut restore = named(&environment, "proxy");
+		restore["entry_id"] = unrecorded["entry_id"].clone();
+		private
+			.post("/api/ansible_variables/restore")
+			.json(&restore)
+			.await
+			.assert_status_bad_request();
+		restore["entry_id"] = of_null["entry_id"].clone();
+		post(&private, "/api/ansible_variables/restore", restore).await;
+		let body = read_served(&private, json!({ "server_group_id": group })).await;
+		assert_eq!(body["vars"]["proxy"], Value::Null);
+		assert!(body["vars"].as_object().unwrap().contains_key("proxy"));
 	})
 	.await
 }

private-web/e2e/ansible.spec.ts

@@ -4,6 +4,7 @@ import {
 	resetSeededTables,
 	seedAnsibleCatalogue,
 	seedAnsibleVariable,
+	seedAuditEntry,
 	seedMachine,
 	seedMaintenanceWindow,
 	seedServer,
@@ -416,6 +417,70 @@ test.describe("group Ansible", () => {
 		await expect(scope.getByTestId("removed-variables")).toHaveCount(0);
 	});
 
+	// A scope's history names each variable changed there, a removal among them
+	// even once the variable is set again and off the list of removals, and
+	// puts it back. Another scope's changes are not in it.
+	// spec: ANSVAR#presentation
+	test("puts back a removed variable from its scope's history", async ({ page, sql }) => {
+		const group = await seedServerGroup(sql, { name: "kamaka", tags: {} });
+		await seedServer(sql, {
+			name: "kamaka-prod-central",
+			type: "tamanu-central",
+			rank: "production",
+			groupId: group.id,
+			tags: {},
+		});
+
+		await page.goto(`/fleet/groups/${group.id}`);
+		const production = page
+			.getByTestId("group-ansible")
+			.getByTestId("environment-production");
+		const scope = production.getByTestId("scope-environment");
+		await setVariable(production.getByTestId("scope-group"), "timezone", "Pacific/Fiji");
+		await setVariable(scope, "log_level", "info");
+		await setVariable(scope, "region", "nz");
+		const logLevel = scope.getByTestId("var").filter({ hasText: "log_level" });
+		await expect(scope.getByTestId("var").filter({ hasText: "region" })).toHaveCount(1);
+		await scope.getByTestId("remove-log_level").click();
+		await expect(logLevel).toHaveCount(0);
+		await page.keyboard.press("Escape");
+		await setVariable(scope, "log_level", "debug");
+		await expect(logLevel).toContainText('"debug"');
+		await expect(scope.getByTestId("removed-variables")).toHaveCount(0);
+		// A removal whose entry holds no value, as one written before entries
+		// held them, names no value and offers nothing to put back.
+		await seedAuditEntry(sql, {
+			action: "inventory_variable_removed",
+			minutesAgo: 60,
+			records: [
+				{
+					kind: "variable",
+					id: `inv-vars-production-${group.id}:proxy`,
+					name: "proxy",
+				},
+				{ kind: "group", id: group.id, name: "kamaka" },
+			],
+			detail: { rank: "production", secret: false },
+		});
+
+		await page.getByRole("button", { name: "History of production", exact: true }).click();
+		const history = page.getByTestId("variable-history");
+		const entries = history.getByTestId("history-entry");
+		await expect(entries).toHaveCount(5);
+		await expect(entries.nth(0)).toContainText('log_level Set to "debug"');
+		await expect(entries.nth(1)).toContainText('log_level Removed "info"');
+		await expect(entries.nth(2)).toContainText('region Set to "nz"');
+		await expect(entries.nth(3)).toContainText('log_level Set to "info"');
+		await expect(entries.nth(4)).toContainText("proxy Removed");
+		await expect(entries.nth(4)).not.toContainText("null");
+		await expect(entries.nth(4).getByTestId("history-restore")).toHaveCount(0);
+		await expect(history).not.toContainText("timezone");
+
+		await entries.nth(1).getByTestId("history-restore").click();
+		await expect(page.getByTestId("variable-history")).toHaveCount(0);
+		await expect(logLevel).toContainText('"info"');
+	});
+
 	// The name completes from the catalogue, showing each variable's type, and
 	// the value is entered in a form shaped by it. A variable limited to other
 	// scopes is not offered.