From 6452834d8770438d681162f429acf60029d97a94 Mon Sep 17 00:00:00 2001 From: JohnnyT Date: Sun, 16 Nov 2025 06:21:43 -0700 Subject: [PATCH] Adds Groups.delete/2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Implements delete operation for Groups module - Fixes Planner tests to add ROPC user as both owner and member of groups (Planner requirement) - Increases wait time to 8 seconds for Azure group membership propagation - Handles Planner API returning 204 No Content for updates in Plans and Tasks modules - Adds Tasks.ReadWrite scope to AuthTestHelpers default scopes - Fixes auth test to check headers at correct location in Req.Request struct - Updates quality task to properly detect and report test failures These changes resolve failing integration tests and complete CRUD operations for Groups. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/mix/tasks/quality.ex | 42 ++++++++++++------- lib/msg/groups.ex | 46 +++++++++++++++++++++ lib/msg/planner/plans.ex | 5 +++ lib/msg/planner/tasks.ex | 5 +++ test/msg/integration/auth_test.exs | 4 +- test/msg/integration/groups_test.exs | 37 ++++++++++++----- test/msg/integration/planner/plans_test.exs | 22 ++++++++-- test/msg/integration/planner/tasks_test.exs | 22 +++++++++- test/support/auth_test_helpers.ex | 1 + 9 files changed, 153 insertions(+), 31 deletions(-) diff --git a/lib/mix/tasks/quality.ex b/lib/mix/tasks/quality.ex index 360ec83..52459d0 100644 --- a/lib/mix/tasks/quality.ex +++ b/lib/mix/tasks/quality.ex @@ -270,20 +270,34 @@ defmodule Mix.Tasks.Quality do defp run_coverage_check do Mix.shell().info("📊 Running test coverage check...") - # Run coveralls with mix task to show output - try do - Mix.Task.run("coveralls", []) - Mix.shell().info("✅ Coverage check passed") - rescue - e in Mix.Error -> - Mix.shell().error("❌ Coverage check failed.") - Mix.shell().info("💡 Run 'MIX_ENV=test mix coveralls.detail' to see uncovered lines") - - Mix.shell().info( - "💡 Add more tests to increase coverage above threshold set in coveralls.json" - ) - - Mix.raise("Coverage check failed: #{Exception.message(e)}") + # Run coveralls using System.cmd to capture output and exit code + case System.cmd("mix", ["coveralls"], stderr_to_stdout: true, env: [{"MIX_ENV", "test"}]) do + {output, 0} -> + # Show the output to user + Mix.shell().info(output) + Mix.shell().info("✅ Coverage check passed") + + {output, exit_code} -> + # Show the output to user + Mix.shell().info(output) + + # Check if it's a test failure vs coverage failure + cond do + String.contains?(output, "tests, ") and + (String.contains?(output, "failure") or String.contains?(output, "error")) -> + Mix.shell().error("❌ Test failures detected.") + Mix.raise("Tests failed - fix failing tests before proceeding") + + true -> + Mix.shell().error("❌ Coverage check failed.") + Mix.shell().info("💡 Run 'MIX_ENV=test mix coveralls.detail' to see uncovered lines") + + Mix.shell().info( + "💡 Add more tests to increase coverage above threshold set in coveralls.json" + ) + + Mix.raise("Coverage check failed with exit code #{exit_code}") + end end end diff --git a/lib/msg/groups.ex b/lib/msg/groups.ex index 39ab2f8..b166fe6 100644 --- a/lib/msg/groups.ex +++ b/lib/msg/groups.ex @@ -335,6 +335,52 @@ defmodule Msg.Groups do end end + @doc """ + Deletes a Microsoft 365 Group. + + **Warning:** This is a destructive operation that permanently removes the group and all + associated resources including the group mailbox, SharePoint site, Planner plans, + calendar, and Teams (if connected). Deleted groups may be recoverable from the + recycle bin for up to 30 days. + + ## Parameters + + - `client` - Authenticated Req.Request client + - `group_id` - ID of the group to delete + + ## Returns + + - `:ok` - Group deleted successfully (204 status) + - `{:error, :not_found}` - Group doesn't exist + - `{:error, :forbidden}` - Insufficient permissions + - `{:error, term}` - Other errors + + ## Required Permissions + + - `Group.ReadWrite.All` (application permission) + + ## Examples + + :ok = Msg.Groups.delete(client, "group-id-123") + + ## See Also + + - [Delete Group API](https://learn.microsoft.com/en-us/graph/api/group-delete) + """ + @spec delete(Req.Request.t(), String.t()) :: :ok | {:error, term()} + def delete(client, group_id) do + case Req.delete(client, url: "/groups/#{group_id}") do + {:ok, %{status: 204}} -> + :ok + + {:ok, %{status: status, body: body}} -> + handle_error(status, body) + + {:error, reason} -> + {:error, reason} + end + end + # Private functions defp handle_error(401, _), do: {:error, :unauthorized} diff --git a/lib/msg/planner/plans.ex b/lib/msg/planner/plans.ex index 00b1a5c..f868f04 100644 --- a/lib/msg/planner/plans.ex +++ b/lib/msg/planner/plans.ex @@ -218,6 +218,11 @@ defmodule Msg.Planner.Plans do json: updates_converted, headers: [{"If-Match", etag}] ) do + {:ok, %{status: 204}} -> + # Planner API returns 204 No Content for updates + # Fetch the updated plan to return it + get(client, plan_id) + {:ok, %{status: status, body: body}} when status in 200..299 -> {:ok, body} diff --git a/lib/msg/planner/tasks.ex b/lib/msg/planner/tasks.ex index 06e3048..859d37b 100644 --- a/lib/msg/planner/tasks.ex +++ b/lib/msg/planner/tasks.ex @@ -237,6 +237,11 @@ defmodule Msg.Planner.Tasks do json: updates_converted, headers: [{"If-Match", etag}] ) do + {:ok, %{status: 204}} -> + # Planner API returns 204 No Content for updates + # Fetch the updated task to return it + get(client, task_id) + {:ok, %{status: status, body: body}} when status in 200..299 -> {:ok, body} diff --git a/test/msg/integration/auth_test.exs b/test/msg/integration/auth_test.exs index c0b8ae3..2ccd7e2 100644 --- a/test/msg/integration/auth_test.exs +++ b/test/msg/integration/auth_test.exs @@ -193,8 +193,8 @@ defmodule Msg.Integration.AuthTest do assert %Req.Request{} = delegated_client assert delegated_client.options.base_url == "https://graph.microsoft.com/v1.0" - # Verify it has authorization header - assert Map.has_key?(delegated_client.options, :headers) + # Verify it has authorization header (in Req, headers are at top level, not in options) + assert Map.has_key?(delegated_client.headers, "authorization") else # Skip if no ROPC credentials or consent not granted assert true diff --git a/test/msg/integration/groups_test.exs b/test/msg/integration/groups_test.exs index 7ebe257..feda733 100644 --- a/test/msg/integration/groups_test.exs +++ b/test/msg/integration/groups_test.exs @@ -109,9 +109,7 @@ defmodule Msg.Integration.GroupsTest do assert "Unified" in group["groupTypes"] # Clean up - delete the created group - # Note: Group deletion requires DELETE /groups/{id} - # We'll add cleanup in a separate operation - cleanup_group(client, group["id"]) + assert :ok = Groups.delete(client, group["id"]) end test "handles error when getting non-existent group", %{client: client} do @@ -195,15 +193,34 @@ defmodule Msg.Integration.GroupsTest do assert :ok = Groups.add_owner(client, group["id"], other_test_user_id) # Clean up - cleanup_group(client, group["id"]) + assert :ok = Groups.delete(client, group["id"]) end - # Helper function to clean up test groups - defp cleanup_group(client, group_id) do + test "deletes a group successfully", %{client: client} do + timestamp = System.system_time(:second) + + attrs = %{ + display_name: "Test Group to Delete #{timestamp}", + mail_enabled: true, + mail_nickname: "test-delete-#{timestamp}", + security_enabled: false, + group_types: ["Unified"], + description: "Temporary test group for delete operation" + } + + {:ok, group} = Groups.create(client, attrs) + group_id = group["id"] + # Delete the group - case Req.delete(client, url: "/groups/#{group_id}") do - {:ok, %{status: 204}} -> :ok - _ -> :ok - end + assert :ok = Groups.delete(client, group_id) + + # Verify the group is deleted + assert {:error, :not_found} = Groups.get(client, group_id) + end + + test "handles error when deleting non-existent group", %{client: client} do + fake_id = "00000000-0000-0000-0000-000000000000" + + assert {:error, :not_found} = Groups.delete(client, fake_id) end end diff --git a/test/msg/integration/planner/plans_test.exs b/test/msg/integration/planner/plans_test.exs index ebe6cf6..826bc4b 100644 --- a/test/msg/integration/planner/plans_test.exs +++ b/test/msg/integration/planner/plans_test.exs @@ -43,8 +43,16 @@ defmodule Msg.Integration.Planner.PlansTest do {:ok, created_group} = Groups.create(app_client, group) group_id = created_group["id"] - # Give Azure some time to provision the group - Process.sleep(2000) + # Add the ROPC user as both owner and member (Planner requirement) + system_user_id = System.get_env("MICROSOFT_TEST_USER_ID") + + if system_user_id do + :ok = Groups.add_owner(app_client, group_id, system_user_id) + :ok = Groups.add_member(app_client, group_id, system_user_id) + end + + # Give Azure time to propagate group membership (needs 8+ seconds) + Process.sleep(8000) # Create plan (requires delegated permissions) {:ok, created_plan} = @@ -113,7 +121,15 @@ defmodule Msg.Integration.Planner.PlansTest do {:ok, created_group} = Groups.create(app_client, group) group_id = created_group["id"] - Process.sleep(2000) + # Add the ROPC user as both owner and member (Planner requirement) + system_user_id = System.get_env("MICROSOFT_TEST_USER_ID") + + if system_user_id do + :ok = Groups.add_owner(app_client, group_id, system_user_id) + :ok = Groups.add_member(app_client, group_id, system_user_id) + end + + Process.sleep(8000) # Create a plan {:ok, plan} = diff --git a/test/msg/integration/planner/tasks_test.exs b/test/msg/integration/planner/tasks_test.exs index dcb9f1c..bf13cb2 100644 --- a/test/msg/integration/planner/tasks_test.exs +++ b/test/msg/integration/planner/tasks_test.exs @@ -36,7 +36,16 @@ defmodule Msg.Integration.Planner.TasksTest do {:ok, created_group} = Groups.create(app_client, group) group_id = created_group["id"] - Process.sleep(2000) + # Add the ROPC user as both owner and member (Planner requirement) + system_user_id = System.get_env("MICROSOFT_TEST_USER_ID") + + if system_user_id do + :ok = Groups.add_owner(app_client, group_id, system_user_id) + :ok = Groups.add_member(app_client, group_id, system_user_id) + end + + # Give Azure time to propagate group membership (needs 8+ seconds) + Process.sleep(8000) {:ok, plan} = Plans.create(delegated_client, %{ @@ -219,7 +228,16 @@ defmodule Msg.Integration.Planner.TasksTest do {:ok, created_group} = Groups.create(app_client, group) group_id = created_group["id"] - Process.sleep(2000) + # Add the ROPC user as both owner and member (Planner requirement) + system_user_id = System.get_env("MICROSOFT_TEST_USER_ID") + + if system_user_id do + :ok = Groups.add_owner(app_client, group_id, system_user_id) + :ok = Groups.add_member(app_client, group_id, system_user_id) + end + + # Give Azure time to propagate group membership (needs 8+ seconds) + Process.sleep(8000) {:ok, plan} = Plans.create(delegated_client, %{ diff --git a/test/support/auth_test_helpers.ex b/test/support/auth_test_helpers.ex index 0dbbf11..cb1c812 100644 --- a/test/support/auth_test_helpers.ex +++ b/test/support/auth_test_helpers.ex @@ -81,6 +81,7 @@ defmodule Msg.AuthTestHelpers do Keyword.get(opts, :scopes, [ "Calendars.ReadWrite.Shared", "Group.ReadWrite.All", + "Tasks.ReadWrite", "offline_access" ])