New Adapter: MeloZen #3784
New Adapter: MeloZen #3784
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code coverage summaryNote:
melozenRefer here for heat map coverage report |
Code coverage summaryNote:
melozenRefer here for heat map coverage report |
gargcreation1992
changed the title
static/bidder-info/melozen.yaml
Outdated
| maintainer: | ||
| email: [email protected] |
There was a problem hiding this comment.
Prebid team has sent email to verify above maintainers email. Requesting to responding back on email thread
There was a problem hiding this comment.
@benben2001, email bounced back. Requesting to confirm maintainer email
There was a problem hiding this comment.
Thank you for pointing out the error with the email address. Indeed, there was a typo in the email provided. The correct email should be [email protected]. I have updated the static/bidder-info/melozen.yaml file with the correct information and pushed the changes to the PR. Please review the updated changes at your convenience. Thank you for your patience and assistance.
There was a problem hiding this comment.
Sent email again to the correct address.
There was a problem hiding this comment.
Hi @gargcreation1992 ,
I have sent the "received" confirmation to the specified email address as requested. Please let me know if there's anything else needed from my side.
Thank you for your assistance.
Best,
MeloZen
There was a problem hiding this comment.
received response from [email protected]
| @@ -0,0 +1,137 @@ | |||
| { | |||
There was a problem hiding this comment.
as per static/bidder-info/melozen.yaml video media type is not supported for app.
should update bidder info yaml to support video media type
else can remove this json test
There was a problem hiding this comment.
Thank you for pointing out the omission of video media type support in the yaml configuration for both site and app. I have updated the bidder info in the yaml file to include video as a supported media type and have also added corresponding JSON tests (app-video.json and web-video.json) to ensure coverage. The PR has been updated accordingly. Please review the changes at your convenience. Thanks again for your guidance!
adapters/melozen/melozen.go
Outdated
|
|
||
| requestCopy := *request | ||
| for _, imp := range request.Imp { | ||
| // Extract Sharethrough Params |
There was a problem hiding this comment.
nitpick: should be // Extract Melozen Params
There was a problem hiding this comment.
Thank you for the feedback on the comment line. I've updated the comment to correctly reflect // Extract Melozen Params as suggested. The change is included in the latest commit.
adapters/melozen/melozen.go
Outdated
| if strImpParams.PubId == "" { | ||
| return nil, []error{&errortypes.BadInput{ | ||
| Message: "The publisher ID must not be empty", | ||
| }} |
There was a problem hiding this comment.
PubId is mentioned as required ext param in bidder info yaml
Prebid core framework will ensure that PubId is present before invoking MakeRequests function
Therefore this check can be removed
There was a problem hiding this comment.
Thank you for your suggestion. I've removed the check for PubId as it's already ensured by the Prebid core framework, as you pointed out. The code is now updated and simplified. The changes are included in the latest commit. Please review the updates when you have a moment.
adapters/melozen/melozen.go
Outdated
| url, err := a.buildEndpointURL(strImpParams.PubId) | ||
| if err != nil { | ||
| return nil, []error{err} | ||
| } |
There was a problem hiding this comment.
@benben2001 we are discarding entire request if PubId is not present for any single imp.
you could collect this error and continue
There was a problem hiding this comment.
Thank you for your feedback. I have revised the code to not exit immediately on errors when building the endpoint URL, but instead to log these errors and continue with processing other imps. This should enhance the robustness of our adapter's error handling. I appreciate your guidance and please let me know if there are any further improvements needed.
| if impression.Banner == nil && impression.Native == nil && impression.Video == nil { | ||
| return nil, &errortypes.BadInput{Message: "Invalid MediaType. MeloZen only supports Banner, Video and Native."} | ||
| } |
There was a problem hiding this comment.
as per static/bidder-info/melozen.yaml video media type is not supported for app.
should update bidder info yaml to support video media type or remove video check
There was a problem hiding this comment.
Thank you for your feedback. I have updated the bidder-info/melozen.yaml file to reflect the correct media type support as suggested. The changes ensure that the video media type is appropriately handled. The updated configuration has been pushed to the PR. Please review the changes at your convenience. Thank you for guiding the improvements.
| if impression.Video != nil { | ||
| impCopy := *impression | ||
| impCopy.Banner = nil | ||
| impCopy.Native = nil | ||
| impressions = append(impressions, impCopy) | ||
| } |
There was a problem hiding this comment.
as per static/bidder-info/melozen.yaml video media type is not supported for app.
should update bidder info yaml to support video media type or remove video check
There was a problem hiding this comment.
Thank you for pointing out the need to align our code with the supported media types defined in bidder-info/melozen.yaml. I have updated the handling of media types within our adapter to ensure consistency with our declared capabilities. The changes have been made in the latest commit. Please review the updates and let me know if further adjustments are required.
There was a problem hiding this comment.
@benben2001 requesting to address open comments
Also should link the Bidder docs PR in PR description
1. Removed unnecessary code sections as per review suggestions. 2. Added JSON tests for better coverage. 3. Corrected errors in static/bidder-info/melozen.yaml.
Code coverage summaryNote:
melozenRefer here for heat map coverage report |
|
@onkarvhanumante Thank you for your guidance throughout the review process. I have addressed all open comments and made the necessary changes to the code as suggested. Additionally, I have updated the PR description to include a link to the Bidder documentation PR here for better context and completeness. Please let me know if there's anything else that needs to be revised or if further information is required. |
adapters/melozen/melozen.go
Outdated
| endpointTemplate *template.Template | ||
| } | ||
|
|
||
| // Builder builds a new instance of the {bidder} adapter for the given bidder with the given config. |
There was a problem hiding this comment.
replace {bidder} with melozen
There was a problem hiding this comment.
Thank you for your helpful suggestion! I have updated the comment to specify "MeloZen" instead of "{bidder}" to clarify that this section is specifically designed for the MeloZen adapter. I appreciate your attention to detail and your patience as we refine the code. The updated comment has been pushed to the PR.
There was a problem hiding this comment.
@benben2001 just one comment #3784 (comment). Good to merge after addressing it
Code coverage summaryNote:
melozenRefer here for heat map coverage report |
This pull request introduces a new bidder adapter called "MeloZen" for Prebid Server.
Hi @onkarvhanumante Documentation for this adapter can be found in PR #5467.