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.

Sign up for GitHub

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

Jump to bottom

Updated YuktaMedia Analytics Adapter: updated request body with more details #5547

Merged
merged 19 commits into from
Aug 11, 2020
Merged

Updated YuktaMedia Analytics Adapter: updated request body with more details #5547

merged 19 commits into from
Aug 11, 2020

Conversation

shrikantpatwari
Copy link
Contributor

@shrikantpatwari shrikantpatwari commented Jul 28, 2020 •
edited
Loading

Type of change

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)

Description of change

  • Fixed issue with information collection related to native ads

  • Updated code to better understand bid status.

  • Added utm info collection mechanism

  • Added local storage based session id

  • Added user Id collection mechanism

  • Used sendBeacon method of browser if supported

  • Added pollyfill for Object.entries and Object.values if not available

  • Fixed typo of spec file

  • contact email of the adapter’s maintainer: info@yuktamedia.com, shrikant.patwari@yuktamedia.com

  • official adapter submission

prebid/prebid.github.io#2165

@lgtm-com
Copy link

lgtm-com bot commented Jul 28, 2020

This pull request introduces 2 alerts when merging 917c21a into ec030f4 - view on LGTM.com

new alerts:

  • 2 for Useless assignment to local variable

@shrikantpatwari
Copy link
Contributor Author

@jsnellbaker / @pm-harshad-mane can you please review changes, approve and merge.

@Fawke Fawke self-requested a review August 5, 2020 12:54
Fawke
Fawke reviewed Aug 5, 2020
View reviewed changes
Copy link
Contributor

@Fawke Fawke left a comment

Choose a reason for hiding this comment

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

Hi @shrikantpatwari,

Can you run gulp test once. I see one of our test cases from adapterManager_spec.js failing. Here's the screenshot for your reference.

Screenshot 2020-08-05 at 6 24 09 PM

@shrikantpatwari
Copy link
Contributor Author

Hi @shrikantpatwari,

Can you run gulp test once. I see one of our test cases from adapterManager_spec.js failing. Here's the screenshot for your reference.

Screenshot 2020-08-05 at 6 24 09 PM

I have fixed broken tests. Can you please review once and merge. Thanks

@Fawke Fawke self-assigned this Aug 6, 2020
@Fawke
Copy link
Contributor

Fawke commented Aug 6, 2020

Hi @shrikantpatwari,

I see that your test coverage is below 80%. Can you include few more unit test cases?

Fawke
Fawke reviewed Aug 6, 2020
View reviewed changes
modules/yuktamediaAnalyticsAdapter.js Outdated
}

var yuktamediaAnalyticsAdapter = Object.assign(adapter(
{
emptyUrl,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why pass an empty url? If you are overriding the track function, I don't think you need to include this property as an argument to the adapter interface call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed emptyUrl. Thanks for improving.

@shrikantpatwari
Copy link
Contributor Author

shrikantpatwari commented Aug 8, 2020 •
edited
Loading

@Fawke I have updated code and test as per sessions, Please review and merge. Thanks.

@Fawke
Copy link
Contributor

Fawke commented Aug 10, 2020

@shrikantpatwari,

Thanks for making those changes. Just two point you may need to look into.

  • I noticed one thing which I wanted to point out. You are only calling the send function, for the BID_WON and AUCTION_END events and not calling it for other events like, BID_REQUESTED, BID_RESPONSE, NO_BID, BID_TIMEOUT and AUCTION_END.

If my understanding is correct, you are sending the event payload to your endpoint for only two events. Is it an oversight or by design?

  • Your test coverage is still below 80%.

Screenshot 2020-08-10 at 11 53 28 AM

You can run this command, gulp test -> gulp test-coverage -> gulp view-coverage to generate the code coverage reports. (Make sure you're on node v12 before you run those commands)

@shrikantpatwari
Copy link
Contributor Author

@shrikantpatwari,

Thanks for making those changes. Just two point you may need to look into.

  • I noticed one thing which I wanted to point out. You are only calling the send function, for the BID_WON and AUCTION_END events and not calling it for other events like, BID_REQUESTED, BID_RESPONSE, NO_BID, BID_TIMEOUT and AUCTION_END.

If my understanding is correct, you are sending the event payload to your endpoint for only two events. Is it an oversight or by design?

I am capturing data from all events in events variable which I finally send on Auction end event as well as when any bid is winning. So answer to question is this is by design.

  • Your test coverage is still below 80%.
Screenshot 2020-08-10 at 11 53 28 AM

You can run this command, gulp test -> gulp test-coverage -> gulp view-coverage to generate the code coverage reports. (Make sure you're on node v12 before you run those commands)

Thanks, I have not checked before test coverage, I have made it above 80% now. Some of things can not be tested as there is no mechanism to test it.

Fawke
Fawke approved these changes Aug 11, 2020
View reviewed changes
Copy link
Contributor

@Fawke Fawke left a comment

Choose a reason for hiding this comment

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

LGTM

@Fawke Fawke merged commit 7d113e0 into prebid:master Aug 11, 2020
@Fawke Fawke added LGTM and removed needs review labels Aug 11, 2020
@shrikantpatwari
Copy link
Contributor Author

LGTM

Thanks for merging

BrightMountainMediaInc pushed a commit to BrightMountainMediaInc/Prebid.js that referenced this pull request Sep 14, 2020
@shrikantpatwari
Updated YuktaMedia Analytics Adapter: updated request body with more …
1a48ad8 
…details (prebid#5547)

* Analytic Adaptor by YuktaMedia

* removed optional bid request params

* test case modified

* Updated YuktaMedia Analytics Adapter as per 3.0 prebid changes

* Added api key in circle ci config file

* reverted changes

* hard coded protocol for the request

* Modified request format and parameters

* removed polyfill for object.entries and removed Useless assignment to local variable

* Added bidId and auctionId in request as well

* fixed failing unit tests

* dealId captured

* Updated code as per recommendation and added tests

* addedadditional test case

* removed non used variables and functions

Co-authored-by: Shrikant Patwari <shrikant.patwari@yuktamedia.com>
BrightMountainMediaInc pushed a commit to BrightMountainMediaInc/Prebid.js that referenced this pull request Sep 14, 2020
Revert "Updated YuktaMedia Analytics Adapter: updated request body wi…
f7a46de 
…th more details (prebid#5547)"

This reverts commit 1a48ad8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fawke Fawke Fawke approved these changes

@pm-harshad-mane pm-harshad-mane Awaiting requested review from pm-harshad-mane

Assignees

@pm-harshad-mane pm-harshad-mane

@Fawke Fawke

Labels
LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shrikantpatwari @Fawke @pm-harshad-mane @jsnellbaker