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

PubCommonId - Add support for localStorage #3661

Merged
merged 2 commits into from
Apr 18, 2019

Conversation

pycnvr
Copy link
Collaborator

@pycnvr pycnvr commented Mar 20, 2019

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

Enable PubCommonId module to store id in localStorage. This is for publishers that have not yet switch to the new user id module. As a transition aid, the pubcid is written both as a cookie and also to the localStorage. Cookie values, if present, will be copied into localStorage. Otherwise a new id will be generated and written to both locations.

The default expiration time has also been reduced to 1 year.

Be sure to test the integration with your adserver using the Hello World sample page.

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

Other information

@mkendall07
Copy link
Member

This pull request introduces 1 alert when merging d764eefd8f637f28eb0821d99eb46087012109ab into 6cd91f9 - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

Comment posted by LGTM.com

@pycnvr pycnvr force-pushed the pubcid_local_storage branch from d764eef to 5ce7a59 Compare March 20, 2019 21:19
@bretg bretg requested a review from matthewlane March 21, 2019 01:14
@jsnellbaker jsnellbaker requested review from idettman and removed request for matthewlane March 26, 2019 16:05
@jsnellbaker jsnellbaker assigned idettman and unassigned matthewlane Mar 26, 2019
@pycnvr
Copy link
Collaborator Author

pycnvr commented Mar 29, 2019

I am going to rework this a bit to avoid copying id from local storage to cookie all the time.

Copy link
Contributor

@idettman idettman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just saw that you plan to rework, I'll review again when you complete your changes.

@idettman idettman self-requested a review March 29, 2019 17:25
@pycnvr
Copy link
Collaborator Author

pycnvr commented Mar 29, 2019

@idettman Thanks.

@idettman
Copy link
Contributor

idettman commented Apr 3, 2019

@pycnvr let me know when you're ready for review, thanks!

@pycnvr
Copy link
Collaborator Author

pycnvr commented Apr 3, 2019

@idettman Sorry about the delay. It should be ready soon. Thanks.

@pycnvr pycnvr force-pushed the pubcid_local_storage branch from 2d5d243 to 95fb0de Compare April 4, 2019 18:26
@pycnvr
Copy link
Collaborator Author

pycnvr commented Apr 4, 2019

@idettman Ready for review once again. Thanks.

Copy link
Contributor

@idettman idettman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@idettman idettman added needs 2nd review Core module updates require two approvals from the core team and removed needs update on hold labels Apr 5, 2019
@pycnvr
Copy link
Collaborator Author

pycnvr commented Apr 12, 2019

@bretg Would it be possible to include this in the next release?

@idettman idettman merged commit 984dfb1 into prebid:master Apr 18, 2019
@idettman idettman removed the needs 2nd review Core module updates require two approvals from the core team label Apr 18, 2019
VideoReach pushed a commit to VideoReach/Prebid.js that referenced this pull request Aug 1, 2019
* Add support for localStorage

* Pubcid uses local storage first
@pycnvr pycnvr deleted the pubcid_local_storage branch November 5, 2019 21:24
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.

4 participants