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.

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

[BD-26][EDUCATOR-5761, EDUCATOR-5762] Add additional pages for proctored exams #10

Merged

Conversation

viktorrusakov
Copy link
Contributor

@viktorrusakov viktorrusakov commented May 26, 2021

This PR adds following pages for proctoring exam flow: user verification, system check, ready to start instructions

NOTE: should be merged first

JIRA: EDUCATOR-5761 EDUCATOR-5762

@openedx-webhooks openedx-webhooks added blended PR is managed through 2U's blended developmnt program community manager review labels May 26, 2021
@openedx-webhooks
Copy link

openedx-webhooks commented May 26, 2021

Thanks for the pull request, @viktorrusakov! I've created BLENDED-855 to keep track of it in Jira. More details are on the BD-26 project page.

When this pull request is ready, tag your edX technical lead.

@zacharis278
Copy link
Contributor

can you update this with the commits from main?

@viktorrusakov viktorrusakov force-pushed the rusakov/EDUCATOR-5761/proctoring-pages branch 2 times, most recently from c74970c to c51257b Compare May 31, 2021 08:53
@viktorrusakov
Copy link
Contributor Author

@zacharis278 review please, this one should be merged first

Copy link
Contributor

@zacharis278 zacharis278 left a comment

Choose a reason for hiding this comment

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

The new views themselves look good but there's a few things I noticed while going through the code.

  1. It seems like we're doubling up API calls to the backend for review policy and settings (it's not just OPTIONS requests but multiple GETS)
  2. If the settings call fails I seem to get an infinite loading animation. (this can definitely be fixed in another PR)
  3. I'm going to take a detailed look at the tests later for what's important but based on codecov it seems like our coverage is lacking.

@@ -168,7 +175,7 @@ export function stopExam() {
};
}

export function continueExam() {
export function continueExam(noLoading = true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is mostly a comment on the existing code here. This continue exam flow isn't making much sense to me, why do we need to make a backend call with a start action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is mainly used when you are on ready_to_submit page and instead of clicking 'submit' you click 'continue my exam', then this function gets called (this is also how it is done in LMS)

Also in this PR this function is used to start proctored exam (I'm not sure why I did this) but it gets refactored in #14

@@ -9,6 +9,7 @@ export const examSlice = createSlice({
activeAttempt: null,
proctoringSettings: {},
exam: {},
verification: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nice to scaffold out the entire model for each of these backend models so developers know what the data will look like. (proctoringSettings, exam, verification)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will create tests for redux later and I believe we will have snapshots / factories for our data, will it work or do you mean something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd hoped to see something more like this for each model

verification: {
      status: '',
      can_verify: false,
    },
...

let's follow up in another PR since I've merged this already.

@viktorrusakov
Copy link
Contributor Author

viktorrusakov commented Jun 3, 2021

The new views themselves look good but there's a few things I noticed while going through the code.

  1. It seems like we're doubling up API calls to the backend for review policy and settings (it's not just OPTIONS requests but multiple GETS)
  2. If the settings call fails I seem to get an infinite loading animation. (this can definitely be fixed in another PR)
  3. I'm going to take a detailed look at the tests later for what's important but based on codecov it seems like our coverage is lacking.
  1. Are you sure that review policy API gets called multiple times? I think this is true only for proctoring settings and verification data API calls, and they are both being addressed in our other PRs (verification data calls are currently addressed in our internal PR here and proctoring settings in [BD-26] Fix a bug with JS worker #14). That was my bad, I should have noticed it during this PR, but I noticed much later...
  2. Yes, we missed that, one of the previous PRs was dealing with handling such errors properly but at the time we didn't have settings API and some others and when we added them we forgot to add functionality of that PR to new API calls. We will create another PR later to make sure every API call is handled correctly.
  3. There are more tests added in [BD-26] feat: add test coverage for all instructions #12, so coverage should get better. Note that these tests are only for components, we are planning to add tests for redux soon which will cover our store / API functionality.

@zacharis278
Copy link
Contributor

  1. So it looks like this is only happening when you load the page directly in a ready_to_start state but not when the state transitions. We can fix it later.

I'm going to go ahead and merge this in and keep track of things we may need to fix as we go

@zacharis278 zacharis278 merged commit addb8f8 into openedx:main Jun 3, 2021
@github-actions
Copy link

github-actions bot commented Jun 3, 2021

🎉 This PR is included in version 1.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blended PR is managed through 2U's blended developmnt program merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants