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

Nobid: Add iframe sync support #3732

Merged
merged 2 commits into from
Jun 27, 2024
Merged

Conversation

redaguermas
Copy link
Contributor

Added support for iframe user sync.

@bsardo bsardo changed the title Added support for iframe sync. Nobid: Add iframe sync support Jun 5, 2024
@redaguermas
Copy link
Contributor Author

Can you please take a look at this PR? The validation failed for a module that is not ours.

@redaguermas
Copy link
Contributor Author

Hello @bsardo ,
What should I do to get this PR going?

@redaguermas
Copy link
Contributor Author

Can you please review this PR?

@redaguermas
Copy link
Contributor Author

@bsardo Can you please review this PR?

@redaguermas
Copy link
Contributor Author

Can someone please review this PR?

@redaguermas
Copy link
Contributor Author

Can some one please review this PR?

@bsardo bsardo self-assigned this Jun 24, 2024
@bsardo
Copy link
Collaborator

bsardo commented Jun 24, 2024

Hi @redaguermas, sorry for the slow response. I and a lot of the team have been out of the office recently on vacation. We'll make this a priority this week.

@SyntaxNode
Copy link
Contributor

SyntaxNode commented Jun 25, 2024

Confirmed the new iframe user sync endpoint works. However, please be advised the user id in my test is 729 characters long. We expect the id to be a much smaller (uuid length) opaque identifier. Based on the length, it looks like this uid may be encoding data.

You run the risk of hosts disabling your cookie sync since it uses a lot of valuable cookie storage space. Consider changing to a smaller id. Approving, since the url is working.

@redaguermas
Copy link
Contributor Author

Thank you @SyntaxNode . We are going to look into creating a shorter ID but it will take some time because of technical changes that need to take place in the backend. Can you please merge this PR and we will address the ID length in another ticket/PR.
-Regards

@SyntaxNode
Copy link
Contributor

We are going to look into creating a shorter ID but it will take some time because of technical changes that need to take place in the backend. Can you please merge this PR and we will address the ID length in another ticket/PR.

Much appreciated. Thank you.

I mentioned this wasn't a blocker and already approved the PR. It's waiting for a second reviewer to approve and then you're good.

@bsardo bsardo merged commit 07f3ee2 into prebid:master Jun 27, 2024
3 checks passed
mefjush pushed a commit to adhese/prebid-server that referenced this pull request Jul 19, 2024
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.

3 participants