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

move remaining "theme" settings from settings.py dictionary to SiteMode model #190

Merged
merged 11 commits into from
Sep 13, 2016

Conversation

tobiasmcnulty
Copy link
Member

Lilia would like the ability to edit various strings on the site without re-deploying and without engaging a developer. This PR moves the remaining settings from SITE_THEMES into the SiteMode model.

@tobiasmcnulty
Copy link
Member Author

@ejucovy Could you give this a review when you have a moment?


@property
def site_mode(self):
# avoid lots of unneeded round trips to memcached in production
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way to stash this on the class instead of the instance? Or even on the get_site_mode function object? If I'm following correctly, even after this we'll still be making N queries (instead of ~5N) when we're rendering a template with N questions on it.

Copy link
Member Author

@tobiasmcnulty tobiasmcnulty Sep 12, 2016

Choose a reason for hiding this comment

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

Good catch! I would like to avoid caching anything on a Python object that may persist in the interpreter outside the request/response cycle (as doing so may lead to some difficult to troubleshoot invalidation issues), however, we already have the site mode in the template context so it's just a matter of figuring out how to get it into the Submission model methods (or how to refactor them/put them elsewhere so that's not required).

For the time being, I added a template tag which pre-fills _cached_site_mode with the SITE_MODE available in the template context, along with a test to make sure the link doesn't get broken.

Longer term, this probably needs some refactoring, but I think this will work for now.

@tobiasmcnulty
Copy link
Member Author

@ejucovy Do you want to take another look at this? I think I've addressed all your feedback. I may go ahead and merge in an hour or two if I haven't heard back so Lilia can test / get production content ready. We can take up any issues you find as new work if that's okay (please do make sure I implemented your suggested changes appropriately!).

@ejucovy
Copy link
Contributor

ejucovy commented Sep 12, 2016

All looks good to me!

@tobiasmcnulty tobiasmcnulty merged commit 28999e6 into develop Sep 13, 2016
@tobiasmcnulty tobiasmcnulty deleted the move_site_theme_settings_to_site_mode branch September 13, 2016 00:03
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