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

migrate dashboard stored default filters to time range format #5556

Closed
wants to merge 2 commits into from
Closed

migrate dashboard stored default filters to time range format #5556

wants to merge 2 commits into from

Conversation

octaviancorlade
Copy link
Contributor

Since #4981 __from and __to default filters from dashboards were treated as normal WHERE filters instead of being used to fill the time range options.

An alternative to this would be some legacy function to handle the previous format.

Fixes #5553

@mistercrunch
Copy link
Member

mistercrunch commented Aug 3, 2018

@john-bodley worked on backend legacy filter handling recently here #5525 , so he's probably the best person to review this.

@codecov-io
Copy link

codecov-io commented Aug 3, 2018

Codecov Report

Merging #5556 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5556   +/-   ##
=======================================
  Coverage   63.12%   63.12%           
=======================================
  Files         349      349           
  Lines       22167    22167           
  Branches     2462     2462           
=======================================
  Hits        13992    13992           
  Misses       8161     8161           
  Partials       14       14

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0aff865...b7ea0d5. Read the comment docs.


metadata['default_filters'] = json.dumps(default_filters, indent=2)
dashboard.json_metadata = json.dumps(metadata, indent=2)
session.merge(dashboard)
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn’t need to explicitly merge the updated dashboard object as SQLAlechemy is aware of these mutation.

metadata['default_filters'] = json.dumps(default_filters, indent=2)
dashboard.json_metadata = json.dumps(metadata, indent=2)
session.merge(dashboard)
session.commit()
Copy link
Member

Choose a reason for hiding this comment

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

The commit should be outside of the for lool.

@octaviancorlade
Copy link
Contributor Author

octaviancorlade commented Aug 8, 2018

Thanks @john-bodley, updated

@kristw kristw added need:update risk:db-migration PRs that require a DB migration labels Mar 19, 2019
@kristw kristw changed the title Time filter migrate dashboard stored default filters to time range format migrate dashboard stored default filters to time range format Mar 19, 2019
@stale
Copy link

stale bot commented May 18, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label May 18, 2019
@stale stale bot closed this May 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Inactive for >= 30 days risk:db-migration PRs that require a DB migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Time filter from older versions break exploring slices from dashboard
5 participants