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

Use the same options to test and set cookies #555

Merged
merged 4 commits into from
Oct 15, 2020

Conversation

kichik
Copy link
Contributor

@kichik kichik commented Oct 9, 2020

Current code only uses some of the configured flags to set test cookies. This leads to cases where testing for cookies can fail even though setting cookies would eventually work.

Motivation

Using RUM inside <iframe> didn't work, even though all the documented flags required for it were enabled.

Changes

areCookiesAuthorized() uses CookieOptions instead of just { secure: useSecureCookie }.

Testing

Use RUM in <iframe> that's on a different domain than the main page.


I have gone over the contributing documentation.

Current code only uses some of the configured flags to set test cookies. This leads to cases where testing for cookies can fail even though setting cookies would eventually work.
@kichik kichik requested a review from a team as a code owner October 9, 2020 01:48
@bits-bot
Copy link

bits-bot commented Oct 9, 2020

CLA assistant check
All committers have signed the CLA.

@bcaudan
Copy link
Contributor

bcaudan commented Oct 9, 2020

Hi @kichik,

Thanks for this PR, it is conflicting with some in progress work so we will try to integrate it in the coming days.

@codecov-io
Copy link

Codecov Report

Merging #555 into master will decrease coverage by 0.02%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #555      +/-   ##
==========================================
- Coverage   88.45%   88.42%   -0.03%     
==========================================
  Files          42       42              
  Lines        2296     2299       +3     
  Branches      479      479              
==========================================
+ Hits         2031     2033       +2     
- Misses        265      266       +1     
Impacted Files Coverage Δ
packages/logs/src/logs.ts 74.19% <0.00%> (ø)
packages/core/src/configuration.ts 86.66% <100.00%> (+0.70%) ⬆️
packages/core/src/cookie.ts 89.58% <100.00%> (ø)
packages/core/src/init.ts 68.42% <100.00%> (ø)
packages/rum/src/rum.entry.ts 94.00% <100.00%> (ø)
packages/rum/src/parentContexts.ts 98.24% <0.00%> (-1.76%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6991c3d...58755b4. Read the comment docs.

@BenoitZugmeyer BenoitZugmeyer merged commit a5bf7c4 into DataDog:master Oct 15, 2020
@kichik
Copy link
Contributor Author

kichik commented Oct 15, 2020

Thanks! Any idea when 1.23.0 might be available on npm?

@BenoitZugmeyer
Copy link
Member

@kichik Thank you for your contribution! v1.23.0 has been published, and should be available on NPM right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants