New Adapter: Loyal #3586
Merged
New Adapter: Loyal #3586
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Code coverage summaryNote:
loyalRefer here for heat map coverage report |
SyntaxNode
reviewed
Mar 29, 2024
adapters/loyal/loyal.go
Outdated
| var bidderExt adapters.ExtImpBidder | ||
| var loyalExt openrtb_ext.ImpExtLoyal | ||
|
|
||
| if err = json.Unmarshal(reqCopy.Imp[0].Ext, &bidderExt); err != nil { |
There was a problem hiding this comment.
Please make it clear in your docs PR that only the ext of the first impression is used and that the other exts are ignored.
There was a problem hiding this comment.
I don’t quite understand you, we create a separate request in a loop (41 line) for each Imp object.
There was a problem hiding this comment.
Ah, I see. Please change reqCopy.Imp[0].Ext to imp.Ext to avoid confusion. You'll avoid a dereference.
| @@ -0,0 +1,14 @@ | |||
| endpoint: "https://us-east-1.loyal.app/pserver" | |||
There was a problem hiding this comment.
Do you have endpoints in other regions? Is only US traffic supported?
There was a problem hiding this comment.
We do not have endpoints in other regions and only support US traffic
There was a problem hiding this comment.
Gotcha. Please add the following in this yaml file to declare US traffic only:
geoscope:
- USA
See https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#bidder-info for further info on the geoscope.
adapters/loyal/loyal_test.go
Outdated
|
|
||
| func TestJsonSamples(t *testing.T) { | ||
| bidder, buildErr := Builder(openrtb_ext.BidderEmtv, config.Adapter{ | ||
| Endpoint: "http://example.com/pserver"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"}) |
There was a problem hiding this comment.
Nice use of an obviously fake testing endpoint. Thank you.
There was a problem hiding this comment.
This was a compliment, not a critique. Sorry if you interpreted this as sarcasm. We prefer the use of a fake testing endpoint in unit tests. Feel free to revert commit cf18f0e
Code coverage summaryNote:
loyalRefer here for heat map coverage report |
| @@ -0,0 +1,14 @@ | |||
| endpoint: "https://us-east-1.loyal.app/pserver" | |||
| maintainer: | |||
| email: "hello@loyal.app" | |||
There was a problem hiding this comment.
@teqblaze Email is being sent from prebid team to verify hello@loyal.app address. Requesting to respond back on email thread.
There was a problem hiding this comment.
received response from loyal team
adapters/loyal/loyal.go
Outdated
Comment on lines
121
to
122
| bidResponse := adapters.NewBidderResponseWithBidsCapacity(len(request.Imp)) | ||
| bidResponse.Currency = response.Cur |
There was a problem hiding this comment.
by default line 130 will initialize bidResponse.Currency as USD
reassigning bidResponse.Currency on line 131 without checking response.Cur length may overwrite bidResponse.Currency with blank value
code block can be refactored as below
bidResponse := adapters.NewBidderResponseWithBidsCapacity(len(request.Imp))
if len(response.Cur) != 0 {
bidResponse.Currency = response.Cur
}
adapters/loyal/loyal.go
Outdated
Comment on lines
124
to
128
| impsMappedByID := make(map[string]openrtb2.Imp, len(request.Imp)) | ||
| for i, imp := range request.Imp { | ||
| impsMappedByID[request.Imp[i].ID] = imp | ||
| } | ||
|
|
There was a problem hiding this comment.
impsMappedByID is not being used. should be removed
|
@teqblaze should update json test
|
added 3 commits
April 7, 2024 21:39
Code coverage summaryNote:
loyalRefer here for heat map coverage report |
thanks for review, tests added |
adapters/loyal/loyal.go
Outdated
| } | ||
|
|
||
| func (a *adapter) MakeRequests(request *openrtb2.BidRequest, reqInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) { | ||
| var err error |
There was a problem hiding this comment.
since scope of err is within the loop, no need to err outside for loop on line 42
line 48 and 52 could be modified as below:
if err := json.Unmarshal(imp.Ext, &bidderExt); err != nil {
errs = append(errs, err)
continue
}
if err := json.Unmarshal(bidderExt.Bidder, &loyalExt); err != nil {
errs = append(errs, err)
continue
}
There was a problem hiding this comment.
just one suggestion
Additionally, @teqblaze PTAL at #3586 (comment)
Code coverage summaryNote:
loyalRefer here for heat map coverage report |
Sonali-More-Xandr
previously approved these changes
Apr 8, 2024
Code coverage summaryNote:
loyalRefer here for heat map coverage report |
|
@teqblaze test suite is failing with following errors |
Code coverage summaryNote:
loyalRefer here for heat map coverage report |
|
@onkarvhanumante fixed |
onkarvhanumante
approved these changes
Apr 18, 2024
Sonali-More-Xandr
approved these changes
Apr 19, 2024
Sonali-More-Xandr
approved these changes
Apr 25, 2024
mefjush
pushed a commit
to adhese/prebid-server
that referenced
this pull request
Apr 25, 2024
Co-authored-by: loyal <hello@loyal.app>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
doc - prebid/prebid.github.io#5223