Merge branch 'Simon-Initiative:master' into master

assets/src/apps/page-editor/PageEditor.tsx

@@ -434,7 +434,7 @@ export class PageEditor extends React.Component<PageEditorProps, PageEditorState
   // of content to be saved.  In this manner, we allow the user interface to display invalid, intermediate
   // states (as the user is creating a selection, for instance) that will always be saved
   // as valid states.
-  adjustContentForConstraints(content: PageEditorContent): PageEditorContent {
+  static adjustContentForConstraints(content: PageEditorContent): PageEditorContent {
     return content.updateAll((c: ResourceContent) => {
       if (c.type === 'selection') {
         return Object.assign({}, c, { logic: guaranteeValididty(c.logic) });
@@ -456,7 +456,7 @@ export class PageEditor extends React.Component<PageEditorProps, PageEditorState
   save() {
     const { projectSlug, resourceSlug } = this.props;
 
-    const adjusted = this.adjustContentForConstraints(this.state.content);
+    const adjusted = PageEditor.adjustContentForConstraints(this.state.content);
 
     const toSave: Persistence.ResourceUpdate = {
       objectives: { attached: this.state.objectives.toArray() },

assets/src/data/editor/PageEditorContent.ts

@@ -169,7 +169,7 @@ export class PageEditorContent extends Immutable.Record(defaultParams()) {
  * @param content
  * @returns
  */
-function withDefaultContent(
+export function withDefaultContent(
   content: Immutable.List<ResourceContent>,
 ): Immutable.List<ResourceContent> {
   if (content.size > 0) {
@@ -301,7 +301,11 @@ function updateAll(
 ): Immutable.List<ResourceContent> {
   return items.map((i) => {
     if (isResourceGroup(i)) {
-      (i as ResourceGroup).children = updateAll((i as ResourceGroup).children, fn);
+      const itemWithChildren = {
+        ...i,
+        children: updateAll((i as ResourceGroup).children, fn),
+      } as ResourceGroup;
+      return fn(itemWithChildren);
     }
 
     return fn(i);

assets/test/editor/page_editor_content_test.ts

@@ -0,0 +1,146 @@
+import * as Immutable from 'immutable';
+import PageEditor from 'apps/page-editor/PageEditor';
+import { PageEditorContent, withDefaultContent } from 'data/editor/PageEditorContent';
+import * as Bank from '../../src/data/content/bank';
+import {
+  ActivityBankSelection,
+  PurposeGroupContent,
+  ResourceContent,
+} from '../../src/data/content/resource';
+
+describe('PageEditorContent', () => {
+  it('should be immutalble', () => {
+    const content = new PageEditorContent({
+      version: '1',
+      model: withDefaultContent(Immutable.List()),
+    });
+    expect(() => (content.version = '2')).toThrow();
+    const child: ActivityBankSelection = {
+      type: 'selection',
+      id: 'testChild',
+      logic: {
+        conditions: {
+          fact: Bank.Fact.objectives,
+          operator: Bank.ExpressionOperator.contains,
+          value: [],
+        },
+      },
+      count: 1,
+      children: undefined,
+    };
+    expect(content.model.size).toEqual(1);
+    const newContent1 = content.update('model', (model) => model.push(child));
+    expect(content.model.size).toBe(1);
+    expect(newContent1.model.size).toBe(2);
+
+    const newContent2 = newContent1.updateAll((item: ResourceContent) => {
+      if (item.type === 'selection') {
+        return { ...item, count: 2 };
+      }
+      return item;
+    });
+
+    expect((newContent1.model.get(1) as ActivityBankSelection)?.count).toBe(1);
+    expect((newContent2.model.get(1) as ActivityBankSelection)?.count).toBe(2);
+  });
+
+  it('should apply adjustContentForConstraints immutably', () => {
+    const content = new PageEditorContent({
+      version: '1',
+      model: withDefaultContent(Immutable.List()),
+    });
+    expect(() => (content.version = '2')).toThrow();
+    const child: ActivityBankSelection = {
+      type: 'selection',
+      id: 'testChild',
+      logic: {
+        conditions: {
+          fact: Bank.Fact.objectives,
+          operator: Bank.ExpressionOperator.contains,
+          value: [],
+        },
+      },
+      count: 1,
+      children: undefined,
+    };
+    expect(content.model.size).toEqual(1);
+    const newContent1 = content.update('model', (model) => model.push(child));
+    expect(content.model.size).toBe(1);
+    expect(newContent1.model.size).toBe(2);
+    const newContent2 = PageEditor.adjustContentForConstraints(newContent1);
+
+    expect((newContent1.model.get(1) as ActivityBankSelection)?.logic).toBe(child.logic);
+    expect(child.logic).toEqual({
+      conditions: {
+        fact: Bank.Fact.objectives,
+        operator: Bank.ExpressionOperator.contains,
+        value: [],
+      },
+    });
+    expect((newContent2.model.get(1) as ActivityBankSelection)?.logic).toEqual({
+      conditions: null,
+    });
+  });
+
+  it('should apply adjustContentForConstraints to groups immutably', () => {
+    const content = new PageEditorContent({
+      version: '1',
+      model: withDefaultContent(Immutable.List()),
+    });
+    expect(() => (content.version = '2')).toThrow();
+
+    const child: ActivityBankSelection = {
+      type: 'selection',
+      id: 'testChild',
+      logic: {
+        conditions: {
+          fact: Bank.Fact.objectives,
+          operator: Bank.ExpressionOperator.contains,
+          value: [],
+        },
+      },
+      count: 1,
+      children: undefined,
+    };
+
+    const group: PurposeGroupContent = {
+      type: 'group',
+      id: 'g1',
+      layout: 'vertical',
+      purpose: 'instruction',
+      children: Immutable.List([child]),
+    };
+
+    expect(content.model.size).toEqual(1);
+    const newContent1 = content.update('model', (model) => model.push(group));
+    expect(content.model.size).toBe(1);
+    expect(newContent1.model.size).toBe(2);
+
+    // At this point we have a PageEditorContent with a group inside it that has an activity bank selection inside the group
+
+    // Sanity check to make sure the child is in the right place before we call adjustContentForConstraints
+    expect((newContent1.model.get(1) as PurposeGroupContent)?.children.get(0)).toEqual(child);
+
+    const newContent2 = PageEditor.adjustContentForConstraints(newContent1);
+
+    // adjustContentForConstraints should not mutate it's input
+    expect((newContent1.model.get(1) as PurposeGroupContent)?.children.get(0)).toEqual(child);
+
+    // The original child should never be mutated
+    expect(child.logic).toEqual({
+      conditions: {
+        fact: Bank.Fact.objectives,
+        operator: Bank.ExpressionOperator.contains,
+        value: [],
+      },
+    });
+
+    // The new content should be modified by adjustContentForConstraints
+    expect(
+      ((newContent2.model.get(1) as PurposeGroupContent)?.children.get(0) as ActivityBankSelection)
+        .logic,
+    ).toEqual({
+      conditions: null,
+    });
+  });
+});

config/config.exs

@@ -162,7 +162,8 @@ config :oli, Oban,
     analytics_export: 3,
     datashop_export: 3,
     objectives: 3
-  ]
+  ],
+  testing: :manual
 
 config :ex_money,
   auto_start_exchange_rate_service: false,

lib/oli/delivery/metrics.ex

@@ -262,18 +262,11 @@ defmodule Oli.Delivery.Metrics do
   """
   def progress_across_for_pages(section_id, pages_ids, user_ids) when is_list(user_ids) do
     from(ra in ResourceAccess,
-      where:
-        ra.resource_id in ^pages_ids and ra.section_id == ^section_id and
-          ra.user_id in ^user_ids,
+      where: ra.resource_id in ^pages_ids,
+      where: ra.section_id == ^section_id,
+      where: ra.user_id in ^user_ids,
       group_by: ra.resource_id,
-      select: {
-        ra.resource_id,
-        fragment(
-          "SUM(?) / (?)",
-          ra.progress,
-          ^Enum.count(user_ids)
-        )
-      }
+      select: {ra.resource_id, fragment("SUM(?) / (?)", ra.progress, ^Enum.count(user_ids))}
     )
     |> Repo.all()
     |> Enum.into(%{})

lib/oli/publishing/delivery_resolver.ex

@@ -59,7 +59,7 @@ defmodule Oli.Publishing.DeliveryResolver do
         map(rev, [:id, :resource_id, :title]),
         map(sr, [:scheduling_type, :end_date])
       },
-      order_by: [asc: sr.numbering_level, asc: sr.numbering_index]
+      order_by: [asc: sr.numbering_index, asc: sr.numbering_level]
     )
     |> Repo.all()
   end

lib/oli_web/components/delivery/practice_activities/practice_assessments_table_model.ex

@@ -109,13 +109,9 @@ defmodule OliWeb.Delivery.PracticeActivities.PracticeAssessmentsTableModel do
       })
 
     ~H"""
-    <%= if @avg_score != nil do %>
-      <div class={if @students_completion < 0.40, do: "text-red-600 font-bold"}>
-        <%= format_value(@students_completion) %>
-      </div>
-    <% else %>
-      -
-    <% end %>
+    <div class={if @students_completion < 0.40, do: "text-red-600 font-bold"}>
+      <%= format_value(@students_completion) %>
+    </div>
     """
   end
 

lib/oli_web/live/delivery/instructor_dashboard/helpers.ex

@@ -63,41 +63,21 @@ defmodule OliWeb.Delivery.InstructorDashboard.Helpers do
     graded_pages_and_section_resources =
       DeliveryResolver.graded_pages_revisions_and_section_resources(section.slug)
 
-    return_page(
-      graded_pages_and_section_resources,
-      section,
-      students
-    )
+    return_page(graded_pages_and_section_resources, section, students)
   end
 
-  defp return_page(
-         graded_pages_and_section_resources,
-         section,
-         students
-       ) do
+  defp return_page(graded_pages_and_section_resources, section, students) do
     student_ids = Enum.map(students, & &1.id)
     page_ids = Enum.map(graded_pages_and_section_resources, fn {rev, _} -> rev.resource_id end)
 
     progress_across_for_pages =
-      Metrics.progress_across_for_pages(
-        section.id,
-        page_ids,
-        student_ids
-      )
+      Metrics.progress_across_for_pages(section.id, page_ids, student_ids)
 
     avg_score_across_for_pages =
-      Metrics.avg_score_across_for_pages(
-        section,
-        page_ids,
-        student_ids
-      )
+      Metrics.avg_score_across_for_pages(section, page_ids, student_ids)
 
     attempts_across_for_pages =
-      Metrics.attempts_across_for_pages(
-        section,
-        page_ids,
-        student_ids
-      )
+      Metrics.attempts_across_for_pages(section, page_ids, student_ids)
 
     container_labels = Sections.map_resources_with_container_labels(section.slug, page_ids)
 
@@ -121,22 +101,14 @@ defmodule OliWeb.Delivery.InstructorDashboard.Helpers do
     ungraded_pages_and_section_resources =
       DeliveryResolver.ungraded_pages_revisions_and_section_resources(section.slug)
 
-    return_page(
-      ungraded_pages_and_section_resources,
-      section,
-      students
-    )
+    return_page(ungraded_pages_and_section_resources, section, students)
   end
 
   def get_assessments_with_surveys(section, students) do
     graded_pages_and_section_resources =
       DeliveryResolver.practice_pages_revisions_and_section_resources_with_surveys(section.slug)
 
-    return_page(
-      graded_pages_and_section_resources,
-      section,
-      students
-    )
+    return_page(graded_pages_and_section_resources, section, students)
   end
 
   def get_students(section, params \\ %{container_id: nil}) do