Skip to content

Publisher allowed domains #736

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

Merged
merged 1 commit into from
Apr 28, 2023
Merged

Conversation

davidfischer
Copy link
Collaborator

This is the first step toward restricting which domains a publisher can show ads on. Right now, this just logs a message at warning if this setting is turned on (off by default).

However, we have more and more publishers with 2 sites and I'd like to start being a bit more systematic there.

Ref: #194.

This is the first step toward restricting which domains
a publisher can show ads on. Right now, this just logs a message
at warning if this setting is turned on (off by default).

However, we have more and more publishers with 2 sites
and I'd like to start being a bit more systematic there.
@davidfischer davidfischer requested a review from a team as a code owner April 18, 2023 18:17
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This seems like a reasonable first step, but will likely need to be a bit smarter. There's gonna be a bunch of edge cases where folks have a bunch of subdomains or similar they serve ads on, but good to start being smarter here 💯

_("Allowed domains"),
max_length=1024,
help_text=_(
"A space separated list of domains where the publisher's ads can appear"
Copy link
Member

Choose a reason for hiding this comment

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

Do we allow wildcards or similar here? Or is it a if in check or similar? Could be useful to be more specific so we know what to include, or is it an exact match?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we may need those at some point. Before we implement it though, I'd like to know how common that is.

@davidfischer davidfischer merged commit b512e87 into main Apr 28, 2023
@davidfischer davidfischer deleted the davidfischer/publisher-allowed-domains branch April 28, 2023 23:38
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.

2 participants