Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: workaround for pollAttempt fail in courseview #142

Merged
merged 5 commits into from
Mar 21, 2024
Merged

fix: workaround for pollAttempt fail in courseview #142

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dacc6e6
fix: workaround for pollAttempt fail in courseview
ilee2u Mar 14, 2024
1ca8517
temp: comments on what to do next
ilee2u Mar 19, 2024
be138b5
test: fixed tests to match new code
ilee2u Mar 20, 2024
47bb4a7
chore: remove extra comments
ilee2u Mar 20, 2024
620f89b
fix: use reference for deadline
ilee2u Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/core/OuterExamTimer.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,9 @@ import { getLatestAttemptData } from '../data';
import { IS_STARTED_STATUS } from '../constants';

const ExamTimer = ({ courseId }) => {
const { activeAttempt } = useSelector(state => state.specialExams);
const { activeAttempt, apiErrorMsg } = useSelector(state => state.specialExams);
const { authenticatedUser } = useContext(AppContext);
const showTimer = !!(activeAttempt && IS_STARTED_STATUS(activeAttempt.attempt_status));
const { apiErrorMsg } = useSelector(state => state.specialExams);
const dispatch = useDispatch();

useEffect(() => {
Expand Down
37 changes: 21 additions & 16 deletions src/data/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { getConfig } from '@edx/frontend-platform';
import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth';
import { logError } from '@edx/frontend-platform/logging';
import { ExamAction } from '../constants';
import { generateHumanizedTime } from '../helpers';

Expand All @@ -13,14 +12,14 @@ async function fetchActiveAttempt() {
return activeAttemptResponse.data;
}

async function fetchLatestExamAttempt(sequenceId) {
async function fetchAttemptForExamSequnceId(sequenceId) {
const attemptUrl = new URL(`${getConfig().EXAMS_BASE_URL}/api/v1/exams/attempt/latest`);
// 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)
const attemptUrl = new URL(`${getConfig().EXAMS_BASE_URL}/api/v1/exams/attempt/latest`);
attemptUrl.searchParams.append('content_id', sequenceId);
const response = await getAuthenticatedHttpClient().get(attemptUrl.href);
return response.data;
const attemptResponse = await getAuthenticatedHttpClient().get(attemptUrl.href);
return attemptResponse.data;
}

export async function fetchExamAttemptsData(courseId, sequenceId) {
Expand Down Expand Up @@ -70,25 +69,31 @@ export async function fetchLatestAttempt(courseId) {

export async function pollExamAttempt(pollUrl, sequenceId) {
let data;

// sites configured with only edx-proctoring must have pollUrl set
if (pollUrl) {
const edxProctoringURL = new URL(
`${getConfig().LMS_BASE_URL}${pollUrl}`,
);
const urlResponse = await getAuthenticatedHttpClient().get(edxProctoringURL.href);
data = urlResponse.data;
} else if (sequenceId && getConfig().EXAMS_BASE_URL) {
data = await fetchLatestExamAttempt(sequenceId);

// Update dictionaries returned by edx-exams to have correct status key for legacy compatibility
if (data.attempt_status) {
data.status = data.attempt_status;
delete data.attempt_status;
}

return data;

// exams configured with edx-exams expect sequenceId if pollUrl is not set when viewing the exam sequence
} if (sequenceId) {
data = await fetchAttemptForExamSequnceId(sequenceId);
// outside the exam sequence, we can't get the sequenceId easily, so here we just call the last active attempt
} else {
zacharis278 marked this conversation as resolved.
Show resolved Hide resolved
// sites configured with only edx-proctoring must have pollUrl set
// sites configured with edx-exams expect sequenceId if pollUrl is not set
logError(`pollExamAttempt recieved unexpected parameters pollUrl=${pollUrl} sequenceId=${sequenceId}`);
data = await fetchActiveAttempt();
}

// Update dictionaries returned by edx-exams to have correct status key for legacy compatibility
if (data.attempt_status) {
data.status = data.attempt_status;
delete data.attempt_status;
}

return data;
}

Expand Down
15 changes: 11 additions & 4 deletions src/data/redux.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -950,6 +950,7 @@ describe('Data layer integration tests', () => {
// Reset history so we can get url at index 0 later
axiosMock.resetHistory();

// FIXME: Have sequenceId initialized
const attemptToPollURL = `${latestAttemptURL}?content_id=block-v1%3Atest%2Bspecial%2Bexam%2Btype%40sequential%2Bblock%40abc123`;
axiosMock.onGet(attemptToPollURL).reply(200, {
time_remaining_seconds: 1739.9,
Expand Down Expand Up @@ -990,16 +991,21 @@ describe('Data layer integration tests', () => {
await api.pollExamAttempt(null, sequenceId);
expect(axiosMock.history.get[0].url).toEqual(expectedUrl);
});
test('pollUrl is required if edx-exams in not enabled, an error should be logged', async () => {
mergeConfig({ EXAMS_BASE_URL: null });
api.pollExamAttempt(null, null);
expect(loggingService.logError).toHaveBeenCalled();
test('should call the latest attempt w/o a sequence if if neither a pollUrl or sequence id is provided', async () => {
const expectedUrl = `${getConfig().EXAMS_BASE_URL}/api/v1/exams/attempt/latest`;
axiosMock.onGet(expectedUrl).reply(200, {
time_remaining_seconds: 1739.9,
status: ExamStatus.STARTED,
});
await api.pollExamAttempt(null);
expect(axiosMock.history.get[0].url).toEqual(expectedUrl);
});
});
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these comments in the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const startedWorkerAttempt = Factory.build('attempt', { attempt_status: ExamStatus.STARTED, desktop_application_js_url: 'http://proctortest.com' });
const startedWorkerExam = Factory.build('exam', { attempt: startedWorkerAttempt });
await initWithExamAttempt(startedWorkerExam, startedWorkerAttempt);
Expand Down Expand Up @@ -1028,6 +1034,7 @@ describe('Data layer integration tests', () => {
});

it('Should get, and save latest attempt', async () => {
// FIXME:
const attemptDataUrl = `${getConfig().LMS_BASE_URL}${BASE_API_URL}/course_id/${courseId}?is_learning_mfe=true`;
axiosMock.onGet(attemptDataUrl)
.reply(200, {
Expand Down
2 changes: 0 additions & 2 deletions src/data/thunks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,8 +271,6 @@ export function pollAttempt(url) {
}

try {
// TODO: make sure sequenceId pulled here is correct both in-exam-sequence and in outline
// test w/ timed exam
const { exam } = getState().specialExams;
const data = await pollExamAttempt(url, exam.content_id);
if (!data) {
Expand Down
3 changes: 1 addition & 2 deletions src/timer/TimerProvider.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,7 @@ const TimerProvider = ({
clearInterval(timerRef.current);
timerRef.current = null;
};
}, [
timerEnds,
}, [ // eslint-disable-line react-hooks/exhaustive-deps
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@rijuma rijuma Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pingInterval,
workerUrl,
processTimeLeft,
Expand Down
Loading