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

GDPR and unique request #4688

Merged
merged 12 commits into from
Jan 14, 2020
Merged

GDPR and unique request #4688

merged 12 commits into from
Jan 14, 2020

Conversation

vincentproxistore
Copy link
Contributor

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

Make the adapter gdpr compliant and send a unique request to our server.
I had a previous previous PR but I had issues with circle ci. You can close it.

@jsnellbaker jsnellbaker self-requested a review January 6, 2020 15:30
@jsnellbaker jsnellbaker self-assigned this Jan 6, 2020
@jsnellbaker
Copy link
Collaborator

@vincentproxistore In regards to your comment about closing an older PR, are you referring to the #4567 PR? Please confirm.

In addition for this PR - can you please try to remove/undo the package-lock.json changes?

Yes it's that branch
@jsnellbaker
Copy link
Collaborator

@vincentproxistore That commit you pushed deletes the package-lock.json file from the project, which isn't ideal since we want to keep it in the project.

Can you checkout a copy of the file from master and push it into this PR? It'll keep the file in the same state when the PR is merged, so there shouldn't be a real net difference to the file.

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

@vincentproxistore When I tried to test the adapter with these updates using the test params in the md file, I encountered a 400 Bad Request response from the server.

Below is a copy of the request that was executed:

{"bidId":["2127b9f0b8e6af"],"auctionId":"e447c763-63ed-4a38-98f3-7f888498555f","transactionId":"75ddd1c3-f216-481d-97f3-6b7f7572efed","sizes":[{"width":[300,250],"height":[300,600]}],"website":"example.com","language":"fr","gdpr":{"applies":false}}

The raw response was huge (around 17k characters) due to the thread stack that was included in the response payload. I copied the value from the message field however for reference:

Bad Request: Could not read JSON document: Can not deserialize instance of java.lang.String out of START_ARRAY token↵ at [Source: java.io.PushbackInputStream@2172082d; line: 1, column: 10] (through reference chain: com.proxistore.advertisement.v3.dto.BidRequestDto["bidId"]); nested exception is com.fasterxml.jackson.databind.JsonMappingException: Can not deserialize instance of java.lang.String out of START_ARRAY token↵ at [Source: java.io.PushbackInputStream@2172082d; line: 1, column: 10] (through reference chain: com.proxistore.advertisement.v3.dto.BidRequestDto["bidId"])

If you want me to include the whole raw response, please let me know. Can you please take a look at the error?

@jsnellbaker
Copy link
Collaborator

Hi @vincentproxistore
I tried to rerun the test again, however now I'm seeing the following errors from the endpoint:

OPTIONS http://abs.proxistore.com/fr/v3/rtb/prebid 502 (Bad Gateway)
Access to XMLHttpRequest at 'http://abs.proxistore.com/fr/v3/rtb/prebid' from origin 'http://test.localhost:9999' has been blocked by CORS policy: Response to preflight request doesn't pass access control check: No 'Access-Control-Allow-Origin' header is present on the requested resource.

Would you be able to take a look at these errors and correct them? If there's something on my end that I need to adjust, please let me know. Thanks.

I am sorry but my backend team is doing some release at the moment.
Could you test again in a couple hours?
This morning I was able to display the ad.

Sorry for the disagreement.

Have a good weekend!

Vincent
Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

@vincentproxistore

When you have the chance, please take a look at the comment below. I am seeing bids return now, but there is something that needs to be addressed.

Please let me know if you have any questions. Thanks!

}
function _mapSizes(sizes) {
return sizes
.flatMap(array2d => array2d
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @vincentproxistore
When I was retesting the adapter, I ran our browserstack tests locally and found an issue with this flatMap function. It is not supported by IE or Edge.

If you want to continue to use this function, you'll need to add a polyfill. There appears to be one in core-js; should be able to import it from core-js/library/fn/array/flat-map. Otherwise you need to implement another form of logic that's browser compatible.

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

Thanks for making the additional update. LGTM

@jsnellbaker jsnellbaker merged commit 0ee2046 into prebid:master Jan 14, 2020
audiencerun pushed a commit to audiencerun/Prebid.js that referenced this pull request Jan 20, 2020
* gdpr compliant

* add unit test for gdpr compliance

* send only one request

* adapt test to changes

* delete log

* delete package-lock.json.
Yes it's that branch

* revert packe lock.json

* send the first bid

* send first bidId only

* Hi jsnellbaker!
I am sorry but my backend team is doing some release at the moment.
Could you test again in a couple hours?
This morning I was able to display the ad.

Sorry for the disagreement.

Have a good weekend!

Vincent

* delete comment

* remove flatMap
hellsingblack pushed a commit to SublimeSkinz/Prebid.js that referenced this pull request Mar 5, 2020
* gdpr compliant

* add unit test for gdpr compliance

* send only one request

* adapt test to changes

* delete log

* delete package-lock.json.
Yes it's that branch

* revert packe lock.json

* send the first bid

* send first bidId only

* Hi jsnellbaker!
I am sorry but my backend team is doing some release at the moment.
Could you test again in a couple hours?
This morning I was able to display the ad.

Sorry for the disagreement.

Have a good weekend!

Vincent

* delete comment

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

Successfully merging this pull request may close these issues.

2 participants