Show/Hide "See paid advertising" checkbox depending on USE_PROMOS #3412
Conversation
readthedocs/settings/base.py
Outdated
| @@ -313,6 +313,7 @@ def INSTALLED_APPS(self): # noqa | |||
|
|
|||
| # Misc application settings | |||
| GLOBAL_ANALYTICS_CODE = 'UA-17997319-1' | |||
| USE_PROMOS = False | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
readthedocs/core/forms.py
Outdated
| fields = ['first_name', 'last_name', 'homepage', 'allow_ads'] | ||
| if not settings.USE_PROMOS: |
There was a problem hiding this comment.
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
force-pushed
the
humitos/profile/allow-promos
branch
from
ecf5b79 to
c9ce5ff
Compare
|
This PR should be ready to go :) |
No description provided.