From cc51f557804da1fe40b7a6a9b5f400af862bd1d3 Mon Sep 17 00:00:00 2001 From: Rory McKinley Date: Thu, 17 Oct 2024 13:09:42 +0200 Subject: [PATCH 1/2] update_project tests in a context of their own --- test/lightning/projects_test.exs | 250 ++++++++++++++++--------------- 1 file changed, 126 insertions(+), 124 deletions(-) diff --git a/test/lightning/projects_test.exs b/test/lightning/projects_test.exs index 0991b1a141..7f9b46b580 100644 --- a/test/lightning/projects_test.exs +++ b/test/lightning/projects_test.exs @@ -178,130 +178,6 @@ defmodule Lightning.ProjectsTest do Projects.create_project(%{"name" => "Can't have spaces!"}) end - test "update_project/2 with valid data updates the project" do - project = project_fixture() - update_attrs = %{name: "some-updated-name"} - - assert {:ok, %Project{} = project} = - Projects.update_project(project, update_attrs) - - assert project.name == "some-updated-name" - end - - test "update_project/2 updates the MFA requirement" do - project = insert(:project) - - refute project.requires_mfa - update_attrs = %{requires_mfa: true} - - assert {:ok, %Project{} = project} = - Projects.update_project(project, update_attrs) - - assert project.requires_mfa - end - - test "update_project/2 updates the data retention periods" do - project = - insert(:project, - project_users: - Enum.map( - [ - :viewer, - :editor, - :admin, - :owner - ], - fn role -> build(:project_user, user: build(:user), role: role) end - ) - ) - - update_attrs = %{ - history_retention_period: 14, - dataclip_retention_period: 7 - } - - assert {:ok, %Project{} = updated_project} = - Projects.update_project(project, update_attrs) - - # admins and owners receives an email - admins = - Enum.filter(project.project_users, fn %{role: role} -> - role in [:admin, :owner] - end) - - assert Enum.count(admins) == 2 - - %{subject: subject, body: body} = data_retention_email(updated_project) - - for %{user: user} <- admins do - email = Swoosh.Email.Recipient.format(user) - - assert_receive {:email, - %Swoosh.Email{ - subject: ^subject, - to: [^email], - text_body: ^body - }} - end - - # editors and viewers do not receive any email - non_admins = - Enum.filter(project.project_users, fn %{role: role} -> - role in [:editor, :viewer] - end) - - assert Enum.count(non_admins) == 2 - - for %{user: %{email: email}} <- non_admins do - refute_receive {:email, - %Swoosh.Email{ - subject: ^subject, - to: [{"", ^email}], - text_body: ^body - }} - - # data_retention_email(user, updated_project) |> assert_email_not_sent() - end - - # no email is sent when there's no change - assert {:ok, updated_project} = - Projects.update_project(updated_project, update_attrs) - - for %{user: %{email: email}} <- project.project_users do - refute_receive {:email, - %Swoosh.Email{ - subject: ^subject, - to: [{"", ^email}], - text_body: ^body - }} - end - - # no email is sent when there's an error in the changeset - assert {:error, _changeset} = - Projects.update_project(updated_project, %{ - history_retention_period: "xyz", - dataclip_retention_period: 7 - }) - - for %{user: %{email: email}} <- project.project_users do - refute_receive {:email, - %Swoosh.Email{ - subject: ^subject, - to: [{"", ^email}], - text_body: ^body - }} - end - end - - test "update_project/2 with invalid data returns error changeset" do - project = project_fixture() |> unload_relation(:project_users) - - assert {:error, %Ecto.Changeset{}} = - Projects.update_project(project, @invalid_attrs) - - assert project == Projects.get_project!(project.id) - end - test "update_project_user/2 with valid data updates the project_user" do project = project_fixture( @@ -1881,6 +1757,132 @@ defmodule Lightning.ProjectsTest do end end + describe ".update_project/2" do + test "update_project/2 with valid data updates the project" do + project = project_fixture() + update_attrs = %{name: "some-updated-name"} + + assert {:ok, %Project{} = project} = + Projects.update_project(project, update_attrs) + + assert project.name == "some-updated-name" + end + + test "update_project/2 updates the MFA requirement" do + project = insert(:project) + + refute project.requires_mfa + update_attrs = %{requires_mfa: true} + + assert {:ok, %Project{} = project} = + Projects.update_project(project, update_attrs) + + assert project.requires_mfa + end + + test "update_project/2 updates the data retention periods" do + project = + insert(:project, + project_users: + Enum.map( + [ + :viewer, + :editor, + :admin, + :owner + ], + fn role -> build(:project_user, user: build(:user), role: role) end + ) + ) + + update_attrs = %{ + history_retention_period: 14, + dataclip_retention_period: 7 + } + + assert {:ok, %Project{} = updated_project} = + Projects.update_project(project, update_attrs) + + # admins and owners receives an email + admins = + Enum.filter(project.project_users, fn %{role: role} -> + role in [:admin, :owner] + end) + + assert Enum.count(admins) == 2 + + %{subject: subject, body: body} = data_retention_email(updated_project) + + for %{user: user} <- admins do + email = Swoosh.Email.Recipient.format(user) + + assert_receive {:email, + %Swoosh.Email{ + subject: ^subject, + to: [^email], + text_body: ^body + }} + end + + # editors and viewers do not receive any email + non_admins = + Enum.filter(project.project_users, fn %{role: role} -> + role in [:editor, :viewer] + end) + + assert Enum.count(non_admins) == 2 + + for %{user: %{email: email}} <- non_admins do + refute_receive {:email, + %Swoosh.Email{ + subject: ^subject, + to: [{"", ^email}], + text_body: ^body + }} + + # data_retention_email(user, updated_project) |> assert_email_not_sent() + end + + # no email is sent when there's no change + assert {:ok, updated_project} = + Projects.update_project(updated_project, update_attrs) + + for %{user: %{email: email}} <- project.project_users do + refute_receive {:email, + %Swoosh.Email{ + subject: ^subject, + to: [{"", ^email}], + text_body: ^body + }} + end + + # no email is sent when there's an error in the changeset + assert {:error, _changeset} = + Projects.update_project(updated_project, %{ + history_retention_period: "xyz", + dataclip_retention_period: 7 + }) + + for %{user: %{email: email}} <- project.project_users do + refute_receive {:email, + %Swoosh.Email{ + subject: ^subject, + to: [{"", ^email}], + text_body: ^body + }} + end + end + + test "update_project/2 with invalid data returns error changeset" do + project = project_fixture() |> unload_relation(:project_users) + + assert {:error, %Ecto.Changeset{}} = + Projects.update_project(project, @invalid_attrs) + + assert project == Projects.get_project!(project.id) + end + end + @spec full_project_fixture(attrs :: Keyword.t()) :: %{optional(any) => any} def full_project_fixture(attrs \\ []) when is_list(attrs) do %{workflows: [workflow_1, workflow_2]} = From 864250a6358c23af30c2a28b5dd9b676e4b87c8d Mon Sep 17 00:00:00 2001 From: Rory McKinley Date: Thu, 17 Oct 2024 15:11:39 +0200 Subject: [PATCH 2/2] Audit retention period changes for projects --- CHANGELOG.md | 3 + lib/lightning/projects.ex | 29 +++- lib/lightning/projects/audit.ex | 41 +++++ .../live/project_live/settings.ex | 9 +- test/lightning/projects/audit_test.exs | 154 ++++++++++++++++++ test/lightning/projects_test.exs | 112 ++++++++++++- .../channels/run_channel_test.exs | 2 +- .../channels/worker_channel_test.exs | 9 +- test/lightning_web/live/project_live_test.exs | 54 ++++++ 9 files changed, 398 insertions(+), 15 deletions(-) create mode 100644 lib/lightning/projects/audit.ex create mode 100644 test/lightning/projects/audit_test.exs diff --git a/CHANGELOG.md b/CHANGELOG.md index e7254b9689..13fb869c67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,9 @@ and this project adheres to Arcade videos [#2563](https://github.com/OpenFn/lightning/issues/2563) - Store user preferences in database [#2564](https://github.com/OpenFn/lightning/issues/2564) +- Create audit events when the retention periods for a project's dataclips + and history are modified. + [#2589](https://github.com/OpenFn/lightning/issues/2589) ### Changed diff --git a/lib/lightning/projects.ex b/lib/lightning/projects.ex index 7facecf0ac..25e5d33748 100644 --- a/lib/lightning/projects.ex +++ b/lib/lightning/projects.ex @@ -16,6 +16,7 @@ defmodule Lightning.Projects do alias Lightning.ExportUtils alias Lightning.Invocation.Dataclip alias Lightning.Invocation.Step + alias Lightning.Projects.Audit alias Lightning.Projects.Events alias Lightning.Projects.File alias Lightning.Projects.Project @@ -286,22 +287,40 @@ defmodule Lightning.Projects do {:error, %Ecto.Changeset{}} """ - def update_project(%Project{} = project, attrs) do + def update_project(%Project{} = project, attrs, user \\ nil) do changeset = Project.changeset(project, attrs) - case Repo.update(changeset) do - {:ok, updated_project} -> + Multi.new() + |> Multi.update(:project, changeset) + |> maybe_audit_changes(changeset, user) + |> Repo.transaction() + |> case do + {:ok, %{project: updated_project}} -> if retention_setting_updated?(changeset) do send_data_retention_change_email(updated_project) end {:ok, updated_project} - error -> - error + {:error, :project, changeset, _changes_so_far} -> + {:error, changeset} + + # 2024-10-29 Not tested at module-level + # due to the difficulty of simulating a failure + # without mocking + {:error, _operation, changeset, _changes_so_far} -> + {:audit_creation_error, changeset} end end + defp maybe_audit_changes(multi, _changeset, nil), do: multi + + defp maybe_audit_changes(multi, changeset, user) do + multi + |> Audit.history_retention_period_updated(changeset, user) + |> Audit.dataclip_retention_period_updated(changeset, user) + end + @spec update_project_with_users(Project.t(), map(), boolean()) :: {:ok, Project.t()} | {:error, Ecto.Changeset.t()} def update_project_with_users( diff --git a/lib/lightning/projects/audit.ex b/lib/lightning/projects/audit.ex new file mode 100644 index 0000000000..0979bb4f4f --- /dev/null +++ b/lib/lightning/projects/audit.ex @@ -0,0 +1,41 @@ +defmodule Lightning.Projects.Audit do + @moduledoc """ + Generate Audit changesets for selected changes to project settings. + """ + use Lightning.Auditing.Audit, + repo: Lightning.Repo, + item: "project", + events: [ + "dataclip_retention_period_updated", + "history_retention_period_updated" + ] + + alias Ecto.Multi + + require Logger + + def history_retention_period_updated(multi, changeset, user) do + event_changeset(:history_retention_period, changeset, user) + |> maybe_extend_multi(multi, :audit_history_retention) + end + + def dataclip_retention_period_updated(multi, changeset, user) do + event_changeset(:dataclip_retention_period, changeset, user) + |> maybe_extend_multi(multi, :audit_dataclip_retention) + end + + defp event_changeset(field, %{data: %{id: project_id}} = changeset, user) do + "#{field}_updated" + |> event(project_id, user.id, filter_changes(changeset, field)) + end + + defp maybe_extend_multi(:no_changes, multi, _op_name), do: multi + + defp maybe_extend_multi(audit_changeset, multi, op_name) do + multi |> Multi.insert(op_name, audit_changeset) + end + + defp filter_changes(%{changes: changes} = changeset, field) do + changeset |> Map.merge(%{changes: changes |> Map.take([field])}) + end +end diff --git a/lib/lightning_web/live/project_live/settings.ex b/lib/lightning_web/live/project_live/settings.ex index cd229bc058..d16373afa9 100644 --- a/lib/lightning_web/live/project_live/settings.ex +++ b/lib/lightning_web/live/project_live/settings.ex @@ -590,7 +590,9 @@ defmodule LightningWeb.ProjectLive.Settings do end defp save_project(socket, project_params) do - case Projects.update_project(socket.assigns.project, project_params) do + socket.assigns.project + |> Projects.update_project(project_params, socket.assigns.current_user) + |> case do {:ok, project} -> {:noreply, socket @@ -599,6 +601,11 @@ defmodule LightningWeb.ProjectLive.Settings do {:error, %Ecto.Changeset{} = changeset} -> {:noreply, assign(socket, :project_changeset, changeset)} + + {:audit_creation_error, _changeset} -> + {:noreply, + socket + |> put_flash(:error, "Changes couldn't be saved, please try again")} end end diff --git a/test/lightning/projects/audit_test.exs b/test/lightning/projects/audit_test.exs new file mode 100644 index 0000000000..c4c1b313bd --- /dev/null +++ b/test/lightning/projects/audit_test.exs @@ -0,0 +1,154 @@ +defmodule Lightning.Projects.AuditTest do + use Lightning.DataCase, async: true + + alias Ecto.Multi + + alias Lightning.Projects.Audit + alias Lightning.Projects.Project + + describe ".history_retention_period_updated" do + setup do + project = + insert( + :project, + dataclip_retention_period: 14, + history_retention_period: 90, + retention_policy: :retain_all + ) + + user = insert(:user) + + %{ + project: project, + user: user + } + end + + test "adds operation to multi if history retention period is updated", %{ + project: %{id: project_id} = project, + user: %{id: user_id} = user + } do + attrs = %{ + dataclip_retention_period: 7, + history_retention_period: 30, + retention_policy: :retain_with_errors + } + + changeset = project |> Project.changeset(attrs) + + [audit_history_retention: {:insert, changeset, []}] = + Multi.new() + |> Audit.history_retention_period_updated(changeset, user) + |> Multi.to_list() + + assert %{ + changes: %{ + event: "history_retention_period_updated", + item_type: "project", + item_id: ^project_id, + actor_id: ^user_id, + changes: %{ + changes: %{ + before: %{history_retention_period: 90}, + after: %{history_retention_period: 30} + } + } + }, + valid?: true + } = changeset + end + + test "does not add operation if history retention period is unchanged", %{ + project: project, + user: user + } do + attrs = %{ + dataclip_retention_period: 7, + history_retention_period: 90, + retention_policy: :retain_with_errors + } + + changeset = project |> Project.changeset(attrs) + + updated_multi = + Multi.new() + |> Audit.history_retention_period_updated(changeset, user) + |> Multi.to_list() + + assert updated_multi == [] + end + end + + describe ".dataclip_retention_period_updated" do + setup do + project = + insert( + :project, + dataclip_retention_period: 14, + history_retention_period: 90, + retention_policy: :retain_all + ) + + user = insert(:user) + + %{ + project: project, + user: user + } + end + + test "adds operation to multi if dataclip retention period is updated", %{ + project: %{id: project_id} = project, + user: %{id: user_id} = user + } do + attrs = %{ + dataclip_retention_period: 7, + history_retention_period: 30, + retention_policy: :retain_with_errors + } + + changeset = project |> Project.changeset(attrs) + + [audit_dataclip_retention: {:insert, changeset, []}] = + Multi.new() + |> Audit.dataclip_retention_period_updated(changeset, user) + |> Multi.to_list() + + assert %{ + changes: %{ + event: "dataclip_retention_period_updated", + item_type: "project", + item_id: ^project_id, + actor_id: ^user_id, + changes: %{ + changes: %{ + before: %{dataclip_retention_period: 14}, + after: %{dataclip_retention_period: 7} + } + } + }, + valid?: true + } = changeset + end + + test "does not add operation if dataclip retention period is unchanged", %{ + project: project, + user: user + } do + attrs = %{ + dataclip_retention_period: 14, + history_retention_period: 30, + retention_policy: :retain_with_errors + } + + changeset = project |> Project.changeset(attrs) + + updated_multi = + Multi.new() + |> Audit.dataclip_retention_period_updated(changeset, user) + |> Multi.to_list() + + assert updated_multi == [] + end + end +end diff --git a/test/lightning/projects_test.exs b/test/lightning/projects_test.exs index 7f9b46b580..8a56c1e24a 100644 --- a/test/lightning/projects_test.exs +++ b/test/lightning/projects_test.exs @@ -8,6 +8,7 @@ defmodule Lightning.ProjectsTest do import Mox import Swoosh.TestAssertions + alias Lightning.Auditing.Audit alias Lightning.Accounts.User alias Lightning.Invocation.Dataclip alias Lightning.Projects @@ -1757,8 +1758,12 @@ defmodule Lightning.ProjectsTest do end end - describe ".update_project/2" do - test "update_project/2 with valid data updates the project" do + describe ".update_project/3" do + setup do + %{user: insert(:user)} + end + + test "update_project/3 with valid data updates the project" do project = project_fixture() update_attrs = %{name: "some-updated-name"} @@ -1768,7 +1773,7 @@ defmodule Lightning.ProjectsTest do assert project.name == "some-updated-name" end - test "update_project/2 updates the MFA requirement" do + test "update_project/3 updates the MFA requirement" do project = insert(:project) refute project.requires_mfa @@ -1780,7 +1785,7 @@ defmodule Lightning.ProjectsTest do assert project.requires_mfa end - test "update_project/2 updates the data retention periods" do + test "update_project/3 updates the data retention periods" do project = insert(:project, project_users: @@ -1873,7 +1878,7 @@ defmodule Lightning.ProjectsTest do end end - test "update_project/2 with invalid data returns error changeset" do + test "update_project/3 with invalid data returns error changeset" do project = project_fixture() |> unload_relation(:project_users) assert {:error, %Ecto.Changeset{}} = @@ -1881,6 +1886,103 @@ defmodule Lightning.ProjectsTest do assert project == Projects.get_project!(project.id) end + + test "creates audit events if retention periods are updated", %{ + user: %{id: user_id} = user + } do + %{id: project_id} = + project = + insert( + :project, + dataclip_retention_period: 7, + history_retention_period: 30, + retention_policy: :retain_all + ) + + update_attrs = %{ + dataclip_retention_period: 14, + history_retention_period: 90, + retention_policy: :retain_with_errors + } + + Projects.update_project(project, update_attrs, user) + + query = + from a in Audit, where: a.event == "history_retention_period_updated" + + history_audit_event = Repo.one!(query) + + assert %{ + item_type: "project", + item_id: ^project_id, + actor_id: ^user_id, + changes: changes + } = history_audit_event + + assert changes == %Audit.Changes{ + before: %{"history_retention_period" => 30}, + after: %{"history_retention_period" => 90} + } + + query = + from a in Audit, where: a.event == "dataclip_retention_period_updated" + + dataclip_audit_event = Repo.one!(query) + + assert %{ + item_type: "project", + item_id: ^project_id, + actor_id: ^user_id, + changes: changes + } = dataclip_audit_event + + assert changes == %Audit.Changes{ + before: %{"dataclip_retention_period" => 7}, + after: %{"dataclip_retention_period" => 14} + } + end + + test "does not create events if no user was provided" do + project = + insert( + :project, + dataclip_retention_period: 7, + history_retention_period: 30, + retention_policy: :retain_all + ) + + update_attrs = %{ + dataclip_retention_period: 14, + history_retention_period: 90, + retention_policy: :retain_with_errors + } + + Projects.update_project(project, update_attrs) + + assert Audit |> Repo.all() |> Enum.empty?() + end + + test "does not create events if the project change fails", %{ + user: user + } do + project = + insert( + :project, + dataclip_retention_period: 7, + history_retention_period: 30, + retention_policy: :retain_all + ) + + update_attrs = %{ + dataclip_retention_period: 14, + history_retention_period: 90, + retention_policy: :no_such_value + } + + Projects.update_project(project, update_attrs, user) + + assert Audit |> Repo.all() |> Enum.empty?() + end end @spec full_project_fixture(attrs :: Keyword.t()) :: %{optional(any) => any} diff --git a/test/lightning_web/channels/run_channel_test.exs b/test/lightning_web/channels/run_channel_test.exs index 15650d7450..46eaa84568 100644 --- a/test/lightning_web/channels/run_channel_test.exs +++ b/test/lightning_web/channels/run_channel_test.exs @@ -1201,7 +1201,7 @@ defmodule LightningWeb.RunChannelTest do "error_message" => nil }) - assert_reply ref, :ok, nil + assert_reply ref, :ok, nil, 1_000 assert %{state: :crashed} = Lightning.Repo.reload!(run) assert %{state: :crashed} = Lightning.Repo.reload!(work_order) diff --git a/test/lightning_web/channels/worker_channel_test.exs b/test/lightning_web/channels/worker_channel_test.exs index ff4435dd6d..3e790ad424 100644 --- a/test/lightning_web/channels/worker_channel_test.exs +++ b/test/lightning_web/channels/worker_channel_test.exs @@ -65,9 +65,12 @@ defmodule LightningWeb.WorkerChannelTest do ref = push(socket, "claim", %{"demand" => 1}) - assert_reply ref, :ok, %{ - runs: [%{"id" => ^run_id, "token" => token}] - } + assert_reply ref, + :ok, + %{ + runs: [%{"id" => ^run_id, "token" => token}] + }, + 1_000 Lightning.Stub.freeze_time(DateTime.utc_now() |> DateTime.add(5, :second)) diff --git a/test/lightning_web/live/project_live_test.exs b/test/lightning_web/live/project_live_test.exs index 46b5518c50..b0465ac27c 100644 --- a/test/lightning_web/live/project_live_test.exs +++ b/test/lightning_web/live/project_live_test.exs @@ -14,8 +14,10 @@ defmodule LightningWeb.ProjectLiveTest do import Lightning.GithubHelpers import Swoosh.TestAssertions + import Mock import Mox + alias Lightning.Auditing.Audit alias Lightning.Name alias Lightning.Projects alias Lightning.Repo @@ -1936,6 +1938,58 @@ defmodule LightningWeb.ProjectLiveTest do assert html =~ "Project updated successfully" end + + @tag role: :admin + test "creates event linked to user when retention period is updated", %{ + conn: conn, + project: project, + user: %{id: user_id} + } do + {:ok, view, _html} = + live( + conn, + ~p"/projects/#{project.id}/settings#data-storage" + ) + + view + |> form("#retention-settings-form", + project: %{ + history_retention_period: 7 + } + ) + |> render_submit() + + assert %{actor_id: ^user_id} = Audit |> Repo.one!() + end + + @tag role: :admin + test "indicates if there was a failure due to audit creation", %{ + conn: conn, + project: project + } do + {:ok, view, _html} = + live( + conn, + ~p"/projects/#{project.id}/settings#data-storage" + ) + + with_mock(Lightning.Repo, [:passthrough], + transaction: fn _multi -> + {:error, :anything_other_than_project, %Ecto.Changeset{}, %{}} + end + ) do + html = + view + |> form("#retention-settings-form", + project: %{ + history_retention_period: 7 + } + ) + |> render_submit() + + assert html =~ "Changes couldn't be saved" + end + end end describe "projects settings:collaboration" do