From b7ef13c08438ca5862827099ff6b2cd6a43848c2 Mon Sep 17 00:00:00 2001 From: Darren Siegel Date: Tue, 10 Sep 2024 15:50:50 -0400 Subject: [PATCH 01/14] delay submission to ensure all save state calls have finished --- .../activities/common/delivery/persistence.ts | 4 +-- .../multi_input/MultiInputDelivery.tsx | 6 ++--- .../short_answer/ShortAnswerDelivery.tsx | 16 ++++++----- assets/src/hooks/delayed_submit.ts | 22 +++++++++++++++ assets/src/hooks/index.ts | 2 ++ .../live/delivery/student/lesson_live.ex | 27 +++++++++++++++++-- 6 files changed, 64 insertions(+), 13 deletions(-) create mode 100644 assets/src/hooks/delayed_submit.ts diff --git a/assets/src/components/activities/common/delivery/persistence.ts b/assets/src/components/activities/common/delivery/persistence.ts index f02519410d2..921aab79bf4 100644 --- a/assets/src/components/activities/common/delivery/persistence.ts +++ b/assets/src/components/activities/common/delivery/persistence.ts @@ -2,8 +2,8 @@ import { LockResult } from 'data/persistence//lock'; import { DeferredPersistenceStrategy } from 'data/persistence/DeferredPersistenceStrategy'; import { PersistenceState, PersistenceStrategy } from 'data/persistence/PersistenceStrategy'; -export function initializePersistence(): PersistenceStrategy { - const p = new DeferredPersistenceStrategy(); +export function initializePersistence(quietPeriodInMs = 2000, maxDeferredTimeInMs = 5000): PersistenceStrategy { + const p = new DeferredPersistenceStrategy(quietPeriodInMs, maxDeferredTimeInMs); const noOpLockAcquire = () => Promise.resolve({ type: 'acquired' } as LockResult); const noOpLockRelease = () => Promise.resolve({ type: 'released' } as LockResult); const noOpSuccess = () => {}; diff --git a/assets/src/components/activities/multi_input/MultiInputDelivery.tsx b/assets/src/components/activities/multi_input/MultiInputDelivery.tsx index 844858e4894..46d8c75ba4c 100644 --- a/assets/src/components/activities/multi_input/MultiInputDelivery.tsx +++ b/assets/src/components/activities/multi_input/MultiInputDelivery.tsx @@ -224,7 +224,7 @@ export const MultiInputComponent: React.FC = () => { }), ); - const fn = () => + const doSave = () => onSaveActivity(uiState.attemptState.attemptGuid, [ { attemptGuid: part.attemptGuid, @@ -236,10 +236,10 @@ export const MultiInputComponent: React.FC = () => { if ((uiState.model as MultiInputSchema).submitPerPart && !context.graded) { handlePerPartSubmission(input.partId, value); } else { - fn(); + doSave(); } } else { - deferredSaves.current[id].save(fn); + doSave(); } } diff --git a/assets/src/components/activities/short_answer/ShortAnswerDelivery.tsx b/assets/src/components/activities/short_answer/ShortAnswerDelivery.tsx index 786426f3695..c8751fcecc4 100644 --- a/assets/src/components/activities/short_answer/ShortAnswerDelivery.tsx +++ b/assets/src/components/activities/short_answer/ShortAnswerDelivery.tsx @@ -77,7 +77,7 @@ export const ShortAnswerComponent: React.FC = () => { const uiState = useSelector((state: ActivityDeliveryState) => state); const { surveyId } = context; const dispatch = useDispatch(); - const deferredSave = useRef(initializePersistence()); + const deferredSave = useRef(initializePersistence(750, 1200)); useEffect(() => { listenForParentSurveySubmit(surveyId, dispatch, onSubmitActivity); @@ -121,11 +121,15 @@ export const ShortAnswerComponent: React.FC = () => { }), ); - deferredSave.current.save(() => - onSaveActivity(uiState.attemptState.attemptGuid, [ - { attemptGuid: uiState.attemptState.parts[0].attemptGuid, response: { input } }, - ]), - ); + const doSave = () => onSaveActivity(uiState.attemptState.attemptGuid, [ + { attemptGuid: uiState.attemptState.parts[0].attemptGuid, response: { input } }, + ]); + + if ((uiState.model as ShortAnswerModelSchema).inputType == 'textarea') { + deferredSave.current.save(doSave); + } else { + doSave(); + } }; return ( diff --git a/assets/src/hooks/delayed_submit.ts b/assets/src/hooks/delayed_submit.ts new file mode 100644 index 00000000000..fb14d5938dc --- /dev/null +++ b/assets/src/hooks/delayed_submit.ts @@ -0,0 +1,22 @@ +export const DelayedSubmit = { + mounted() { + this.el.addEventListener("click", (event: any) => { + event.preventDefault(); // Prevent immediate click action + + // Disable the button to prevent additional clicks + this.el.disabled = true; + + // Change the button label and show the spinner + this.el.querySelector(".button-text").textContent = "Submitting Answers..."; + this.el.querySelector(".spinner").classList.remove("hidden"); + + // Delay the phx-click event by two full seconds + setTimeout(() => { + // Trigger the Phoenix event after the delay + this.pushEvent("finalize_attempt"); + + // Optionally, remove the spinner and reset button state if needed + }, 2000); + }); + }, +}; diff --git a/assets/src/hooks/index.ts b/assets/src/hooks/index.ts index 58ff3cd2071..4d1e220279e 100644 --- a/assets/src/hooks/index.ts +++ b/assets/src/hooks/index.ts @@ -37,8 +37,10 @@ import { ToggleReadMore } from './toggle_read_more'; import { TooltipInit, TooltipWithTarget } from './tooltip'; import { VideoPlayer } from './video_player'; import { PauseOthersOnSelected, VideoPreview } from './video_preview'; +import { DelayedSubmit } from './delayed_submit'; export const Hooks = { + DelayedSubmit, GraphNavigation, DropTarget, DragSource, diff --git a/lib/oli_web/live/delivery/student/lesson_live.ex b/lib/oli_web/live/delivery/student/lesson_live.ex index cbf7412e3e5..559fec823bd 100644 --- a/lib/oli_web/live/delivery/student/lesson_live.ex +++ b/lib/oli_web/live/delivery/student/lesson_live.ex @@ -834,10 +834,33 @@ defmodule OliWeb.Delivery.Student.LessonLive do
<.references ctx={@ctx} bib_app_params={@bib_app_params} /> From d56817dcdbdc430eb55f499e8ac0cb729c2782bb Mon Sep 17 00:00:00 2001 From: Darren Siegel Date: Tue, 10 Sep 2024 15:52:01 -0400 Subject: [PATCH 02/14] set version --- mix.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mix.exs b/mix.exs index 56cba8828fe..ec4104883a9 100644 --- a/mix.exs +++ b/mix.exs @@ -4,7 +4,7 @@ defmodule Oli.MixProject do def project do [ app: :oli, - version: "0.28.7", + version: "0.28.8", elixir: "~> 1.15.5", elixirc_paths: elixirc_paths(Mix.env()), elixirc_options: elixirc_options(Mix.env()), From 92d1264f06f6ff8298f00379aa67f544e38e30dc Mon Sep 17 00:00:00 2001 From: Darren Siegel Date: Tue, 10 Sep 2024 16:08:17 -0400 Subject: [PATCH 03/14] reduce deferred period --- .../activities/response_multi/ResponseMultiInputDelivery.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assets/src/components/activities/response_multi/ResponseMultiInputDelivery.tsx b/assets/src/components/activities/response_multi/ResponseMultiInputDelivery.tsx index ed8217ee98f..bb71d165270 100644 --- a/assets/src/components/activities/response_multi/ResponseMultiInputDelivery.tsx +++ b/assets/src/components/activities/response_multi/ResponseMultiInputDelivery.tsx @@ -62,7 +62,7 @@ export const ResponseMultiInputComponent: React.FC = () => { const deferredSaves = useRef( model.inputs.reduce((m: any, input: MultiInput) => { - const p = initializePersistence(); + const p = initializePersistence(750, 1200); m[input.id] = p; return m; }, {}), From 4d858d805b095daa20b9618cf6ee9b93366037d9 Mon Sep 17 00:00:00 2001 From: Darren Siegel Date: Tue, 10 Sep 2024 16:14:48 -0400 Subject: [PATCH 04/14] disable all activity inputs --- assets/src/hooks/delayed_submit.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/assets/src/hooks/delayed_submit.ts b/assets/src/hooks/delayed_submit.ts index fb14d5938dc..d18cf380c00 100644 --- a/assets/src/hooks/delayed_submit.ts +++ b/assets/src/hooks/delayed_submit.ts @@ -3,6 +3,14 @@ export const DelayedSubmit = { this.el.addEventListener("click", (event: any) => { event.preventDefault(); // Prevent immediate click action + const inputs = document.querySelectorAll('input[type="text"], input[type="number"], textarea, select'); + + // Loop through each element and disable it. This prevents students from making any + // edits in activities while the submission is processing. + inputs.forEach((input: any) => { + input.disabled = true; + }); + // Disable the button to prevent additional clicks this.el.disabled = true; From 07ed109944a2e5f86e7def98be06c3ceaa3dfd61 Mon Sep 17 00:00:00 2001 From: darrensiegel Date: Tue, 10 Sep 2024 20:24:40 +0000 Subject: [PATCH 05/14] Auto format --- .../activities/common/delivery/persistence.ts | 5 ++++- .../activities/short_answer/ShortAnswerDelivery.tsx | 7 ++++--- assets/src/hooks/delayed_submit.ts | 12 +++++++----- assets/src/hooks/index.ts | 2 +- lib/oli_web/live/delivery/student/lesson_live.ex | 6 ++++-- 5 files changed, 20 insertions(+), 12 deletions(-) diff --git a/assets/src/components/activities/common/delivery/persistence.ts b/assets/src/components/activities/common/delivery/persistence.ts index 921aab79bf4..cd25b497eb2 100644 --- a/assets/src/components/activities/common/delivery/persistence.ts +++ b/assets/src/components/activities/common/delivery/persistence.ts @@ -2,7 +2,10 @@ import { LockResult } from 'data/persistence//lock'; import { DeferredPersistenceStrategy } from 'data/persistence/DeferredPersistenceStrategy'; import { PersistenceState, PersistenceStrategy } from 'data/persistence/PersistenceStrategy'; -export function initializePersistence(quietPeriodInMs = 2000, maxDeferredTimeInMs = 5000): PersistenceStrategy { +export function initializePersistence( + quietPeriodInMs = 2000, + maxDeferredTimeInMs = 5000, +): PersistenceStrategy { const p = new DeferredPersistenceStrategy(quietPeriodInMs, maxDeferredTimeInMs); const noOpLockAcquire = () => Promise.resolve({ type: 'acquired' } as LockResult); const noOpLockRelease = () => Promise.resolve({ type: 'released' } as LockResult); diff --git a/assets/src/components/activities/short_answer/ShortAnswerDelivery.tsx b/assets/src/components/activities/short_answer/ShortAnswerDelivery.tsx index c8751fcecc4..31cdb8187ce 100644 --- a/assets/src/components/activities/short_answer/ShortAnswerDelivery.tsx +++ b/assets/src/components/activities/short_answer/ShortAnswerDelivery.tsx @@ -121,9 +121,10 @@ export const ShortAnswerComponent: React.FC = () => { }), ); - const doSave = () => onSaveActivity(uiState.attemptState.attemptGuid, [ - { attemptGuid: uiState.attemptState.parts[0].attemptGuid, response: { input } }, - ]); + const doSave = () => + onSaveActivity(uiState.attemptState.attemptGuid, [ + { attemptGuid: uiState.attemptState.parts[0].attemptGuid, response: { input } }, + ]); if ((uiState.model as ShortAnswerModelSchema).inputType == 'textarea') { deferredSave.current.save(doSave); diff --git a/assets/src/hooks/delayed_submit.ts b/assets/src/hooks/delayed_submit.ts index d18cf380c00..6156f399ed8 100644 --- a/assets/src/hooks/delayed_submit.ts +++ b/assets/src/hooks/delayed_submit.ts @@ -1,9 +1,11 @@ export const DelayedSubmit = { mounted() { - this.el.addEventListener("click", (event: any) => { + this.el.addEventListener('click', (event: any) => { event.preventDefault(); // Prevent immediate click action - const inputs = document.querySelectorAll('input[type="text"], input[type="number"], textarea, select'); + const inputs = document.querySelectorAll( + 'input[type="text"], input[type="number"], textarea, select', + ); // Loop through each element and disable it. This prevents students from making any // edits in activities while the submission is processing. @@ -15,13 +17,13 @@ export const DelayedSubmit = { this.el.disabled = true; // Change the button label and show the spinner - this.el.querySelector(".button-text").textContent = "Submitting Answers..."; - this.el.querySelector(".spinner").classList.remove("hidden"); + this.el.querySelector('.button-text').textContent = 'Submitting Answers...'; + this.el.querySelector('.spinner').classList.remove('hidden'); // Delay the phx-click event by two full seconds setTimeout(() => { // Trigger the Phoenix event after the delay - this.pushEvent("finalize_attempt"); + this.pushEvent('finalize_attempt'); // Optionally, remove the spinner and reset button state if needed }, 2000); diff --git a/assets/src/hooks/index.ts b/assets/src/hooks/index.ts index 4d1e220279e..4986fa90449 100644 --- a/assets/src/hooks/index.ts +++ b/assets/src/hooks/index.ts @@ -8,6 +8,7 @@ import { Countdown } from './countdown'; import { CountdownTimer } from './countdown_timer'; import { CustomFocusWrap } from './custom_focus_wrap'; import { DateTimeLocalInputListener } from './datetimelocal_input_listener'; +import { DelayedSubmit } from './delayed_submit'; import { DragSource, DropTarget } from './dragdrop'; import { EmailList } from './email_list'; import { EndDateTimer } from './end_date_timer'; @@ -37,7 +38,6 @@ import { ToggleReadMore } from './toggle_read_more'; import { TooltipInit, TooltipWithTarget } from './tooltip'; import { VideoPlayer } from './video_player'; import { PauseOthersOnSelected, VideoPreview } from './video_preview'; -import { DelayedSubmit } from './delayed_submit'; export const Hooks = { DelayedSubmit, diff --git a/lib/oli_web/live/delivery/student/lesson_live.ex b/lib/oli_web/live/delivery/student/lesson_live.ex index 559fec823bd..1b36b745d9c 100644 --- a/lib/oli_web/live/delivery/student/lesson_live.ex +++ b/lib/oli_web/live/delivery/student/lesson_live.ex @@ -853,12 +853,14 @@ defmodule OliWeb.Delivery.Student.LessonLive do r="10" stroke="currentColor" stroke-width="4" - > + > + + > + From 55d7ee6fb816a97989216a844244e20490f0b2ba Mon Sep 17 00:00:00 2001 From: Darren Siegel Date: Wed, 11 Sep 2024 10:31:35 -0400 Subject: [PATCH 06/14] [BUG FIX] [MER-3775] Allow embedded page links to open (#5092) * account for unordered pages * use page_context instead of current_page --- .../components/layouts/student_delivery_lesson.html.heex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/oli_web/components/layouts/student_delivery_lesson.html.heex b/lib/oli_web/components/layouts/student_delivery_lesson.html.heex index 49dc534d7c9..8c67a64e5cd 100644 --- a/lib/oli_web/components/layouts/student_delivery_lesson.html.heex +++ b/lib/oli_web/components/layouts/student_delivery_lesson.html.heex @@ -89,7 +89,7 @@ <.back_arrow to={ - if assigns[:request_path] in [nil, ""], + if assigns[:request_path] in [nil, ""] and @current_page != nil, do: ~p"/sections/#{@section.slug}/learn?target_resource_id=#{@current_page["id"]}", else: assigns[:request_path] @@ -116,7 +116,7 @@ <%= live_render(@socket, OliWeb.Dialogue.WindowLive, session: %{ "section_slug" => @section.slug, - "resource_id" => @current_page["id"], + "resource_id" => @page_context.page.resource_id, "revision_id" => @page_context.page.id, "is_page" => true }, From 5c3bb25f29c6a7d3db35bf455eb2075e4d664a15 Mon Sep 17 00:00:00 2001 From: Darren Siegel Date: Thu, 12 Sep 2024 10:57:33 -0400 Subject: [PATCH 07/14] [CHORE] [MER-3786] allow setting of the interval and target for db_connection (#5093) --- config/runtime.exs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config/runtime.exs b/config/runtime.exs index b731b228f1d..d12cbc2e748 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -61,6 +61,8 @@ if config_env() == :prod do database: System.get_env("DB_NAME", "oli"), pool_size: String.to_integer(System.get_env("POOL_SIZE") || "10"), timeout: db_timeout, + queue_target: String.to_integer(System.get_env("DB_QUEUE_TARGET") || "50"), + queue_interval: String.to_integer(System.get_env("DB_QUEUE_INTERVAL") || "1000"), ownership_timeout: 600_000, socket_options: maybe_ipv6 From 85c81e755fc096c601fdb9dbedbf0b6ce0e8e7a4 Mon Sep 17 00:00:00 2001 From: Darren Siegel Date: Thu, 12 Sep 2024 14:34:49 -0400 Subject: [PATCH 08/14] [BUG FIX] [MER-3797] Fix Youtube XAPI emitting (#5099) * directly provide atempt guid * Auto format --------- Co-authored-by: darrensiegel --- .../editing/elements/youtube/YoutubeElement.tsx | 2 +- .../components/youtube_player/YoutubePlayer.tsx | 14 ++++++-------- assets/src/data/content/writers/html.tsx | 5 +++++ lib/oli/rendering/content/html.ex | 7 +++++++ lib/oli_web/controllers/api/xapi_controller.ex | 2 ++ 5 files changed, 21 insertions(+), 9 deletions(-) diff --git a/assets/src/components/editing/elements/youtube/YoutubeElement.tsx b/assets/src/components/editing/elements/youtube/YoutubeElement.tsx index aa54e3002fd..26c5108e06b 100644 --- a/assets/src/components/editing/elements/youtube/YoutubeElement.tsx +++ b/assets/src/components/editing/elements/youtube/YoutubeElement.tsx @@ -84,7 +84,7 @@ export const YouTubeEditor = (props: YouTubeProps) => { } > - + = ({ video, children, authorMode, context, pointMarkerContext }) => { +}> = ({ video, children, authorMode, context, pointMarkerContext, pageAttemptGuid }) => { const stopInterval = useRef(); const [videoTarget, setVideoTarget] = useState(null); const segments = useRef([]); @@ -101,15 +102,14 @@ export const YoutubePlayer: React.FC<{ }, [authorMode, video.endTime, video.startTime, videoTarget]); const onStateChange = (e: any) => { - if (!videoTarget) return; + if (!videoTarget || pageAttemptGuid == '') return; switch (e.data) { case 0: XAPI.emit_delivery( { type: 'page_video_key', - page_attempt_guid: - context?.resourceAttemptGuid !== undefined ? context?.resourceAttemptGuid : '', + page_attempt_guid: pageAttemptGuid, }, { type: 'video_completed', @@ -132,8 +132,7 @@ export const YoutubePlayer: React.FC<{ XAPI.emit_delivery( { type: 'page_video_key', - page_attempt_guid: - context?.resourceAttemptGuid !== undefined ? context?.resourceAttemptGuid : '', + page_attempt_guid: pageAttemptGuid, }, { type: 'video_played', @@ -155,8 +154,7 @@ export const YoutubePlayer: React.FC<{ XAPI.emit_delivery( { type: 'page_video_key', - page_attempt_guid: - context?.resourceAttemptGuid !== undefined ? context?.resourceAttemptGuid : '', + page_attempt_guid: pageAttemptGuid, }, { type: 'video_paused', diff --git a/assets/src/data/content/writers/html.tsx b/assets/src/data/content/writers/html.tsx index 7d8c758cdd6..d94e76049d6 100644 --- a/assets/src/data/content/writers/html.tsx +++ b/assets/src/data/content/writers/html.tsx @@ -365,11 +365,16 @@ export class HtmlParser implements WriterImpl { youtube(context: WriterContext, next: Next, attrs: YouTube) { if (!attrs.src) return <>; + let guid = ''; + if (context.resourceAttemptGuid !== undefined) { + guid = context.resourceAttemptGuid; + } return ( ); diff --git a/lib/oli/rendering/content/html.ex b/lib/oli/rendering/content/html.ex index fed75360bf9..87d1594f587 100644 --- a/lib/oli/rendering/content/html.ex +++ b/lib/oli/rendering/content/html.ex @@ -131,9 +131,16 @@ defmodule Oli.Rendering.Content.Html do end def youtube(%Context{} = context, _, %{"src" => _} = attrs) do + attempt_guid = + case context.resource_attempt do + nil -> "" + attempt -> attempt.attempt_guid + end + {:safe, video_player} = OliWeb.Common.React.component(context, "Components.YoutubePlayer", %{ "video" => attrs, + "pageAttemptGuid" => attempt_guid, "pointMarkerContext" => %{ renderPointMarkers: context.render_opts.render_point_markers, isAnnotationLevel: context.is_annotation_level diff --git a/lib/oli_web/controllers/api/xapi_controller.ex b/lib/oli_web/controllers/api/xapi_controller.ex index bde2eb338cd..b97db7e3dfc 100644 --- a/lib/oli_web/controllers/api/xapi_controller.ex +++ b/lib/oli_web/controllers/api/xapi_controller.ex @@ -22,6 +22,8 @@ defmodule OliWeb.Api.XAPIController do {:error, e} -> Logger.error("Error constructing xapi bundle: #{inspect(e)}") + Oli.Utils.Appsignal.capture_error("Error constructing xapi bundle: #{inspect(e)}") + json(conn, %{"result" => "failure", "reason" => e}) end end From 26accb64ca31647bfddf03cc24cfa43aa6a6b598 Mon Sep 17 00:00:00 2001 From: Eli Knebel Date: Thu, 12 Sep 2024 16:10:44 -0400 Subject: [PATCH 09/14] bump version number --- mix.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mix.exs b/mix.exs index ec4104883a9..caa63dc9248 100644 --- a/mix.exs +++ b/mix.exs @@ -4,7 +4,7 @@ defmodule Oli.MixProject do def project do [ app: :oli, - version: "0.28.8", + version: "0.28.9", elixir: "~> 1.15.5", elixirc_paths: elixirc_paths(Mix.env()), elixirc_options: elixirc_options(Mix.env()), From 9a529380f85d972ce4178744eee9655312fe2119 Mon Sep 17 00:00:00 2001 From: Eli Knebel Date: Tue, 17 Sep 2024 16:32:29 -0400 Subject: [PATCH 10/14] bump version number --- mix.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mix.exs b/mix.exs index caa63dc9248..99cbff4aaf6 100644 --- a/mix.exs +++ b/mix.exs @@ -4,7 +4,7 @@ defmodule Oli.MixProject do def project do [ app: :oli, - version: "0.28.9", + version: "0.28.10", elixir: "~> 1.15.5", elixirc_paths: elixirc_paths(Mix.env()), elixirc_options: elixirc_options(Mix.env()), From 259b7bbe200caabff94c3ff9aa010b1d0b215299 Mon Sep 17 00:00:00 2001 From: Darren Siegel Date: Wed, 18 Sep 2024 09:19:47 -0400 Subject: [PATCH 11/14] [BUG FIX] [MER-3823] Convert numeric to text input (#5109) * convert numeric to text input * cleanup * Auto format * change regex to allow 300. and .300 as valid numbers * allow same numbers in answer authoring as in delivery --------- Co-authored-by: darrensiegel Co-authored-by: Anders Weinstein --- .../common/delivery/inputs/NumericInput.tsx | 24 +++++-- .../short_answer/sections/NumericInput.tsx | 5 +- assets/src/utils/number.ts | 3 + assets/styles/delivery/activity.scss | 4 ++ assets/test/utils/number_test.ts | 63 +++++++++++++++++++ 5 files changed, 91 insertions(+), 8 deletions(-) create mode 100644 assets/src/utils/number.ts create mode 100644 assets/test/utils/number_test.ts diff --git a/assets/src/components/activities/common/delivery/inputs/NumericInput.tsx b/assets/src/components/activities/common/delivery/inputs/NumericInput.tsx index 4f65b6b413c..047de52afbc 100644 --- a/assets/src/components/activities/common/delivery/inputs/NumericInput.tsx +++ b/assets/src/components/activities/common/delivery/inputs/NumericInput.tsx @@ -1,7 +1,8 @@ -import React, { createRef } from 'react'; +import React, { createRef, useEffect, useState } from 'react'; import { MultiInputSize } from 'components/activities/multi_input/schema'; import { disableScrollWheelChange } from 'components/activities/short_answer/utils'; import { classNames } from 'utils/classNames'; +import { isValidNumber } from 'utils/number'; interface Props { value: string; @@ -12,20 +13,35 @@ interface Props { onBlur?: () => void; onKeyUp: (e: React.KeyboardEvent) => void; } + export const NumericInput: React.FC = (props) => { const numericInputRef = createRef(); + const [hasError, setHasError] = useState(false); + + useEffect(() => { + if (props.value === '' || !isValidNumber(props.value)) { + setHasError(true); + } else { + setHasError(false); + } + }, [props.value]); return ( props.onChange(e.target.value)} + onChange={(e) => { + const value = e.target.value; + props.onChange(value); + }} onBlur={props.onBlur} onKeyUp={props.onKeyUp} value={props.value} diff --git a/assets/src/components/activities/short_answer/sections/NumericInput.tsx b/assets/src/components/activities/short_answer/sections/NumericInput.tsx index b0612cdadae..4f4b4077eb1 100644 --- a/assets/src/components/activities/short_answer/sections/NumericInput.tsx +++ b/assets/src/components/activities/short_answer/sections/NumericInput.tsx @@ -12,6 +12,7 @@ import { } from 'data/activities/model/rules'; import { classNames } from 'utils/classNames'; import guid from 'utils/guid'; +import { isValidNumber } from 'utils/number'; import { disableScrollWheelChange } from '../utils'; // here we defined a "editable number" variant data type that contains information @@ -39,10 +40,6 @@ export type EditableNumber = InvalidNumber | ValidNumber; const isVariable = (s: numberOrVar): boolean => typeof s === 'string' && s.match(/^@@\w+@@$/) !== null; -const isValidNumber = (s: string): boolean => - // allowing .333 but not 33. Need disjunction to allow both - typeof s === 'string' && s.match(/^[+-]?\d*\.?\d+(?:[Ee][+-]?\d+)?$/) != null; - const parsedNumberOrEmptyString = (num: number | undefined) => num != undefined && !isNaN(num) ? num : ''; diff --git a/assets/src/utils/number.ts b/assets/src/utils/number.ts new file mode 100644 index 00000000000..36a2d71b29d --- /dev/null +++ b/assets/src/utils/number.ts @@ -0,0 +1,3 @@ +// disjunction allows for both .300 and 300. +const regex = /^[+-]?((\d+\.?\d*)|(\.\d+))([eE][-+]?\d+)?$/; +export const isValidNumber = (value: string) => regex.test(value); diff --git a/assets/styles/delivery/activity.scss b/assets/styles/delivery/activity.scss index b8f54a41018..5636204c95c 100644 --- a/assets/styles/delivery/activity.scss +++ b/assets/styles/delivery/activity.scss @@ -2,6 +2,10 @@ margin-top: 1em; margin-bottom: 4em; + .input-error { + border-color: red !important; /* Override any other border color */ + } + .activity-content { display: flex; flex-direction: column; diff --git a/assets/test/utils/number_test.ts b/assets/test/utils/number_test.ts new file mode 100644 index 00000000000..082fc13a979 --- /dev/null +++ b/assets/test/utils/number_test.ts @@ -0,0 +1,63 @@ +import { isValidNumber } from '../../src/utils/number'; + +describe('isValidNumber', () => { + // Valid cases + test('should return true for valid integers', () => { + expect(isValidNumber('0')).toBe(true); + expect(isValidNumber('3')).toBe(true); + expect(isValidNumber('-3')).toBe(true); + expect(isValidNumber('+3')).toBe(true); + expect(isValidNumber('123456')).toBe(true); + }); + + test('should return true for valid floating-point numbers', () => { + expect(isValidNumber('0.0')).toBe(true); + expect(isValidNumber('3.14')).toBe(true); + expect(isValidNumber('-3.14')).toBe(true); + expect(isValidNumber('123.456')).toBe(true); + expect(isValidNumber('.456')).toBe(true); + // not allowed in HTML standard but desired in significant figure contexts: + expect(isValidNumber('320.')).toBe(true); + }); + + test('should return true for valid scientific notation', () => { + expect(isValidNumber('3e10')).toBe(true); + expect(isValidNumber('-3e10')).toBe(true); + expect(isValidNumber('3.14e-2')).toBe(true); + expect(isValidNumber('2.5E+5')).toBe(true); + }); + + // Invalid cases + test('should return false for invalid numbers', () => { + expect(isValidNumber('3g')).toBe(false); + expect(isValidNumber('abc')).toBe(false); + expect(isValidNumber('3.14.15')).toBe(false); + expect(isValidNumber('++3')).toBe(false); + expect(isValidNumber('--3')).toBe(false); + expect(isValidNumber('3e+')).toBe(false); + expect(isValidNumber('e3')).toBe(false); + expect(isValidNumber('3e3e4')).toBe(false); + expect(isValidNumber('.')).toBe(false); + }); + + // Edge cases + test('should return false for empty strings', () => { + expect(isValidNumber('')).toBe(false); + }); + + test('should return false for whitespace', () => { + expect(isValidNumber(' ')).toBe(false); + expect(isValidNumber('\t')).toBe(false); + expect(isValidNumber('\n')).toBe(false); + }); + + test('should return false for only a decimal point', () => { + expect(isValidNumber('.')).toBe(false); + expect(isValidNumber('-')).toBe(false); + }); + + test('should return true for zero in different formats', () => { + expect(isValidNumber('0')).toBe(true); + expect(isValidNumber('0.0')).toBe(true); + }); +}); From 61180dc86d3b36db772673eb8efdb88980dcb268 Mon Sep 17 00:00:00 2001 From: Eli Knebel Date: Wed, 18 Sep 2024 10:00:30 -0400 Subject: [PATCH 12/14] [BUG FIX] [MER-3619] Fix an issue where account creation silently fails in certain cases (#5108) * fix an issue where account creation was not properly filtering out LTI accounts * remove redundant clause * Auto format --------- Co-authored-by: eliknebel --- lib/oli/accounts.ex | 11 ++++++ lib/oli_web/pow/messages.ex | 14 +------- lib/oli_web/pow/user_context.ex | 7 ++-- lib/oli_web/pow/user_identities_context.ex | 3 +- .../email/account_already_exists.html.eex | 35 ++++++++++++------- 5 files changed, 40 insertions(+), 30 deletions(-) diff --git a/lib/oli/accounts.ex b/lib/oli/accounts.ex index 820be3acb3e..5dfbd0bed2a 100644 --- a/lib/oli/accounts.ex +++ b/lib/oli/accounts.ex @@ -199,6 +199,17 @@ defmodule Oli.Accounts do """ def get_user_by(clauses), do: Repo.get_by(User, clauses) + @doc """ + Gets a single independent user by query parameter + ## Examples + iex> get_independent_user_by(email: "student1@example.com") + %User{independent_learner: true, ...} + iex> get_independent_user_by(email: "student2@example.com") + nil + """ + def get_independent_user_by(clauses), + do: Repo.get_by(User, Enum.into([independent_learner: true], clauses)) + @doc """ Gets a single user with platform roles and author preloaded Returns `nil` if the User does not exist. diff --git a/lib/oli_web/pow/messages.ex b/lib/oli_web/pow/messages.ex index 3c53a653e97..5f3fd1cb04f 100644 --- a/lib/oli_web/pow/messages.ex +++ b/lib/oli_web/pow/messages.ex @@ -58,17 +58,5 @@ defmodule OliWeb.Pow.Messages do end def pow_assent_login_with_provider(conn), - do: - interpolate("Continue with %{provider}", provider: Naming.humanize(conn.params["provider"])) - - defp interpolate(msg, opts) do - Enum.reduce(opts, msg, fn {key, value}, msg -> - token = "%{#{key}}" - - case String.contains?(msg, token) do - true -> String.replace(msg, token, to_string(value), global: false) - false -> msg - end - end) - end + do: "Continue with #{Naming.humanize(conn.params["provider"])}" end diff --git a/lib/oli_web/pow/user_context.ex b/lib/oli_web/pow/user_context.ex index daadbe00346..a47d3647d63 100644 --- a/lib/oli_web/pow/user_context.ex +++ b/lib/oli_web/pow/user_context.ex @@ -61,7 +61,7 @@ defmodule OliWeb.Pow.UserContext do """ @impl true def create(params) do - case Accounts.get_user_by(%{email: params["email"]}) do + case Accounts.get_independent_user_by(%{email: params["email"]}) do %User{email: email} = user -> if user.email_confirmed_at, do: @@ -70,8 +70,9 @@ defmodule OliWeb.Pow.UserContext do "Account already exists", "account_already_exists.html", %{ - url: Utils.ensure_absolute_url(Routes.pow_session_path(OliWeb.Endpoint, :new)), - forgot_password: + login_url: + Utils.ensure_absolute_url(Routes.pow_session_path(OliWeb.Endpoint, :new)), + forgot_password_url: Utils.ensure_absolute_url( Routes.pow_reset_password_reset_password_path(OliWeb.Endpoint, :new) ) diff --git a/lib/oli_web/pow/user_identities_context.ex b/lib/oli_web/pow/user_identities_context.ex index 9cdd4c18b9a..4fcf29da6d2 100644 --- a/lib/oli_web/pow/user_identities_context.ex +++ b/lib/oli_web/pow/user_identities_context.ex @@ -12,10 +12,9 @@ defmodule OliWeb.Pow.UserIdentities do %{"email" => email, "email_verified" => true} = user_params, user_id_params ) do - case Accounts.get_user_by(email: email) do + case Accounts.get_independent_user_by(email: email) do nil -> # user account with the given email doesnt exist, so create it - # user_params = Map.merge(user_params, %{"sub" => UUID.uuid4()}) pow_assent_create_user(user_identity_params, user_params, user_id_params) user -> diff --git a/lib/oli_web/templates/email/account_already_exists.html.eex b/lib/oli_web/templates/email/account_already_exists.html.eex index a558391a0e3..29fd6e16640 100644 --- a/lib/oli_web/templates/email/account_already_exists.html.eex +++ b/lib/oli_web/templates/email/account_already_exists.html.eex @@ -1,12 +1,23 @@ -
-

Are you trying to create a new account?

-

- Someone tried to create an account with this email but an account already exists. - If this was you, you can login <%= link "here", to: @url, target: "_blank" %> with your existing email and password. - If you forgot your password, you can reset it by clicking <%= link "Forgot Password?", to: @forgot_password, target: "_blank" %>. -

-

If this was not you, you can disregard this email.

-
- - - + + + + +
+ + + + +
+

Trying to create a new account?

+

+ We noticed there was an attempt to create a new account with this email but an account already exists. +

+

+ If this was you, <%= link "sign in to your existing account", to: @login_url, target: "_blank", style: "color: #2C67C4;" %>. + If you forgot your password, you can reset it <%= link "here", to: @forgot_password_url, target: "_blank", style: "color: #2C67C4;" %>. +

+

+ If this was not you, you can ignore this email. +

+
+
From 19a16d93610ae1beb84612a619d9b74f845a04e7 Mon Sep 17 00:00:00 2001 From: Eli Knebel Date: Tue, 24 Sep 2024 12:13:40 -0400 Subject: [PATCH 13/14] fix pow_test --- lib/oli_web/pow/user_context.ex | 4 ++-- lib/oli_web/templates/email/account_already_exists.html.eex | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/oli_web/pow/user_context.ex b/lib/oli_web/pow/user_context.ex index a47d3647d63..0ab52d96766 100644 --- a/lib/oli_web/pow/user_context.ex +++ b/lib/oli_web/pow/user_context.ex @@ -70,9 +70,9 @@ defmodule OliWeb.Pow.UserContext do "Account already exists", "account_already_exists.html", %{ - login_url: + url: Utils.ensure_absolute_url(Routes.pow_session_path(OliWeb.Endpoint, :new)), - forgot_password_url: + forgot_password: Utils.ensure_absolute_url( Routes.pow_reset_password_reset_password_path(OliWeb.Endpoint, :new) ) diff --git a/lib/oli_web/templates/email/account_already_exists.html.eex b/lib/oli_web/templates/email/account_already_exists.html.eex index 29fd6e16640..06a364009e5 100644 --- a/lib/oli_web/templates/email/account_already_exists.html.eex +++ b/lib/oli_web/templates/email/account_already_exists.html.eex @@ -9,8 +9,8 @@ We noticed there was an attempt to create a new account with this email but an account already exists.

- If this was you, <%= link "sign in to your existing account", to: @login_url, target: "_blank", style: "color: #2C67C4;" %>. - If you forgot your password, you can reset it <%= link "here", to: @forgot_password_url, target: "_blank", style: "color: #2C67C4;" %>. + If this was you, <%= link "sign in to your existing account", to: @url, target: "_blank", style: "color: #2C67C4;" %>. + If you forgot your password, you can reset it <%= link "here", to: @forgot_password, target: "_blank", style: "color: #2C67C4;" %>.

If this was not you, you can ignore this email. From 27981122ae472d018101ee8bdec848a1f2053981 Mon Sep 17 00:00:00 2001 From: Eli Knebel Date: Tue, 24 Sep 2024 12:20:09 -0400 Subject: [PATCH 14/14] fix formatting --- lib/oli_web/pow/user_context.ex | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/oli_web/pow/user_context.ex b/lib/oli_web/pow/user_context.ex index 0ab52d96766..0ba01a90eee 100644 --- a/lib/oli_web/pow/user_context.ex +++ b/lib/oli_web/pow/user_context.ex @@ -70,8 +70,7 @@ defmodule OliWeb.Pow.UserContext do "Account already exists", "account_already_exists.html", %{ - url: - Utils.ensure_absolute_url(Routes.pow_session_path(OliWeb.Endpoint, :new)), + url: Utils.ensure_absolute_url(Routes.pow_session_path(OliWeb.Endpoint, :new)), forgot_password: Utils.ensure_absolute_url( Routes.pow_reset_password_reset_password_path(OliWeb.Endpoint, :new)