feat: improve handling of tombstoned aggregators (#732)

previously, we treated tombstoned aggregators as fully deleted, but tombstoning needs to be less thorough in order to display tasks that were provisioned against an aggregator that has since been tombstoned.

This PR also includes a fix to a bug discovered in process, wherein tasks could still be provisioned against tombstoned aggregators, even though they were not visible.

Before this PR: tombstoned aggregators were invisible, but provisionable
After this PR tombstoned aggregators are visible, but not provisionable

Listing in index routes is unchanged (tombstoned aggregators are invisible)

src/entity/task/new_task.rs

@@ -56,7 +56,12 @@ async fn load_aggregator(
         return Ok(None);
     };
 
-    let Some(aggregator) = Aggregators::find_by_id(id).one(db).await? else {
+    let aggregator = Aggregators::find_by_id(id)
+        .filter(AggregatorColumn::DeletedAt.is_null())
+        .one(db)
+        .await?;
+
+    let Some(aggregator) = aggregator else {
         return Ok(None);
     };
 

src/routes/aggregators.rs

@@ -18,10 +18,7 @@ impl FromConn for Aggregator {
         let actor = PermissionsActor::from_conn(conn).await?;
         let db: &Db = conn.state()?;
         let id = conn.param("aggregator_id")?.parse::<Uuid>().ok()?;
-        let aggregator = Aggregators::find_by_id(id)
-            .filter(AggregatorColumn::DeletedAt.is_null())
-            .one(db)
-            .await;
+        let aggregator = Aggregators::find_by_id(id).one(db).await;
         match aggregator {
             Ok(Some(aggregator)) => actor.if_allowed(conn.method(), aggregator),
             Ok(None) => None,

tests/integration/aggregators.rs

@@ -600,7 +600,9 @@ mod show {
             .with_state(user)
             .run_async(&app)
             .await;
-        assert_not_found!(conn);
+        assert_ok!(conn);
+        let response_aggregator: Aggregator = conn.response_json().await;
+        assert_same_json_representation(&response_aggregator, &aggregator);
         Ok(())
     }
 
@@ -618,7 +620,9 @@ mod show {
             .with_state(user)
             .run_async(&app)
             .await;
-        assert_not_found!(conn);
+        assert_ok!(conn);
+        let response_aggregator: Aggregator = conn.response_json().await;
+        assert_same_json_representation(&response_aggregator, &aggregator);
         Ok(())
     }
 
@@ -636,7 +640,9 @@ mod show {
             .with_state(admin)
             .run_async(&app)
             .await;
-        assert_not_found!(conn);
+        assert_ok!(conn);
+        let response_aggregator: Aggregator = conn.response_json().await;
+        assert_same_json_representation(&response_aggregator, &aggregator);
         Ok(())
     }
 
@@ -653,7 +659,9 @@ mod show {
             .with_state(admin)
             .run_async(&app)
             .await;
-        assert_not_found!(conn);
+        assert_ok!(conn);
+        let response_aggregator: Aggregator = conn.response_json().await;
+        assert_same_json_representation(&response_aggregator, &aggregator);
         Ok(())
     }
 
@@ -903,14 +911,17 @@ mod update {
             .tombstone()
             .update(app.db())
             .await?;
-
+        let name = fixtures::random_name();
         let mut conn = patch(format!("/api/aggregators/{}", aggregator.id))
             .with_api_headers()
-            .with_request_json(json!({ "name": "new_name" }))
+            .with_request_json(json!({ "name": name }))
             .with_state(user)
             .run_async(&app)
             .await;
-        assert_not_found!(conn);
+        assert_ok!(conn);
+        let response_aggregator: Aggregator = conn.response_json().await;
+        assert_eq!(response_aggregator.name, name);
+        assert_eq!(aggregator.reload(app.db()).await?.unwrap().name, name);
         Ok(())
     }
 
@@ -922,10 +933,10 @@ mod update {
             .tombstone()
             .update(app.db())
             .await?;
-
+        let name = fixtures::random_name();
         let mut conn = patch(format!("/api/aggregators/{}", aggregator.id))
             .with_api_headers()
-            .with_request_json(json!({ "name": "new_name" }))
+            .with_request_json(json!({ "name": name }))
             .with_state(user)
             .run_async(&app)
             .await;
@@ -942,13 +953,17 @@ mod update {
             .tombstone()
             .update(app.db())
             .await?;
+        let name = fixtures::random_name();
         let mut conn = patch(format!("/api/aggregators/{}", aggregator.id))
             .with_api_headers()
-            .with_request_json(json!({ "name": "new_name" }))
+            .with_request_json(json!({ "name": name }))
             .with_state(admin)
             .run_async(&app)
             .await;
-        assert_not_found!(conn);
+        assert_ok!(conn);
+        let response_aggregator: Aggregator = conn.response_json().await;
+        assert_eq!(response_aggregator.name, name);
+        assert_eq!(aggregator.reload(app.db()).await?.unwrap().name, name);
         Ok(())
     }
 
@@ -960,13 +975,18 @@ mod update {
             .tombstone()
             .update(app.db())
             .await?;
+        let name = fixtures::random_name();
         let mut conn = patch(format!("/api/aggregators/{}", aggregator.id))
             .with_api_headers()
-            .with_request_json(json!({ "name": "new_name" }))
+            .with_request_json(json!({ "name": name }))
             .with_state(admin)
             .run_async(&app)
             .await;
-        assert_not_found!(conn);
+
+        assert_ok!(conn);
+        let response_aggregator: Aggregator = conn.response_json().await;
+        assert_eq!(response_aggregator.name, name);
+        assert_eq!(aggregator.reload(app.db()).await?.unwrap().name, name);
         Ok(())
     }
 
@@ -982,6 +1002,7 @@ mod update {
             .with_request_json(json!({ "name": name }))
             .run_async(&app)
             .await;
+
         assert_ok!(conn);
         let response_aggregator: Aggregator = conn.response_json().await;
         assert_eq!(response_aggregator.name, name);

tests/integration/tasks.rs

@@ -248,6 +248,54 @@ mod create {
         Ok(())
     }
 
+    #[test(harness = with_client_logs)]
+    async fn attempting_to_provision_against_a_tombstoned_leader(
+        app: DivviupApi,
+        client_logs: ClientLogs,
+    ) -> TestResult {
+        let (user, account, ..) = fixtures::member(&app).await;
+        let (leader, helper) = fixtures::aggregator_pair(&app, &account).await;
+        let collector_credential = fixtures::collector_credential(&app, &account).await;
+        let leader = leader.tombstone().update(app.db()).await.unwrap();
+
+        let mut conn = post(format!("/api/accounts/{}/tasks", account.id))
+            .with_api_headers()
+            .with_state(user)
+            .with_request_json(valid_task_json(&collector_credential, &leader, &helper))
+            .run_async(&app)
+            .await;
+
+        assert_response!(conn, 400);
+        let error: Value = conn.response_json().await;
+        assert!(error.get("leader_aggregator_id").is_some());
+        assert!(client_logs.is_empty());
+        Ok(())
+    }
+
+    #[test(harness = with_client_logs)]
+    async fn attempting_to_provision_against_a_tombstoned_helper(
+        app: DivviupApi,
+        client_logs: ClientLogs,
+    ) -> TestResult {
+        let (user, account, ..) = fixtures::member(&app).await;
+        let (leader, helper) = fixtures::aggregator_pair(&app, &account).await;
+        let collector_credential = fixtures::collector_credential(&app, &account).await;
+        let helper = helper.tombstone().update(app.db()).await.unwrap();
+
+        let mut conn = post(format!("/api/accounts/{}/tasks", account.id))
+            .with_api_headers()
+            .with_state(user)
+            .with_request_json(valid_task_json(&collector_credential, &leader, &helper))
+            .run_async(&app)
+            .await;
+
+        assert_response!(conn, 400);
+        let error: Value = conn.response_json().await;
+        assert!(error.get("helper_aggregator_id").is_some());
+        assert!(client_logs.is_empty());
+        Ok(())
+    }
+
     #[test(harness = set_up)]
     async fn invalid(app: DivviupApi) -> TestResult {
         let (user, account, ..) = fixtures::member(&app).await;