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

Show/Hide "See paid advertising" checkbox depending on USE_PROMOS #3412

Merged
merged 6 commits into from Dec 21, 2017

Conversation

@humitos
Copy link
Member

@humitos humitos commented Dec 18, 2017

No description provided.

@@ -313,6 +313,7 @@ def INSTALLED_APPS(self): # noqa

# Misc application settings
GLOBAL_ANALYTICS_CODE = 'UA-17997319-1'
USE_PROMOS = False
Copy link
Member

@ericholscher ericholscher Dec 18, 2017

Thoughts on doing this dynamically by checking for the donate app? That's how it's done other places (settings, urls) for the ad stuff.

Copy link
Member Author

@humitos humitos Dec 19, 2017

I don't have a position on this since I don't konw what the donate app really involves and if it would fit correctly here. Besdies, wasn't the variable/app renamed to readthedocs-ext?

If it fits correctly, I'm 👍

On the other hand, this way with just a setting it's very easy to understand, override and follow. Maybe base this into an installed app adds a confusing layer when it's not necessary and we don't have many benefits.

Copy link
Contributor

@agjohnson agjohnson Dec 20, 2017

If the promo code doesn't work without readthedocs-ext installed, then doing dynamically based on the package being imported seems like a good choice.

fields = ['first_name', 'last_name', 'homepage', 'allow_ads']
if not settings.USE_PROMOS:
Copy link
Member

@safwanrahman safwanrahman Dec 19, 2017

I think its better to append allow_ads instead of deleting from index.
It can be something like

fields = ['first_name', 'last_name', 'homepage']
if settings.USE_PROMOS:
    fields.append('allow_ads')

@humitos humitos force-pushed the humitos/profile/allow-promos branch from ecf5b79 to c9ce5ff Dec 20, 2017
@humitos
Copy link
Member Author

@humitos humitos commented Dec 21, 2017

This PR should be ready to go :)

@ericholscher ericholscher merged commit 561d15d into master Dec 21, 2017
1 check passed
@stsewd stsewd deleted the humitos/profile/allow-promos branch Aug 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants