fix: workaround for pollAttempt fail in courseview #142

ilee2u · 2024-03-14T19:29:19Z

Re-fix for: https://2u-internal.atlassian.net/browse/COSMO-125

Combined fetchActiveAttempt & fetchLatestExamAttempt into one function with an optional "sequenceId" variable. At the very least, this works without causing errors in non-exam-sequence views. Since the sequenceId is undefined in that case, due to the sequenceId needing to be extracted from the sequence page itself, we can at least still get the latest exam attempt data.
To my best knowledge, we originally called for the attempt of an exam with a given sequenceId in order to get the latest attempt for an exam regardless of whether or not its active, such that the timer can be seen on non exam-sequence pages. However, we don't truly need the sequence_id for this.
It's also possible that this was also designed this way so that we could make sure a learner hadn't started two exams at once, and so their "latest exam attempt" would pretain to the exam they started most recently. We already have safeguards for this in edx_exams (See: https://github.com/edx/edx-exams/blob/main/edx_exams/apps/core/api.py#L201).

EDIT: I have not updated my tests yet because I want to make sure this approach is good first

- Combined fetchActiveAttempt & fetchLatestExamAttempt into one function with an optional "sequenceId" variable. At the very least, this works without causing errors in non-exam-sequence views. Since the sequenceId is undefined in that case, due to the sequenceId needing to be extracted from the sequence page itself, we can at least still get the latest exam attempt data. - To my best knowledge, we originally called for the attempt of an exam with a given sequenceId in order to get the latest attempt for an exam regardless of whether or not its active, such that the timer can be seen on non exam-sequence pages. However, we don't truly need the sequence_id for this. - It's also possible that this was also designed this way so that we could make sure a learner hadn't started two exams at once, and so their "latest exam attempt" would pretain to the exam they started most recently. We already have safeguards for this in edx_exams (See: https://github.com/edx/edx-exams/blob/main/edx_exams/apps/core/api.py#L201).

alangsto · 2024-03-14T19:41:39Z

src/data/thunks.js

@@ -271,8 +271,7 @@ export function pollAttempt(url) {
    }

    try {
-      // TODO: make sure sequenceId pulled here is correct both in-exam-sequence and in outline
-      // test w/ timed exam
+      // Why is this undefined in the course view? Let's see how it's called there...


Did you mean to include this comment?

alangsto · 2024-03-14T19:43:28Z

src/data/api.js


+  // sites configured with edx-exams expect sequenceId if pollUrl is not set
+  // and the learner is viewing the exam sequence
+  // (the exam sequence is not consumed outside the exam sequence, e.g. in the course view)


I think this comment could be a bit clearer, specifically the piece in parenthesis.

alangsto · 2024-03-14T19:44:35Z

src/data/api.js

+  if (sequenceId) {
+    // the calls the same endpoint as fetchActiveAttempt but it behaves slightly different
+    // with an exam's section specified. The attempt for that requested exam is always returned
+    // even if it is not 'active' (timer is not running)


I think this comment should be updated as well, because there is only one fetchActiveAttempt function, so the wording here doesn't make that much sense anymore.

alangsto · 2024-03-14T19:45:32Z

src/data/api.js

 import { ExamAction } from '../constants';
 import { generateHumanizedTime } from '../helpers';

 const BASE_API_URL = '/api/edx_proctoring/v1/proctored_exam/attempt';

-async function fetchActiveAttempt() {
+async function fetchActiveAttempt(sequenceId = null) {


Is there test coverage for the updated function now?

Zacharis278

I think there might be a root issue with the sequence id we're pulling here: https://github.com/openedx/frontend-lib-special-exams/blob/main/src/data/thunks.js#L277

This change handles the sequence id being none on the outline but what if the learner navigates elsewhere in the course?

src/data/api.js

Zacharis278 · 2024-03-18T13:07:12Z

@ilee2u I had a thought on this that might help if it turns out passing the correct sequence id is not simple.

The cause of this bug is this PR here #113 that assumes pollExamAttempt is always called with a sequence id (either in the pollUrl or directly). I think the simple fix here is adding a 3rd case to this function where we call fetchActiveAttempt instead of fetchLatestAttempt if no sequence (or poll url) is provided.

- also removed timerEnds param from TimerProvider useEffect, replaced it with a lint ignore

codecov · 2024-03-20T18:59:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.06%. Comparing base (52826e0) to head (620f89b).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #142      +/-   ##
==========================================
+ Coverage   94.05%   94.06%   +0.01%     
==========================================
  Files          68       68              
  Lines        1077     1079       +2     
  Branches      295      295              
==========================================
+ Hits         1013     1015       +2     
  Misses         59       59              
  Partials        5        5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ilee2u · 2024-03-20T20:17:20Z

I discovered that this line in TimerProvider in frontend-lib-special-exams causes the OuterExamTimer to ping the exams backend non-stop, as its value changes every time the exam is polled, which causes pollExam to be called as its in the dependency array of a useEffect(), causing a vicious cycle.

It seems like just removing this one line causes things to work again, though removing it triggers lint since it's "missing" from the dependency array, so I added a lint ignore

schenedx · 2024-03-20T20:40:00Z

src/data/redux.test.jsx

      });
    });
  });

  describe('Test pingAttempt', () => {
    it('Should send attempt to error state on ping failure', async () => {
+      // FIXME:


Why are these comments in the PR?

Ah I left those in by accident, let me remove those real quick

MichaelRoytman · 2024-03-20T21:12:20Z

src/timer/TimerProvider.jsx

@@ -142,8 +142,7 @@ const TimerProvider = ({
      clearInterval(timerRef.current);
      timerRef.current = null;
    };
-  }, [
-    timerEnds,
+  }, [ // eslint-disable-line react-hooks/exhaustive-deps


My understanding of timerEnds is that it should have a consistent value across calls to pollAttempt. Its purpose is to be a fixed time reference. What are the values of timerEnds that you're seeing locally that are not equal? I'd like for you or Marcos to double-check me on my understanding, though!

Unfortunately, written as is, this useEffect hook needs to have timerEnds in the dependency array, because it's used within the hook; see Specifying Reactive dependencies. If we were to move deadline outside of the hook, we could remove timerEnds from the dependency array. I think that should be fine, assuming timerEnds represents a static time.

Yes, I've mentioned this on our slack thread. This was an oversight from me when I worked on the TimerProvider. Removing the dependency is not the best approach.

I think the best would be to move this dependency to a reference so it does not need to be added as a dependency and if it gets updated it will not re-run the useEffect. We should just make sure that we are always using the reference instead of the primitive to avoid any issues.

To reply the question above:

What are the values of timerEnds that you're seeing locally that are not equal? I'd like for you or Marcos to double-check me on my understanding, though!

The problem here is caused by timerEnds being recalculated in each poll. Since the endpoint returns only the remaining seconds, this value can mean different things depending the time is evaluated. In this case, since timerEnds gets calculated when is saved at the store, this value probably gets a slightly minor offset every time (depending on network and execution delay), so the value is slightly different and the hook reacts to it.

This was the original problem for ticket COSMO-180 since every render it would take the remaining seconds again from the store based on the poll time and not the current time.

Is there a possibility to update the endpoint to return a datetime instead of the remaining seconds? Although this would mean that we need to deal with time offsets...

This problem could use some brainstorming.

Marcos paired with me and we were able to fix this :)

rijuma · 2024-03-21T14:53:18Z

src/timer/TimerProvider.jsx

+  const deadline = useRef(new Date(timerEnds));
+  useEffect(() => {
+    deadline.current = new Date(timerEnds);
+  }, [timerEnds]);


Thank you for fixing this!

rijuma

LGTM

Zacharis278

awesome looks good 👍

alangsto reviewed Mar 14, 2024

View reviewed changes

Zacharis278 reviewed Mar 14, 2024

View reviewed changes

src/data/api.js Show resolved Hide resolved

ilee2u added 2 commits March 19, 2024 15:39

temp: comments on what to do next

1ca8517

test: fixed tests to match new code

be138b5

- also removed timerEnds param from TimerProvider useEffect, replaced it with a lint ignore

schenedx reviewed Mar 20, 2024

View reviewed changes

chore: remove extra comments

47bb4a7

MichaelRoytman reviewed Mar 20, 2024

View reviewed changes

fix: use reference for deadline

620f89b

rijuma reviewed Mar 21, 2024

View reviewed changes

rijuma approved these changes Mar 21, 2024

View reviewed changes

Zacharis278 approved these changes Mar 21, 2024

View reviewed changes

ilee2u merged commit 10dd052 into main Mar 21, 2024
8 checks passed

ilee2u deleted the ilee2u/fix-sequenceId-null-bug branch March 21, 2024 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: workaround for pollAttempt fail in courseview #142

fix: workaround for pollAttempt fail in courseview #142

ilee2u commented Mar 14, 2024 •

edited

Loading

alangsto Mar 14, 2024

alangsto Mar 14, 2024

alangsto Mar 14, 2024

alangsto Mar 14, 2024

Zacharis278 left a comment

Zacharis278 commented Mar 18, 2024

codecov bot commented Mar 20, 2024 •

edited

Loading

ilee2u commented Mar 20, 2024

schenedx Mar 20, 2024

ilee2u Mar 20, 2024

MichaelRoytman Mar 20, 2024

rijuma Mar 20, 2024 •

edited

Loading

rijuma Mar 20, 2024

ilee2u Mar 21, 2024

rijuma Mar 21, 2024

rijuma left a comment

Zacharis278 left a comment

fix: workaround for pollAttempt fail in courseview #142

fix: workaround for pollAttempt fail in courseview #142

Conversation

ilee2u commented Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zacharis278 left a comment

Choose a reason for hiding this comment

Zacharis278 commented Mar 18, 2024

codecov bot commented Mar 20, 2024 • edited Loading

Codecov Report

ilee2u commented Mar 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rijuma Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rijuma left a comment

Choose a reason for hiding this comment

Zacharis278 left a comment

Choose a reason for hiding this comment

ilee2u commented Mar 14, 2024 •

edited

Loading

codecov bot commented Mar 20, 2024 •

edited

Loading

rijuma Mar 20, 2024 •

edited

Loading