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

FIX: Add exclude groups for each ad platforms #197

Merged
merged 8 commits into from
Feb 9, 2024
Merged

Conversation

oblakeerickson
Copy link
Member

@oblakeerickson oblakeerickson commented Feb 8, 2024

With the new group system for displaying ads we no longer can check if a
user belongs to a trust level group lower than specified. The other
problem is that ALL users including staff and higher trust levels all
belong to trust level 0. So without this fix if we say that an ad should
be visible to trust level 0 users then it will be shown to all users.

This fix adds a new default setting for each ad platform for excluding
trust level 3, 4, and staff users from being shown ads. It also makes the x_display_groups setting hidden as it will be removed in a later commit.

With the new group system for displaying ads we no longer can check if a
user belongs to a trust level group lower than specified. The other
problem is that ALL users including staff and higher trust levels all
belong to trust level 0. So without this fix if we say that an ad should
be visible to trust level 0 users then it will be shown to all users.

This fix adds a new default setting for each ad platform for excluding
trust level 3, 4, and staff users from being shown ads.
@oblakeerickson
Copy link
Member Author

oblakeerickson commented Feb 8, 2024

I'm working on adding a current_user_serializer_spec.

EDIT: Done.

- Make display_groups hidden (they will be removed in a later commit)
- Switch to using only exclude_groups instead of display groups and
  exclude groups
- rename showToDisplayGroups to showXAds for each provider
Copy link
Contributor

@martin-brennan martin-brennan left a comment

Choose a reason for hiding this comment

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

I read through the internal topic and I agree it makes more sense to make this an exclude setting instead of an include one. Thanks for fixing, go ahead! 👍

@oblakeerickson
Copy link
Member Author

Thanks @martin-brennan :)

@oblakeerickson oblakeerickson merged commit 9581367 into main Feb 9, 2024
5 checks passed
@oblakeerickson oblakeerickson deleted the exclude-groups branch February 9, 2024 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants