fix login cookie parse issue #26032
fix login cookie parse issue #26032
Conversation
| var cookie = document.cookie.match('(^|;)\\s*' + edxUserCookie + '\\s*=\\s*([^;]+)'); | ||
| var userCookie = cookie ? cookie.pop() : $.cookie(edxUserCookie); | ||
| cookie = document.cookie.match('(^|;)\\s*' + edxUserCookie + '\\s*=\\s*([^;]+)'); | ||
| userCookie = cookie ? cookie.pop() : $.cookie(edxUserCookie); |
There was a problem hiding this comment.
- I don't do much frontend development, so I think we should ask #fedx about this.
- It would be great to have some sort of test coverage if possible.
- I wonder if it makes sense to use the universal-cookie library?: https://github.com/edx/edx-platform/blob/master/package.json#L67
There was a problem hiding this comment.
I'd agree that using a library like universal-cookie is going to be preferable to doing a regex match to pull a value out of document.cookie - assuming we can use it in this file relatively easily. I expect you could add it on line 3 in this file and then use it.
There was a problem hiding this comment.
I tried to use universal-cookie in a define function in my own devstack and couldn't get it to work. There may be some way to get an installed npm package to work in RequireJS in our webpack config? I'm not quite familiar with how this works, unfortunately.
There was a problem hiding this comment.
I appreciate you tagging me. To make sure it is clear, we were giving ideas, but please don’t feel blocked by this. When you are confident in your fix, please proceed with reviews within your own team. And of course, feel free to ask questions if you need to. I just don’t want you to feel blocked. Thanks.
irfanuddinahmad
requested review from
zamanafzal,
muhammad-ammar,
robrap,
saleem-latif and
hasnain-naveed
|
I am looking into using some cookie library in requirejs, in the meantime I think it would be better to have at least some fix to go live. We have been getting more and more requests on the CR ticket. I will add some test coverage for the change I made |
zainab-amir
force-pushed
the
zamir/VAN-285/fix_login_cookie_parsing_issue
branch
from
158df41 to
81dd7ce
Compare
|
jenkins run js |
zainab-amir
force-pushed
the
zamir/VAN-285/fix_login_cookie_parsing_issue
branch
from
06d0f2a to
15ced93
Compare
zainab-amir
force-pushed
the
zamir/VAN-285/fix_login_cookie_parsing_issue
branch
from
15ced93 to
a0d3f0e
Compare
|
Your PR has finished running tests. There were no failures. |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/deploy_to_stage |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/deploy_to_stage |
|
EdX Release Notice: This PR has been deployed to the production environment. |
The cookie value returned by
$.cookie()is incomplete. The value returned by$.cookie()is truncated on the first occurrence of?v=The fix fetches the cookie value from document.cookie instead.
code for parsing document.cookie taken from https://stackoverflow.com/a/25490531/10035855
Ticket: https://openedx.atlassian.net/browse/VAN-285