-
Notifications
You must be signed in to change notification settings - Fork 225
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
feat: update proctoring info panel api call #1579
Conversation
@@ -54,7 +54,7 @@ describe('Outline Tab', () => { | |||
const goalUrl = `${getConfig().LMS_BASE_URL}/api/course_home/save_course_goal`; | |||
const masqueradeUrl = `${getConfig().LMS_BASE_URL}/courses/${courseId}/masquerade`; | |||
const outlineUrl = `${getConfig().LMS_BASE_URL}/api/course_home/outline/${courseId}`; | |||
const proctoringInfoUrl = `${getConfig().LMS_BASE_URL}/api/edx_proctoring/v1/user_onboarding/status?is_learning_mfe=true&course_id=${encodeURIComponent(courseId)}&username=MockUser`; | |||
const proctoringInfoUrl = `${getConfig().EXAMS_BASE_URL}/api/v1/student/course_id/${encodeURIComponent(courseId)}/onboarding?username=MockUser`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent a while trying to figure out how to mock out getConfig
to try adding a test for the legacy endpoint, but wasn't able to mock it successfully. I've updated the URL in the tests to match what is being called by the getProctoringInfoData
in the case where EXAMS_BASE_URL
is truthy, because that setting has a truthy value for the test environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense! I think it's sufficient/okay to not have testing for the legacy endpoint here. That being said, is it possible to test locally instead? Also okay with leaving as is, but just figured it could be a nice check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I did test locally as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just leaving a comment: Is this example useful to mock the getConfig
values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rijuma that's what I tried to do for the mock, but it wasn't actually mocking the correct thing.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1579 +/- ##
==========================================
+ Coverage 89.82% 89.89% +0.06%
==========================================
Files 326 333 +7
Lines 5601 5659 +58
Branches 1396 1399 +3
==========================================
+ Hits 5031 5087 +56
- Misses 554 555 +1
- Partials 16 17 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
@@ -54,7 +54,7 @@ describe('Outline Tab', () => { | |||
const goalUrl = `${getConfig().LMS_BASE_URL}/api/course_home/save_course_goal`; | |||
const masqueradeUrl = `${getConfig().LMS_BASE_URL}/courses/${courseId}/masquerade`; | |||
const outlineUrl = `${getConfig().LMS_BASE_URL}/api/course_home/outline/${courseId}`; | |||
const proctoringInfoUrl = `${getConfig().LMS_BASE_URL}/api/edx_proctoring/v1/user_onboarding/status?is_learning_mfe=true&course_id=${encodeURIComponent(courseId)}&username=MockUser`; | |||
const proctoringInfoUrl = `${getConfig().EXAMS_BASE_URL}/api/v1/student/course_id/${encodeURIComponent(courseId)}/onboarding?username=MockUser`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense! I think it's sufficient/okay to not have testing for the legacy endpoint here. That being said, is it possible to test locally instead? Also okay with leaving as is, but just figured it could be a nice check.
COSMO-626
All exams related requests should be routed through the exams service, if the
EXAMS_BASE_URL
is truthy. This follows the same pattern of https://github.com/openedx/frontend-lib-special-exams/blob/main/src/data/api.js.