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.

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

docereeAdManager Bid Adapter : Updated bid adapter #11996

Merged
merged 11 commits into from
Jul 19, 2024

Conversation

Doceree-techStack
Copy link
Contributor

Type of change

  • Bugfix

  • Feature

  • New bidder adapter

  • Updated bidder adapter

  • Code style update (formatting, local variables)

  • Refactoring (no functional changes, no api changes)

  • Build related changes

  • CI related changes

  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

  • Other

Description of change

Updated field for user inputs

Other information

Copy link
Collaborator

@patmmccann patmmccann left a comment

Choose a reason for hiding this comment

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

Same questions as before #11966

@@ -25,6 +25,7 @@ describe('docereeadmanager', function () {
userid: '',
zipcode: '',
userconsent: '',
platformUid: ''
},
Copy link
Collaborator

@patmmccann patmmccann Jul 18, 2024

Choose a reason for hiding this comment

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

As discussed in the last submission, this needs some improvement. Also could you discuss why you're passing full names in the clear? This is a quite unusual practice

Copy link
Contributor Author

@Doceree-techStack Doceree-techStack Jul 18, 2024

Choose a reason for hiding this comment

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

This adapter has already been deployed and is actively used by several clients in production. The proposed alteration to the current signature could disrupt our workflows. The submitted modification involves a minor adjustment to the field 'userid' and the introduction of a new field ('mobile'). Please merge the changes with master to expedite availability of these changes in the adapter

Copy link
Collaborator

Choose a reason for hiding this comment

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

no changes will be merged without appropriate test coverage

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi; I talked with other prebid leadership, we decided to merge your pr with a few tweaks

Copy link
Collaborator

@patmmccann patmmccann left a comment

Choose a reason for hiding this comment

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

please cover in tests

@@ -25,6 +25,7 @@ describe('docereeadmanager', function () {
userid: '',
zipcode: '',
userconsent: '',
platformUid: ''
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

no changes will be merged without appropriate test coverage

Copy link

Tread carefully! This PR adds 5 linter errors (possibly disabled through directives):

  • modules/docereeAdManagerBidAdapter.js (+5 errors)

@patmmccann patmmccann merged commit d38a06d into prebid:master Jul 19, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants