Conversation
Code coverage summaryNote:
pixfutureRefer here for heat map coverage report |
Code coverage summaryNote:
pixfutureRefer here for heat map coverage report |
|
received
…On Tue, 24 Dec 2024 at 04:56, Ashish Garg ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In static/bidder-info/pixfuture.yaml
<#4117 (comment)>
:
> @@ -0,0 +1,16 @@
+endpoint: "https://srv-adapter.pixfuture.com/pixservices"
+maintainer:
+ email: ***@***.***"
Sent an email for verification. Please reply with "received".
—
Reply to this email directly, view it on GitHub
<#4117 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AUDNEQT5OAVWPASNEC4GV732HEVTPAVCNFSM6AAAAABT5HIYBCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMRRG43DAOBRGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
I would like to ask for your help with reviewing a pull request [New Adapter: Pixfuture #4117]. |
Code coverage summaryNote:
pixfutureRefer here for heat map coverage report |
|
Thank you, Sheridan, |
Code coverage summaryNote:
pixfutureRefer here for heat map coverage report |
|
Dear Sheridan, we made the requested changes. Please review and approve the merge. |
|
Hi team, please review our code and merge it. |
scr-oath
left a comment
There was a problem hiding this comment.
Review Update - Pixfuture Adapter
Thanks for the continued work on this adapter. I've done a thorough re-review of the current state. Many of the earlier comments have been addressed (struct naming, alphabetical ordering in exchange/adapter_builders.go, use of http.MethodPost, deleted commented-out code, etc.). However, several issues remain unresolved:
🔴 BLOCKING
1. Alphabetical ordering in openrtb_ext/bidders.go (still broken)
BidderPixfuture is at the end of the coreBidderNames array (after BidderZmaticoo). It needs to be moved to its correct alphabetical position between BidderPGAMSsp and BidderPlaydigo. The const block is correct but the array is not.
2. Missing userMacro in cookie sync configuration (pixfuture.yaml)
The userSync.redirect section is missing a userMacro field. Without this, {{.UserMacro}} in the default RedirectURL template resolves to an empty string, meaning the /setuid callback receives uid= (empty) and no user ID is ever stored. The entire cookie sync becomes non-functional.
Please add the macro your sync endpoint uses to substitute the user ID, e.g.:
userSync:
redirect:
url: "https://sync.pixfuture.com/cookiesync?gdpr={{.GDPR}}&consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&redirect={{.RedirectURL}}"
userMacro: "${UID}"3. Cookie sync endpoint must redirect to PBS /setuid
As @bsardo and @bretg noted, the sync endpoint must ultimately redirect to the PBS host's /setuid endpoint. The {{.RedirectURL}} macro handles constructing that URL, but your server at sync.pixfuture.com/cookiesync must honor the redirect query parameter and redirect the user there with their UID appended. Please confirm this is working.
4. Missing GPP privacy parameters in sync URL
Add GPP macros for compliance with multi-state privacy frameworks:
&gpp={{.GPP}}&gpp_sid={{.GPPSID}}
🟡 SUGGESTIONS (non-blocking but strongly recommended)
5. Memory efficiency in loop patterns (pixfuture.go)
In MakeBids, the for _, bid := range loop copies each openrtb2.Bid struct (which is large). Use index-based iteration instead:
for i := range bidResp.SeatBid {
for j := range bidResp.SeatBid[i].Bid {
bid := &bidResp.SeatBid[i].Bid[j]
bidType, err := getMediaTypeForBid(bid)
// ...
bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{
Bid: bid,
BidType: bidType,
})
}
}Also update getMediaTypeForBid to accept a pointer: func getMediaTypeForBid(bid *openrtb2.Bid).
6. Duplicate test files
supplemental/no_bids.json and supplemental/status_204.json are byte-identical (both test HTTP 204). Change no_bids.json to return HTTP 200 with an empty seatbid array to test the empty-bids code path at line 121.
7. Test coverage gaps
Consider adding tests for:
- Malformed JSON response body (exercises error path at lines 88-92)
- Multi-impression request (all current tests are single-impression)
- Bid response with
mtypefield set (currently all tests use theext.prebid.typefallback — none exercise the OpenRTB 2.6 MType code path)
✅ Previously raised comments that are now resolved
- Struct renamed from
PixfutureAdaptertoadapter✅ testdatadirectory replaced withpixfuturetest/exemplary+supplemental✅config.gochanges reverted ✅go.mod/go.sumno longer modified ✅coverage.outno longer in the diff ✅docs/pixfuture.mdremoved from PR ✅exchange/adapter_builders.goalphabetical ordering fixed ✅TestJsonSamplesnow callsRunJSONBidderTest✅minLength: 3added topix_idschema ✅- Uses
http.MethodPostinstead of"POST"✅ - Commented-out code deleted ✅
- Uses
adapters.IsResponseStatusCodeNoContent/CheckResponseStatusCodeForErrors✅ - GVL Vendor ID 839 verified ✅
- Maintainer email verified ✅
Code coverage summaryNote:
pixfutureRefer here for heat map coverage report |
Fix malformed JSON error handling in Pixfuture adapter
Code coverage summaryNote:
pixfutureRefer here for heat map coverage report |
Fixed issue with macroses
Code coverage summaryNote:
pixfutureRefer here for heat map coverage report |
Reordered by alphabet BidderPixfuture from the list of bidders.
fix order for BidderPixfuture,
Code coverage summaryNote:
pixfutureRefer here for heat map coverage report |
|
Hi Sheridan, We have resolved the following issues: Best regards, |
This Pixfuture Prebid Server Adapter enables seamless integration with Pixfuture's ad exchange, allowing publishers to leverage their demand through server-side header bidding. The adapter formats outgoing bid requests, processes incoming bid responses, and adheres to OpenRTB standards for efficient and privacy-compliant ad delivery.