From 228e5e6fe5b6dedf6a8a5067d1aa9594e16ec82a Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 8 Jan 2019 16:39:54 -0800 Subject: [PATCH 1/2] feat(discover): Use URL parameters for each visualization Instead of using component state, use url parameters as data source for which chart gets displayed. This will be helpful when navigating from other parts of the application to a discover query. --- .../views/organizationDiscover/discover.jsx | 22 ++++++-- .../organizationDiscover/result/index.jsx | 51 +++++++++++-------- .../result/visualizationsToggle.jsx | 27 +++++++--- .../organizationDiscover/discover.spec.jsx | 40 +++++++++++++++ .../result/index.spec.jsx | 38 +++++++++++--- 5 files changed, 140 insertions(+), 38 deletions(-) diff --git a/src/sentry/static/sentry/app/views/organizationDiscover/discover.jsx b/src/sentry/static/sentry/app/views/organizationDiscover/discover.jsx index 2600c793fcc00a..6d87fdefd7ec03 100644 --- a/src/sentry/static/sentry/app/views/organizationDiscover/discover.jsx +++ b/src/sentry/static/sentry/app/views/organizationDiscover/discover.jsx @@ -67,7 +67,7 @@ export default class OrganizationDiscover extends React.Component { componentDidMount() { if (this.props.savedQuery) { - this.runQuery(); + this.runQuery({keepVisual: true}); } } @@ -92,7 +92,14 @@ export default class OrganizationDiscover extends React.Component { return; } - if (currentSearch === search) { + // Don't compare `visual` since there is no need to re-query if queries are equal + // but views are changing + const visualRegex = /&?visual=[^&]+/; + if ( + currentSearch && + search && + currentSearch.replace(visualRegex, '') === search.replace(visualRegex, '') + ) { return; } @@ -177,8 +184,11 @@ export default class OrganizationDiscover extends React.Component { this.runQuery(); }; - runQuery = () => { - const {queryBuilder, organization} = this.props; + // runQuery can get called from many different paths - only keep visual in URL params + // when the option is passed. We want it to reset in most cases (but not when running query + // when initially mounted) + runQuery = ({keepVisual} = {}) => { + const {queryBuilder, organization, location} = this.props; const {resultManager} = this.state; // Track query for analytics @@ -210,9 +220,11 @@ export default class OrganizationDiscover extends React.Component { const shouldRedirect = !this.props.params.savedQueryId; if (shouldRedirect) { + const visual = + keepVisual && location.query.visual ? `&visual=${location.query.visual}` : ''; browserHistory.push({ pathname: `/organizations/${organization.slug}/discover/`, - search: getQueryStringFromQuery(queryBuilder.getInternal()), + search: `${getQueryStringFromQuery(queryBuilder.getInternal())}${visual}`, }); } diff --git a/src/sentry/static/sentry/app/views/organizationDiscover/result/index.jsx b/src/sentry/static/sentry/app/views/organizationDiscover/result/index.jsx index 77aa4d2fb6405b..54733557922a0d 100644 --- a/src/sentry/static/sentry/app/views/organizationDiscover/result/index.jsx +++ b/src/sentry/static/sentry/app/views/organizationDiscover/result/index.jsx @@ -1,19 +1,16 @@ -import React from 'react'; -import PropTypes from 'prop-types'; import {throttle} from 'lodash'; +import {withRouter} from 'react-router'; +import PropTypes from 'prop-types'; +import React from 'react'; -import SentryTypes from 'app/sentryTypes'; import {t} from 'app/locale'; -import getDynamicText from 'app/utils/getDynamicText'; import BarChart from 'app/components/charts/barChart'; -import LineChart from 'app/components/charts/lineChart'; import InlineSvg from 'app/components/inlineSvg'; +import LineChart from 'app/components/charts/lineChart'; import PageHeading from 'app/components/pageHeading'; +import SentryTypes from 'app/sentryTypes'; +import getDynamicText from 'app/utils/getDynamicText'; -import {getChartData, getChartDataByDay, getRowsPageRange, downloadAsCsv} from './utils'; -import Table from './table'; -import Pagination from './pagination'; -import VisualizationsToggle from './visualizationsToggle'; import { HeadingContainer, ResultSummary, @@ -25,19 +22,26 @@ import { ResultSummaryAndButtons, } from '../styles'; import {NUMBER_OF_SERIES_BY_DAY} from '../data'; +import {getChartData, getChartDataByDay, getRowsPageRange, downloadAsCsv} from './utils'; +import Pagination from './pagination'; +import Table from './table'; +import VisualizationsToggle from './visualizationsToggle'; + +const DEFAULT_VIEW = 'table'; -export default class Result extends React.Component { +class Result extends React.Component { static propTypes = { + location: PropTypes.object, data: PropTypes.object.isRequired, savedQuery: SentryTypes.DiscoverSavedQuery, // Provided if it's a saved search onFetchPage: PropTypes.func.isRequired, onToggleEdit: PropTypes.func, }; - constructor() { - super(); + constructor(props) { + super(props); this.state = { - view: 'table', + view: props.location.query.visual || DEFAULT_VIEW, height: null, width: null, }; @@ -50,6 +54,17 @@ export default class Result extends React.Component { componentWillReceiveProps(nextProps) { const {baseQuery, byDayQuery} = nextProps.data; + if (nextProps.location.query.visual) { + this.setState({ + view: nextProps.location.query.visual, + }); + } else if (this.props.location.query.visual) { + // Going from selected -> none (e.g. clicking back), should show default + this.setState({ + view: DEFAULT_VIEW, + }); + } + if (!byDayQuery.data && ['line-by-day', 'bar-by-day'].includes(this.state.view)) { this.setState({ view: 'table', @@ -88,12 +103,6 @@ export default class Result extends React.Component { throttledUpdateDimensions = throttle(this.updateDimensions, 200, {trailing: true}); - handleToggleVisualizations = opt => { - this.setState({ - view: opt, - }); - }; - renderToggle() { const {baseQuery, byDayQuery} = this.props.data; @@ -116,7 +125,6 @@ export default class Result extends React.Component {
@@ -269,3 +277,6 @@ export default class Result extends React.Component { ); } } + +export default withRouter(Result); +export {Result}; diff --git a/src/sentry/static/sentry/app/views/organizationDiscover/result/visualizationsToggle.jsx b/src/sentry/static/sentry/app/views/organizationDiscover/result/visualizationsToggle.jsx index b6635f20d61477..777c53a41f0a9c 100644 --- a/src/sentry/static/sentry/app/views/organizationDiscover/result/visualizationsToggle.jsx +++ b/src/sentry/static/sentry/app/views/organizationDiscover/result/visualizationsToggle.jsx @@ -1,9 +1,11 @@ +import {Link, withRouter} from 'react-router'; import PropTypes from 'prop-types'; import React from 'react'; import classNames from 'classnames'; + +import {t} from 'app/locale'; import DropdownLink from 'app/components/dropdownLink'; import MenuItem from 'app/components/menuItem'; -import {t} from 'app/locale'; import { ResultViewActions, @@ -20,18 +22,31 @@ class VisualizationsToggle extends React.Component { name: PropTypes.string, }) ).isRequired, - handleChange: PropTypes.func.isRequired, handleCsvDownload: PropTypes.func.isRequired, visualization: PropTypes.string.isRequired, }; + getUrl = id => { + const {location} = this.props; + + return { + ...location, + query: { + ...location.query, + visual: id, + }, + }; + }; + getMenuItem = opt => { + const {location, visualization} = this.props; return ( {opt.name} @@ -42,7 +57,7 @@ class VisualizationsToggle extends React.Component { const active = opt.id === this.props.visualization; return (
  • - this.props.handleChange(opt.id)}>{opt.name} + {opt.name}
    • ); }; @@ -81,4 +96,4 @@ class VisualizationsToggle extends React.Component { } } -export default VisualizationsToggle; +export default withRouter(VisualizationsToggle); diff --git a/tests/js/spec/views/organizationDiscover/discover.spec.jsx b/tests/js/spec/views/organizationDiscover/discover.spec.jsx index 451adbf4be3752..0a5b4e6eb3202f 100644 --- a/tests/js/spec/views/organizationDiscover/discover.spec.jsx +++ b/tests/js/spec/views/organizationDiscover/discover.spec.jsx @@ -123,6 +123,46 @@ describe('Discover', function() { // TODO: check that query is run with correct params }); + + it('does not runquery if queries are equal and visual changes in url params', async function() { + queryBuilder.fetch = jest.fn(() => + Promise.resolve({ + timing: {}, + data: [{foo: 'bar', 'project.id': project.id}], + meta: [{name: 'foo'}], + }) + ); + const wrapper = mount( + , + TestStubs.routerContext([{organization}]) + ); + wrapper.setProps({ + location: { + search: + 'projects=%5B%5D&fields=%5B%22id%22%2C%22issue.id%22%2C%22project.name%22%2C%22platform%22%2C%22timestamp%22%5D&conditions=%5B%5D&aggregations=%5B%5D&range=%2214d%22&orderby=%22-timestamp%22&limit=1000&start=null&end=null', + }, + }); + await tick(); + wrapper.update(); + queryBuilder.fetch.mockClear(); + wrapper.setProps({ + location: { + search: + 'projects=%5B%5D&fields=%5B%22id%22%2C%22issue.id%22%2C%22project.name%22%2C%22platform%22%2C%22timestamp%22%5D&conditions=%5B%5D&aggregations=%5B%5D&range=%2214d%22&orderby=%22-timestamp%22&limit=1000&start=null&end=null&visual=bar', + }, + }); + await tick(); + wrapper.update(); + expect(queryBuilder.fetch).not.toHaveBeenCalled(); + }); }); describe('Pagination', function() { diff --git a/tests/js/spec/views/organizationDiscover/result/index.spec.jsx b/tests/js/spec/views/organizationDiscover/result/index.spec.jsx index 862951768681e2..64d2b6b18e2992 100644 --- a/tests/js/spec/views/organizationDiscover/result/index.spec.jsx +++ b/tests/js/spec/views/organizationDiscover/result/index.spec.jsx @@ -1,7 +1,9 @@ -import React from 'react'; import {mount, shallow} from 'enzyme'; +import React from 'react'; -import Result from 'app/views/organizationDiscover/result'; +import {mockRouterPush} from 'app-test/helpers/mockRouterPush'; +import {initializeOrg} from 'app-test/helpers/initializeOrg'; +import Result, {Result as ResultShallow} from 'app/views/organizationDiscover/result'; import createQueryBuilder from 'app/views/organizationDiscover/queryBuilder'; describe('Result', function() { @@ -25,7 +27,12 @@ describe('Result', function() { }, }; wrapper = shallow( - , + , { context: {organization}, disableLifecycleMethods: false, @@ -140,11 +147,23 @@ describe('Result', function() { }); describe('Toggles Visualizations', function() { + organization = TestStubs.Organization(); + const {routerContext, router} = initializeOrg({ + organization, + router: { + location: { + pathname: `/organizations/${organization.slug}/discover/`, + query: {}, + }, + }, + }); + beforeEach(function() { wrapper = mount( , - TestStubs.routerContext([{organization}]) + routerContext ); + mockRouterPush(wrapper, router); }); it('displays options', function() { @@ -158,9 +177,14 @@ describe('Result', function() { wrapper .find('ResultViewButtons') - .find('a') + .find('Link a') .at(1) - .simulate('click'); + .simulate('click', {button: 0}); + + expect(router.push).toHaveBeenCalledWith({ + pathname: '/organizations/org-slug/discover/', + query: {visual: 'line'}, + }); wrapper.update(); expect(wrapper.find('ResultTable')).toHaveLength(0); @@ -175,7 +199,7 @@ describe('Result', function() { .find('ul.dropdown-menu') .find('a') .at(1) - .simulate('click'); + .simulate('click', {button: 0}); expect(wrapper.find('ResultTable')).toHaveLength(0); expect(wrapper.find('LineChart')).toHaveLength(1); From 6a44b318565059bd590bf483bf6fb86f518dd51b Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Thu, 10 Jan 2019 10:46:44 -0800 Subject: [PATCH 2/2] refactor to compare using location.query instead of location.search --- .../views/organizationDiscover/discover.jsx | 21 ++++---- .../app/views/organizationDiscover/utils.jsx | 52 ++++++++++++------- .../organizationDiscover/discover.spec.jsx | 21 +++++--- .../views/organizationDiscover/utils.spec.jsx | 15 ++++-- 4 files changed, 69 insertions(+), 40 deletions(-) diff --git a/src/sentry/static/sentry/app/views/organizationDiscover/discover.jsx b/src/sentry/static/sentry/app/views/organizationDiscover/discover.jsx index 6d87fdefd7ec03..d739be9881dce5 100644 --- a/src/sentry/static/sentry/app/views/organizationDiscover/discover.jsx +++ b/src/sentry/static/sentry/app/views/organizationDiscover/discover.jsx @@ -22,7 +22,8 @@ import { SavedQueryWrapper, } from './styles'; import { - getQueryStringFromQuery, + areQueriesEqual, + getQueryObjectFromQuery, getQueryFromQueryString, deleteSavedQuery, updateSavedQuery, @@ -74,12 +75,12 @@ export default class OrganizationDiscover extends React.Component { componentWillReceiveProps(nextProps) { const { queryBuilder, - location: {search}, + location: {search, query}, savedQuery, isEditingSavedQuery, params, } = nextProps; - const currentSearch = this.props.location.search; + const currentQuery = this.props.location.query; const {resultManager} = this.state; if (savedQuery && savedQuery !== this.props.savedQuery) { @@ -94,12 +95,7 @@ export default class OrganizationDiscover extends React.Component { // Don't compare `visual` since there is no need to re-query if queries are equal // but views are changing - const visualRegex = /&?visual=[^&]+/; - if ( - currentSearch && - search && - currentSearch.replace(visualRegex, '') === search.replace(visualRegex, '') - ) { + if (areQueriesEqual(currentQuery, query)) { return; } @@ -220,11 +216,12 @@ export default class OrganizationDiscover extends React.Component { const shouldRedirect = !this.props.params.savedQueryId; if (shouldRedirect) { - const visual = - keepVisual && location.query.visual ? `&visual=${location.query.visual}` : ''; browserHistory.push({ pathname: `/organizations/${organization.slug}/discover/`, - search: `${getQueryStringFromQuery(queryBuilder.getInternal())}${visual}`, + query: { + ...getQueryObjectFromQuery(queryBuilder.getInternal()), + visual: keepVisual && location.query.visual, + }, }); } diff --git a/src/sentry/static/sentry/app/views/organizationDiscover/utils.jsx b/src/sentry/static/sentry/app/views/organizationDiscover/utils.jsx index a9099e2ec3be44..4a26f43140ab76 100644 --- a/src/sentry/static/sentry/app/views/organizationDiscover/utils.jsx +++ b/src/sentry/static/sentry/app/views/organizationDiscover/utils.jsx @@ -1,21 +1,25 @@ +import {isEqual, pick} from 'lodash'; import moment from 'moment'; + import {Client} from 'app/api'; -import {isValidAggregation} from './aggregations/utils'; +import {defined} from 'app/utils'; + import {NON_SNUBA_FIELDS} from './data'; +import {isValidAggregation} from './aggregations/utils'; -export function getQueryFromQueryString(queryString) { - const validQueryKeys = new Set([ - 'projects', - 'fields', - 'conditions', - 'aggregations', - 'range', - 'start', - 'end', - 'orderby', - 'limit', - ]); +const validQueryKeys = new Set([ + 'projects', + 'fields', + 'conditions', + 'aggregations', + 'range', + 'start', + 'end', + 'orderby', + 'limit', +]); +export function getQueryFromQueryString(queryString) { const result = {}; let parsedQuery = queryString; parsedQuery = parsedQuery.replace(/^\?|\/$/g, '').split('&'); @@ -31,12 +35,19 @@ export function getQueryFromQueryString(queryString) { return result; } -export function getQueryStringFromQuery(query) { - const queryProperties = Object.entries(query).map(([key, value]) => { - return key + '=' + encodeURIComponent(JSON.stringify(value)); - }); +export function getQueryObjectFromQuery(query) { + // Filter out undefined/null values + const queryProperties = Object.entries(query) + .filter(([key, value]) => defined(value)) + .map(([key, value]) => { + return [key, JSON.stringify(value)]; + }) + .reduce((acc, [key, val]) => { + acc[key] = val; + return acc; + }, {}); - return `?${queryProperties.join('&')}`; + return queryProperties; } export function getOrderbyFields(queryBuilder) { @@ -99,6 +110,11 @@ export function getView(params, requestedView) { } } +export function areQueriesEqual(query, other) { + const validKeys = Array.from(validQueryKeys); + return isEqual(pick(query, validKeys), pick(other, validKeys)); +} + /** * Takes a saved query and strips associated query metadata in order to match * our internal representation of queries. diff --git a/tests/js/spec/views/organizationDiscover/discover.spec.jsx b/tests/js/spec/views/organizationDiscover/discover.spec.jsx index 0a5b4e6eb3202f..e463035b0a3548 100644 --- a/tests/js/spec/views/organizationDiscover/discover.spec.jsx +++ b/tests/js/spec/views/organizationDiscover/discover.spec.jsx @@ -1,5 +1,6 @@ import {browserHistory} from 'react-router'; import React from 'react'; +import qs from 'query-string'; import {mount} from 'enzyme'; import ConfigStore from 'app/stores/configStore'; @@ -69,7 +70,7 @@ describe('Discover', function() { }); }); - describe('componentWillRecieveProps()', function() { + describe('componentWillReceiveProps()', function() { it('handles navigating to saved query', function() { const wrapper = mount(