[BUG FIX] [MER-3849] Explanation not showing (#5188)

* Oli.Delivery.Settings.show_explanation?/2

* add showExplanation flag to the activity context

* use effective settings when checking explanation condition while getting the activity state from an attempt

* fix tests - max_attempts for explanation should be checked at page level and not at activity level

* remove unnecessary showExplanation flag - it is already considered while generating the activity state

* reduce the amount of queries when finalizing an attempt - reuse the effective settings from the FinalizationContext or the socket

* reduce to 1 amount of times the effective_settings are calculated when building the page context

* fix TS error - showExplanation is no more used in component

assets/src/components/activities/common/delivery/evaluation/Evaluation.tsx

@@ -6,17 +6,12 @@ import { isDefined } from 'utils/common';
 
 interface Props {
   shouldShow?: boolean;
-  showExplanation?: boolean;
   attemptState: ActivityState;
   context: WriterContext;
   partOrder?: string[];
 }
 
-export function renderPartFeedback(
-  partState: PartState,
-  context: WriterContext,
-  showExplanation: boolean,
-) {
+export function renderPartFeedback(partState: PartState, context: WriterContext) {
   if (!partState.score && !partState.outOf) {
     return null;
   }
@@ -43,7 +38,7 @@ export function renderPartFeedback(
           direction={feedbackDirection}
         />
       </Component>
-      {showExplanation && explanation && resultCl !== 'correct' && (
+      {explanation && resultCl !== 'correct' && (
         <Component
           key={`${partState.partId}-explanation`}
           resultClass="explanation"
@@ -69,7 +64,6 @@ export function renderPartFeedback(
 
 export const Evaluation: React.FC<Props> = ({
   shouldShow = true,
-  showExplanation = true,
   attemptState,
   context,
   partOrder,
@@ -80,7 +74,7 @@ export const Evaluation: React.FC<Props> = ({
   }
 
   if (parts.length === 1) {
-    return renderPartFeedback(parts[0], context, showExplanation);
+    return renderPartFeedback(parts[0], context);
   }
 
   // part order for migrated multi-inputs may be random, so allow caller to specify appropriate one
@@ -90,9 +84,7 @@ export const Evaluation: React.FC<Props> = ({
     if (newOrder.length === parts.length) orderedParts = newOrder;
   }
 
-  return (
-    <>{orderedParts.map((partState) => renderPartFeedback(partState, context, showExplanation))}</>
-  );
+  return <>{orderedParts.map((partState) => renderPartFeedback(partState, context))}</>;
 };
 
 interface ComponentProps {

assets/src/components/activities/custom_dnd/FocusedFeedback.tsx

@@ -28,14 +28,14 @@ export const FocusedFeedback: React.FC<FocusedFeedbackProps> = (props: FocusedFe
       <React.Fragment>
         {uiState.attemptState.parts.map((part) => (
           <React.Fragment key={part.partId}>
-            {renderPartFeedback(part, writerContext, true)}
+            {renderPartFeedback(part, writerContext)}
           </React.Fragment>
         ))}
       </React.Fragment>
     );
   } else {
     // otherwise, only render the currently focused part feedback
     const part = uiState.attemptState.parts.find((ps) => ps.partId === focusedPart);
-    return part !== undefined ? renderPartFeedback(part, writerContext, true) : null;
+    return part !== undefined ? renderPartFeedback(part, writerContext) : null;
   }
 };

lib/oli/activities/state.ex

@@ -7,7 +7,7 @@ defmodule Oli.Activities.State do
   produce the corresponding activity state representation,
   returning those in a map of activity ids to the state.
   """
-  def from_attempts(latest_attempts, resource_attempt, page_revision) do
+  def from_attempts(latest_attempts, resource_attempt, page_revision, effective_settings) do
     Enum.map(latest_attempts, fn {id, {activity_attempt, part_attempts}} ->
       {:ok, model} = Oli.Delivery.Attempts.Core.select_model(activity_attempt) |> Model.parse()
 
@@ -17,7 +17,8 @@ defmodule Oli.Activities.State do
          Map.values(part_attempts),
          model,
          resource_attempt,
-         page_revision
+         page_revision,
+         effective_settings
        )}
     end)
     |> Map.new()

lib/oli/activities/state/activity_state.ex

@@ -34,19 +34,21 @@ defmodule Oli.Activities.State.ActivityState do
   ]
 
   @spec from_attempt(
-          Oli.Delivery.Attempts.Core.ActivityAttempt.t(),
-          [Oli.Delivery.Attempts.Core.PartAttempt.t()],
-          Oli.Activities.Model.t(),
-          Oli.Delivery.Attempts.Core.ResourceAttempt.t(),
-          Oli.Resources.Revision.t()
+          %Oli.Delivery.Attempts.Core.ActivityAttempt{},
+          [%Oli.Delivery.Attempts.Core.PartAttempt{}],
+          %Oli.Activities.Model{},
+          %Oli.Delivery.Attempts.Core.ResourceAttempt{} | nil,
+          %Oli.Resources.Revision{} | nil,
+          %Oli.Delivery.Settings.Combined{}
         ) ::
           %Oli.Activities.State.ActivityState{}
   def from_attempt(
         %ActivityAttempt{} = attempt,
         part_attempts,
         %Model{} = model,
         resource_attempt,
-        page_revision
+        page_revision,
+        effective_settings
       ) do
     # Create the part states, and where we encounter parts from the model
     # that do not have an attempt we create the default state
@@ -63,7 +65,8 @@ defmodule Oli.Activities.State.ActivityState do
             part_attempt: part_attempt,
             activity_attempt: attempt,
             resource_attempt: resource_attempt,
-            resource_revision: page_revision
+            resource_revision: page_revision,
+            effective_settings: effective_settings
           })
       end
     end
@@ -74,7 +77,7 @@ defmodule Oli.Activities.State.ActivityState do
       end)
 
     has_more_attempts =
-      case attempt.revision.max_attempts do
+      case effective_settings.max_attempts do
         0 -> true
         max -> attempt.attempt_number < max
       end

lib/oli/delivery/attempts/activity_lifecycle.ex

@@ -100,6 +100,8 @@ defmodule Oli.Delivery.Attempts.ActivityLifecycle do
     activity_attempt = get_activity_attempt_by(attempt_guid: activity_attempt_guid)
     resource_attempt = get_resource_attempt_and_revision(activity_attempt.resource_attempt_id)
 
+    effective_settings = Oli.Delivery.Settings.get_combined_settings(resource_attempt)
+
     result =
       Repo.transaction(fn ->
         if is_nil(activity_attempt) do
@@ -119,8 +121,8 @@ defmodule Oli.Delivery.Attempts.ActivityLifecycle do
               activity_attempt.resource_id
             )
 
-          if activity_attempt.revision.max_attempts > 0 and
-               activity_attempt.revision.max_attempts <= attempt_count do
+          if effective_settings.max_attempts > 0 and
+               effective_settings.max_attempts <= attempt_count do
             Repo.rollback({:no_more_attempts})
           else
             part_attempts = get_latest_part_attempts(activity_attempt_guid)
@@ -166,7 +168,8 @@ defmodule Oli.Delivery.Attempts.ActivityLifecycle do
                  new_part_attempts,
                  model,
                  resource_attempt,
-                 resource_attempt.revision
+                 resource_attempt.revision,
+                 effective_settings
                ), ModelPruner.prune(working_model)}
             else
               {:error, error} -> Repo.rollback(error)
@@ -293,7 +296,8 @@ defmodule Oli.Delivery.Attempts.ActivityLifecycle do
             part_attempt: part_attempt,
             activity_attempt: activity_attempt,
             resource_attempt: resource_attempt,
-            resource_revision: resource_attempt.revision
+            resource_revision: resource_attempt.revision,
+            effective_settings: Oli.Delivery.Settings.get_combined_settings(resource_attempt)
           })
         end
 

lib/oli/delivery/attempts/activity_lifecycle/evaluate.ex

@@ -409,7 +409,11 @@ defmodule Oli.Delivery.Attempts.ActivityLifecycle.Evaluate do
     {:ok, evaluations}
   end
 
-  def update_part_attempts_and_get_activity_attempts(resource_attempt, datashop_session_id) do
+  def update_part_attempts_and_get_activity_attempts(
+        resource_attempt,
+        datashop_session_id,
+        effective_settings
+      ) do
     activity_attempts =
       case resource_attempt.revision do
         %{content: %{"advancedDelivery" => true}} ->
@@ -432,7 +436,11 @@ defmodule Oli.Delivery.Attempts.ActivityLifecycle.Evaluate do
         if activity_attempt.lifecycle_state != :evaluated and activity_attempt.scoreable do
           activity_attempt = Map.put(activity_attempt, :resource_attempt, resource_attempt)
 
-          case update_part_attempts_for_activity(activity_attempt, datashop_session_id) do
+          case update_part_attempts_for_activity(
+                 activity_attempt,
+                 datashop_session_id,
+                 effective_settings
+               ) do
             {:ok, part_inputs} ->
               part_attempts = get_latest_part_attempts(activity_attempt.attempt_guid)
 
@@ -474,7 +482,7 @@ defmodule Oli.Delivery.Attempts.ActivityLifecycle.Evaluate do
     )
   end
 
-  def update_part_attempts_for_activity(activity_attempt, datashop_session_id) do
+  def update_part_attempts_for_activity(activity_attempt, datashop_session_id, effective_settings) do
     part_attempts = get_latest_part_attempts(activity_attempt.attempt_guid)
 
     part_inputs =
@@ -493,7 +501,7 @@ defmodule Oli.Delivery.Attempts.ActivityLifecycle.Evaluate do
       |> filter_already_evaluated(part_attempts)
 
     case activity_attempt
-         |> do_evaluate_submissions(part_inputs, part_attempts)
+         |> do_evaluate_submissions(part_inputs, part_attempts, effective_settings)
          |> persist_evaluations(part_inputs, fn result -> result end, datashop_session_id) do
       {:ok, _} -> {:ok, part_inputs}
       e -> e
@@ -724,13 +732,28 @@ defmodule Oli.Delivery.Attempts.ActivityLifecycle.Evaluate do
     do_evaluate_submissions(activity_attempt, part_inputs, part_attempts)
   end
 
+  defp do_evaluate_submissions(
+         activity_attempt,
+         part_inputs,
+         part_attempts,
+         effective_settings \\ nil
+       )
+
+  defp do_evaluate_submissions(activity_attempt, part_inputs, part_attempts, nil) do
+    effective_settings =
+      Oli.Delivery.Settings.get_combined_settings(activity_attempt.resource_attempt)
+
+    do_evaluate_submissions(activity_attempt, part_inputs, part_attempts, effective_settings)
+  end
+
   defp do_evaluate_submissions(
          %ActivityAttempt{
            resource_attempt: resource_attempt,
            attempt_number: attempt_number
          } = activity_attempt,
          part_inputs,
-         part_attempts
+         part_attempts,
+         effective_settings
        ) do
     activity_model = select_model(activity_attempt)
 
@@ -766,7 +789,8 @@ defmodule Oli.Delivery.Attempts.ActivityLifecycle.Evaluate do
               part_attempt: attempt,
               activity_attempt: activity_attempt,
               resource_attempt: resource_attempt,
-              resource_revision: resource_attempt.revision
+              resource_revision: resource_attempt.revision,
+              effective_settings: effective_settings
             })
           end)
 

lib/oli/delivery/attempts/page_lifecycle/graded.ex

@@ -149,7 +149,11 @@ defmodule Oli.Delivery.Attempts.PageLifecycle.Graded do
       }) do
     # Collect all of the part attempt guids for all of the activities that get finalized
     with {:ok, part_attempt_guids} <-
-           finalize_activity_and_part_attempts(resource_attempt, datashop_session_id),
+           finalize_activity_and_part_attempts(
+             resource_attempt,
+             datashop_session_id,
+             effective_settings
+           ),
          {:ok, resource_attempt} <-
            roll_up_activities_to_resource_attempt(resource_attempt, effective_settings),
          {:ok, resource_attempt} <- cancel_pending_auto_submit(resource_attempt) do
@@ -201,7 +205,11 @@ defmodule Oli.Delivery.Attempts.PageLifecycle.Graded do
   def finalize(_), do: {:error, {:already_submitted}}
 
   @decorate transaction_event("Graded.finalize_activity_and_part_attempts")
-  defp finalize_activity_and_part_attempts(resource_attempt, datashop_session_id) do
+  defp finalize_activity_and_part_attempts(
+         resource_attempt,
+         datashop_session_id,
+         effective_settings
+       ) do
     case resource_attempt.revision do
       # For adaptive pages, we never want to evaluate anything at finalization time
       %{content: %{"advancedDelivery" => true}} ->
@@ -211,7 +219,8 @@ defmodule Oli.Delivery.Attempts.PageLifecycle.Graded do
         with {_, activity_attempt_values, activity_attempt_params, part_attempt_guids} <-
                Evaluate.update_part_attempts_and_get_activity_attempts(
                  resource_attempt,
-                 datashop_session_id
+                 datashop_session_id,
+                 effective_settings
                ),
              {:ok, _} <-
                Persistence.bulk_update_activity_attempts(

lib/oli/delivery/evaluation/explanation.ex

@@ -4,6 +4,7 @@ defmodule Oli.Delivery.Evaluation.Explanation do
   alias Oli.Delivery.Evaluation.ExplanationContext
   alias Oli.Delivery.Attempts.Core.{ActivityAttempt, ResourceAttempt}
   alias Oli.Delivery.Evaluation.Actions.FeedbackAction
+  alias Oli.Delivery.Settings.Combined
 
   @doc """
   Determines whether an explanation should be shown using the given explanation context.
@@ -51,14 +52,16 @@ defmodule Oli.Delivery.Evaluation.Explanation do
   # (ignore unscored pages for now)
   defp check_explanation_condition(%ExplanationContext{
          resource_revision: %Revision{
-           graded: true,
+           graded: true
+         },
+         resource_attempt: %ResourceAttempt{
+           attempt_number: resource_attempt_number
+         },
+         effective_settings: %Combined{
            explanation_strategy: %ExplanationStrategy{
              type: :after_max_resource_attempts_exhausted
            },
            max_attempts: max_attempts
-         },
-         resource_attempt: %ResourceAttempt{
-           attempt_number: resource_attempt_number
          }
        }) do
     if resource_attempt_number >= max_attempts && max_attempts > 0 do
@@ -71,14 +74,16 @@ defmodule Oli.Delivery.Evaluation.Explanation do
   # show after set number of attempts strategy for scored pages
   defp check_explanation_condition(%ExplanationContext{
          resource_revision: %Revision{
-           graded: true,
+           graded: true
+         },
+         resource_attempt: %ResourceAttempt{
+           attempt_number: resource_attempt_number
+         },
+         effective_settings: %Combined{
            explanation_strategy: %ExplanationStrategy{
              type: :after_set_num_attempts,
              set_num_attempts: set_num_attempts
            }
-         },
-         resource_attempt: %ResourceAttempt{
-           attempt_number: resource_attempt_number
          }
        }) do
     if resource_attempt_number >= set_num_attempts do
@@ -91,17 +96,19 @@ defmodule Oli.Delivery.Evaluation.Explanation do
   # show after set number of attempts strategy for unscored pages
   defp check_explanation_condition(%ExplanationContext{
          resource_revision: %Revision{
-           graded: false,
-           explanation_strategy: %ExplanationStrategy{
-             type: :after_set_num_attempts,
-             set_num_attempts: set_num_attempts
-           }
+           graded: false
          },
          activity_attempt: %ActivityAttempt{
            attempt_number: activity_attempt_number,
            part_attempts: part_attempts
          },
-         part_attempt: part_attempt
+         part_attempt: part_attempt,
+         effective_settings: %Combined{
+           explanation_strategy: %ExplanationStrategy{
+             type: :after_set_num_attempts,
+             set_num_attempts: set_num_attempts
+           }
+         }
        }) do
     if length(part_attempts) > 1 do
       if part_attempt.attempt_number >= set_num_attempts,