Fix cookie parsing -- values can contain equal signs #364
Conversation
|
It looks like the uncompressed bundle size difference is about 1.1K. Seems acceptable to me. |
There was a problem hiding this comment.
Why do we have tests for a third-party component? This isn't our code to test. Nothing about these tests is about the integration of js-cookie into our code; it's just testing the raw js-cookie behaviour.
In terms of the confidence that = signs in cookie values are handled correctly by js-cookie, then https://github.com/js-cookie/js-cookie/blob/0ba77141dd215782cc7770347a457906908c66ff/test/encoding.js#L315-L334 (at a very quick skim) would seem to cover that.
Instead, what I'd rather see are tests in our plugin for widgetLoad() that grab a cookie value with a = and ensure the fullUrl that is created is correct - and if that isn't easy to write a test for, then widgetLoad() needs to be broken down into smaller functions so that the logical parts of it can be tested.
Edit: Of course, writing and committing that integration test first, and see it fail with the existing code, and then adding in js-cookie and see it pass, would be the best workflow.
Include failing test cases for equal sign
jblz
force-pushed
the
fix-cookie-parsing
branch
from
343826b
to
2f1d7a4
Compare
| const uuid = personalized ? getUuidFromVisitorCookie() : undefined; | ||
|
|
||
| if ( personalized && uuid ) { | ||
| fullUrl += `&uuid=${ uuid }`; | ||
| if ( uuid ) { |
There was a problem hiding this comment.
This change is so we don't waste the i/o looking at cookies if there's no chance of using it. uuid is only referenced when personalized is truthy.
|
|
||
| test( 'set visitor cookie should return value', () => { | ||
| global.document.cookie = 'genre=Science Fiction'; | ||
| global.document.cookie = '_parsely_visitor=Species 8472'; |
There was a problem hiding this comment.
That's...niche, even amongst the rest of the geekness 😁
Co-authored-by: Gary Jones <gary.jones@automattic.com>
Description
Pull in a library to parse out the value for the
_parsely_visitorcookie instead of rolling our own.Additionally, pass the
uuidthroughencodeURIComponetfor good measure.Motivation and Context
Unfortunately, when the recommended content widget was refactored in #279, the lib that we wrote to parse out cookies neglected to take into account cookie values that contain an equal sign (
=). I noticed on a site that has personalization enabled, the cookie containing the uuid was not getting parsed. This means that personalization of results in the widget has been broken since version 2.5.The js-cookie library is active, mature, well-tested, and relatively small. Leveraging it allows us to rely on its battery of testing. Even so, I've adapted the testing for our previous lib to cover the previously covered use cases as well as the issue this PR intends to fix all the same.
How Has This Been Tested?
Automated testing
npm run testManual testing
uuidquery argument in the call tohttps://api.parsely.com/v2/relatedas per this line.Screenshots (if appropriate):
Types of changes
Bug fix (non-breaking change which fixes an issue)