Skip to content

Gumgum Bid Adapter: add local time and timezone offset in requests#7587

Merged
ChrisHuie merged 2 commits intoprebid:masterfrom
gumgum:ADTS-164-send-local-time-and-timezone-offset-in-requests
Oct 20, 2021
Merged

Gumgum Bid Adapter: add local time and timezone offset in requests#7587
ChrisHuie merged 2 commits intoprebid:masterfrom
gumgum:ADTS-164-send-local-time-and-timezone-offset-in-requests

Conversation

@lbenmore
Copy link
Contributor

added unit test; updated an unrelated unit test that appears to not h…ave been testing the way it shouldve been

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

Adds the user's local time and their timezone offset (in minutes) to the requests sent to our ad server

const bidRequest = spec.buildRequests(bidRequests)[0];
const actualKeys = Object.keys(JSON.parse(bidRequest.data.jcsi)).sort();
expect(actualKeys).to.eq(actualKeys);
expect(actualKeys).to.eql(expectedKeys);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i only modified this because i don't think it was testing the way it was intended

@lgtm-com
Copy link

lgtm-com bot commented Oct 14, 2021

This pull request introduces 2 alerts when merging 2452e15 into ff18876 - view on LGTM.com

new alerts:

  • 2 for Unneeded defensive code


const date = new Date();
const lt = date && date.getTime();
const to = date && date.getTimezoneOffset();
Copy link
Collaborator

Choose a reason for hiding this comment

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

why waste your time with offset when you can get something more detailed like this console.log(Intl.DateTimeFormat().resolvedOptions().timeZone)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

our ad server is currently expecting the timezone offset in minutes. this keeps it consistent with all of our other requests. also, as it uses a pretty basic and straightforward method, i wouldn't consider it a waste of time.

@ChrisHuie ChrisHuie changed the title Gumgum: ADTS-164 Add local time and timezone offset in requests Gumgum Bid Adapter: add local time and timezone offset in requests Oct 18, 2021
@ChrisHuie ChrisHuie self-requested a review October 20, 2021 13:31
@ChrisHuie ChrisHuie self-assigned this Oct 20, 2021
@ChrisHuie ChrisHuie merged commit fdfe85e into prebid:master Oct 20, 2021
@lbenmore lbenmore deleted the ADTS-164-send-local-time-and-timezone-offset-in-requests branch January 4, 2022 17:27
cpabst pushed a commit to sovrn/Prebid.js that referenced this pull request Jan 10, 2022
…rebid#7587)

* Gumgum: ADTS-164 Send local time and timezone offset in ad requests

* object existence check before accessing property
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments