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

TL-23327: Additional filterEids logic #39

Merged
merged 28 commits into from
Oct 12, 2021
Merged

Conversation

nllerandi3lift
Copy link

@nllerandi3lift nllerandi3lift commented Oct 7, 2021

Type of change

  • Other

Description of change

Resolves https://triplelift.atlassian.net/browse/TL-23327

Publishers are customizing how the pubcommonid module fundamentally works, causing our endpoint to error out/bids not to monetize. There are 2 primary consistent non-standard use cases that this PR will either reject or allow through.

id should be a string.

Rejects id:{}
image

Use id.id as last resort if it's a string
image

nllerandi3lift and others added 22 commits February 18, 2021 19:22
Bumps [elliptic](https://github.com/indutny/elliptic) from 6.5.3 to 6.5.4.
- [Release notes](https://github.com/indutny/elliptic/releases)
- [Commits](indutny/elliptic@v6.5.3...v6.5.4)

Signed-off-by: dependabot[bot] <support@github.com>
…rn/elliptic-6.5.4

Bump elliptic from 6.5.3 to 6.5.4
…npm_and_yarn/elliptic-6.5.4

Revert "Bump elliptic from 6.5.3 to 6.5.4"
TL-22981: Increase Prebid JS Instream TTL
@nllerandi3lift nllerandi3lift marked this pull request as draft October 7, 2021 19:25
@nllerandi3lift nllerandi3lift marked this pull request as ready for review October 8, 2021 15:10
.map(formatEid(source, rtiPartner)); // userIds -> eid objects
}

const filterEids = type => (userId, i, arr) => {
Copy link
Author

Choose a reason for hiding this comment

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

Not sure how to do this another way but ES6 is ok to use.

Copy link

Choose a reason for hiding this comment

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

I think this is syntactically correct (albeit, looks a bit weird because of the implicit return) but it doesn't follow the convention of other function declarations in this file like

function getUserId(type) {
  return (bid) => {

.map(formatEid(source, rtiPartner)); // userIds -> eid objects
}

const filterEids = type => (userId, i, arr) => {
try {
let isValidUserId =
Copy link
Author

@nllerandi3lift nllerandi3lift Oct 8, 2021

Choose a reason for hiding this comment

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

The following is almost painfully explicit in what the adapter will permit as a valid userId. Was thinking of writing a 'deep check for "id"' recursive function or similar but I don't think it's necessary. Maybe if in the future we see pubs sending things like {lev1: {lev2: {lev3: {id: "abc"}}}} then we can change but I'm not seeing any examples of that.

Copy link

Choose a reason for hiding this comment

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

Looks fine to me. Seems like a case where being more strict is better than not.

expect(payload.user.ext.eids).to.be.an('array');
expect(payload.user.ext.eids).to.have.lengthOf(2);

tdidId = {}; // fail; can't be empty object
Copy link

Choose a reason for hiding this comment

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

Is it possible to test all the conditions of isValidUserId in a single AAA setup?

@nllerandi3lift nllerandi3lift merged commit 0eb280f into master Oct 12, 2021
@nllerandi3lift nllerandi3lift deleted the TL-23327-filterEids branch October 12, 2021 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants