fix(oncall): stage 2 fix, review pass 3

A platform skip is no longer counted as a delivery, so a test page or a
notice that only reaches PagerDuty, Opsgenie or ServiceNow reports that
nothing was sent. The policy handler checks the template first, writes the
policy, and only then writes and replicates the template override, so a
refused policy leaves no half-applied override. The upgrade line also scans
the team's own channel destinations for email and SNS.

src/api/management/src/request/oncall/mod.rs

@@ -1674,29 +1674,22 @@ pub async fn set_policy(
                 other => internal_error("set_policy node versions", &other),
             };
         }
-        if let Some(template) = body.template.clone() {
-            let template = template
-                .map(|t| t.trim().to_string())
-                .filter(|t| !t.is_empty());
-            if let Some(name) = template.as_deref()
-                && let Err(e) = openobserve_core::alerts::templates::check_binding(
-                    &org_id,
-                    name,
-                    config::meta::alerts::notification_kind::NotificationKind::OnCall,
-                )
-                .await
-            {
-                return MetaHttpResponse::bad_request(e.as_str());
-            }
-            if let Err(e) = o2_enterprise::enterprise::oncall::service::set_policy_template(
-                &org_id, &team_id, template,
+        // Checked before any write, written after the policy, so a refused policy leaves no half.
+        let template = body
+            .template
+            .clone()
+            .map(|t| t.map(|t| t.trim().to_string()).filter(|t| !t.is_empty()));
+        if let Some(Some(name)) = template.as_ref()
+            && let Err(e) = openobserve_core::alerts::templates::check_binding(
+                &org_id,
+                name,
+                config::meta::alerts::notification_kind::NotificationKind::OnCall,
             )
             .await
-            {
-                return to_response(e);
-            }
+        {
+            return MetaHttpResponse::bad_request(e.as_str());
         }
-        match o2_enterprise::enterprise::oncall::service::set_policy(
+        let policy = match o2_enterprise::enterprise::oncall::service::set_policy(
             &org_id,
             &team_id,
             body.rungs,
@@ -1704,6 +1697,17 @@ pub async fn set_policy(
             body.l0,
         )
         .await
+        {
+            Ok(policy) => policy,
+            Err(e) => return to_response(e),
+        };
+        let Some(template) = template else {
+            return MetaHttpResponse::json(policy);
+        };
+        match o2_enterprise::enterprise::oncall::service::set_policy_template(
+            &org_id, &team_id, template,
+        )
+        .await
         {
             Ok(policy) => MetaHttpResponse::json(policy),
             Err(e) => to_response(e),

src/core/src/alerts/notifications/oncall.rs

@@ -20,7 +20,7 @@ use o2_enterprise::enterprise::oncall::sender::{OnCallNotification, OnCallSendEr
 
 use super::{
     LegacyPayload, NotificationContext, STATUS_FIRING,
-    notify::{NotifyError, SkipReason, notify},
+    notify::{NotifyError, notify},
 };
 
 /// Sends one on-call message to one destination through the shared path.
@@ -44,7 +44,16 @@ pub async fn log_mail_destination_policies() {
     };
     for policy in policies {
         let mut mail = Vec::new();
-        for name in &policy.destinations {
+        // The team's own channel pages too, so its destinations count as well (D9).
+        let channel = infra::table::oncall_teams::get_channel(&policy.org_id, &policy.team_id)
+            .await
+            .ok()
+            .flatten()
+            .unwrap_or_default();
+        let mut names: Vec<&String> = policy.destinations.iter().chain(channel.iter()).collect();
+        names.sort();
+        names.dedup();
+        for name in names {
             if let Ok(Some(dest)) = infra::table::destinations::get(&policy.org_id, name).await
                 && let Module::Alert {
                     destination_type: DestinationType::Email(_) | DestinationType::Sns(_),
@@ -119,7 +128,8 @@ pub fn on_call_context(n: &OnCallNotification) -> NotificationContext {
 /// A Retryable error is retried by the engine; a platform skip is neither delivered nor failed.
 fn send_result(result: Result<(), NotifyError>) -> Result<(), OnCallSendError> {
     match result {
-        Ok(()) | Err(NotifyError::Skipped(SkipReason::PlatformNotForKind)) => Ok(()),
+        Ok(()) => Ok(()),
+        // A skip is not a delivery, so a send that only reached platforms reports nothing sent.
         Err(NotifyError::Skipped(reason)) => Err(OnCallSendError::Skipped(reason.as_str().into())),
         Err(e) if e.is_retryable() => Err(OnCallSendError::Retryable(e.to_string())),
         Err(e) => Err(OnCallSendError::Permanent(e.to_string())),
@@ -132,7 +142,7 @@ mod tests {
 
     use super::*;
     use crate::alerts::notifications::{
-        notify::{Route, route},
+        notify::{Route, SkipReason, route},
         org_default::{EffectiveTemplate, TemplateSource, compiled_fallback_for},
     };
 
@@ -245,10 +255,10 @@ mod tests {
     #[test]
     fn send_results_map_to_what_the_engine_retries() {
         assert_eq!(send_result(Ok(())), Ok(()));
-        assert_eq!(
+        assert!(matches!(
             send_result(Err(NotifyError::Skipped(SkipReason::PlatformNotForKind))),
-            Ok(())
-        );
+            Err(OnCallSendError::Skipped(_))
+        ));
         assert!(matches!(
             send_result(Err(NotifyError::Skipped(SkipReason::NoOrgMemberRecipient))),
             Err(OnCallSendError::Skipped(_))