Issue862 - implemented multi media bids #993
Conversation
… This requires using aliases. Updated the documentation and similar to descrbe that.
…equests. Removed range calls from for loops. I asume that's faster.
…in keeping the indexes straight in MakeRequests. I've added back a couple of test cases, which now pass.
|
@mansinahar LGTM, again. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@hhhjort @mansinahar Thank you for all of your help on this. I hope that your very helpful advice was not the result of too much frustration on your part. I'm wondering where we're at on this, considering:
Should I put in issues for those? Is there anything else with this PR? Should it be merged, or should it be closed in favor of a PR that covers the points above? |
|
@muncha This just got delayed due to the long holiday weekend. I will find some time to get a review on this tomorrow. |
|
@mansinahar no worries - I've got one of these problematic test cases worked out (I wasn't including the status in the mockResponse - doh) and close on the other, I hope. So I'll push when I have both of those, and I'll save my RTB video implementation for another PR. |
|
@muncha Awesome! Just lemme know when it is ready to review again and I will be happy to take a look. Thanks :) |
…em was that an empty adapters.RequestData was being added. Also added test case unmarshal-error-but-another-good-video.json, which throws and tracks the unmarshal error and dumps that bad one, then adds and requests the good video imp.
|
@mansinahar LGTM |
|
Is this ready for review now? |
|
Yes, please. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@hhhjort - I'm not sure if this should be merged to master of if it should wait for my next PR, which includes all of this. It's no rush as far as I'm concerned. Unless there is a reason to merge as far as repo history sanity or similar, I'd think wait. My next PR enables options for ADM vs. nurl responses on video. I should have that pushed in the next day or two. Assuming waiting, should I go ahead and close this? |
|
i am good with waiting |
|
@muncha I am good with waiting too. Let's close this once you have the other PR up |
|
I'm closing this and submitting a new pull request which includes these changes and adds the ability to return AdM or nurl responses for video. |
This is on top of the changes from #993, which has not been merged, and this pretty much wraps up the major changes that we are doing for now. In addition to giving the titular choice, this makes a simple guess at device type and security.
This is on top of the changes from prebid#993, which has not been merged, and this pretty much wraps up the major changes that we are doing for now. In addition to giving the titular choice, this makes a simple guess at device type and security.
This is on top of the changes from prebid#993, which has not been merged, and this pretty much wraps up the major changes that we are doing for now. In addition to giving the titular choice, this makes a simple guess at device type and security.
This is on top of the changes from prebid#993, which has not been merged, and this pretty much wraps up the major changes that we are doing for now. In addition to giving the titular choice, this makes a simple guess at device type and security.
This is on top of the changes from prebid#993, which has not been merged, and this pretty much wraps up the major changes that we are doing for now. In addition to giving the titular choice, this makes a simple guess at device type and security.
This is a correction to closed PR #965. The "forceBanner" flag is removed, and as suggested by @hhhjort, multiple bid requests are being added to the stack for multimedia.