From 4969e8ee3d8abc8c05df37a6f12802891dd75c65 Mon Sep 17 00:00:00 2001 From: Darren Siegel Date: Mon, 16 Sep 2024 07:51:43 -0400 Subject: [PATCH 1/8] [BUG FIX] [MER-3814] Clear section cache (#5101) * clear section cache * put clearing in a safer spot --- lib/oli/delivery/sections/updates.ex | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/oli/delivery/sections/updates.ex b/lib/oli/delivery/sections/updates.ex index d580221fde6..55fa1154e2f 100644 --- a/lib/oli/delivery/sections/updates.ex +++ b/lib/oli/delivery/sections/updates.ex @@ -73,6 +73,8 @@ defmodule Oli.Delivery.Sections.Updates do case result do {:ok, _} -> + Oli.Delivery.Sections.SectionCache.clear(section.slug) + Broadcaster.broadcast_update_progress(section.id, new_publication.id, :complete) _ -> From 7ebf38fb78a25a1cc34a8b312c1f289f1ee1763f Mon Sep 17 00:00:00 2001 From: Santiago Simoncelli Date: Tue, 17 Sep 2024 07:20:32 -0300 Subject: [PATCH 2/8] [FEATURE] [MER-3603] Migrate insights liveview (#5100) * [MER-3603] Migrates liveview and components * [MER-3603] Migrates liveview tests * [MER-3603] Adds test of access from workspace * [MER-3603] Rework to use the latest version of insights * [MER-3603] Adds text search to browse insigths query * [MER-3603] Update sidebar collapser URL logic --------- Co-authored-by: Francisco-Castro --- lib/oli/analytics/summary/browse_insights.ex | 12 +- .../summary/browse_insights_options.ex | 6 +- lib/oli_web/components/delivery/layouts.ex | 3 + .../insights/activity_table_model.ex | 95 ++++ .../course_author/insights/common.ex | 19 + .../insights/objective_table_model.ex | 58 ++ .../insights/page_table_model.ex | 76 +++ .../workspaces/course_author/insights_live.ex | 503 +++++++++++++++++- .../course_author/insights_live_test.exs | 287 ++++++++++ .../live/workspaces/course_author_test.exs | 26 + 10 files changed, 1074 insertions(+), 11 deletions(-) create mode 100644 lib/oli_web/live/workspaces/course_author/insights/activity_table_model.ex create mode 100644 lib/oli_web/live/workspaces/course_author/insights/common.ex create mode 100644 lib/oli_web/live/workspaces/course_author/insights/objective_table_model.ex create mode 100644 lib/oli_web/live/workspaces/course_author/insights/page_table_model.ex create mode 100644 test/oli_web/live/workspaces/course_author/insights_live_test.exs diff --git a/lib/oli/analytics/summary/browse_insights.ex b/lib/oli/analytics/summary/browse_insights.ex index 2405edc6112..0174dad7c74 100644 --- a/lib/oli/analytics/summary/browse_insights.ex +++ b/lib/oli/analytics/summary/browse_insights.ex @@ -19,11 +19,20 @@ defmodule Oli.Analytics.Summary.BrowseInsights do def browse_insights( %Paging{limit: limit, offset: offset}, %Sorting{} = sorting, - %BrowseInsightsOptions{project_id: project_id, section_ids: section_ids} = options + %BrowseInsightsOptions{ + project_id: project_id, + section_ids: section_ids, + text_search: text_search + } = options ) do where_by = build_where_by(options) total_count = get_total_count(project_id, section_ids, where_by) + text_search_condition = + if text_search && text_search != "", + do: dynamic([_s, _pub, _pr, rev], ilike(rev.title, ^"%#{text_search}%")), + else: true + # Now build the main query with limit, offset, and aggregations query = ResourceSummary @@ -31,6 +40,7 @@ defmodule Oli.Analytics.Summary.BrowseInsights do |> join(:left, [s, pub], pr in PublishedResource, on: pr.publication_id == pub.id) |> join(:left, [s, pub, pr], rev in Revision, on: rev.id == pr.revision_id) |> where(^where_by) + |> where(^text_search_condition) |> add_select(total_count, options) |> add_order_by(sorting, options) |> limit(^limit) diff --git a/lib/oli/analytics/summary/browse_insights_options.ex b/lib/oli/analytics/summary/browse_insights_options.ex index 3625d6b914c..7fcd8749809 100644 --- a/lib/oli/analytics/summary/browse_insights_options.ex +++ b/lib/oli/analytics/summary/browse_insights_options.ex @@ -12,12 +12,14 @@ defmodule Oli.Analytics.Summary.BrowseInsightsOptions do defstruct [ :project_id, :section_ids, - :resource_type_id + :resource_type_id, + :text_search ] @type t() :: %__MODULE__{ project_id: integer(), section_ids: list(), - resource_type_id: integer() + resource_type_id: integer(), + text_search: String.t() } end diff --git a/lib/oli_web/components/delivery/layouts.ex b/lib/oli_web/components/delivery/layouts.ex index 61ee0cf2ea7..183e01d5dab 100644 --- a/lib/oli_web/components/delivery/layouts.ex +++ b/lib/oli_web/components/delivery/layouts.ex @@ -560,6 +560,9 @@ defmodule OliWeb.Components.Delivery.Layouts do ["", "workspaces", "course_author", project_slug, "products"] -> ~p"/workspaces/course_author/#{project_slug}/products?#{params}" + ["", "workspaces", "course_author", project_slug, "insights"] -> + ~p"/workspaces/course_author/#{project_slug}/insights?#{params}" + _ -> ~p"/" end diff --git a/lib/oli_web/live/workspaces/course_author/insights/activity_table_model.ex b/lib/oli_web/live/workspaces/course_author/insights/activity_table_model.ex new file mode 100644 index 00000000000..37ba5fd9f3c --- /dev/null +++ b/lib/oli_web/live/workspaces/course_author/insights/activity_table_model.ex @@ -0,0 +1,95 @@ +defmodule OliWeb.Workspaces.CourseAuthor.Insights.ActivityTableModel do + use Phoenix.Component + + import OliWeb.Workspaces.CourseAuthor.Insights.Common + + alias OliWeb.Common.Table.{ColumnSpec, SortableTableModel} + alias OliWeb.Router.Helpers, as: Routes + + def new(rows, activity_types_map, parent_pages, project_slug, ctx) do + SortableTableModel.new( + rows: rows, + column_specs: [ + %ColumnSpec{ + name: :title, + label: "Activity", + render_fn: &render_title/3 + }, + %ColumnSpec{ + name: :activity_type_id, + label: "Type", + render_fn: &render_activity_type/3 + }, + %ColumnSpec{ + name: :part_id, + label: "Part" + }, + %ColumnSpec{ + name: :num_attempts, + label: "# Attempts" + }, + %ColumnSpec{ + name: :num_first_attempts, + label: "# First Attempts" + }, + %ColumnSpec{ + name: :first_attempt_correct, + label: "First Attempt Correct%", + render_fn: &render_percentage/3 + }, + %ColumnSpec{ + name: :eventually_correct, + label: "Eventually Correct%", + render_fn: &render_percentage/3 + }, + %ColumnSpec{ + name: :relative_difficulty, + label: "Relative Difficulty", + render_fn: &render_float/3 + } + ], + event_suffix: "", + id_field: [:id], + data: %{ + activity_types_map: activity_types_map, + parent_pages: parent_pages, + project_slug: project_slug, + ctx: ctx + } + ) + end + + def render_title(%{parent_pages: parent_pages} = data, row, assigns) do + case Map.has_key?(parent_pages, row.resource_id) do + true -> render_with_link(data, row, assigns) + false -> render_without_link(data, row, assigns) + end + end + + defp render_with_link(%{parent_pages: parent_pages, project_slug: project_slug}, row, assigns) do + parent_page = Map.get(parent_pages, row.resource_id) + assigns = Map.put(assigns, :project_slug, project_slug) + assigns = Map.put(assigns, :parent_page, parent_page) + assigns = Map.put(assigns, :row, row) + + ~H""" + + <%= @row.title %> + + """ + end + + defp render_without_link(_, row, _assigns) do + row.title + end + + def render_activity_type(%{activity_types_map: activity_types_map}, row, _) do + Map.get(activity_types_map, row.activity_type_id).petite_label + end + + def render(assigns) do + ~H""" +
nothing
+ """ + end +end diff --git a/lib/oli_web/live/workspaces/course_author/insights/common.ex b/lib/oli_web/live/workspaces/course_author/insights/common.ex new file mode 100644 index 00000000000..8497d979528 --- /dev/null +++ b/lib/oli_web/live/workspaces/course_author/insights/common.ex @@ -0,0 +1,19 @@ +defmodule OliWeb.Workspaces.CourseAuthor.Insights.Common do + alias OliWeb.Common.Table.ColumnSpec + + def truncate(float_or_nil) when is_nil(float_or_nil), do: nil + def truncate(float_or_nil) when is_float(float_or_nil), do: Float.round(float_or_nil, 2) + + def format_percent(float_or_nil) when is_nil(float_or_nil), do: nil + + def format_percent(float_or_nil) when is_float(float_or_nil), + do: "#{round(100 * float_or_nil)}%" + + def render_percentage(_, row, %ColumnSpec{name: field}) do + row[field] |> format_percent() + end + + def render_float(_, row, %ColumnSpec{name: field}) do + row[field] |> truncate() + end +end diff --git a/lib/oli_web/live/workspaces/course_author/insights/objective_table_model.ex b/lib/oli_web/live/workspaces/course_author/insights/objective_table_model.ex new file mode 100644 index 00000000000..0695ad6b2f3 --- /dev/null +++ b/lib/oli_web/live/workspaces/course_author/insights/objective_table_model.ex @@ -0,0 +1,58 @@ +defmodule OliWeb.Workspaces.CourseAuthor.Insights.ObjectiveTableModel do + use Phoenix.Component + + import OliWeb.Workspaces.CourseAuthor.Insights.Common + + alias OliWeb.Common.Table.{ColumnSpec, SortableTableModel} + + def new(rows, ctx) do + SortableTableModel.new( + rows: rows, + column_specs: [ + %ColumnSpec{ + name: :title, + label: "Objective", + render_fn: &render_title/3 + }, + %ColumnSpec{ + name: :num_attempts, + label: "# Attempts" + }, + %ColumnSpec{ + name: :num_first_attempts, + label: "# First Attempts" + }, + %ColumnSpec{ + name: :first_attempt_correct, + label: "First Attempt Correct%", + render_fn: &render_percentage/3 + }, + %ColumnSpec{ + name: :eventually_correct, + label: "Eventually Correct%", + render_fn: &render_percentage/3 + }, + %ColumnSpec{ + name: :relative_difficulty, + label: "Relative Difficulty", + render_fn: &render_float/3 + } + ], + event_suffix: "", + id_field: [:id], + data: %{ + ctx: ctx + } + ) + end + + def render_title(_, row, _assigns) do + row.title + end + + def render(assigns) do + ~H""" +
nothing
+ """ + end +end diff --git a/lib/oli_web/live/workspaces/course_author/insights/page_table_model.ex b/lib/oli_web/live/workspaces/course_author/insights/page_table_model.ex new file mode 100644 index 00000000000..425cffea766 --- /dev/null +++ b/lib/oli_web/live/workspaces/course_author/insights/page_table_model.ex @@ -0,0 +1,76 @@ +defmodule OliWeb.Workspaces.CourseAuthor.Insights.PageTableModel do + use Phoenix.Component + + import OliWeb.Workspaces.CourseAuthor.Insights.Common + + alias OliWeb.Common.Table.{ColumnSpec, SortableTableModel} + alias OliWeb.Router.Helpers, as: Routes + + def new(rows, project_slug, ctx) do + SortableTableModel.new( + rows: rows, + column_specs: [ + %ColumnSpec{ + name: :title, + label: "Page", + render_fn: &render_title/3 + }, + %ColumnSpec{ + name: :num_attempts, + label: "# Attempts" + }, + %ColumnSpec{ + name: :num_first_attempts, + label: "# First Attempts" + }, + %ColumnSpec{ + name: :first_attempt_correct, + label: "First Attempt Correct%", + render_fn: &render_percentage/3 + }, + %ColumnSpec{ + name: :eventually_correct, + label: "Eventually Correct%", + render_fn: &render_percentage/3 + }, + %ColumnSpec{ + name: :relative_difficulty, + label: "Relative Difficulty", + render_fn: &render_float/3 + } + ], + event_suffix: "", + id_field: [:id], + data: %{ + ctx: ctx, + project_slug: project_slug + } + ) + end + + def render_title(%{project_slug: project_slug}, row, assigns) do + assigns = Map.put(assigns, :project_slug, project_slug) + assigns = Map.put(assigns, :row, row) + + ~H""" + + <%= @row.title %> + + """ + end + + def render_type(%{graded: graded}, _row, assigns) do + assigns = assign(assigns, :graded, graded) + + case assigns.graded do + true -> "Graded" + false -> "Practice" + end + end + + def render(assigns) do + ~H""" +
nothing
+ """ + end +end diff --git a/lib/oli_web/live/workspaces/course_author/insights_live.ex b/lib/oli_web/live/workspaces/course_author/insights_live.ex index 7684bf20975..cfc7806bde7 100644 --- a/lib/oli_web/live/workspaces/course_author/insights_live.ex +++ b/lib/oli_web/live/workspaces/course_author/insights_live.ex @@ -1,30 +1,517 @@ defmodule OliWeb.Workspaces.CourseAuthor.InsightsLive do use OliWeb, :live_view + import Ecto.Query + import OliWeb.Common.Params + import OliWeb.DelegatedEvents + + alias Oli.{Accounts, Activities, Publishing} + alias Oli.Analytics.Summary.{BrowseInsights, BrowseInsightsOptions} + alias Oli.Authoring.{Broadcaster, Course} + alias Oli.Authoring.Broadcaster.Subscriber + alias Oli.Delivery.Sections + alias Oli.Delivery.Sections.Section + alias Oli.Repo + alias Oli.Repo.{Paging, Sorting} + alias Oli.Resources.ResourceType + alias OliWeb.Components.Project.AsyncExporter + alias OliWeb.Common.MultiSelect.Option + alias OliWeb.Common.MultiSelectInput + alias OliWeb.Common.{PagedTable, TextSearch} + alias OliWeb.Common.Table.SortableTableModel + + alias OliWeb.Workspaces.CourseAuthor.Insights.{ + ActivityTableModel, + PageTableModel, + ObjectiveTableModel + } + + @limit 25 + @impl Phoenix.LiveView def mount(_params, _session, socket) do - project = socket.assigns.project + %{project: project, ctx: ctx} = socket.assigns + + {sections, products} = + Sections.get_sections_containing_resources_of_given_project(project.id) + |> Enum.reduce({[], []}, fn section, {sections, products} -> + if section.type == :blueprint, + do: {sections, [%Option{id: section.id, name: section.title} | products]}, + else: {[%Option{id: section.id, name: section.title} | sections], products} + end) + + sections_by_product_id = get_sections_by_product_id(project.id) + + activity_type_id = ResourceType.get_id_by_type("activity") + + options = %BrowseInsightsOptions{ + project_id: project.id, + resource_type_id: activity_type_id, + section_ids: [] + } + + insights = + BrowseInsights.browse_insights( + %Paging{offset: 0, limit: @limit}, + %Sorting{direction: :desc, field: :first_attempt_correct}, + options + ) + + latest_publication = Publishing.get_latest_published_publication_by_slug(project.slug) + + parent_pages = parent_pages(project.slug) + + activity_types_map = + Activities.list_activity_registrations() + |> Enum.reduce(%{}, fn a, m -> Map.put(m, a.id, a) end) + + total_count = determine_total(insights) + + {:ok, table_model} = + ActivityTableModel.new(insights, activity_types_map, parent_pages, project.slug, ctx) + + {analytics_export_status, analytics_export_url, analytics_export_timestamp} = + case Course.analytics_export_status(project) do + {:available, url, timestamp} -> {:available, url, timestamp} + {:expired, _, _} -> {:expired, nil, nil} + {status} -> {status, nil, nil} + end + + # Subscribe to any raw analytics snapshot progress updates for this project + Subscriber.subscribe_to_analytics_export_status(project.slug) {:ok, assign(socket, + active_view: :insights, + active_workspace: :course_author, resource_slug: project.slug, resource_title: project.title, - active_workspace: :course_author, - active_view: :insights + active: :insights, + sections_by_product_id: sections_by_product_id, + ctx: ctx, + is_admin?: Accounts.is_system_admin?(ctx.author), + project: project, + parent_pages: parent_pages, + selected: :by_activity, + latest_publication: latest_publication, + analytics_export_status: analytics_export_status, + analytics_export_url: analytics_export_url, + analytics_export_timestamp: analytics_export_timestamp, + products: products, + sections: sections, + is_product: false, + section_ids: [], + product_ids: [], + form_uuid_for_product: "", + form_uuid_for_section: "", + table_model: table_model, + options: options, + offset: 0, + total_count: total_count, + active_rows: insights, + query: "", + limit: @limit )} end @impl Phoenix.LiveView - def handle_params(_params, _url, socket) do - {:noreply, socket} + def handle_params(params, _, socket) do + table_model = SortableTableModel.update_from_params(socket.assigns.table_model, params) + + offset = get_int_param(params, "offset", 0) + + options = socket.assigns.options + + insights = + BrowseInsights.browse_insights( + %Paging{offset: offset, limit: @limit}, + %Sorting{direction: table_model.sort_order, field: table_model.sort_by_spec.name}, + options + ) + + table_model = Map.put(table_model, :rows, insights) + total_count = determine_total(insights) + + {:noreply, + assign(socket, + offset: offset, + table_model: table_model, + total_count: total_count, + options: options + )} end @impl Phoenix.LiveView def render(assigns) do ~H""" -

- Placeholder for Insights -

+
+
+

+ Insights can help you improve your course by providing a statistical analysis of + the skills covered by each question to find areas where students are struggling. +

+ <%= if @is_admin? do %> +
+ +
+ <% end %> +
+
    +
  • + +
    • +
  • + +
    • + +
  • + +
    • + +
  • +
    + <.live_component + id="sections" + module={MultiSelectInput} + options={@sections} + disabled={@sections == []} + on_select_message="section_selected" + placeholder="Select a section" + uuid={@form_uuid_for_section} + /> + <.live_component + id="products" + module={MultiSelectInput} + options={@products} + disabled={@products == []} + on_select_message="product_selected" + placeholder="Select a product" + uuid={@form_uuid_for_product} + /> +
    +
    • +
+ +
+
+
+ +
+
+
+
+ Viewing analytics by <%= case @selected do + :by_page -> "page" + :by_activity -> "activity" + :by_objective -> "objective" + _ -> "activity" + end %> +
+ + +
+
+
""" end + + @impl Phoenix.LiveView + def handle_event("filter_by_activity", _params, socket) do + activity_types_map = + Activities.list_activity_registrations() + |> Enum.reduce(%{}, fn a, m -> Map.put(m, a.id, a) end) + + {:ok, table_model} = + ActivityTableModel.new( + [], + activity_types_map, + socket.assigns.parent_pages, + socket.assigns.project.slug, + socket.assigns.ctx + ) + + filter_by( + socket, + ResourceType.get_id_by_type("activity"), + :by_activity, + table_model + ) + end + + def handle_event("filter_by_page", _params, socket) do + {:ok, table_model} = PageTableModel.new([], socket.assigns.project.slug, socket.assigns.ctx) + filter_by(socket, ResourceType.get_id_by_type("page"), :by_page, table_model) + end + + def handle_event("filter_by_objective", _params, socket) do + {:ok, table_model} = ObjectiveTableModel.new([], socket.assigns.ctx) + + filter_by( + socket, + ResourceType.get_id_by_type("objective"), + :by_objective, + table_model + ) + end + + def handle_event("generate_analytics_snapshot", _params, socket) do + project = socket.assigns.project + + case Course.generate_analytics_snapshot(project) do + {:ok, _job} -> + Broadcaster.broadcast_analytics_export_status(project.slug, {:in_progress}) + + {:noreply, socket} + + {:error, _changeset} -> + socket = + socket + |> put_flash(:error, "Raw analytics snapshot could not be generated.") + + {:noreply, socket} + end + end + + def handle_event(event, params, socket) do + delegate_to( + {event, params, socket, &patch_with/2}, + [&TextSearch.handle_delegated/4, &PagedTable.handle_delegated/4] + ) + end + + @impl Phoenix.LiveView + def handle_info({:option_selected, "section_selected", selected_ids}, socket) do + socket = + assign(socket, + section_ids: selected_ids, + form_uuid_for_product: generate_uuid(), + product_ids: [], + is_product: false + ) + + change_section_ids(socket, selected_ids) + end + + def handle_info({:option_selected, "product_selected", selected_ids}, socket) do + socket = + assign(socket, + product_ids: selected_ids, + form_uuid_for_section: generate_uuid(), + section_ids: [], + is_product: true + ) + + section_ids = + Enum.reduce(selected_ids, MapSet.new(), fn id, all -> + Map.get(socket.assigns.sections_by_product_id, id) + |> MapSet.new() + |> MapSet.union(all) + end) + |> Enum.to_list() + + change_section_ids(socket, section_ids) + end + + def handle_info( + {:analytics_export_status, + {:available, analytics_export_url, analytics_export_timestamp}}, + socket + ) do + {:noreply, + assign(socket, + analytics_export_status: :available, + analytics_export_url: analytics_export_url, + analytics_export_timestamp: analytics_export_timestamp + )} + end + + def handle_info( + {:analytics_export_status, {:error, _e}}, + socket + ) do + {:noreply, + assign(socket, + analytics_export_status: :error + )} + end + + def handle_info({:analytics_export_status, {status}}, socket) do + {:noreply, assign(socket, analytics_export_status: status)} + end + + # Runs a query to find all sections for this project which have a + # product associated with them. (blueprint_id) + defp get_sections_by_product_id(project_id) do + query = + from s in Section, + where: + s.base_project_id == ^project_id and not is_nil(s.blueprint_id) and + s.type == :enrollable, + select: {s.id, s.blueprint_id} + + Repo.all(query) + |> Enum.reduce(%{}, fn {id, blueprint_id}, m -> + case Map.get(m, blueprint_id) do + nil -> Map.put(m, blueprint_id, [id]) + ids -> Map.put(m, blueprint_id, [id | ids]) + end + end) + end + + defp determine_total(items) do + case items do + [] -> 0 + [hd | _] -> hd.total_count + end + end + + defp parent_pages(project_slug) do + publication = Publishing.project_working_publication(project_slug) + Publishing.determine_parent_pages(publication.id) + end + + defp is_loading?(assigns) do + is_nil(assigns.active_rows) + end + + def patch_with(socket, changes) do + # convert param keys from atoms to strings + changes = Enum.into(changes, %{}, fn {k, v} -> {Atom.to_string(k), v} end) + # convert atom values to string values + changes = + Enum.into(changes, %{}, fn {k, v} -> + case v do + atom when is_atom(atom) -> {k, Atom.to_string(v)} + _ -> {k, v} + end + end) + + table_model = SortableTableModel.update_from_params(socket.assigns.table_model, changes) + + options = Map.put(socket.assigns.options, :text_search, Map.get(changes, "text_search", "")) + offset = get_param(changes, "offset", 0) + + insights = + BrowseInsights.browse_insights( + %Paging{offset: offset, limit: @limit}, + %Sorting{direction: table_model.sort_order, field: table_model.sort_by_spec.name}, + options + ) + + table_model = Map.put(table_model, :rows, insights) + total_count = determine_total(insights) + + {:noreply, + assign(socket, + offset: offset, + table_model: table_model, + total_count: total_count, + options: options + )} + end + + defp filter_by(socket, resource_type_id, by_type, table_model) do + options = %BrowseInsightsOptions{ + project_id: socket.assigns.options.project_id, + resource_type_id: resource_type_id, + section_ids: socket.assigns.options.section_ids + } + + insights = + BrowseInsights.browse_insights( + %Paging{offset: 0, limit: @limit}, + %Sorting{direction: table_model.sort_order, field: table_model.sort_by_spec.name}, + options + ) + + table_model = Map.put(table_model, :rows, insights) + total_count = determine_total(insights) + + {:noreply, + assign(socket, + offset: 0, + table_model: table_model, + total_count: total_count, + options: options, + selected: by_type + )} + end + + defp change_section_ids(socket, section_ids) do + options = %BrowseInsightsOptions{socket.assigns.options | section_ids: section_ids} + table_model = socket.assigns.table_model + + insights = + BrowseInsights.browse_insights( + %Paging{offset: 0, limit: @limit}, + %Sorting{direction: table_model.sort_order, field: table_model.sort_by_spec.name}, + options + ) + + table_model = Map.put(table_model, :rows, insights) + total_count = determine_total(insights) + + {:noreply, + assign(socket, + offset: 0, + table_model: table_model, + total_count: total_count, + options: options + )} + end + + defp generate_uuid do + UUID.uuid4() + end + + defp is_disabled(selected, title) do + if selected == title, + do: [disabled: true], + else: [] + end end diff --git a/test/oli_web/live/workspaces/course_author/insights_live_test.exs b/test/oli_web/live/workspaces/course_author/insights_live_test.exs new file mode 100644 index 00000000000..ff936ea9410 --- /dev/null +++ b/test/oli_web/live/workspaces/course_author/insights_live_test.exs @@ -0,0 +1,287 @@ +defmodule OliWeb.Workspaces.CourseAuthor.InsightsLiveTest do + use OliWeb.ConnCase, async: true + + import Oli.Factory + import Phoenix.LiveViewTest + + alias Oli.Delivery.Sections + alias Oli.Resources.ResourceType + + defp insights_path(project_slug) do + ~p"/workspaces/course_author/#{project_slug}/insights" + end + + defp create_elixir_project(%{author: author}) do + project = insert(:project, authors: [author]) + + # revisions... + + ## pages... + page_1_revision = + insert(:revision, + resource_type_id: ResourceType.get_id_by_type("page"), + title: "Page 1", + duration_minutes: 10 + ) + + page_2_revision = + insert(:revision, + resource_type_id: ResourceType.get_id_by_type("page"), + title: "Page 2", + duration_minutes: 15 + ) + + page_3_revision = + insert(:revision, + resource_type_id: ResourceType.get_id_by_type("page"), + title: "Page 3", + graded: true, + purpose: :application + ) + + page_4_revision = + insert(:revision, + resource_type_id: ResourceType.get_id_by_type("page"), + title: "Page 4", + graded: true + ) + + ## modules... + module_1_revision = + insert(:revision, %{ + resource_type_id: ResourceType.get_id_by_type("container"), + children: [page_1_revision.resource_id, page_2_revision.resource_id], + title: "How to use this course" + }) + + module_2_revision = + insert(:revision, %{ + resource_type_id: ResourceType.get_id_by_type("container"), + children: [page_3_revision.resource_id], + title: "Configure your setup" + }) + + ## units... + unit_1_revision = + insert(:revision, %{ + resource_type_id: ResourceType.get_id_by_type("container"), + children: [ + module_1_revision.resource_id, + module_2_revision.resource_id + ], + title: "Introduction" + }) + + ## root container... + container_revision = + insert(:revision, %{ + resource_type_id: ResourceType.get_id_by_type("container"), + intro_content: %{ + "children" => [ + %{ + "children" => [%{"text" => "Welcome to the best course ever!"}], + "id" => "3477687079", + "type" => "p" + } + ], + "type" => "p" + }, + children: [ + unit_1_revision.resource_id, + page_4_revision.resource_id + ], + title: "Root Container" + }) + + all_revisions = + [ + page_1_revision, + page_2_revision, + page_3_revision, + page_4_revision, + module_1_revision, + module_2_revision, + unit_1_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) + + # create sections... + section_1 = + insert(:section, + base_project: project, + title: "The best course ever!", + start_date: ~U[2023-10-30 20:00:00Z], + analytics_version: :v2, + type: :enrollable + ) + + section_2 = + insert(:section, + base_project: project, + title: "Another best course ever!", + start_date: ~U[2023-10-30 20:00:00Z], + analytics_version: :v2, + type: :enrollable + ) + + # create a product... + product = + insert(:section, + base_project: project, + title: "The best product ever!", + start_date: ~U[2023-10-30 20:00:00Z], + analytics_version: :v2 + ) + + # build section resources... + Enum.each([section_1, section_2, product], fn section -> + {:ok, section} = Sections.create_section_resources(section, publication) + {:ok, _} = Sections.rebuild_contained_pages(section) + {:ok, _} = Sections.rebuild_contained_objectives(section) + end) + + %{ + author: author, + section_1: section_1, + project: project, + publication: publication, + page_1: page_1_revision, + page_2: page_2_revision, + page_3: page_3_revision, + page_4: page_4_revision, + module_1: module_1_revision, + module_2: module_2_revision, + unit_1: unit_1_revision, + container_revision: container_revision + } + end + + defp create_project_of_another_author(%{}) do + another_author = insert(:author) + project = insert(:project, authors: [another_author]) + + container_revision = + insert(:revision, %{ + resource_type_id: ResourceType.get_id_by_type("container"), + children: [], + title: "Root Container" + }) + + insert(:project_resource, %{ + project_id: project.id, + resource_id: container_revision.resource_id + }) + + # publish project + publication = + insert(:publication, %{ + project: project, + root_resource_id: container_revision.resource_id, + published: nil + }) + + # publish resources + insert(:published_resource, %{ + publication: publication, + resource: container_revision.resource, + revision: container_revision, + author: another_author + }) + + [project: project] + end + + describe "author cannot access when is not logged in" do + test "redirects to new session", %{conn: conn} do + assert {:error, + {:redirect, + %{ + to: "/workspaces/course_author" + }}} = + live(conn, insights_path("testproject")) + end + end + + describe "author can not access projects that do not belong to him" do + setup [:author_conn, :create_project_of_another_author] + + test "and gets redirected to the projects path", %{conn: conn, project: project} do + assert {:error, + {:redirect, + %{ + to: "/workspaces/course_author", + flash: %{"error" => "You don't have access to that project"} + }}} = + live(conn, insights_path(project.slug)) + end + end + + describe "project insights as author" do + setup [:author_conn, :create_elixir_project] + + test "loads correctly", %{conn: conn, project: project} do + {:ok, view, _html} = live(conn, insights_path(project.slug)) + + assert has_element?(view, "h5", "Viewing analytics by activity") + end + + test "lists all sections and products in corresponding multiselect", %{ + conn: conn, + project: project + } do + {:ok, view, _html} = live(conn, insights_path(project.slug)) + + # sections are listed in the sections multiselect + assert view + |> element("#sections-options-container") + |> render() =~ "The best course ever!" + + assert view + |> element("#sections-options-container") + |> render() =~ "Another best course ever!" + + refute view + |> element("#sections-options-container") + |> render() =~ "The best product ever!" + + # products are listed in the product multiselect + refute view + |> element("#products-options-container") + |> render() =~ "The best course ever!" + + refute view + |> element("#products-options-container") + |> render() =~ "Another best course ever!" + + assert view + |> element("#products-options-container") + |> render() =~ "The best product ever!" + end + end +end diff --git a/test/oli_web/live/workspaces/course_author_test.exs b/test/oli_web/live/workspaces/course_author_test.exs index 5718be04ab7..c1622f90291 100644 --- a/test/oli_web/live/workspaces/course_author_test.exs +++ b/test/oli_web/live/workspaces/course_author_test.exs @@ -627,6 +627,32 @@ defmodule OliWeb.Workspace.CourseAuthorTest do assert has_element?(view, "button", "Connect with LTI 1.3") assert has_element?(view, "button", "Publish") end + + test "insights menu is shown correctly", %{ + conn: conn, + project: project + } do + {:ok, view, _html} = live(conn, ~p"/workspaces/course_author/#{project.slug}/insights") + + assert has_element?( + view, + "p", + "Insights can help you improve your course by providing a statistical analysis of + the skills covered by each question to find areas where students are struggling." + ) + + assert has_element?(view, "button", "Raw Analytics") + + assert has_element?( + view, + "div", + "Project must be published to generate an analytics snapshot." + ) + + assert has_element?(view, "button", "By Activity") + assert has_element?(view, "button", "By Page") + assert has_element?(view, "button", "By Objective") + end end defp create_project_with_owner(owner, attrs \\ %{}) do From 5d50c54e06a02666a836ab8fc75bbca45fbaea64 Mon Sep 17 00:00:00 2001 From: Darren Siegel Date: Tue, 17 Sep 2024 10:00:19 -0400 Subject: [PATCH 3/8] [PERFORMANCE] [MER-3820] Speed up manual scoring queries (#5105) * add partial index * Auto format --------- Co-authored-by: darrensiegel --- .../20240917111312_add_lifecycle_index.exs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 priv/repo/migrations/20240917111312_add_lifecycle_index.exs diff --git a/priv/repo/migrations/20240917111312_add_lifecycle_index.exs b/priv/repo/migrations/20240917111312_add_lifecycle_index.exs new file mode 100644 index 00000000000..9d6b1f3a253 --- /dev/null +++ b/priv/repo/migrations/20240917111312_add_lifecycle_index.exs @@ -0,0 +1,14 @@ +defmodule Oli.Repo.Migrations.AddLifecycleIndex do + use Ecto.Migration + + def up do + # Creates a partial index on the lifecycle_state column where the value is 'submitted' + execute( + "CREATE INDEX activity_attempts_submitted ON activity_attempts (lifecycle_state) WHERE lifecycle_state = 'submitted';" + ) + end + + def down do + execute("DROP INDEX activity_attempts_submitted;") + end +end From 6e546642d65b47fa93049356441ca659eb7208cc Mon Sep 17 00:00:00 2001 From: Eli Knebel Date: Wed, 18 Sep 2024 07:31:18 -0400 Subject: [PATCH 4/8] [BUG FIX] [MER-3620] User account menu overflow and link author account fix (#5102) * fix edit registration redirect * fix OliWeb.Workspaces scopes * fix route * fix account linking and add unlink capability fix an issue where direct delivery redirects lead to LMS sign in * add drop shadow to user menu * always show user menu first, then author fix a timezone triggering update issue * fix errors and warnings * fix test * Auto format --------- Co-authored-by: eliknebel --- .../src/components/common/SelectTimezone.tsx | 6 +- .../src/components/misc/DarkModeSelector.tsx | 6 +- lib/oli/accounts.ex | 4 + lib/oli/delivery/sections.ex | 14 + lib/oli_web/components/common.ex | 22 +- lib/oli_web/components/delivery/layouts.ex | 2 +- .../components/delivery/user_account.ex | 83 +++--- .../controllers/delivery_controller.ex | 240 ++++++++---------- .../pow/registration_html/edit.html.heex | 2 +- .../session_html/link_authoring_account.heex | 62 +++++ .../live/workspaces/account_details_live.ex | 2 +- lib/oli_web/live/workspaces/sub_menu_item.ex | 2 +- lib/oli_web/live/workspaces/utils.ex | 4 +- lib/oli_web/plugs/maybe_load_lti_params.ex | 29 +++ lib/oli_web/plugs/registration_captcha.ex | 35 --- lib/oli_web/pow/author_routes.ex | 10 +- lib/oli_web/pow/pow_helpers.ex | 6 +- lib/oli_web/pow/user_routes.ex | 11 +- lib/oli_web/router.ex | 16 +- .../controllers/delivery_controller_test.exs | 32 +-- .../controllers/workspace_controller_test.exs | 4 +- .../live/workspaces/course_author_test.exs | 2 +- .../instructor/dashboard_live_test.exs | 2 +- .../live/workspaces/instructor_test.exs | 28 +- test/oli_web/live/workspaces/student_test.exs | 21 +- 25 files changed, 345 insertions(+), 300 deletions(-) create mode 100644 lib/oli_web/controllers/pow/session_html/link_authoring_account.heex create mode 100644 lib/oli_web/plugs/maybe_load_lti_params.ex diff --git a/assets/src/components/common/SelectTimezone.tsx b/assets/src/components/common/SelectTimezone.tsx index d89e9e145d6..0b3db51eb30 100644 --- a/assets/src/components/common/SelectTimezone.tsx +++ b/assets/src/components/common/SelectTimezone.tsx @@ -10,7 +10,11 @@ export const SelectTimezone: React.FC = ({ submitAction, }) => { const ref = useRef(null); - const onSelect = ({ target: { value } }: any) => { + const onSelect = ({ target: { value }, isTrusted, nativeEvent }: any) => { + // Only submit the form if the change event was triggered by a user action to + // prevent this from being triggered by the browser's autofill feature or + // any react re-renders. + if (!isTrusted || !nativeEvent) return; ref.current?.submit(); }; diff --git a/assets/src/components/misc/DarkModeSelector.tsx b/assets/src/components/misc/DarkModeSelector.tsx index 2b2e8bab55a..2bbe3c90dbe 100644 --- a/assets/src/components/misc/DarkModeSelector.tsx +++ b/assets/src/components/misc/DarkModeSelector.tsx @@ -50,7 +50,7 @@ export const DarkModeSelector = ({ showLabels = true }: DarkModeSelectorProps) = -
+
- "rounded text-body-color bg-transparent hover:bg-gray-200 active:text-white active:bg-primary-700 focus:ring-2 focus:ring-primary-400 dark:text-body-color-dark dark:hover:bg-gray-600 dark:active:bg-primary-400 focus:outline-none dark:focus:ring-primary-700 hover:no-underline" + "rounded text-body-color hover:text-body-color bg-transparent hover:bg-gray-200 active:text-white active:bg-primary-700 focus:ring-2 focus:ring-primary-400 dark:text-body-color-dark dark:hover:bg-gray-600 dark:active:bg-primary-400 focus:outline-none dark:focus:ring-primary-700 hover:no-underline" :tertiary -> - "rounded text-primary-700 bg-primary-50 hover:bg-primary-100 active:bg-primary-200 focus:ring-2 focus:ring-primary-100 dark:text-primary-300 dark:bg-primary-800 dark:hover:bg-primary-700 dark:active:bg-primary-600 focus:outline-none dark:focus:ring-primary-800 hover:no-underline" + "rounded text-primary-700 hover:text-primary-700 bg-primary-50 hover:bg-primary-100 active:bg-primary-200 focus:ring-2 focus:ring-primary-100 dark:text-primary-300 dark:bg-primary-800 dark:hover:bg-primary-700 dark:active:bg-primary-600 focus:outline-none dark:focus:ring-primary-800 hover:no-underline" :light -> - "rounded text-body-color bg-gray-100 hover:bg-gray-200 active:bg-gray-300 focus:ring-2 focus:ring-gray-100 dark:text-white dark:bg-gray-800 dark:hover:bg-gray-700 dark:active:bg-gray-600 focus:outline-none dark:focus:ring-gray-800 hover:no-underline" + "rounded text-body-color hover:text-body-color bg-gray-100 hover:bg-gray-200 active:bg-gray-300 focus:ring-2 focus:ring-gray-100 dark:text-white dark:bg-gray-800 dark:hover:bg-gray-700 dark:active:bg-gray-600 focus:outline-none dark:focus:ring-gray-800 hover:no-underline" :dark -> - "rounded text-white bg-gray-700 hover:bg-gray-800 active:bg-gray-600 focus:ring-2 focus:ring-gray-600 dark:text-white dark:bg-gray-700 dark:hover:bg-gray-600 dark:active:bg-gray-500 focus:outline-none dark:focus:ring-gray-500 hover:no-underline" + "rounded text-white hover:text-white bg-gray-700 hover:bg-gray-800 active:bg-gray-600 focus:ring-2 focus:ring-gray-600 dark:text-white dark:bg-gray-700 dark:hover:bg-gray-600 dark:active:bg-gray-500 focus:outline-none dark:focus:ring-gray-500 hover:no-underline" :info -> - "rounded text-white bg-gray-500 hover:bg-gray-600 active:bg-gray-700 focus:ring-2 focus:ring-gray-400 dark:bg-gray-600 dark:hover:bg-gray-500 dark:active:bg-gray-400 focus:outline-none dark:focus:ring-gray-700 hover:no-underline" + "rounded text-white hover:text-white bg-gray-500 hover:bg-gray-600 active:bg-gray-700 focus:ring-2 focus:ring-gray-400 dark:bg-gray-600 dark:hover:bg-gray-500 dark:active:bg-gray-400 focus:outline-none dark:focus:ring-gray-700 hover:no-underline" :success -> - "rounded text-white bg-green-600 hover:bg-green-700 active:bg-green-800 focus:ring-2 focus:ring-green-700 dark:bg-green-600 dark:hover:bg-green-500 dark:active:bg-green-400 focus:outline-none dark:focus:ring-green-700 hover:no-underline" + "rounded text-white hover:text-white bg-green-600 hover:bg-green-700 active:bg-green-800 focus:ring-2 focus:ring-green-700 dark:bg-green-600 dark:hover:bg-green-500 dark:active:bg-green-400 focus:outline-none dark:focus:ring-green-700 hover:no-underline" :warning -> - "rounded text-white bg-yellow-500 hover:bg-yellow-600 active:bg-yellow-700 focus:ring-2 focus:ring-yellow-400 dark:bg-yellow-600 dark:hover:bg-yellow-500 dark:active:bg-yellow-400 focus:outline-none dark:focus:ring-yellow-700 hover:no-underline" + "rounded text-white hover:text-white bg-yellow-500 hover:bg-yellow-600 active:bg-yellow-700 focus:ring-2 focus:ring-yellow-400 dark:bg-yellow-600 dark:hover:bg-yellow-500 dark:active:bg-yellow-400 focus:outline-none dark:focus:ring-yellow-700 hover:no-underline" :danger -> - "rounded text-white bg-red-500 hover:bg-red-600 active:bg-red-700 focus:ring-2 focus:ring-red-400 dark:bg-red-600 dark:hover:bg-red-500 dark:active:bg-red-400 focus:outline-none dark:focus:ring-red-700 hover:no-underline" + "rounded text-white hover:text-white bg-red-500 hover:bg-red-600 active:bg-red-700 focus:ring-2 focus:ring-red-400 dark:bg-red-600 dark:hover:bg-red-500 dark:active:bg-red-400 focus:outline-none dark:focus:ring-red-700 hover:no-underline" :link -> - "rounded text-blue-500 hover:text-blue-600 active:text-blue-700 focus:ring-2 focus:ring-blue-400 dark:text-blue-600 dark:hover:text-blue-500 dark:active:text-blue-400 focus:outline-none dark:focus:ring-blue-700 hover:underline cursor-pointer" + "rounded text-blue-500 hover:text-blue-600 hover:text-blue-600 active:text-blue-700 focus:ring-2 focus:ring-blue-400 dark:text-blue-600 dark:hover:text-blue-500 dark:active:text-blue-400 focus:outline-none dark:focus:ring-blue-700 hover:underline cursor-pointer" :link_info -> "rounded text-gray-500 hover:text-gray-600 active:text-gray-700 focus:ring-2 focus:ring-gray-400 dark:text-gray-600 dark:hover:text-gray-500 dark:active:text-gray-400 focus:outline-none dark:focus:ring-gray-700 hover:underline cursor-pointer" @@ -205,7 +205,7 @@ defmodule OliWeb.Components.Common do <.dropdown_menu id={"#{@id}-dropdown"} class={@dropdown_class}> - <%= case assigns.ctx do %> - <% %SessionContext{author: %Author{}} -> %> + <%= case {assigns.ctx, @is_system_admin} do %> + <% {%SessionContext{author: %Author{}}, true} -> %> <.author_menu_items id={"#{@id}-menu-items-admin"} ctx={@ctx} is_system_admin={@is_system_admin} /> - <% %SessionContext{user: %User{guest: true}} -> %> + <% {%SessionContext{user: %User{guest: true}}, _} -> %> <.guest_menu_items id={"#{@id}-menu-items-admin"} ctx={@ctx} /> - <% %SessionContext{user: %User{}} -> %> + <% {%SessionContext{user: %User{}}, _} -> %> <.user_menu_items id={"#{@id}-menu-items-admin"} ctx={@ctx} /> + <% {%SessionContext{author: %Author{}}, _} -> %> + <.author_menu_items + id={"#{@id}-menu-items-admin"} + ctx={@ctx} + is_system_admin={@is_system_admin} + /> <% _ -> %> <% end %> @@ -205,7 +211,6 @@ defmodule OliWeb.Components.Delivery.UserAccount do def user_menu_items(assigns) do ~H""" - <.menu_item_maybe_linked_account user={@ctx.user} /> <.maybe_menu_item_edit_user_account user={@ctx.user} /> <.maybe_my_courses_menu_item_link user={@ctx.user} /> <.menu_item_dark_mode_selector id={"#{@id}-dark-mode-selector"} ctx={@ctx} /> @@ -213,6 +218,7 @@ defmodule OliWeb.Components.Delivery.UserAccount do <.menu_item_timezone_selector id={"#{@id}-tz-selector"} ctx={@ctx} /> <.menu_divider /> <.maybe_research_consent_link ctx={@ctx} /> + <.menu_item_maybe_linked_account user={@ctx.user} /> <.menu_item_link href={ Routes.session_path(OliWeb.Endpoint, :signout, @@ -258,7 +264,7 @@ defmodule OliWeb.Components.Delivery.UserAccount do
    <%= render_slot(@inner_block) %> @@ -271,7 +277,7 @@ defmodule OliWeb.Components.Delivery.UserAccount do def menu_item(assigns) do ~H""" -
  • +
  • <%= render_slot(@inner_block) %>
    • """ @@ -279,8 +285,8 @@ defmodule OliWeb.Components.Delivery.UserAccount do def menu_divider(assigns) do ~H""" -
  • - +
  • +
    • """ end @@ -294,14 +300,14 @@ defmodule OliWeb.Components.Delivery.UserAccount do case assigns[:method] do nil -> ~H""" - <%= link to: @href, class: "w-full text-gray-800 hover:text-gray-800 dark:text-white hover:text-white text-sm font-normal font-['Roboto'] h-[26px] p-[5px] rounded-md justify-start items-center inline-flex block hover:no-underline dark:hover:bg-white/5 hover:bg-gray-200 cursor-pointer", target: @target do %> + <%= link to: @href, class: "w-full text-gray-800 hover:text-gray-800 dark:text-white hover:text-white text-sm font-normal font-['Roboto'] h-[26px] p-[5px] rounded-md justify-start items-center inline-flex block hover:no-underline dark:hover:bg-white/5 hover:bg-gray-100 cursor-pointer", target: @target do %> <%= render_slot(@inner_block) %> <% end %> """ _method -> ~H""" - <%= link to: @href, method: @method, class: "w-[190px] text-gray-800 hover:text-white dark:text-white text-sm font-normal font-['Roboto'] h-8 px-1.5 py-2 mt-[10px] m-[5px] rounded-md border border-rose-400 justify-center items-center gap-2.5 inline-flex cursor-pointer hover:no-underline hover:bg-red-300 hover:border-red-500 dark:hover:bg-[#33181A]", target: @target do %> + <%= link to: @href, method: @method, class: "w-full text-gray-800 hover:text-white dark:text-white text-sm font-normal font-['Roboto'] h-8 px-1.5 py-2 mt-[10px] m-[5px] rounded-md border border-rose-400 justify-center items-center gap-2.5 inline-flex cursor-pointer hover:no-underline hover:bg-red-300 hover:border-red-500 dark:hover:bg-[#33181A]", target: @target do %> <%= render_slot(@inner_block) %> <% end %> """ @@ -312,35 +318,34 @@ defmodule OliWeb.Components.Delivery.UserAccount do def menu_item_maybe_linked_account(assigns) do ~H""" - <%= case linked_author_account(@user) do %> - <% nil -> %> - <%= if Sections.is_independent_instructor?(@user) do %> + <%= if can_manage_linked_account?(@user) do %> + <%= case linked_author_account(@user) do %> + <% nil -> %> <.menu_item_link href={Routes.delivery_path(OliWeb.Endpoint, :link_account)}> Link authoring account + <% linked_author_account_email -> %> + <.menu_item> + <.menu_item_label>Linked Authoring Account + - <.menu_divider /> - <% end %> - <% linked_author_account_email -> %> - <.menu_item> -
    Linked Authoring Account:
    - -
    -
    <%= linked_author_account_email %>
    -
    + <.menu_item_link href={Routes.delivery_path(OliWeb.Endpoint, :link_account)}> +
    + <%= linked_author_account_email %>
    -     - - - <.menu_item_link href={Routes.delivery_path(OliWeb.Endpoint, :link_account)}> - Link a different account - + + <% end %> - <.menu_divider /> + <.menu_divider /> <% end %> """ end + defp can_manage_linked_account?(user) do + Sections.is_independent_instructor?(user) || Sections.is_institution_instructor?(user) || + Sections.is_institution_admin?(user) + end + attr(:user, User, required: true) def maybe_menu_item_edit_user_account(assigns) do @@ -374,7 +379,7 @@ defmodule OliWeb.Components.Delivery.UserAccount do def menu_item_edit_author_account(assigns) do ~H""" - <.menu_item_link href={Routes.live_path(OliWeb.Endpoint, OliWeb.Workspace.AccountDetailsLive)}> + <.menu_item_link href={Routes.live_path(OliWeb.Endpoint, OliWeb.Workspaces.AccountDetailsLive)}> Edit Account @@ -388,7 +393,7 @@ defmodule OliWeb.Components.Delivery.UserAccount do def menu_item_dark_mode_selector(assigns) do ~H""" <.menu_item> -
    THEME
    + <.menu_item_label>Theme
    <%= React.component( @ctx, @@ -409,14 +414,24 @@ defmodule OliWeb.Components.Delivery.UserAccount do def menu_item_timezone_selector(assigns) do ~H""" <.menu_item> -
    TIMEZONE
    -
    + <.menu_item_label>Timezone +
    """ end + slot :inner_block, required: true + + defp menu_item_label(assigns) do + ~H""" +
    + <%= render_slot(@inner_block) %> +
    + """ + end + attr(:is_system_admin, :boolean, required: true) @spec maybe_menu_item_open_admin_panel(map()) :: Phoenix.LiveView.Rendered.t() diff --git a/lib/oli_web/controllers/delivery_controller.ex b/lib/oli_web/controllers/delivery_controller.ex index 617cb51af81..00a7bd3e85d 100644 --- a/lib/oli_web/controllers/delivery_controller.ex +++ b/lib/oli_web/controllers/delivery_controller.ex @@ -16,6 +16,7 @@ defmodule OliWeb.DeliveryController do alias Oli.Repo.{Paging, Sorting} alias OliWeb.Common.Params alias OliWeb.Delivery.InstructorDashboard.Helpers + alias OliWeb.Components.Delivery.UserAccount require Logger @@ -41,47 +42,62 @@ defmodule OliWeb.DeliveryController do ) end - def index(conn, _params) do - user = conn.assigns.current_user - lti_params = conn.assigns.lti_params - - lti_roles = lti_params["https://purl.imsglobal.org/spec/lti/claim/roles"] - context_roles = ContextRoles.get_roles_by_uris(lti_roles) - platform_roles = PlatformRoles.get_roles_by_uris(lti_roles) - roles = MapSet.new(context_roles ++ platform_roles) - allow_configure_section_roles = MapSet.new(@allow_configure_section_roles) - - # allow section configuration if user has any of the allowed roles - allow_configure_section = - MapSet.intersection(roles, allow_configure_section_roles) |> MapSet.size() > 0 - - section = Sections.get_section_from_lti_params(lti_params) - - case section do - # author account has not been linked - nil when allow_configure_section -> - render_getting_started(conn) + @doc """ + This is the default entry point for delivery users. It will redirect to the appropriate page based + on whether the user is an independent learner or an LTI user. - nil -> - render_course_not_configured(conn) - - section when allow_configure_section -> - redirect_to_instructor_dashboard(conn, section) + If the user is an independent learner, they will be redirected to the student workspace. - # section has been configured - section -> - {institution, _registration, _deployment} = - Institutions.get_institution_registration_deployment( - lti_params["iss"], - LtiParams.peek_client_id(lti_params), - lti_params["https://purl.imsglobal.org/spec/lti/claim/deployment_id"] - ) + If the user is an LTI user, the user's roles will be checked to determine if they are allowed to + configure the section. If they are allowed to configure the section, they will be redirected to + the instructor dashboard. If they are not allowed to configure the section, the student will be + redirected to the page delivery. + """ + def index(conn, _params) do + user = conn.assigns.current_user - if institution.research_consent != :no_form and is_nil(user.research_opt_out) do - render_research_consent(conn, ~p"/sections/#{section.slug}") - else - redirect_to_page_delivery(conn, section) - end + if user.independent_learner do + redirect(conn, to: ~p"/workspaces/student") + else + lti_params = conn.assigns.lti_params + + lti_roles = lti_params["https://purl.imsglobal.org/spec/lti/claim/roles"] + context_roles = ContextRoles.get_roles_by_uris(lti_roles) + platform_roles = PlatformRoles.get_roles_by_uris(lti_roles) + roles = MapSet.new(context_roles ++ platform_roles) + allow_configure_section_roles = MapSet.new(@allow_configure_section_roles) + + # allow section configuration if user has any of the allowed roles + allow_configure_section = + MapSet.intersection(roles, allow_configure_section_roles) |> MapSet.size() > 0 + + section = Sections.get_section_from_lti_params(lti_params) + + case section do + nil when allow_configure_section -> + render_getting_started(conn) + + nil -> + render_course_not_configured(conn) + + section when allow_configure_section -> + redirect_to_instructor_dashboard(conn, section) + + # section has been configured + section -> + {institution, _registration, _deployment} = + Institutions.get_institution_registration_deployment( + lti_params["iss"], + LtiParams.peek_client_id(lti_params), + lti_params["https://purl.imsglobal.org/spec/lti/claim/deployment_id"] + ) + + if institution.research_consent != :no_form and is_nil(user.research_opt_out) do + render_research_consent(conn, institution, ~p"/sections/#{section.slug}") + else + redirect_to_page_delivery(conn, section) + end + end end end @@ -93,7 +109,7 @@ defmodule OliWeb.DeliveryController do render(conn, "getting_started.html") end - defp render_research_consent(conn, redirect_url) do + defp render_research_consent(conn, institution, redirect_url) do case conn.assigns.current_user do nil -> conn @@ -117,9 +133,6 @@ defmodule OliWeb.DeliveryController do # LTI users user -> - # check institution research consent setting - institution = Institutions.get_institution_by_lti_user(user) - case institution do %Institution{research_consent: :oli_form} -> conn @@ -165,9 +178,11 @@ defmodule OliWeb.DeliveryController do ~p"/course" end + institution = Institutions.get_institution_by_lti_user(user) + conn |> assign(:research_opt_out, user.research_opt_out) - |> render_research_consent(redirect_url) + |> render_research_consent(institution, redirect_url) end def research_consent(conn, %{"consent" => consent, "redirect_url" => redirect_url}) do @@ -204,28 +219,25 @@ defmodule OliWeb.DeliveryController do end def render_link_account_form(conn, opts \\ []) do - title = Keyword.get(opts, :title, "Link Existing Account") changeset = Keyword.get(opts, :changeset, Author.noauth_changeset(%Author{})) - action = Keyword.get(opts, :action, Routes.delivery_path(conn, :process_link_account_user)) - - create_account_path = - Keyword.get( - opts, - :create_account_path, - Routes.delivery_path(conn, :create_and_link_account) - ) + action = Keyword.get(opts, :action, Routes.delivery_path(conn, :process_link_account)) cancel_path = Keyword.get(opts, :cancel_path, Routes.delivery_path(conn, :index)) + linked_account_email = UserAccount.linked_author_account(conn.assigns.current_user) + conn - |> assign(:title, title) |> assign(:changeset, changeset) |> assign(:action, action) - |> assign(:create_account_path, create_account_path) + |> assign(:linked_account, linked_account_email) + # link_account_provider_path assign required for proper provider link generation + |> assign( + :link_account_provider_path, + &Routes.authoring_delivery_path(conn, :process_link_account_provider, &1) + ) |> assign(:cancel_path, cancel_path) - |> assign(:link_account, true) |> put_view(OliWeb.Pow.SessionHTML) - |> Phoenix.Controller.render("new.html") + |> Phoenix.Controller.render("link_authoring_account.html") end def process_link_account_provider(conn, %{"provider" => provider}) do @@ -243,33 +255,6 @@ defmodule OliWeb.DeliveryController do end end - def process_link_account_user(conn, %{"user" => author_params}) do - conn - |> use_pow_config(:author) - |> Pow.Plug.authenticate_user(author_params) - |> case do - {:ok, conn} -> - conn - |> put_flash( - :info, - Pow.Phoenix.Controller.messages(conn, Pow.Phoenix.Messages).signed_in(conn) - ) - |> redirect( - to: Pow.Phoenix.Controller.routes(conn, Pow.Phoenix.Routes).after_sign_in_path(conn) - ) - - {:error, conn} -> - conn - |> put_flash( - :error, - Pow.Phoenix.Controller.messages(conn, Pow.Phoenix.Messages).invalid_credentials(conn) - ) - |> render_link_account_form( - changeset: PowAssent.Plug.change_user(conn, %{}, author_params) - ) - end - end - def link_account_callback(conn, %{"provider" => provider} = params) do conn = conn @@ -300,40 +285,56 @@ defmodule OliWeb.DeliveryController do |> PowAssent.Phoenix.AuthorizationController.respond_callback() end - def create_and_link_account(conn, _params) do - # sign out current author account - conn - |> delete_pow_user(:author) - |> render_create_and_link_form() - end + def process_link_account(conn, %{"action" => "unlink"}) do + case Accounts.unlink_user_author_account(conn.assigns.current_user) do + {:ok, _user} -> + conn + |> put_flash(:info, "Account is now unlinked") + |> redirect(to: Routes.delivery_path(conn, :index)) - @spec process_create_and_link_account_user(Plug.Conn.t(), map()) :: Plug.Conn.t() - def process_create_and_link_account_user(conn, %{"user" => user_params}) do - %{current_user: current_user} = conn.assigns + _ -> + conn + |> put_flash(:error, "Failed to unlink account") + |> redirect(to: Routes.delivery_path(OliWeb.Endpoint, :link_account)) + end + end + def process_link_account(conn, %{"link_account" => author_params}) do conn |> use_pow_config(:author) - |> Pow.Plug.create_user(user_params) + |> Pow.Plug.authenticate_user(author_params) |> case do - {:ok, _user, conn} -> - conn - |> put_flash( - :info, - Pow.Phoenix.Controller.messages(conn, Pow.Phoenix.Messages).user_has_been_created(conn) - ) + {:ok, conn} -> + %{current_user: current_user} = conn.assigns + current_author = Accounts.get_author_by_email(author_params["email"]) - Pow.Phoenix.Controller.routes(conn, Pow.Phoenix.Routes).after_registration_path(conn) - conn = Pow.Plug.Session.do_delete(conn, get_pow_config(:author)) + case Accounts.link_user_author_account(current_user, current_author) do + {:ok, _user} -> + conn + |> put_flash( + :info, + "Account is now linked to authoring account #{current_author.email}" + ) + |> redirect(to: Routes.delivery_path(conn, :index)) - if current_user.independent_learner do - redirect(conn, to: ~p"/workspaces/student") - else - redirect(conn, to: Routes.delivery_path(conn, :index)) + _ -> + conn + |> put_flash( + :error, + "Failed to link authoring account #{current_author.email}" + ) + |> redirect(to: Routes.delivery_path(conn, :index)) end - {:error, changeset, conn} -> + {:error, conn} -> conn - |> render_create_and_link_form(changeset: changeset) + |> put_flash( + :error, + Pow.Phoenix.Controller.messages(conn, Pow.Phoenix.Messages).invalid_credentials(conn) + ) + |> render_link_account_form( + changeset: PowAssent.Plug.change_user(conn, %{}, author_params) + ) end end @@ -361,31 +362,6 @@ defmodule OliWeb.DeliveryController do |> Phoenix.Controller.render("new.html") end - def render_create_and_link_form(conn, opts \\ []) do - title = Keyword.get(opts, :title, "Create and Link Account") - changeset = Keyword.get(opts, :changeset, Author.noauth_changeset(%Author{})) - - action = - Keyword.get( - opts, - :action, - Routes.delivery_path(conn, :process_create_and_link_account_user) - ) - - sign_in_path = Keyword.get(opts, :sign_in_path, Routes.delivery_path(conn, :link_account)) - cancel_path = Keyword.get(opts, :cancel_path, Routes.delivery_path(conn, :index)) - - conn - |> assign(:title, title) - |> assign(:changeset, changeset) - |> assign(:action, action) - |> assign(:sign_in_path, sign_in_path) - |> assign(:cancel_path, cancel_path) - |> assign(:link_account, true) - |> put_view(OliWeb.Pow.RegistrationHTML) - |> Phoenix.Controller.render("new.html") - end - def signin(conn, %{"section" => section}) do conn |> delete_pow_user(:user) diff --git a/lib/oli_web/controllers/pow/registration_html/edit.html.heex b/lib/oli_web/controllers/pow/registration_html/edit.html.heex index c6c72ca34f7..aa8ddacd2ac 100644 --- a/lib/oli_web/controllers/pow/registration_html/edit.html.heex +++ b/lib/oli_web/controllers/pow/registration_html/edit.html.heex @@ -74,7 +74,7 @@ to: value_or( assigns[:cancel_path], - Routes.live_path(@conn, OliWeb.Workspace.AccountDetailsLive) + Routes.live_path(@conn, OliWeb.Workspaces.AccountDetailsLive) ), class: "btn btn-md btn-outline-secondary btn-block mt-3" ) %> diff --git a/lib/oli_web/controllers/pow/session_html/link_authoring_account.heex b/lib/oli_web/controllers/pow/session_html/link_authoring_account.heex new file mode 100644 index 00000000000..1671813b2b1 --- /dev/null +++ b/lib/oli_web/controllers/pow/session_html/link_authoring_account.heex @@ -0,0 +1,62 @@ +<%= render OliWeb.SharedView, "_box_form_container.html", Map.merge(assigns, %{title: value_or(assigns[:title], "Link Authoring Account"), bs_col_class: "mx-auto mt-10"}) do %> +

    + Sign in with your authoring account credentials below to link your account. +

    + + <%= for link <- OliWeb.Pow.PowHelpers.provider_links(@conn), do: raw(link) %> + +
    + + <%= form_for @changeset, @action, [as: :link_account], fn f -> %> +
    + <%= email_input(f, Pow.Ecto.Schema.user_id_field(@changeset), + class: "form-control", + placeholder: "Email", + required: true, + autofocus: true + ) %> + <%= label(f, Pow.Ecto.Schema.user_id_field(@changeset), class: "control-label") %> + <%= error_tag(f, Pow.Ecto.Schema.user_id_field(@changeset)) %> +
    +
    + <%= password_input(f, :password, + class: "form-control", + placeholder: "Password", + required: true + ) %> + <%= label(f, :password, class: "control-label") %> + <%= error_tag(f, :password) %> +
    +
    +
    + <%= link("Forgot password?", + to: Routes.authoring_pow_reset_password_reset_password_path(@conn, :new), + tabindex: "1" + ) %> +
    +
    + + <.button type="submit" variant={:primary} class="w-full inline-block text-center"> + Link Account + + <%= if @linked_account do %> + <.button + variant={:tertiary} + type="submit" + name="action" + value="unlink" + class="w-full inline-block text-center mt-2" + > + Unlink <%= @linked_account %> + + <% end %> + + <.button + variant={:secondary} + href={value_or(assigns[:cancel_path], Routes.static_page_path(@conn, :index))} + class="w-full inline-block text-center mt-2" + > + Cancel + + <% end %> +<% end %> diff --git a/lib/oli_web/live/workspaces/account_details_live.ex b/lib/oli_web/live/workspaces/account_details_live.ex index 3b2801d3e55..6a9ed75b166 100644 --- a/lib/oli_web/live/workspaces/account_details_live.ex +++ b/lib/oli_web/live/workspaces/account_details_live.ex @@ -1,4 +1,4 @@ -defmodule OliWeb.Workspace.AccountDetailsLive do +defmodule OliWeb.Workspaces.AccountDetailsLive do use OliWeb, :live_view alias OliWeb.Router.Helpers, as: Routes diff --git a/lib/oli_web/live/workspaces/sub_menu_item.ex b/lib/oli_web/live/workspaces/sub_menu_item.ex index 681a0811106..b966bd48691 100644 --- a/lib/oli_web/live/workspaces/sub_menu_item.ex +++ b/lib/oli_web/live/workspaces/sub_menu_item.ex @@ -1,4 +1,4 @@ -defmodule OliWeb.Workspace.SubMenuItem do +defmodule OliWeb.Workspaces.SubMenuItem do @moduledoc """ Struct for a sub-menu item. """ diff --git a/lib/oli_web/live/workspaces/utils.ex b/lib/oli_web/live/workspaces/utils.ex index b5d28434165..35d1069cae3 100644 --- a/lib/oli_web/live/workspaces/utils.ex +++ b/lib/oli_web/live/workspaces/utils.ex @@ -1,4 +1,4 @@ -defmodule OliWeb.Workspace.Utils do +defmodule OliWeb.Workspaces.Utils do @moduledoc """ Utility functions for workspace components. """ @@ -7,7 +7,7 @@ defmodule OliWeb.Workspace.Utils do import OliWeb.Components.Utils alias Phoenix.LiveView.JS - alias OliWeb.Workspace.SubMenuItem + alias OliWeb.Workspaces.SubMenuItem alias OliWeb.Icons attr :hierarchy, :list diff --git a/lib/oli_web/plugs/maybe_load_lti_params.ex b/lib/oli_web/plugs/maybe_load_lti_params.ex new file mode 100644 index 00000000000..5ecb1aa8a58 --- /dev/null +++ b/lib/oli_web/plugs/maybe_load_lti_params.ex @@ -0,0 +1,29 @@ +defmodule Oli.Plugs.MaybeLoadLtiParams do + import Plug.Conn + + alias OliWeb.Common.LtiSession + alias Oli.Lti.LtiParams + + def init(opts), do: opts + + def call(conn, _opts) do + case LtiSession.get_session_lti_params(conn) do + nil -> + conn + + lti_params_id -> + # load cached lti params from database + load_lti_params(conn, lti_params_id) + end + end + + defp load_lti_params(conn, lti_params_id) do + case LtiParams.get_lti_params(lti_params_id) do + nil -> + conn + + %{params: params} -> + assign(conn, :lti_params, params) + end + end +end diff --git a/lib/oli_web/plugs/registration_captcha.ex b/lib/oli_web/plugs/registration_captcha.ex index 74ed60806f1..71f79861846 100644 --- a/lib/oli_web/plugs/registration_captcha.ex +++ b/lib/oli_web/plugs/registration_captcha.ex @@ -10,18 +10,6 @@ defmodule Oli.Plugs.RegistrationCaptcha do author_register_path = Routes.authoring_pow_registration_path(conn, :create) - register_and_link_provider_path = - case conn do - %{params: %{"provider" => provider}} -> - Routes.pow_assent_registration_path(conn, :create, provider) - - _ -> - nil - end - - register_and_link_user_path = - Routes.delivery_path(conn, :process_create_and_link_account_user) - case conn do %Plug.Conn{method: "POST", request_path: ^author_register_path} when author_register_path != nil -> @@ -30,14 +18,6 @@ defmodule Oli.Plugs.RegistrationCaptcha do %Plug.Conn{method: "POST", request_path: ^register_path} when register_path != nil -> verify_captcha(conn, :register) - %Plug.Conn{method: "POST", request_path: ^register_and_link_provider_path} - when register_and_link_provider_path != nil -> - verify_captcha(conn, :create_and_link_account) - - %Plug.Conn{method: "POST", request_path: ^register_and_link_user_path} - when register_and_link_user_path != nil -> - verify_captcha(conn, :create_and_link_account) - _ -> conn end @@ -90,19 +70,4 @@ defmodule Oli.Plugs.RegistrationCaptcha do |> OliWeb.DeliveryController.render_user_register_form(changeset) |> halt() end - - defp render_captcha_error(conn, :create_and_link_account) do - conn = - conn - |> OliWeb.Pow.PowHelpers.use_pow_config(:author) - - changeset = Pow.Plug.change_user(conn, conn.params["user"]) - changeset = Ecto.Changeset.add_error(changeset, :captcha, "failed, please try again") - - conn - |> OliWeb.DeliveryController.render_create_and_link_form( - changeset: %{changeset | action: :insert} - ) - |> halt() - end end diff --git a/lib/oli_web/pow/author_routes.ex b/lib/oli_web/pow/author_routes.ex index e487b162e08..98c04155ca3 100644 --- a/lib/oli_web/pow/author_routes.ex +++ b/lib/oli_web/pow/author_routes.ex @@ -83,28 +83,28 @@ defmodule OliWeb.Pow.AuthorRoutes do @impl true def after_user_updated_path(conn) do - Routes.live_path(conn, OliWeb.Workspace.AccountDetailsLive) + Routes.live_path(conn, OliWeb.Workspaces.AccountDetailsLive) end @impl true def path_for( - %Plug.Conn{assigns: %{link_account: true}} = conn, + %Plug.Conn{assigns: %{link_account_provider_path: link_account_provider_path}}, PowAssent.Phoenix.AuthorizationController, :new, [provider], _query_params ) do - Routes.authoring_delivery_path(conn, :process_link_account_provider, provider) + link_account_provider_path.(provider) end def path_for( - %Plug.Conn{assigns: %{link_account: true}} = conn, + %Plug.Conn{assigns: %{link_account_provider_path: link_account_provider_path}}, PowAssent.Phoenix.AuthorizationController, :create, [provider], _query_params ) do - Routes.authoring_delivery_path(conn, :process_link_account_provider, provider) + link_account_provider_path.(provider) end def path_for(conn, PowInvitation.Phoenix.InvitationController, :update, [token], query_params) do diff --git a/lib/oli_web/pow/pow_helpers.ex b/lib/oli_web/pow/pow_helpers.ex index 67e0be214fc..f3ca0f66a6f 100644 --- a/lib/oli_web/pow/pow_helpers.ex +++ b/lib/oli_web/pow/pow_helpers.ex @@ -126,7 +126,7 @@ defmodule OliWeb.Pow.PowHelpers do on through the URL query params. """ def authorization_link(conn, provider, opts \\ []) do - opts = build_otps(opts, conn, provider) + opts = build_opts(opts, conn, provider) button_box = build_button_box(conn, provider) @@ -155,14 +155,14 @@ defmodule OliWeb.Pow.PowHelpers do ) end - defp build_otps(opts, conn, provider) do + defp build_opts(opts, conn, provider) do path = build_authentication_path(conn, provider) opts = Keyword.merge(opts, to: path) Keyword.merge(opts, class: - "btn btn-md flex items-center no-underline hover:no-underline hover:text-black w-80 m-auto mb-2 h-11 bg-white rounded-md text-center text-black text-base font-normal font-['Open Sans'] leading-snug" + "btn btn-md flex items-center no-underline hover:no-underline hover:text-black w-80 m-auto mb-2 h-11 bg-white border border-gray-300 rounded-md text-center text-black text-base font-normal font-['Open Sans'] leading-snug" ) end diff --git a/lib/oli_web/pow/user_routes.ex b/lib/oli_web/pow/user_routes.ex index 4c496e2b834..9561925d59b 100644 --- a/lib/oli_web/pow/user_routes.ex +++ b/lib/oli_web/pow/user_routes.ex @@ -4,7 +4,6 @@ defmodule OliWeb.Pow.UserRoutes do use OliWeb, :verified_routes alias OliWeb.Router.Helpers, as: Routes - alias Oli.Accounts.User alias Oli.Delivery.Sections alias Oli.Delivery.Sections.Section @@ -54,15 +53,7 @@ defmodule OliWeb.Pow.UserRoutes do @impl true def after_user_updated_path(conn) do conn - |> request_path_or( - case conn.assigns[:current_user] do - %User{independent_learner: true} -> - Routes.live_path(conn, OliWeb.Workspace.Student) - - _ -> - Routes.delivery_path(conn, :index) - end - ) + |> request_path_or(Routes.pow_registration_path(OliWeb.Endpoint, :edit)) end # Pow stores the request redirect path in the assigns. If that value is diff --git a/lib/oli_web/router.ex b/lib/oli_web/router.ex index 93f8f3c3c65..6aeb17446b1 100644 --- a/lib/oli_web/router.ex +++ b/lib/oli_web/router.ex @@ -112,6 +112,10 @@ defmodule OliWeb.Router do plug(Oli.Plugs.MaybeGatedResource) end + pipeline :maybe_load_lti_params do + plug(Oli.Plugs.MaybeLoadLtiParams) + end + pipeline :require_lti_params do plug(Oli.Plugs.RequireLtiParams) end @@ -253,6 +257,7 @@ defmodule OliWeb.Router do ) post("/jcourse/dashboard/log/server", OliWeb.LegacyLogsController, :process) + pow_assent_authorization_post_callback_routes() end @@ -364,7 +369,7 @@ defmodule OliWeb.Router do get("/products/:product_id/payments/:count", PaymentController, :download_codes) - live("/account", Workspace.AccountDetailsLive) + live("/account", Workspaces.AccountDetailsLive) put("/account", WorkspaceController, :update_author) @@ -1279,18 +1284,17 @@ defmodule OliWeb.Router do end scope "/course", OliWeb do - pipe_through([:browser, :delivery_protected, :pow_email_layout]) + pipe_through([:browser, :delivery_protected, :maybe_load_lti_params, :pow_email_layout]) + + get("/", DeliveryController, :index) get("/link_account", DeliveryController, :link_account) - post("/link_account", DeliveryController, :process_link_account_user) - get("/create_and_link_account", DeliveryController, :create_and_link_account) - post("/create_and_link_account", DeliveryController, :process_create_and_link_account_user) + post("/link_account", DeliveryController, :process_link_account) end scope "/course", OliWeb do pipe_through([:browser, :delivery_protected, :require_lti_params, :pow_email_layout]) - get("/", DeliveryController, :index) live("/select_project", Delivery.NewCourse, :lms_instructor, as: :select_source) end diff --git a/test/oli_web/controllers/delivery_controller_test.exs b/test/oli_web/controllers/delivery_controller_test.exs index ad51340e21a..2f7b1b731f1 100644 --- a/test/oli_web/controllers/delivery_controller_test.exs +++ b/test/oli_web/controllers/delivery_controller_test.exs @@ -107,7 +107,7 @@ defmodule OliWeb.DeliveryControllerTest do |> LtiSession.put_session_lti_params(lti_param_ids.instructor) |> get(Routes.delivery_path(conn, :link_account)) - assert html_response(conn, 200) =~ "Link Existing Account" + assert html_response(conn, 200) =~ "Link Authoring Account" end end @@ -158,8 +158,8 @@ defmodule OliWeb.DeliveryControllerTest do conn = conn |> LtiSession.put_session_lti_params(lti_param_ids.instructor) - |> post(Routes.delivery_path(conn, :process_link_account_user), - user: author_params + |> post(Routes.delivery_path(conn, :process_link_account), + link_account: author_params ) assert html_response(conn, 200) =~ @@ -180,7 +180,7 @@ defmodule OliWeb.DeliveryControllerTest do conn = conn |> LtiSession.put_session_lti_params(lti_param_ids.instructor) - |> post(Routes.delivery_path(conn, :process_link_account_user), user: author_params) + |> post(Routes.delivery_path(conn, :process_link_account), link_account: author_params) assert html_response(conn, 302) =~ "redirect" end @@ -189,8 +189,8 @@ defmodule OliWeb.DeliveryControllerTest do conn: conn, section: section } do - enrolled_user = user_fixture() - other_user = user_fixture() + enrolled_user = user_fixture(%{independent_learner: false}) + other_user = user_fixture(%{independent_learner: false}) {:ok, _enrollment} = Sections.enroll(enrolled_user.id, section.id, [ContextRoles.get_role(:context_learner)]) @@ -213,8 +213,8 @@ defmodule OliWeb.DeliveryControllerTest do conn: conn, section: section } do - enrolled_user = user_fixture() - other_user = user_fixture() + enrolled_user = user_fixture(%{independent_learner: false}) + other_user = user_fixture(%{independent_learner: false}) {:ok, _enrollment} = Sections.enroll(enrolled_user.id, section.id, [ContextRoles.get_role(:context_learner)]) @@ -431,8 +431,8 @@ defmodule OliWeb.DeliveryControllerTest do conn: conn, oaf_section_1: section } do - enrolled_user = user_fixture() - other_user = user_fixture() + enrolled_user = user_fixture(%{independent_learner: false}) + other_user = user_fixture(%{independent_learner: false}) {:ok, _enrollment} = Sections.enroll(enrolled_user.id, section.id, [ContextRoles.get_role(:context_learner)]) @@ -772,12 +772,12 @@ defmodule OliWeb.DeliveryControllerTest do section_no_rc = insert(:section, institution: institution_no_rc, lti_1p3_deployment: deployment_no_rc) - user = user_fixture() - student = user_fixture() - student_no_section = user_fixture() - instructor = user_fixture() - instructor_no_section = user_fixture() - student_instructor_no_section = user_fixture() + user = user_fixture(%{independent_learner: false}) + student = user_fixture(%{independent_learner: false}) + student_no_section = user_fixture(%{independent_learner: false}) + instructor = user_fixture(%{independent_learner: false}) + instructor_no_section = user_fixture(%{independent_learner: false}) + student_instructor_no_section = user_fixture(%{independent_learner: false}) %{project: project, publication: publication} = project_fixture(author) diff --git a/test/oli_web/controllers/workspace_controller_test.exs b/test/oli_web/controllers/workspace_controller_test.exs index 56ab6486572..4dfa37c8db7 100644 --- a/test/oli_web/controllers/workspace_controller_test.exs +++ b/test/oli_web/controllers/workspace_controller_test.exs @@ -61,14 +61,14 @@ defmodule OliWeb.WorkspaceControllerTest do setup [:author_conn] test "displays the page", %{conn: conn} do - conn = get(conn, Routes.live_path(conn, OliWeb.Workspace.AccountDetailsLive)) + conn = get(conn, Routes.live_path(conn, OliWeb.Workspaces.AccountDetailsLive)) assert html_response(conn, 200) =~ "Account" end test "shows a sign out link", %{conn: conn} do conn = conn - |> get(Routes.live_path(conn, OliWeb.Workspace.AccountDetailsLive)) + |> get(Routes.live_path(conn, OliWeb.Workspaces.AccountDetailsLive)) assert html_response(conn, 200) =~ "Sign out" end diff --git a/test/oli_web/live/workspaces/course_author_test.exs b/test/oli_web/live/workspaces/course_author_test.exs index c1622f90291..e3cf66cbe21 100644 --- a/test/oli_web/live/workspaces/course_author_test.exs +++ b/test/oli_web/live/workspaces/course_author_test.exs @@ -1,4 +1,4 @@ -defmodule OliWeb.Workspace.CourseAuthorTest do +defmodule OliWeb.Workspaces.CourseAuthorTest do use ExUnit.Case, async: true use OliWeb.ConnCase diff --git a/test/oli_web/live/workspaces/instructor/dashboard_live_test.exs b/test/oli_web/live/workspaces/instructor/dashboard_live_test.exs index 132ca41449f..0c32febf5e1 100644 --- a/test/oli_web/live/workspaces/instructor/dashboard_live_test.exs +++ b/test/oli_web/live/workspaces/instructor/dashboard_live_test.exs @@ -1,4 +1,4 @@ -defmodule OliWeb.Workspace.Instructor.DashboardLiveTest do +defmodule OliWeb.Workspaces.Instructor.DashboardLiveTest do use OliWeb.ConnCase import Phoenix.LiveViewTest diff --git a/test/oli_web/live/workspaces/instructor_test.exs b/test/oli_web/live/workspaces/instructor_test.exs index c3032b47d3c..54dd2074dfa 100644 --- a/test/oli_web/live/workspaces/instructor_test.exs +++ b/test/oli_web/live/workspaces/instructor_test.exs @@ -1,4 +1,4 @@ -defmodule OliWeb.Workspace.InstructorTest do +defmodule OliWeb.Workspaces.InstructorTest do use ExUnit.Case, async: true use OliWeb.ConnCase @@ -84,19 +84,6 @@ defmodule OliWeb.Workspace.InstructorTest do refute has_element?(view, "div[role='account label']") end - test "sees linked account email on user menu", %{conn: conn, user: user} do - author = insert(:author) - Accounts.link_user_author_account(user, author) - - {:ok, view, _html} = live(conn, ~p"/workspaces/instructor") - - assert has_element?( - view, - "div[id='workspace-user-menu-dropdown'] div[role='linked authoring account email']", - author.email - ) - end - test "can see product title, image and description in sections index with a link to manage it on instructor workspace", %{ conn: conn, @@ -452,6 +439,19 @@ defmodule OliWeb.Workspace.InstructorTest do assert has_element?(view, "div[role='account label']", "Instructor") end + test "sees linked account email on user menu", %{conn: conn, instructor: instructor} do + author = insert(:author) + Accounts.link_user_author_account(instructor, author) + + {:ok, view, _html} = live(conn, ~p"/workspaces/instructor") + + assert has_element?( + view, + "div[id='workspace-user-menu-dropdown'] div[role='linked authoring account email']", + author.email + ) + end + test "can signout from instructor account and return to instructor workspace (and author account stays signed in)", %{conn: conn} do author = insert(:author, email: "author_account@test.com") diff --git a/test/oli_web/live/workspaces/student_test.exs b/test/oli_web/live/workspaces/student_test.exs index 5d5e33de7e6..a62b53e31d7 100644 --- a/test/oli_web/live/workspaces/student_test.exs +++ b/test/oli_web/live/workspaces/student_test.exs @@ -1,4 +1,4 @@ -defmodule OliWeb.Workspace.StudentTest do +defmodule OliWeb.Workspaces.StudentTest do use ExUnit.Case, async: true use OliWeb.ConnCase @@ -83,25 +83,6 @@ defmodule OliWeb.Workspace.StudentTest do refute has_element?(view, "div[role='account label']") end - test "sees linked account email on user menu", %{conn: conn, user: user} do - section_1 = insert(:section, %{open_and_free: true, title: "The best course ever!"}) - section_2 = insert(:section, %{open_and_free: true, title: "Maths"}) - - Sections.enroll(user.id, section_1.id, [ContextRoles.get_role(:context_learner)]) - Sections.enroll(user.id, section_2.id, [ContextRoles.get_role(:context_instructor)]) - - author = insert(:author) - Accounts.link_user_author_account(user, author) - - {:ok, view, _html} = live(conn, ~p"/workspaces/student") - - assert has_element?( - view, - "div[id='workspace-user-menu-dropdown'] div[role='linked authoring account email']", - author.email - ) - end - test "can access student workspace when not enrolled to any section", %{conn: conn} do {:ok, view, _html} = live(conn, ~p"/workspaces/student") From 9ffcd59033c28b67693f1afecb5d6d99cb7ea0f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Abell=C3=A1?= Date: Wed, 18 Sep 2024 10:06:01 -0300 Subject: [PATCH 5/8] [BUGFIX] [MER-3613] Grade Passback Issue (#5107) * [MER-3613] Fix Liveview console log error * [MER-3613] Disable Synchronize Grades button when the process is in progress * [MER-3613] Add throttle value to avoid rate limit error when fetching the LTI access token * [MER-3613] Remove unused access token call * [MER-3613] Add tests --- lib/oli_web/live/grades/grade_sync.ex | 12 +++- lib/oli_web/live/grades/grades.ex | 85 +++++++++++++------------- test/oli_web/live/grades_live_test.exs | 44 +++++++------ 3 files changed, 75 insertions(+), 66 deletions(-) diff --git a/lib/oli_web/live/grades/grade_sync.ex b/lib/oli_web/live/grades/grade_sync.ex index 9b9e684bcf3..5387aedd73f 100644 --- a/lib/oli_web/live/grades/grade_sync.ex +++ b/lib/oli_web/live/grades/grade_sync.ex @@ -1,7 +1,6 @@ defmodule OliWeb.Grades.GradeSync do use OliWeb, :html - attr(:task_queue, :list) attr(:total_jobs, :list) attr(:failed_jobs, :list) attr(:succeeded_jobs, :list) @@ -9,11 +8,16 @@ defmodule OliWeb.Grades.GradeSync do attr(:selected_page, :map) def render(assigns) do + %{total_jobs: total_jobs, failed_jobs: failed_jobs, succeeded_jobs: succeeded_jobs} = assigns + + started_sync? = Enum.all?([total_jobs, failed_jobs, succeeded_jobs], &(!is_nil(&1))) + completed_sync? = started_sync? and total_jobs == failed_jobs + succeeded_jobs + assigns = assign( assigns, :disabled, - if length(assigns.task_queue) > 0 or assigns.selected_page == nil do + if (started_sync? and not completed_sync?) or assigns.selected_page == nil do [disabled: true] else [] @@ -40,11 +44,13 @@ defmodule OliWeb.Grades.GradeSync do were manually adjusted or overridden by the instructor.") %>
    +