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 1325: Slice Cache Timeout Setting Ignored in 0.11.0 #1372

Closed
wants to merge 3 commits into from
Closed

Fix 1325: Slice Cache Timeout Setting Ignored in 0.11.0 #1372

wants to merge 3 commits into from

Conversation

nickbarnwell
Copy link

Fixes #1325

RE: 308f6da, I think that save test passed because request.args contained "Slice Name", but there was no persistence of the Slice to the database; I'm not sure whether this was intended or not. As far as I could tell there was no code in views.py to handle the action=save query param.

This also caused the Cache Warm Up test to fail, since it's tied to DB state affected by the Slice Save test.

@@ -1171,6 +1171,9 @@ def get_viz(
args=None,
datasource_type=None,
datasource_id=None):

slice_id = slice_id or request.args.get('slice_id')
Copy link
Member

Choose a reason for hiding this comment

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

we should keep request out of models.py and only in views.py, this logic belongs in methods that call this method.

Copy link
Author

Choose a reason for hiding this comment

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

It's confusing but there's both a get_viz method in views.py and on everything that inherits from BaseViz in viz.py. This method is in the former, otherwise the request object wouldn't even be in scope. I did originally have this just using the args object that's passed in, but it seemed to break a number of the callers in odd ways I wasn't super keen to debug at the time. Will dig out that branch tonight…

@nickbarnwell
Copy link
Author

Waiting for Travis to confirm but it looks like my testing without a direct line into the request object only failed tests due to persistent state in the test suite if you don't clear the DB…this should work just fine (waiting for Travis to confirm)

@mistercrunch
Copy link
Member

Should be fixed in master

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