Huawei Ads: fix cta text empty and fill rate issue #3070
Merged
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:
huaweiadsRefer here for heat map coverage report |
SyntaxNode
changed the title
SyntaxNode
previously requested changes
Sep 1, 2023
There was a problem hiding this comment.
IMPORTANT NOTICE:
This adapter is scheduled to be removed from Prebid Server for not having a working contact email address. For us to review this PR, please include an updated group email address in either this PR or via a separate issue.
Please see #2626 for further details.
Thanks, the contact email address has been changed. @SyntaxNode |
This is fixed in #3071 |
I got it, this was the requested change for this pr. Can you review again |
SyntaxNode
requested review from
SyntaxNode
and removed request for
SyntaxNode
SyntaxNode
reviewed
Sep 5, 2023
adapters/huaweiads/huaweiads.go
Outdated
| var width int64 | ||
| var height int64 | ||
| */ |
There was a problem hiding this comment.
Please remove the commented out code.
adapters/huaweiads/huaweiads.go
Outdated
| width = asset.Img.W | ||
| height = asset.Img.H | ||
| } else if asset.Img.WMin != 0 && asset.Img.HMin != 0 { | ||
| width = asset.Img.WMin | ||
| height = asset.Img.HMin | ||
| } | ||
| }*/ |
There was a problem hiding this comment.
Same here. Please remove the commented out code.
adapters/huaweiads/huaweiads.go
Outdated
| adslot30.W = width | ||
| adslot30.H = height | ||
| adslot30.H = height */ |
There was a problem hiding this comment.
thanks. removed.
| @@ -1169,6 +1173,9 @@ func (a *adapter) extractAdmNative(adType int32, content *content, bidType openr | |||
| dataObject.Label = "desc" | |||
| dataObject.Value = getDecodeValue(content.MetaData.Description) | |||
| } | |||
| if asset.Data.Type == native1.DataAssetTypeCTAText { | |||
| dataObject.Value = getDecodeValue(content.MetaData.Cta) | |||
| } | |||
There was a problem hiding this comment.
Is this change covered by a test case?
There was a problem hiding this comment.
this insctance was missing, it is covered by test case
Code coverage summaryNote:
huaweiadsRefer here for heat map coverage report |
| @@ -166,6 +164,7 @@ | |||
| "appId": "101219405", | |||
| "appPromotionChannel": "401721412", | |||
| "clickUrl": "https://ads.huawei.com/usermgtportal/home/index.html#/", | |||
| "cta": "%e5%ae%89%e8%a3%85", | |||
There was a problem hiding this comment.
As per code change, cta value should be assigned as value to label . Butlabel` is empty
Should update tests so that cta value is reflected in adm
There was a problem hiding this comment.
thanks for the comment. i changed and tested it again. Can you check it please? @onkarvhanumante
Code coverage summaryNote:
huaweiadsRefer here for heat map coverage report |
onkarvhanumante
approved these changes
Sep 7, 2023
|
Hi @SyntaxNode. May you review please, thanks. |
gargcreation1992
approved these changes
Sep 7, 2023
gargcreation1992
dismissed
SyntaxNode’s stale review
Dismissing it since it was supposed to be blocked on huawei to update their contact information. This has been fixed.
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.
fix cta text empty and low fill rate issue