-
Notifications
You must be signed in to change notification settings - Fork 76
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
refactor: handle relative proctoring link #974
refactor: handle relative proctoring link #974
Conversation
Thanks for the pull request, @navinkarkera! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
1 similar comment
Thanks for the pull request, @navinkarkera! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
if (proctoringExamConfigurationLink && !( | ||
proctoringExamConfigurationLink.indexOf('http://') === 0 || proctoringExamConfigurationLink.indexOf('https://') === 0 | ||
)) { | ||
fullProctoringExamConfigurationLink = new URL(proctoringExamConfigurationLink, getConfig().STUDIO_BASE_URL).href; |
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.
fullProctoringExamConfigurationLink = new URL(proctoringExamConfigurationLink, getConfig().STUDIO_BASE_URL).href; | |
fullProctoringExamConfigurationLink = () => new URL(proctoringExamConfigurationLink, getConfig().STUDIO_BASE_URL).href; |
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.
There are known issues with Tutor where the variable will not generate the link unless new URL
is called as the return of a function.
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.
@KristinAoki Thanks! updated.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #974 +/- ##
=======================================
Coverage 92.20% 92.20%
=======================================
Files 703 703
Lines 12334 12336 +2
Branches 2671 2665 -6
=======================================
+ Hits 11372 11374 +2
Misses 926 926
Partials 36 36 ☔ View full report in Codecov by Sentry. |
03f362a
to
c0368d2
Compare
let fullProctoringExamConfigurationLink = () => proctoringExamConfigurationLink; | ||
|
||
// check if proctoringExamConfigurationLink is a relative link | ||
if (proctoringExamConfigurationLink && !( | ||
proctoringExamConfigurationLink.indexOf('http://') === 0 || proctoringExamConfigurationLink.indexOf('https://') === 0 | ||
)) { | ||
// new URL needs to called inside a function for tutor | ||
fullProctoringExamConfigurationLink = () => new URL( | ||
proctoringExamConfigurationLink, | ||
getConfig().STUDIO_BASE_URL, | ||
).href; | ||
} |
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 this this code is more complex than it needs to be.
let fullProctoringExamConfigurationLink = () => proctoringExamConfigurationLink; | |
// check if proctoringExamConfigurationLink is a relative link | |
if (proctoringExamConfigurationLink && !( | |
proctoringExamConfigurationLink.indexOf('http://') === 0 || proctoringExamConfigurationLink.indexOf('https://') === 0 | |
)) { | |
// new URL needs to called inside a function for tutor | |
fullProctoringExamConfigurationLink = () => new URL( | |
proctoringExamConfigurationLink, | |
getConfig().STUDIO_BASE_URL, | |
).href; | |
} | |
const fullProctoringExamConfigurationLink = () => ( | |
proctoringExamConfigurationLink && new URL(proctoringExamConfigurationLink, getConfig().STUDIO_BASE_URL.href; | |
); |
If the link is defined, and is relative then this will make the url relative to the studio base. If it's not a relative url it will override the base url.
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'm also not sure why this needs to be a function, nothing that runs in it should change between the component being mounted and the function being called.
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.
@xitij2000 Thanks! updated.
I'm also not sure why this needs to be a function, nothing that runs in it should change between the component being mounted and the function being called.
Same here. @KristinAoki Do we have a link or a PR which explains this tutor related bug?
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.
c0368d2
to
3e74e63
Compare
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 tested this: tested on tutor devstack
- I read through the code
- I checked for accessibility issues
- Includes documentation
@navinkarkera 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
If proctoring link is not an absolute url, update it to use studio as base url.
Supporting information
Private-ref
: BB-8832