From 12f596c5f70103216b58909b85848b6896f7de52 Mon Sep 17 00:00:00 2001 From: Anders Weinstein Date: Mon, 9 Sep 2024 09:44:57 -0400 Subject: [PATCH 1/5] [BUG FIX] [MER-3711] Style tables within feedback to ensure legibility (#5066) * adjust table styles within feedback * use variable for feedback table color * set border on all table elements to override tailwind preflight defaults * tweak comment --------- Co-authored-by: Anders Weinstein --- assets/styles/delivery/activity.scss | 12 ++++++++++-- assets/tailwind.theme.js | 1 + 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/assets/styles/delivery/activity.scss b/assets/styles/delivery/activity.scss index b8f54a41018..b6f392f379d 100644 --- a/assets/styles/delivery/activity.scss +++ b/assets/styles/delivery/activity.scss @@ -112,6 +112,16 @@ margin-right: 3px; } } + + // override tailwind table styles to ensure table borders and text are + // legible against colored feedback background and no dark mode change + table, + th, + tr, + td { + color: var(--color-feedback-table-color); + border-color: var(--color-feedback-table-color); + } } } @@ -162,8 +172,6 @@ /* MER-2961 - Some wide content (specifically formulas) were getting cut off in hints, making it scrollable */ overflow-x: auto; - - } p:last-of-type { diff --git a/assets/tailwind.theme.js b/assets/tailwind.theme.js index f8e47984ad4..363fd7dcd52 100644 --- a/assets/tailwind.theme.js +++ b/assets/tailwind.theme.js @@ -155,6 +155,7 @@ module.exports = { 'feedback-partially-correct-bg': colors.yellow['200'], 'feedback-partially-correct-color': colors.black, 'feedback-partially-correct-graphic-color': colors.yellow['500'], + 'feedback-table-color': colors.black, toolbar: { bg: { DEFAULT: '#f8f9fb', From 9948c7b4de561252b48878f1cf769b575b5887cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Cirio?= Date: Mon, 9 Sep 2024 10:54:42 -0300 Subject: [PATCH 2/5] [BUG FIX] [MER-3704] Wrong scheduling type label on pages (#5078) * group index items by scheduling type and due date * update and extend tests * fix test --- .../live/delivery/student/learn_live.ex | 78 +++++++++++++------ .../live/delivery/student/learn_live_test.exs | 29 ++++--- 2 files changed, 73 insertions(+), 34 deletions(-) diff --git a/lib/oli_web/live/delivery/student/learn_live.ex b/lib/oli_web/live/delivery/student/learn_live.ex index d0e06b35335..be46b869ce9 100644 --- a/lib/oli_web/live/delivery/student/learn_live.ex +++ b/lib/oli_web/live/delivery/student/learn_live.ex @@ -1466,13 +1466,13 @@ defmodule OliWeb.Delivery.Student.LearnLive do intro_video_viewed={@intro_video_viewed} />
- <%= "Due: #{format_date(grouped_due_date, @ctx, "{WDshort} {Mshort} {D}, {YYYY}")}" %> + <%= "#{label_for_scheduling_type(grouped_scheduling_type)}#{format_date(grouped_due_date, @ctx, "{WDshort} {Mshort} {D}, {YYYY}")}" %>
<.index_item @@ -1481,6 +1481,7 @@ defmodule OliWeb.Delivery.Student.LearnLive do display_module_item?( @show_completed_pages, grouped_due_date, + grouped_scheduling_type, @student_end_date_exceptions_per_resource_id, child, @ctx @@ -1505,6 +1506,7 @@ defmodule OliWeb.Delivery.Student.LearnLive do ctx={@ctx} student_end_date_exceptions_per_resource_id={@student_end_date_exceptions_per_resource_id} parent_due_date={grouped_due_date} + parent_scheduling_type={grouped_scheduling_type} due_date={ get_due_date_for_student( child["section_resource"].end_date, @@ -1542,6 +1544,7 @@ defmodule OliWeb.Delivery.Student.LearnLive do attr :student_progress_per_resource_id, :map attr :due_date, Date attr :parent_due_date, Date + attr :parent_scheduling_type, :atom attr :progress, :float attr :show_completed_pages, :boolean @@ -1559,6 +1562,7 @@ defmodule OliWeb.Delivery.Student.LearnLive do display_module_item?( @show_completed_pages, @parent_due_date, + @parent_scheduling_type, @student_end_date_exceptions_per_resource_id, @section_attrs, @ctx @@ -1566,7 +1570,7 @@ defmodule OliWeb.Delivery.Student.LearnLive do } role={"#{@type} #{@numbering_index} details"} class="w-full pl-[5px] pr-[7px] py-2.5 justify-start items-center gap-5 flex rounded-lg" - id={"index_item_#{@resource_id}_#{@parent_due_date}"} + id={"index_item_#{@resource_id}_#{@parent_scheduling_type}_#{@parent_due_date}"} phx-value-resource_id={@resource_id} phx-value-parent_due_date={@parent_due_date} phx-value-module_resource_id={@module_resource_id} @@ -1600,6 +1604,7 @@ defmodule OliWeb.Delivery.Student.LearnLive do display_module_item?( @show_completed_pages, @parent_due_date, + @parent_scheduling_type, @student_end_date_exceptions_per_resource_id, child, @ctx @@ -1624,6 +1629,7 @@ defmodule OliWeb.Delivery.Student.LearnLive do ctx={@ctx} student_end_date_exceptions_per_resource_id={@student_end_date_exceptions_per_resource_id} parent_due_date={@parent_due_date} + parent_scheduling_type={@parent_scheduling_type} due_date={ get_due_date_for_student( child["section_resource"].end_date, @@ -2384,15 +2390,28 @@ defmodule OliWeb.Delivery.Student.LearnLive do defp display_module_item?( _show_completed_pages, _grouped_due_date, + _grouped_scheduling_type, _student_end_date_exceptions_per_resource_id, %{"section_resource" => %{scheduling_type: :inclass_activity}} = _child, _ctx ), do: false + defp display_module_item?( + _show_completed_pages, + _grouped_due_date, + "Not yet scheduled" = _grouped_scheduling_type, + _student_end_date_exceptions_per_resource_id, + %{"section_resource" => %{end_date: end_date}} = _child, + _ctx + ) + when end_date in [nil, ""], + do: true + defp display_module_item?( show_completed_pages, grouped_due_date, + grouped_scheduling_type, student_end_date_exceptions_per_resource_id, child, ctx @@ -2403,6 +2422,7 @@ defmodule OliWeb.Delivery.Student.LearnLive do &display_module_item?( show_completed_pages, grouped_due_date, + grouped_scheduling_type, student_end_date_exceptions_per_resource_id, &1, ctx @@ -2419,9 +2439,11 @@ defmodule OliWeb.Delivery.Student.LearnLive do |> then(&if is_nil(&1), do: "Not yet scheduled", else: to_localized_date(&1, ctx)) if show_completed_pages do - student_due_date == grouped_due_date + student_due_date == grouped_due_date and + grouped_scheduling_type == child["section_resource"].scheduling_type else - !child["completed"] and student_due_date == grouped_due_date + !child["completed"] and student_due_date == grouped_due_date and + grouped_scheduling_type == child["section_resource"].scheduling_type end end end @@ -2441,14 +2463,20 @@ defmodule OliWeb.Delivery.Student.LearnLive do ctx ) |> Enum.uniq() - |> then(fn dates -> - if nil in dates do - dates - |> Enum.reject(&is_nil/1) - |> Enum.sort_by(& &1, {:asc, Date}) - |> Enum.concat(["Not yet scheduled"]) + |> then(fn scheduling_type_date_keywords -> + has_a_not_scheduled_resource = + Enum.any?(scheduling_type_date_keywords, fn {_scheduling_type, date} -> + is_nil(date) + end) + + if has_a_not_scheduled_resource do + # this guarantees not scheduled pages are grouped at the bootom of the list + scheduling_type_date_keywords + |> Enum.reject(fn {_st, date} -> is_nil(date) end) + |> Enum.sort_by(fn {_st, date} -> date end, {:asc, Date}) + |> Enum.concat([{"Not yet scheduled", "Not yet scheduled"}]) else - Enum.sort_by(dates, & &1, {:asc, Date}) + Enum.sort_by(scheduling_type_date_keywords, fn {_st, date} -> date end, {:asc, Date}) end end) end @@ -2477,15 +2505,16 @@ defmodule OliWeb.Delivery.Student.LearnLive do [] else [ - Map.get(student_end_date_exceptions_per_resource_id, resource_id, end_date) && - to_localized_date( - Map.get( - student_end_date_exceptions_per_resource_id, - resource_id, - end_date - ), - ctx - ) + {scheduling_type, + Map.get(student_end_date_exceptions_per_resource_id, resource_id, end_date) && + to_localized_date( + Map.get( + student_end_date_exceptions_per_resource_id, + resource_id, + end_date + ), + ctx + )} ] end @@ -2631,6 +2660,11 @@ defmodule OliWeb.Delivery.Student.LearnLive do Map.get(student_end_date_exceptions_per_resource_id, resource_id, end_date) end + defp label_for_scheduling_type(:due_by), do: "Due by: " + defp label_for_scheduling_type(:read_by), do: "Read by: " + defp label_for_scheduling_type(:inclass_activity), do: "In-class activity by: " + defp label_for_scheduling_type(_), do: "" + defp format_date("Not yet scheduled", _context, _format), do: "Not yet scheduled" defp format_date(due_date, context, format) do diff --git a/test/oli_web/live/delivery/student/learn_live_test.exs b/test/oli_web/live/delivery/student/learn_live_test.exs index 34a467771a7..33928f92252 100644 --- a/test/oli_web/live/delivery/student/learn_live_test.exs +++ b/test/oli_web/live/delivery/student/learn_live_test.exs @@ -473,6 +473,7 @@ defmodule OliWeb.Delivery.Student.ContentLiveTest do Sections.get_section_resource(section.id, page_12_revision.resource_id) |> Sections.update_section_resource(%{ + scheduling_type: :due_by, start_date: ~U[2023-11-02 20:00:00Z], end_date: ~U[2023-11-03 20:00:00Z] }) @@ -1514,7 +1515,7 @@ defmodule OliWeb.Delivery.Student.ContentLiveTest do assert has_element?( view, - "#index_item_#{section_1.resource_id}_2023-11-03" + "#index_item_#{section_1.resource_id}_due_by_2023-11-03" ) end @@ -1987,13 +1988,13 @@ defmodule OliWeb.Delivery.Student.ContentLiveTest do section_1_element = element( view, - "#index_item_#{section_1.resource_id}_2023-11-03" + "#index_item_#{section_1.resource_id}_read_by_2023-11-03" ) subsection_1_element = element( view, - "#index_item_#{subsection_1.resource_id}_2023-11-03" + "#index_item_#{subsection_1.resource_id}_read_by_2023-11-03" ) assert render(section_1_element) =~ "Why Elixir?" @@ -2003,7 +2004,7 @@ defmodule OliWeb.Delivery.Student.ContentLiveTest do assert render(subsection_1_element) =~ "ml-[20px]" end - test "groups pages within a module index by due date (even if some pages do not yet have a scheduled date)", + test "groups pages within a module index by due date or read by (even if some pages do not yet have a scheduled date)", %{conn: conn, section: section} do {:ok, view, _html} = live(conn, Utils.learn_live_path(section.slug)) @@ -2011,16 +2012,20 @@ defmodule OliWeb.Delivery.Student.ContentLiveTest do |> element(~s{div[role="unit_5"] div[role="card_4"]}) |> render_click() - group_by_due_date_div = element(view, ~s{div[id="pages_grouped_by_2023-11-03"]}) + group_by_read_by_date_div = element(view, ~s{div[id="pages_grouped_by_read_by_2023-11-03"]}) + + group_by_due_by_date_div = element(view, ~s{div[id="pages_grouped_by_due_by_2023-11-03"]}) group_by_not_yet_scheduled_div = - element(view, ~s{div[id="pages_grouped_by_Not yet scheduled"]}) + element(view, ~s{div[id="pages_grouped_by_Not yet scheduled_Not yet scheduled"]}) + + assert render(group_by_read_by_date_div) =~ "Read by: Fri Nov 3, 2023" + assert render(group_by_read_by_date_div) =~ "Page 11" - assert render(group_by_due_date_div) =~ "Due: Fri Nov 3, 2023" - assert render(group_by_due_date_div) =~ "Page 11" - assert render(group_by_due_date_div) =~ "Page 12" + assert render(group_by_due_by_date_div) =~ "Due by: Fri Nov 3, 2023" + assert render(group_by_due_by_date_div) =~ "Page 12" - assert render(group_by_not_yet_scheduled_div) =~ "Due: Not yet scheduled" + assert render(group_by_not_yet_scheduled_div) =~ "Not yet scheduled" assert render(group_by_not_yet_scheduled_div) =~ "Page 13" assert render(group_by_not_yet_scheduled_div) =~ "Page 14" end @@ -2048,10 +2053,10 @@ defmodule OliWeb.Delivery.Student.ContentLiveTest do |> element(~s{div[role="unit_5"] div[role="card_4"]}) |> render_click() - group_by_due_date_div = element(view, ~s{div[id="pages_grouped_by_2023-11-10"]}) + group_by_due_date_div = element(view, ~s{div[id="pages_grouped_by_read_by_2023-11-10"]}) # page 13 is due on Nov 10, 2023 as defined in the student exception - assert render(group_by_due_date_div) =~ "Due: Fri Nov 10, 2023" + assert render(group_by_due_date_div) =~ "Read by: Fri Nov 10, 2023" assert render(group_by_due_date_div) =~ "Page 13" end From 0fc556e765dc99fb21e5cacb6618503acd6e26bd Mon Sep 17 00:00:00 2001 From: Anders Weinstein Date: Mon, 9 Sep 2024 09:58:00 -0400 Subject: [PATCH 3/5] guard against undefined process.env (was causing error) (#5081) Co-authored-by: Anders Weinstein --- assets/src/components/editing/editor/paste/onHtmlPaste.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assets/src/components/editing/editor/paste/onHtmlPaste.ts b/assets/src/components/editing/editor/paste/onHtmlPaste.ts index 3644b6b7b1f..5e4af24285a 100644 --- a/assets/src/components/editing/editor/paste/onHtmlPaste.ts +++ b/assets/src/components/editing/editor/paste/onHtmlPaste.ts @@ -30,7 +30,7 @@ type PartialTagDeserializer = { action: DeserializerAction; }; -const isJest = typeof process !== 'undefined' && process.env.JEST_WORKER_ID !== undefined; +const isJest = typeof process !== 'undefined' && process.env?.JEST_WORKER_ID !== undefined; const DEBUG_OUTPUT = !isJest; export const filterNullModelElements = (arr: DeserializeTypes): arr is DeserializeTypesNoNull => From 896fc19717ac64761ae9b04b72b897e44dff44a5 Mon Sep 17 00:00:00 2001 From: Darren Siegel Date: Mon, 9 Sep 2024 16:04:00 -0400 Subject: [PATCH 4/5] [PERFORMANCE] [MER-3709] Streamline course section update application (#5075) * streamline * stashing * stashing * stashing * fix unreachable traverse * cleanup warnings * Auto format --------- Co-authored-by: darrensiegel --- lib/oli/delivery/previous_next_index.ex | 8 +- lib/oli/delivery/sections.ex | 4 +- lib/oli/delivery/sections/updates.ex | 442 ++++++++++++++++++++++++ lib/oli/delivery/updates/worker.ex | 6 +- test/oli/sections_test.exs | 18 +- 5 files changed, 463 insertions(+), 15 deletions(-) create mode 100644 lib/oli/delivery/sections/updates.ex diff --git a/lib/oli/delivery/previous_next_index.ex b/lib/oli/delivery/previous_next_index.ex index de49d40d7ea..3f8eeb82c2d 100644 --- a/lib/oli/delivery/previous_next_index.ex +++ b/lib/oli/delivery/previous_next_index.ex @@ -3,6 +3,7 @@ defmodule Oli.Delivery.PreviousNextIndex do alias Oli.Delivery.Sections alias Oli.Publishing.DeliveryResolver alias Oli.Delivery.Hierarchy + alias Oli.Resources.Numbering alias Oli.Repo @doc """ @@ -127,8 +128,11 @@ defmodule Oli.Delivery.PreviousNextIndex do """ def rebuild(%Section{slug: slug} = section) do case Repo.transaction(fn _ -> - DeliveryResolver.full_hierarchy(slug) - |> Hierarchy.build_navigation_link_map() + {new_hierarchy, _} = + DeliveryResolver.full_hierarchy(slug) + |> Numbering.renumber_hierarchy() + + Hierarchy.build_navigation_link_map(new_hierarchy) |> then(fn previous_next_index -> Sections.update_section(section, %{previous_next_index: previous_next_index}) end) diff --git a/lib/oli/delivery/sections.ex b/lib/oli/delivery/sections.ex index 6095681a35c..96ca1b98f00 100644 --- a/lib/oli/delivery/sections.ex +++ b/lib/oli/delivery/sections.ex @@ -1237,7 +1237,7 @@ defmodule Oli.Delivery.Sections do # determine and return the list of page resource ids that are not reachable from that # hierarchy, taking into account links from pages to other pages and the 'relates_to' # relationship between pages. - defp determine_unreachable_pages(publication_ids, hierarchy_ids) do + def determine_unreachable_pages(publication_ids, hierarchy_ids) do # Start with all pages unreachable = Oli.Publishing.all_page_resource_ids(publication_ids) @@ -3593,7 +3593,7 @@ defmodule Oli.Delivery.Sections do # For a given section resource, clean the children attribute to ensure that: # 1. Any nil records are removed # 2. All non-nil sr id references map to a non-deleted revision in the new pub - defp clean_children(section_resource, sr_id_to_resource_id, new_published_resources_map) do + def clean_children(section_resource, sr_id_to_resource_id, new_published_resources_map) do updated_children = section_resource.children |> Enum.filter(fn child_id -> !is_nil(child_id) end) diff --git a/lib/oli/delivery/sections/updates.ex b/lib/oli/delivery/sections/updates.ex new file mode 100644 index 00000000000..d580221fde6 --- /dev/null +++ b/lib/oli/delivery/sections/updates.ex @@ -0,0 +1,442 @@ +defmodule Oli.Delivery.Sections.Updates do + require Logger + import Ecto.Query, warn: false + + alias Oli.Delivery.Sections.MinimalHierarchy + alias Oli.Repo + alias Oli.Delivery.Sections + alias Oli.Publishing + alias Oli.Resources.ResourceType + alias Oli.Publishing.Publications.PublicationDiff + alias Oli.Delivery.Updates.Broadcaster + alias Oli.Delivery.Sections.PostProcessing + + alias Oli.Delivery.Sections.{ + Section, + SectionResource + } + + @section_resources_on_conflict {:replace_all_except, + [ + :inserted_at, + :scoring_strategy_id, + :scheduling_type, + :manually_scheduled, + :start_date, + :end_date, + :collab_space_config, + :explanation_strategy, + :max_attempts, + :retake_mode, + :assessment_mode, + :password, + :late_submit, + :late_start, + :time_limit, + :grace_period, + :review_submission, + :feedback_mode, + :feedback_scheduled_date + ]} + + @doc """ + Gracefully applies the specified publication update to a given section by leaving the existing + curriculum and section modifications in-tact while applying the structural changes that + occurred between the old and new publication. + + This implementation makes the assumption that a resource_id is unique within a curriculum. + That is, a resource can only allowed to be added once in a single location within a curriculum. + This makes it simpler to apply changes to the existing curriculum but if necessary, this implementation + could be extended to not just apply the changes to the first node found that contains the changed resource, + but any/all nodes in the hierarchy which reference the changed resource. + """ + def apply_publication_update( + %Section{id: section_id} = section, + publication_id + ) do + Broadcaster.broadcast_update_progress(section.id, publication_id, 0) + + new_publication = Publishing.get_publication!(publication_id) + project = Oli.Repo.get(Oli.Authoring.Course.Project, new_publication.project_id) + current_publication = Sections.get_current_publication(section_id, project.id) + + result = + Oli.Repo.transaction(fn -> + case do_update(section, project.id, current_publication, new_publication) do + {:ok, _} -> + do_post_processing_steps(section, project) + + e -> + Oli.Repo.rollback(e) + end + end) + + case result do + {:ok, _} -> + Broadcaster.broadcast_update_progress(section.id, new_publication.id, :complete) + + _ -> + Broadcaster.broadcast_update_progress(section.id, new_publication.id, 0) + end + + result + end + + defp do_post_processing_steps(section, project) do + # For a section based on this project, update the has_experiments in the section to match that + # setting in the project. + if section.base_project_id == project.id and + project.has_experiments != section.has_experiments do + Oli.Delivery.Sections.update_section(section, %{has_experiments: project.has_experiments}) + end + + PostProcessing.apply(section, :all) + + Oli.Delivery.maybe_update_section_contains_explorations(section) + Oli.Delivery.maybe_update_section_contains_deliberate_practice(section) + end + + # Implements the logic to determine *how* to apply the update to the course section, + # taking into account the update type (minor / major) and the source of the section + # (project / product) + defp do_update(section, project_id, current_publication, new_publication) do + case Publishing.get_publication_diff(current_publication, new_publication) do + %PublicationDiff{classification: :minor} = diff -> + do_minor_update(diff, section, project_id, new_publication) + + %PublicationDiff{classification: :major} = diff -> + cond do + # Case 1: The course section is based on this project, but is not a product and is not seeded from a product + section.base_project_id == project_id and section.type == :enrollable and + is_nil(section.blueprint_id) -> + do_major_update(diff, section, project_id, current_publication, new_publication) + + # Case 2: The course section is based on this project and was seeded from a product + section.base_project_id == project_id and !is_nil(section.blueprint_id) -> + if section.blueprint.apply_major_updates do + do_major_update(diff, section, project_id, current_publication, new_publication) + else + do_minor_update(diff, section, project_id, new_publication) + end + + # Case 3: The course section is a product based on this project + section.base_project_id == project_id and section.type == :blueprint -> + do_minor_update(diff, section, project_id, new_publication) + + # Case 4: The course section is not based on this project (but it remixes some materials from project) + true -> + do_minor_update(diff, section, project_id, new_publication) + end + end + end + + # Perform a MINOR update: + # 1. Add SR records for all resource types that were added in this pub, except for containers + # 2. Delete SR records for all resource types that were deleted EXCEPT for pages and containers. + # We cannot delete page SR records because we may be applying a major update as a minor + # update - and we need that page to stay present if we are not processing the removal from + # the container + # 3. Move forard the SPP records to the new publication + # 4. Cull unreachable pages. + defp do_minor_update(%PublicationDiff{} = diff, section, project_id, new_publication) do + mark = Oli.Timing.mark() + + container_type_id = Oli.Resources.ResourceType.get_id_by_type("container") + page_type_id = Oli.Resources.ResourceType.get_id_by_type("page") + + case diff + |> filter_for_revisions(:added, fn r -> r.resource_type_id != container_type_id end) + |> bulk_create_section_resources(section, project_id) do + {:ok, _} -> + diff + |> filter_for_revisions(:deleted, fn r -> + r.resource_type_id != container_type_id and + r.resource_type_id != page_type_id + end) + |> bulk_delete_section_resources(section) + + Sections.update_section_project_publication(section, project_id, new_publication.id) + + cull_unreachable_pages(section) + + Logger.info( + "perform_update.MINOR: section[#{section.slug}] #{Oli.Timing.elapsed(mark) / 1000 / 1000}ms" + ) + + {:ok, :ok} + + {:error, _} -> + {:error, :unexpected_count} + end + end + + # Add all and delete all SR records that were added/deleted in the publication diff + defp add_remove_srs(%PublicationDiff{} = diff, section, project_id) do + case diff + |> filter_for_revisions(:added, fn _r -> true end) + |> bulk_create_section_resources(section, project_id) do + {:ok, _} -> + diff + |> filter_for_revisions(:deleted, fn _r -> true end) + |> bulk_delete_section_resources(section) + + {:ok, :ok} + + {:error, _} -> + {:error, :unexpected_count} + end + end + + # For a publication diff and a desired type (:added, :deleted) return + # all of the revisions that pass the supplied filter func + defp filter_for_revisions(%PublicationDiff{} = diff, desired_type, filter_fn) do + Map.filter(diff.changes, fn {_k, {this_type, _}} -> this_type == desired_type end) + |> Map.values() + |> Enum.map(fn {_, %{revision: r}} -> r end) + |> Enum.filter(filter_fn) + end + + # Bulk create a collection of Section Resource records (SRs) for a collection + # of revisions + defp bulk_create_section_resources(revisions, section, project_id) do + now = DateTime.utc_now() |> DateTime.truncate(:second) + placeholders = %{timestamp: now} + + section_resource_rows = + Enum.map(revisions, fn r -> + %{ + resource_id: r.resource_id, + project_id: project_id, + section_id: section.id, + children: nil, + scoring_strategy_id: r.scoring_strategy_id, + slug: Oli.Utils.Slug.generate("section_resources", r.title), + inserted_at: {:placeholder, :timestamp}, + updated_at: {:placeholder, :timestamp} + } + end) + + expected_count = Enum.count(section_resource_rows) + + case Oli.Utils.Database.batch_insert_all(SectionResource, section_resource_rows, + placeholders: placeholders, + on_conflict: @section_resources_on_conflict, + conflict_target: [:section_id, :resource_id] + ) do + {^expected_count, _} -> {:ok, Enum.count(section_resource_rows)} + _ -> {:error, :unexpected_count} + end + end + + # Bulk delete a collection of SR records that match a collection of revisions + defp bulk_delete_section_resources(revisions, %Section{id: section_id}) do + resource_ids = Enum.map(revisions, & &1.resource_id) + + from(sr in SectionResource, + where: sr.section_id == ^section_id and sr.resource_id in ^resource_ids + ) + |> Repo.delete_all() + end + + # Do a MAJOR update + # 1. Add / Remove all SR records per the publication diff. This step is different than minor + # updates because we add and remove containers as well + # 2. Move forward the SPP record to the new publication id + # 3. Update contain children - this is the key step that differentiates major from minor + # updates where we update the :children attr of the containers to change the hiearchy + # 4. Rebuild previous next index, contained pages, contained objective + defp do_major_update( + %PublicationDiff{} = diff, + section, + project_id, + prev_publication, + new_publication + ) do + mark = Oli.Timing.mark() + + add_remove_srs(diff, section, project_id) + Sections.update_section_project_publication(section, project_id, new_publication.id) + + with {:ok, _} <- update_container_children(section, prev_publication, new_publication), + {:ok, _} <- cull_unreachable_pages(section), + {:ok, _} <- Oli.Delivery.PreviousNextIndex.rebuild(section), + {:ok, _} <- Sections.rebuild_contained_pages(section), + {:ok, _} <- Sections.rebuild_contained_objectives(section) do + Logger.info( + "perform_update.MAJOR: section[#{section.slug}] #{Oli.Timing.elapsed(mark) / 1000 / 1000}ms" + ) + + {:ok, :ok} + else + e -> e + end + end + + defp cull_unreachable_pages(section) do + section_id = section.id + + map = + Oli.Delivery.Sections.MinimalHierarchy.full_hierarchy(section.slug) + |> Oli.Delivery.Hierarchy.flatten() + |> Enum.reduce(%{}, fn s, m -> Map.put(m, s.section_resource.id, s) end) + + hierarchy_ids = + Map.values(map) + |> Enum.map(fn s -> + [ + s.section_resource.resource_id + | Enum.map(s.children, fn c -> + Map.get(map, c.section_resource.id).section_resource.resource_id + end) + ] + end) + |> List.flatten() + + publication_ids = + Oli.Repo.all(Oli.Delivery.Sections.SectionsProjectsPublications, section_id: section.id) + |> Enum.map(fn spp -> spp.publication_id end) + + unreachable_page_resource_ids = + case section.required_survey_resource_id do + nil -> + Oli.Delivery.Sections.determine_unreachable_pages(publication_ids, hierarchy_ids) + + id -> + Oli.Delivery.Sections.determine_unreachable_pages(publication_ids, [id | hierarchy_ids]) + end + + case unreachable_page_resource_ids do + [] -> + {:ok, true} + + _ -> + from(sr in SectionResource, + where: sr.section_id == ^section_id and sr.resource_id in ^unreachable_page_resource_ids + ) + |> Repo.delete_all() + + {:ok, true} + end + end + + defp update_container_children(section, prev_publication, new_publication) do + container = ResourceType.id_for_container() + + prev_published_resources_map = + MinimalHierarchy.published_resources_map(prev_publication.id) + + new_published_resources_map = + MinimalHierarchy.published_resources_map(new_publication.id) + + # get all section resources including freshly minted ones + section_resources = Sections.get_section_resources(section.id) + + # build mappings from section_resource_id to resource_id and the inverse + {sr_id_to_resource_id, resource_id_to_sr_id} = + section_resources + |> Enum.reduce({%{}, %{}}, fn %SectionResource{id: id, resource_id: resource_id}, + {sr_id_to_resource_id, resource_id_to_sr_id} -> + {Map.put(sr_id_to_resource_id, id, resource_id), + Map.put(resource_id_to_sr_id, resource_id, id)} + end) + + # For all container section resources in the course project whose children attribute differs + # from the new publication’s container children, execute the three way merge algorithm + merged_section_resources = + section_resources + |> Enum.map(fn section_resource -> + %SectionResource{ + resource_id: resource_id, + children: current_children + } = section_resource + + prev_published_resource = prev_published_resources_map[resource_id] + + is_container? = + case prev_published_resource do + %{resource_type_id: ^container} -> + true + + _ -> + false + end + + if is_container? or is_nil(current_children) do + new_published_resource = new_published_resources_map[resource_id] + new_children = new_published_resource.children + + updated_section_resource = + case current_children do + nil -> + # this section resource was just created so it can assume the newly published value + %SectionResource{ + section_resource + | children: Enum.map(new_children, &resource_id_to_sr_id[&1]) + } + + current_children -> + # ensure we are comparing resource_ids to resource_ids (and not section_resource_ids) + # by translating the current section_resource children ids to resource_ids + current_children_resource_ids = + Enum.map(current_children, &sr_id_to_resource_id[&1]) + + # check if the children resource_ids have diverged from the new value + if current_children_resource_ids != new_children do + # There is a merge conflict between the current section resource and the new published resource. + # Use the AIRRO three way merge algorithm to resolve + base = prev_published_resource.children + source = new_published_resource.children + target = current_children_resource_ids + + case Oli.Publishing.Updating.Merge.merge(base, source, target) do + {:ok, merged} -> + %SectionResource{ + section_resource + | children: Enum.map(merged, &resource_id_to_sr_id[&1]) + } + + {:no_change} -> + section_resource + end + else + section_resource + end + end + + Sections.clean_children( + updated_section_resource, + sr_id_to_resource_id, + new_published_resources_map + ) + else + section_resource + end + end) + + # Upsert all merged section resource records. Some of these records may have just been created + # and some may not have been changed, but that's okay we will just update them again. There + # isn't a lot of them as these are just the container resources. + now = DateTime.utc_now() |> DateTime.truncate(:second) + placeholders = %{timestamp: now} + + section_resource_rows = + merged_section_resources + |> Enum.map(fn section_resource -> + %{ + SectionResource.to_map(section_resource) + | updated_at: {:placeholder, :timestamp} + } + end) + + expected_count = Enum.count(section_resource_rows) + + case Oli.Utils.Database.batch_insert_all(SectionResource, section_resource_rows, + placeholders: placeholders, + on_conflict: @section_resources_on_conflict, + conflict_target: [:section_id, :resource_id] + ) do + {^expected_count, _} -> {:ok, expected_count} + _ -> {:error, :unexpected_count} + end + end +end diff --git a/lib/oli/delivery/updates/worker.ex b/lib/oli/delivery/updates/worker.ex index 00e472b1b2b..164ac8f9b9f 100644 --- a/lib/oli/delivery/updates/worker.ex +++ b/lib/oli/delivery/updates/worker.ex @@ -4,7 +4,7 @@ defmodule Oli.Delivery.Updates.Worker do """ use Oban.Worker, queue: :updates, max_attempts: 3 - alias Oli.Delivery.Sections + alias Oli.Delivery.Sections.Updates @impl Oban.Worker def perform(%Oban.Job{ @@ -14,9 +14,9 @@ defmodule Oli.Delivery.Updates.Worker do end def perform_now(section_slug, publication_id) do - section = Sections.get_section_by_slug(section_slug) + section = Oli.Delivery.Sections.get_section_by_slug(section_slug) - Sections.apply_publication_update( + Updates.apply_publication_update( section, publication_id ) diff --git a/test/oli/sections_test.exs b/test/oli/sections_test.exs index 257375a58d8..31084843e5c 100644 --- a/test/oli/sections_test.exs +++ b/test/oli/sections_test.exs @@ -858,6 +858,7 @@ defmodule Oli.SectionsTest do # verify the curriculum precondition hierarchy = DeliveryResolver.full_hierarchy(section.slug) + original_flattened = Oli.Delivery.Hierarchy.flatten(hierarchy) assert hierarchy.children |> Enum.count() == 2 assert hierarchy.children |> Enum.at(0) |> Map.get(:resource_id) == page1.id @@ -928,7 +929,7 @@ defmodule Oli.SectionsTest do working_pub ) - # remove page 2 + # remove page _deleted_revision = Seeder.delete_page(page2, revision2, container_resource, container_revision, working_pub) @@ -936,17 +937,18 @@ defmodule Oli.SectionsTest do {:ok, latest_publication} = Publishing.publish_project(project, "some changes", author.id) # apply the new publication update to the section - Sections.apply_publication_update(section, latest_publication.id) + Oli.Delivery.Sections.Updates.apply_publication_update(section, latest_publication.id) # reload latest hierarchy hierarchy = DeliveryResolver.full_hierarchy(section.slug) + updated_flattened = Oli.Delivery.Hierarchy.flatten(hierarchy) + # verify non-structural changes are applied as expected assert hierarchy.children |> Enum.at(0) |> then(& &1.revision.content) == page1_changes["content"] # verify the updated curriculum structure matches the expected result - assert hierarchy.children |> Enum.count() == 4 assert hierarchy.children |> Enum.at(0) |> Map.get(:resource_id) == page1.id assert hierarchy.children |> Enum.at(1) |> Map.get(:resource_id) == p1_new_page1.id @@ -1087,7 +1089,7 @@ defmodule Oli.SectionsTest do assert latest_publication.minor == 1 # apply the new publication update to the section - Sections.apply_publication_update(section, latest_publication.id) + Oli.Delivery.Sections.Updates.apply_publication_update(section, latest_publication.id) # reload latest hierarchy hierarchy = DeliveryResolver.full_hierarchy(section.slug) @@ -1257,7 +1259,7 @@ defmodule Oli.SectionsTest do {:ok, latest_publication} = Publishing.publish_project(project, "some changes", author.id) # apply the new publication update to the product - Sections.apply_publication_update(product, latest_publication.id) + Oli.Delivery.Sections.Updates.apply_publication_update(product, latest_publication.id) # reload latest hierarchy product_hierarchy = DeliveryResolver.full_hierarchy(product.slug) @@ -1284,7 +1286,7 @@ defmodule Oli.SectionsTest do assert product_section_resources |> Enum.count() == 3 # apply the new publication update to the section - Sections.apply_publication_update(section, latest_publication.id) + Oli.Delivery.Sections.Updates.apply_publication_update(section, latest_publication.id) # reload latest hierarchy hierarchy = DeliveryResolver.full_hierarchy(section.slug) @@ -1507,7 +1509,7 @@ defmodule Oli.SectionsTest do {:ok, latest_publication} = Publishing.publish_project(project, "some changes", author.id) # apply the new publication update to the section - Sections.apply_publication_update(section, latest_publication.id) + Oli.Delivery.Sections.Updates.apply_publication_update(section, latest_publication.id) # reload latest hierarchy hierarchy = DeliveryResolver.full_hierarchy(section.slug) @@ -1729,7 +1731,7 @@ defmodule Oli.SectionsTest do {:ok, latest_publication} = Publishing.publish_project(project, "some changes", author.id) # apply the new publication update to the section - Sections.apply_publication_update(section, latest_publication.id) + Oli.Delivery.Sections.Updates.apply_publication_update(section, latest_publication.id) # reload latest hierarchy section_hierarchy = DeliveryResolver.full_hierarchy(section.slug) From 004595a58a802ebe03854ce83fdc7f84ddef90aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Cirio?= Date: Mon, 9 Sep 2024 20:47:02 -0300 Subject: [PATCH 5/5] [BUG FIX] [MER-3704] Wrong scheduling type label on page header (#5083) * add scheduling type to Combined settings struct * implement scheduling type label on page header * update tests * extend test --- lib/oli/delivery/settings.ex | 1 + lib/oli/delivery/settings/combined.ex | 2 ++ lib/oli_web/common/format_date_time.ex | 4 ++-- lib/oli_web/live/delivery/student/learn_live.ex | 7 +------ lib/oli_web/live/delivery/student/utils.ex | 12 +++++++++++- .../live/delivery/student/learn_live_test.exs | 4 ++-- .../live/delivery/student/lesson_live_test.exs | 1 + 7 files changed, 20 insertions(+), 11 deletions(-) diff --git a/lib/oli/delivery/settings.ex b/lib/oli/delivery/settings.ex index 7f5ed4d1617..4b3f023ded0 100644 --- a/lib/oli/delivery/settings.ex +++ b/lib/oli/delivery/settings.ex @@ -154,6 +154,7 @@ defmodule Oli.Delivery.Settings do %Combined{ resource_id: resolved_revision.resource_id, + scheduling_type: section_resource.scheduling_type, start_date: combine_field(:start_date, section_resource, student_exception), end_date: combine_field(:end_date, section_resource, student_exception), max_attempts: max_attempts, diff --git a/lib/oli/delivery/settings/combined.ex b/lib/oli/delivery/settings/combined.ex index 12bc0bb56fa..b57300cf0d0 100644 --- a/lib/oli/delivery/settings/combined.ex +++ b/lib/oli/delivery/settings/combined.ex @@ -1,5 +1,6 @@ defmodule Oli.Delivery.Settings.Combined do defstruct resource_id: nil, + scheduling_type: :read_by, start_date: nil, end_date: nil, max_attempts: 0, @@ -19,6 +20,7 @@ defmodule Oli.Delivery.Settings.Combined do @type t() :: %__MODULE__{ resource_id: integer(), + scheduling_type: :due_by | :read_by | :inclass_activity, start_date: DateTime.t(), end_date: DateTime.t(), max_attempts: integer(), diff --git a/lib/oli_web/common/format_date_time.ex b/lib/oli_web/common/format_date_time.ex index dea5aace247..d3f9b948e01 100644 --- a/lib/oli_web/common/format_date_time.ex +++ b/lib/oli_web/common/format_date_time.ex @@ -282,7 +282,7 @@ defmodule OliWeb.Common.FormatDateTime do def to_formatted_datetime(datetime, ctx, format \\ "{WDshort} {Mshort} {D}, {YYYY}") - def to_formatted_datetime(nil, _ctx, _format), do: "not yet scheduled" + def to_formatted_datetime(nil, _ctx, _format), do: "Not yet scheduled" def to_formatted_datetime(datetime, ctx, format) do if is_binary(datetime) do @@ -294,7 +294,7 @@ defmodule OliWeb.Common.FormatDateTime do end end - defp to_datetime(nil), do: "not yet scheduled" + defp to_datetime(nil), do: "Not yet scheduled" defp to_datetime(string_datetime) do {:ok, datetime, _} = DateTime.from_iso8601(string_datetime) diff --git a/lib/oli_web/live/delivery/student/learn_live.ex b/lib/oli_web/live/delivery/student/learn_live.ex index be46b869ce9..b231338bf6f 100644 --- a/lib/oli_web/live/delivery/student/learn_live.ex +++ b/lib/oli_web/live/delivery/student/learn_live.ex @@ -1472,7 +1472,7 @@ defmodule OliWeb.Delivery.Student.LearnLive do >
- <%= "#{label_for_scheduling_type(grouped_scheduling_type)}#{format_date(grouped_due_date, @ctx, "{WDshort} {Mshort} {D}, {YYYY}")}" %> + <%= "#{Utils.label_for_scheduling_type(grouped_scheduling_type)}#{format_date(grouped_due_date, @ctx, "{WDshort} {Mshort} {D}, {YYYY}")}" %>
<.index_item @@ -2660,11 +2660,6 @@ defmodule OliWeb.Delivery.Student.LearnLive do Map.get(student_end_date_exceptions_per_resource_id, resource_id, end_date) end - defp label_for_scheduling_type(:due_by), do: "Due by: " - defp label_for_scheduling_type(:read_by), do: "Read by: " - defp label_for_scheduling_type(:inclass_activity), do: "In-class activity by: " - defp label_for_scheduling_type(_), do: "" - defp format_date("Not yet scheduled", _context, _format), do: "Not yet scheduled" defp format_date(due_date, context, format) do diff --git a/lib/oli_web/live/delivery/student/utils.ex b/lib/oli_web/live/delivery/student/utils.ex index 45cea538da8..d4487611be2 100644 --- a/lib/oli_web/live/delivery/student/utils.ex +++ b/lib/oli_web/live/delivery/student/utils.ex @@ -93,7 +93,12 @@ defmodule OliWeb.Delivery.Student.Utils do
-
Due:
+
+ <%= label_for_scheduling_type(@page_context.effective_settings.scheduling_type) %> +
<%= FormatDateTime.to_formatted_datetime( @page_context.effective_settings.end_date, @@ -150,6 +155,11 @@ defmodule OliWeb.Delivery.Student.Utils do """ end + def label_for_scheduling_type(:due_by), do: "Due by: " + def label_for_scheduling_type(:read_by), do: "Read by: " + def label_for_scheduling_type(:inclass_activity), do: "In-class activity by: " + def label_for_scheduling_type(_), do: "" + def proficiency_explanation_modal(assigns) do assigns = assign(assigns, %{ diff --git a/test/oli_web/live/delivery/student/learn_live_test.exs b/test/oli_web/live/delivery/student/learn_live_test.exs index 33928f92252..61104f2f94b 100644 --- a/test/oli_web/live/delivery/student/learn_live_test.exs +++ b/test/oli_web/live/delivery/student/learn_live_test.exs @@ -1580,7 +1580,7 @@ defmodule OliWeb.Delivery.Student.ContentLiveTest do # unit 2 has not been scheduled by instructor, so there must not be a schedule details data assert view |> element(~s{div[role="unit_2"] div[role="schedule_details"]}) - |> render() =~ "Due:\n \n not yet scheduled" + |> render() =~ "Due:\n \n Not yet scheduled" end test "can see units, modules and page (at module level) progresses", %{ @@ -1751,7 +1751,7 @@ defmodule OliWeb.Delivery.Student.ContentLiveTest do |> element( ~s{div[id="top_level_page_#{top_level_page.resource_id}"] div[role="schedule_details"]} ) - |> render() =~ "Due:\n \n not yet scheduled" + |> render() =~ "Not yet scheduled" assert view |> element(~s{div[id="page_#{top_level_page.resource_id}"][role="card_1"]}) diff --git a/test/oli_web/live/delivery/student/lesson_live_test.exs b/test/oli_web/live/delivery/student/lesson_live_test.exs index bc0eff1173b..a618bbcea2a 100644 --- a/test/oli_web/live/delivery/student/lesson_live_test.exs +++ b/test/oli_web/live/delivery/student/lesson_live_test.exs @@ -839,6 +839,7 @@ defmodule OliWeb.Delivery.Student.LessonLiveTest do assert has_element?(view, ~s{div[role="page numbering index"]}, "2.") assert has_element?(view, ~s{div[role="page title"]}, "Page 2") assert has_element?(view, ~s{div[role="page read time"]}, "15") + assert has_element?(view, ~s{div[role="page schedule"]}, "Read by:") assert has_element?(view, ~s{div[role="page schedule"]}, "Tue Nov 14, 2023") end