From 2b609fe5b765ed799d22b3ab9d42c4bdc975bc75 Mon Sep 17 00:00:00 2001 From: Francisco-Castro <47334502+Francisco-Castro@users.noreply.github.com> Date: Fri, 26 Jul 2024 13:28:01 -0400 Subject: [PATCH 01/10] [FEATURE] [MER-3079] Block admin delete a resource referenced by other resource(s) (#4969) * [FEATURE] [MER-3079] Block admin delete a resource referenced by other resource(s) * [FEATURE] [MER-3079] Add test * [FEATURE] [MER-3079] Change variable name * [FEATURE] [MER-3079] Change another variable name * [FEATURE] [MER-3079] Replace CTE for JSONPath Expression * [FEATURE] [MER-3079] Delete migration * [FEATURE] [MER-3079] Rename variable * [FEATURE] [MER-3079] Refactor - split function * [FEATURE] [MER-3079] Adjust funcion name and update a comment --- lib/oli/publishing/authoring_resolver.ex | 104 ++++++++++++++--- .../curriculum/container/container_live.ex | 43 ++++--- .../entries/hyperlink_dependency_modal.ex | 54 +++++++++ lib/oli_web/live/resources/pages_view.ex | 90 ++++++++++----- .../controllers/resource_controller_test.exs | 29 ----- .../live/curriculum/container_test.exs | 107 ++++++++++++++++++ test/support/test_helpers.ex | 79 +++++++++++++ 7 files changed, 420 insertions(+), 86 deletions(-) create mode 100644 lib/oli_web/live/curriculum/entries/hyperlink_dependency_modal.ex diff --git a/lib/oli/publishing/authoring_resolver.ex b/lib/oli/publishing/authoring_resolver.ex index db84530dcc2..595aee62a18 100644 --- a/lib/oli/publishing/authoring_resolver.ex +++ b/lib/oli/publishing/authoring_resolver.ex @@ -17,6 +17,9 @@ defmodule Oli.Publishing.AuthoringResolver do @behaviour Resolver + @page_id Oli.Resources.ResourceType.id_for_page() + @container_id Oli.Resources.ResourceType.id_for_container() + @impl Resolver def from_resource_id(project_slug, resource_ids) when is_list(resource_ids) do fn -> @@ -101,15 +104,13 @@ defmodule Oli.Publishing.AuthoringResolver do @impl Resolver def all_pages(project_slug) do - page_id = Oli.Resources.ResourceType.id_for_page() - fn -> from(m in PublishedResource, join: rev in Revision, on: rev.id == m.revision_id, where: m.publication_id in subquery(project_working_publication(project_slug)) and - rev.resource_type_id == ^page_id and rev.deleted == false, + rev.resource_type_id == @page_id and rev.deleted == false, select: rev ) |> Repo.all() @@ -152,16 +153,13 @@ defmodule Oli.Publishing.AuthoringResolver do @impl Resolver def all_revisions_in_hierarchy(project_slug) do - page_id = Oli.Resources.ResourceType.id_for_page() - container_id = Oli.Resources.ResourceType.id_for_container() - fn -> from(m in PublishedResource, join: rev in Revision, on: rev.id == m.revision_id, where: m.publication_id in subquery(project_working_publication(project_slug)) and - (rev.resource_type_id == ^page_id or rev.resource_type_id == ^container_id), + (rev.resource_type_id == @page_id or rev.resource_type_id == @container_id), select: rev ) |> Repo.all() @@ -297,8 +295,6 @@ defmodule Oli.Publishing.AuthoringResolver do """ def get_by_purpose(project_slug, purpose) do - page_id = Oli.Resources.ResourceType.id_for_page() - Repo.all( from( revision in Revision, @@ -308,7 +304,7 @@ defmodule Oli.Publishing.AuthoringResolver do pub_res.publication_id in subquery(project_working_publication(project_slug)) and revision.purpose == ^purpose and - revision.resource_type_id == ^page_id and revision.deleted == false, + revision.resource_type_id == @page_id and revision.deleted == false, order_by: [asc: :resource_id] ) ) @@ -325,8 +321,6 @@ defmodule Oli.Publishing.AuthoringResolver do """ def targeted_via_related_to(project_slug, resource_id) do - page_id = Oli.Resources.ResourceType.id_for_page() - Repo.all( from( revision in Revision, @@ -335,7 +329,7 @@ defmodule Oli.Publishing.AuthoringResolver do where: pub_res.publication_id in subquery(project_working_publication(project_slug)) and ^resource_id in revision.relates_to and - revision.resource_type_id == ^page_id and + revision.resource_type_id == @page_id and revision.deleted == false, order_by: [asc: :resource_id] ) @@ -362,4 +356,88 @@ defmodule Oli.Publishing.AuthoringResolver do ) ) end + + @fragment """ + SELECT jsonb_agg( + jsonb_build_object( + 'idref', (elem->>'idref')::INT, + 'href', split_part(elem->>'href', '/', 4) + ) + ) + FROM LATERAL jsonb_path_query( + ?, 'strict $.** \\? ((@.type == "a" && @.linkType == "page") || @.type == "page_link")' + ) AS elem + """ + |> String.replace(~r/\s+/, " ") + + @doc """ + Returns a list of pages that contains links pointing to the given page + """ + @spec find_hyperlink_references(project_slug :: String.t(), page_slug :: String.t()) :: [ + %{title: String.t(), slug: String.t()} + ] + def find_hyperlink_references(project_slug, page_slug) do + project_slug + |> find_raw_references() + |> process_and_filter_references(project_slug, page_slug) + end + + defp find_raw_references(project_slug) do + PublishedResource + |> join(:inner, [pr], r in Revision, on: r.id == pr.revision_id, as: :rev) + |> where([pr, _r], pr.publication_id in subquery(project_working_publication(project_slug))) + |> where([_pr, r], r.resource_type_id == @page_id) + |> where([_pr, r], r.deleted == false) + |> where([_pr, r], not is_nil(fragment(@fragment, r.content))) + |> select([_pr, r], %{slug: r.slug, refs: fragment(@fragment, r.content), title: r.title}) + |> Repo.all() + end + + # Function that processes the given references to return only the ones pointing to the specified page slug. + defp process_and_filter_references(raw_references_data, project_slug, page_slug) do + # Collect the resource_ids and transform the refs from a map to a list + # For instance, %{refs: [%{"href" => nil, "idref" => 1}, %{"href" => "some_slug", "idref" => nil}]} + # will be transformed to [1, "some_slug"] + {resource_ids, data_with_refs_as_list} = + Enum.reduce(raw_references_data, {[], []}, fn + %{refs: refs} = data, acc -> + {hrefs, idrefs} = + Enum.reduce(refs, {[], []}, fn + %{"href" => nil, "idref" => idref}, acc2 -> {elem(acc2, 0), [idref | elem(acc2, 1)]} + %{"href" => href, "idref" => nil}, acc2 -> {[href | elem(acc2, 0)], elem(acc2, 1)} + end) + + resource_ids = idrefs ++ elem(acc, 0) + references_list = [%{data | refs: hrefs ++ idrefs} | elem(acc, 1)] + {resource_ids, references_list} + end) + + res_id_to_rev_slug_map = map_resource_id_to_rev_slug(project_slug, resource_ids) + + # Map resource_ids and filter references by the given page_slug + Enum.reduce(data_with_refs_as_list, [], fn data, acc -> + refs_maped = + Enum.reduce(data.refs, [], fn + reference, acc2 when is_number(reference) -> [res_id_to_rev_slug_map[reference] | acc2] + reference, acc2 -> [reference | acc2] + end) + + if page_slug in refs_maped, + do: [%{title: data.title, slug: data.slug} | acc], + else: acc + end) + end + + defp map_resource_id_to_rev_slug(project_slug, resource_ids) do + from(m in PublishedResource, + join: rev in Revision, + on: rev.id == m.revision_id, + where: + m.publication_id in subquery(project_working_publication(project_slug)) and + m.resource_id in ^resource_ids, + select: {rev.resource_id, rev.slug} + ) + |> Repo.all() + |> Enum.into(%{}) + end end diff --git a/lib/oli_web/live/curriculum/container/container_live.ex b/lib/oli_web/live/curriculum/container/container_live.ex index 29ba848ce06..4c352afd99e 100644 --- a/lib/oli_web/live/curriculum/container/container_live.ex +++ b/lib/oli_web/live/curriculum/container/container_live.ex @@ -20,7 +20,8 @@ defmodule OliWeb.Curriculum.ContainerLive do Entry, OptionsModalContent, DeleteModal, - NotEmptyModal + NotEmptyModal, + HyperlinkDependencyModal } alias OliWeb.Common.Hierarchy.MoveModal @@ -364,7 +365,13 @@ defmodule OliWeb.Curriculum.ContainerLive do case Enum.find(socket.assigns.children, fn r -> r.slug == slug end) do %{children: []} = item -> - proceed_with_deletion_warning(socket, container, project, author, item) + case AuthoringResolver.find_hyperlink_references(project.slug, slug) do + [] -> + proceed_with_deletion_warning(socket, container, project, author, item) + + references -> + show_hyperlink_dependency_modal(socket, container, project, references, item) + end item -> notify_not_empty(socket, container, project, author, item) @@ -603,12 +610,25 @@ defmodule OliWeb.Curriculum.ContainerLive do """ end - {:noreply, - show_modal( - socket, - modal, - modal_assigns: modal_assigns - )} + {:noreply, show_modal(socket, modal, modal_assigns: modal_assigns)} + end + + defp show_hyperlink_dependency_modal(socket, container, project, hyperlinks, item) do + modal_assigns = %{ + id: "not_empty_#{item.slug}", + revision: item, + container: container, + project: project, + hyperlinks: hyperlinks + } + + modal = fn assigns -> + ~H""" + + """ + end + + {:noreply, show_modal(socket, modal, modal_assigns: modal_assigns)} end defp notify_not_empty(socket, container, project, author, item) do @@ -626,12 +646,7 @@ defmodule OliWeb.Curriculum.ContainerLive do """ end - {:noreply, - show_modal( - socket, - modal, - modal_assigns: modal_assigns - )} + {:noreply, show_modal(socket, modal, modal_assigns: modal_assigns)} end defp update_author_view_pref(author, curriculum_view) do diff --git a/lib/oli_web/live/curriculum/entries/hyperlink_dependency_modal.ex b/lib/oli_web/live/curriculum/entries/hyperlink_dependency_modal.ex new file mode 100644 index 00000000000..356e41113ce --- /dev/null +++ b/lib/oli_web/live/curriculum/entries/hyperlink_dependency_modal.ex @@ -0,0 +1,54 @@ +defmodule OliWeb.Curriculum.HyperlinkDependencyModal do + use Phoenix.LiveComponent + use Phoenix.HTML + use OliWeb, :verified_routes + + def render(assigns) do + ~H""" + + """ + end +end diff --git a/lib/oli_web/live/resources/pages_view.ex b/lib/oli_web/live/resources/pages_view.ex index 86ce401c1ec..315bf4e37b4 100644 --- a/lib/oli_web/live/resources/pages_view.ex +++ b/lib/oli_web/live/resources/pages_view.ex @@ -29,6 +29,7 @@ defmodule OliWeb.Resources.PagesView do alias OliWeb.Curriculum.OptionsModalContent alias OliWeb.Components.Modal alias OliWeb.Curriculum.Container.ContainerLiveHelpers + alias OliWeb.Curriculum.HyperlinkDependencyModal @limit 25 @@ -348,38 +349,15 @@ defmodule OliWeb.Resources.PagesView do [container] -> container end - modal_assigns = %{ - id: "delete_#{revision.slug}", - redirect_url: - Routes.live_path( - socket, - __MODULE__, - socket.assigns.project.slug, - %{ - sort_by: socket.assigns.table_model.sort_by_spec.name, - sort_order: socket.assigns.table_model.sort_order, - offset: socket.assigns.offset, - text_search: socket.assigns.options.text_search - } - ), - revision: revision, - container: container, - project: project, - author: author - } + project = socket.assigns.project - modal = fn assigns -> - ~H""" - - """ - end + case AuthoringResolver.find_hyperlink_references(project.slug, slug) do + [] -> + proceed_with_deletion_warning(socket, container, project, author, revision) - {:noreply, - show_modal( - socket, - modal, - modal_assigns: modal_assigns - )} + references -> + show_hyperlink_dependency_modal(socket, container, project, references, revision) + end end def handle_event("DeleteModal.delete", %{"slug" => slug}, socket) do @@ -698,6 +676,10 @@ defmodule OliWeb.Resources.PagesView do patch_with(socket, %{}) end + def handle_event("dismiss", _, socket) do + {:noreply, hide_modal(socket, modal_assigns: nil)} + end + def handle_event(event, params, socket) do {event, params, socket, &__MODULE__.patch_with/2} |> delegate_to([ @@ -706,6 +688,54 @@ defmodule OliWeb.Resources.PagesView do ]) end + defp show_hyperlink_dependency_modal(socket, container, project, references, revision) do + modal_assigns = %{ + id: "not_empty_#{revision.slug}", + revision: revision, + container: container, + project: project, + hyperlinks: references + } + + modal = fn assigns -> + ~H""" + + """ + end + + {:noreply, show_modal(socket, modal, modal_assigns: modal_assigns)} + end + + defp proceed_with_deletion_warning(socket, container, project, author, revision) do + modal_assigns = %{ + id: "delete_#{revision.slug}", + redirect_url: + Routes.live_path( + socket, + __MODULE__, + socket.assigns.project.slug, + %{ + sort_by: socket.assigns.table_model.sort_by_spec.name, + sort_order: socket.assigns.table_model.sort_order, + offset: socket.assigns.offset, + text_search: socket.assigns.options.text_search + } + ), + revision: revision, + container: container, + project: project, + author: author + } + + modal = fn assigns -> + ~H""" + + """ + end + + {:noreply, show_modal(socket, modal, modal_assigns: modal_assigns)} + end + defp disconnected_page_node(%Revision{} = revision, %Project{} = project) do %HierarchyNode{ uuid: uuid(), diff --git a/test/oli_web/controllers/resource_controller_test.exs b/test/oli_web/controllers/resource_controller_test.exs index c1740dea6d4..f210c2f8d8e 100644 --- a/test/oli_web/controllers/resource_controller_test.exs +++ b/test/oli_web/controllers/resource_controller_test.exs @@ -203,33 +203,4 @@ defmodule OliWeb.ResourceControllerTest do {:ok, Map.merge(%{conn: conn}, seeds)} end - - def create_hyperlink_content(revision_2_slug) do - %{ - "model" => [ - %{ - "children" => [ - %{ - "children" => [ - %{"text" => " "}, - %{ - "children" => [%{"text" => "link"}], - "href" => "/course/link/#{revision_2_slug}", - "id" => "1914651063", - "target" => "self", - "type" => "a" - }, - %{"text" => ""} - ], - "id" => "3636822762", - "type" => "p" - } - ], - "id" => "481882791", - "purpose" => "None", - "type" => "content" - } - ] - } - end end diff --git a/test/oli_web/live/curriculum/container_test.exs b/test/oli_web/live/curriculum/container_test.exs index ca54ff6f172..3bee2aafb59 100644 --- a/test/oli_web/live/curriculum/container_test.exs +++ b/test/oli_web/live/curriculum/container_test.exs @@ -4,6 +4,7 @@ defmodule OliWeb.Curriculum.ContainerLiveTest do alias Oli.Seeder alias Oli.Publishing alias Oli.Publishing.AuthoringResolver + alias Oli.Resources.ResourceType import Oli.Factory import Phoenix.ConnTest @@ -612,6 +613,112 @@ defmodule OliWeb.Curriculum.ContainerLiveTest do end end + describe "Delete page" do + setup [:admin_conn] + + test "renders deletion restriction message and lists linking resources", %{conn: conn} do + author = insert(:author) + + project = insert(:project, authors: [author]) + + page_1_revision = + insert(:revision, %{ + slug: "page_1", + title: "Page 1", + resource_type_id: ResourceType.id_for_page(), + author_id: author.id + }) + + page_2_revision = + insert(:revision, %{ + slug: "page_2", + title: "Page 2", + resource_type_id: ResourceType.id_for_page(), + author_id: author.id + }) + + page_3_revision = + insert(:revision, %{ + slug: "page_3", + title: "Page 3", + resource_type_id: ResourceType.id_for_page(), + author_id: author.id + }) + + container_revision = + insert(:revision, %{ + objectives: %{}, + resource_type_id: ResourceType.id_for_container(), + children: [ + page_1_revision.resource_id, + page_2_revision.resource_id, + page_3_revision.resource_id + ], + content: %{}, + slug: "root_container", + title: "Root Container", + author_id: author.id + }) + + all_revisions = [page_1_revision, page_2_revision, page_3_revision, container_revision] + + # asociate resources to project + Enum.each(all_revisions, fn revision -> + insert(:project_resource, %{project_id: project.id, resource_id: revision.resource_id}) + end) + + # publish project + publication = + insert(:publication, %{ + project: project, + root_resource_id: container_revision.resource_id, + published: nil + }) + + # publish resources + Enum.each(all_revisions, fn revision -> + insert(:published_resource, %{ + publication: publication, + resource: revision.resource, + revision: revision, + author: author + }) + end) + + # Replace content to have a hyperlink pointing to page 3 + Oli.Resources.update_revision(page_1_revision, %{ + content: create_hyperlink_content("page_3") + }) + + # Replace content to have a page link pointing to page 3 + Oli.Resources.update_revision(page_2_revision, %{ + content: create_page_link_content(page_3_revision.resource_id) + }) + + {:ok, view, _html} = live(conn, ~p"/authoring/project/#{project.slug}/curriculum") + + render_click(view, "show_delete_modal", %{"slug" => "#{page_3_revision.slug}"}) + + # Extract titles and hyperlinks all in one list + results = + view + |> element("#not_empty_#{page_3_revision.slug}") + |> render() + |> Floki.parse_document!() + |> Floki.find("li") + |> Enum.reduce([], fn a_tag, acc -> + href = Floki.find(a_tag, "a") |> Floki.attribute("href") |> List.first() + text = Floki.text(a_tag) + [text, href | acc] + end) + + assert Enum.any?(results, fn result -> result =~ page_1_revision.title end) + assert Enum.any?(results, fn result -> result =~ page_2_revision.title end) + assert Enum.any?(results, fn result -> result =~ page_1_revision.slug end) + assert Enum.any?(results, fn result -> result =~ page_2_revision.slug end) + end + end + defp setup_session(%{conn: conn}) do map = Seeder.base_project_with_resource2() diff --git a/test/support/test_helpers.ex b/test/support/test_helpers.ex index 81a79218477..b09196b59b9 100644 --- a/test/support/test_helpers.ex +++ b/test/support/test_helpers.ex @@ -3574,4 +3574,83 @@ defmodule Oli.TestHelpers do end end end + + def create_hyperlink_content(revision_2_slug) do + %{ + "model" => [ + %{ + "children" => [ + %{ + "children" => [ + %{"text" => " "}, + %{ + "children" => [%{"text" => "link"}], + "href" => "/course/link/#{revision_2_slug}", + "id" => "1914651063", + "target" => "self", + "type" => "a", + "linkType" => "page" + }, + %{"text" => ""} + ], + "id" => "3636822762", + "type" => "p" + } + ], + "id" => "481882791", + "purpose" => "None", + "type" => "content" + } + ] + } + end + + def create_page_link_content(revision_2_resource_id) do + """ + { + "model": [ + { + "id": "961779836", + "type": "content", + "editor": "slate", + "children": [ + { + "id": "3761056253", + "type": "p", + "children": [ + { + "text": "" + } + ] + }, + { + "id": "3722062784", + "type": "page_link", + "idref": "#{revision_2_resource_id}", + "purpose": "none", + "children": [ + { + "text": "" + } + ] + }, + { + "id": "2711915655", + "type": "p", + "children": [ + { + "text": "" + } + ] + } + ], + "textDirection": "ltr" + } + ], + "bibrefs": [], + "version": "0.1.0" + } + """ + |> Jason.decode!() + end end From df8da86b22165199fc498fa94ca64a805754a079 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Cirio?= Date: Fri, 26 Jul 2024 15:58:04 -0300 Subject: [PATCH 02/10] [ENHANCEMENT] [MER-3298] Workspace nav bar and new instructor dashboard - part 2 (#4982) * split OpenAndFreeIndex liveview into 3 liveviews with shared layout * redirect to target workspace after signin * fix test for learn live * update OpenAndFreeIndex links * instructor and student workspace tests * fix route warnings * remove alias warnings * delete OpenAndFreeIndex liveview and its tests * remove unused test * Auto format --------- Co-authored-by: nicocirio --- .../delivery/instructor_dashboard.ex | 2 +- lib/oli_web/components/delivery/layouts.ex | 116 ++-- .../components/delivery/user_account.ex | 4 +- lib/oli_web/components/delivery/utils.ex | 2 +- .../components/layouts/workspace.html.heex | 8 +- lib/oli_web/controllers/cognito_controller.ex | 2 +- .../controllers/delivery_controller.ex | 4 +- .../payment_providers/cashnet_controller.ex | 2 +- .../pow/registration_html/edit.html.heex | 2 +- lib/oli_web/controllers/session_controller.ex | 2 +- .../student_dashboard/components/helpers.ex | 2 +- .../delivery/student_onboarding/wizard.ex | 2 +- lib/oli_web/live/new_course/new_course.ex | 2 +- lib/oli_web/live/sections/overview_view.ex | 2 +- lib/oli_web/live/workspace/course_author.ex | 68 +++ lib/oli_web/live/workspace/instructor.ex | 259 +++++++++ .../student.ex} | 156 +---- .../live_session_plugs/require_enrollment.ex | 2 +- lib/oli_web/live_session_plugs/set_section.ex | 2 +- lib/oli_web/pow/user_routes.ex | 12 +- lib/oli_web/router.ex | 36 +- .../templates/cognito/create_prompt.html.eex | 2 +- .../controllers/cognito_controller_test.exs | 8 +- .../static_page_controller_test.exs | 2 +- .../delivery/open_and_free_index_test.exs | 539 ------------------ .../live/delivery/student/learn_live_test.exs | 4 +- .../live/workspace/instructor_test.exs | 339 +++++++++++ test/oli_web/live/workspace/student_test.exs | 204 +++++++ test/oli_web/pow/pow_test.exs | 8 +- test/oli_web/pow/users_context_test.exs | 2 +- 30 files changed, 1011 insertions(+), 784 deletions(-) create mode 100644 lib/oli_web/live/workspace/course_author.ex create mode 100644 lib/oli_web/live/workspace/instructor.ex rename lib/oli_web/live/{delivery/open_and_free_index.ex => workspace/student.ex} (63%) delete mode 100644 test/oli_web/live/delivery/open_and_free_index_test.exs create mode 100644 test/oli_web/live/workspace/instructor_test.exs create mode 100644 test/oli_web/live/workspace/student_test.exs diff --git a/lib/oli_web/components/delivery/instructor_dashboard.ex b/lib/oli_web/components/delivery/instructor_dashboard.ex index bc913588359..16231431887 100644 --- a/lib/oli_web/components/delivery/instructor_dashboard.ex +++ b/lib/oli_web/components/delivery/instructor_dashboard.ex @@ -105,7 +105,7 @@ defmodule OliWeb.Components.Delivery.InstructorDashboard do """ end - defp logo_link(nil, _), do: ~p"/sections" + defp logo_link(nil, _), do: ~p"/sections/workspace/instructor" defp logo_link(section, preview_mode) do if preview_mode do diff --git a/lib/oli_web/components/delivery/layouts.ex b/lib/oli_web/components/delivery/layouts.ex index bfc1db2cdc4..cdba985cefa 100644 --- a/lib/oli_web/components/delivery/layouts.ex +++ b/lib/oli_web/components/delivery/layouts.ex @@ -216,19 +216,19 @@ defmodule OliWeb.Components.Delivery.Layouts do """ end - attr(:active, :atom, required: true, doc: "The current selected menu link") attr(:preview_mode, :boolean) - attr(:url_params, :map, required: true) + attr(:sidebar_expanded, :boolean) + attr(:active_workspace, :atom) def workspace_sidebar_toggler(assigns) do ~H""" @@ -238,15 +238,13 @@ defmodule OliWeb.Components.Delivery.Layouts do attr(:ctx, SessionContext) attr(:is_system_admin, :boolean, required: true) attr(:active_workspace, :atom) - attr(:url_params, :map, required: true) + attr(:sidebar_expanded, :boolean) attr(:preview_mode, :boolean) def workspace_sidebar_nav(assigns) do ~H"""
-