diff --git a/src/sentry/static/sentry/app/views/organizationDiscover/discover.jsx b/src/sentry/static/sentry/app/views/organizationDiscover/discover.jsx index 2600c793fcc00a..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, @@ -67,19 +68,19 @@ export default class OrganizationDiscover extends React.Component { componentDidMount() { if (this.props.savedQuery) { - this.runQuery(); + this.runQuery({keepVisual: true}); } } 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) { @@ -92,7 +93,9 @@ 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 + if (areQueriesEqual(currentQuery, query)) { return; } @@ -177,8 +180,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 @@ -212,7 +218,10 @@ export default class OrganizationDiscover extends React.Component { if (shouldRedirect) { browserHistory.push({ pathname: `/organizations/${organization.slug}/discover/`, - search: getQueryStringFromQuery(queryBuilder.getInternal()), + query: { + ...getQueryObjectFromQuery(queryBuilder.getInternal()), + visual: keepVisual && location.query.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/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 451adbf4be3752..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( + 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() { @@ -306,10 +349,11 @@ describe('Discover', function() { let wrapper; beforeEach(function() { const mockResponse = {timing: {}, data: [], meta: []}; - browserHistory.push.mockImplementation(function({search}) { + browserHistory.push.mockImplementation(function({search, query}) { wrapper.setProps({ location: { - search: search || '', + search: search || qs.stringify(query), + query, }, }); }); @@ -321,7 +365,7 @@ describe('Discover', function() { , + , { 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); diff --git a/tests/js/spec/views/organizationDiscover/utils.spec.jsx b/tests/js/spec/views/organizationDiscover/utils.spec.jsx index 1595e59302690c..174db322a1bfec 100644 --- a/tests/js/spec/views/organizationDiscover/utils.spec.jsx +++ b/tests/js/spec/views/organizationDiscover/utils.spec.jsx @@ -1,6 +1,6 @@ import { getQueryFromQueryString, - getQueryStringFromQuery, + getQueryObjectFromQuery, getOrderbyFields, parseSavedQuery, generateQueryName, @@ -40,9 +40,18 @@ describe('getQueryFromQueryString()', function() { }); }); -describe('getQueryStringFromQuery()', function() { +describe('getQueryObjectFromQuery()', function() { it('parses query from query string', function() { - expect(getQueryStringFromQuery(query)).toEqual(queryString); + expect(getQueryObjectFromQuery(query)).toEqual({ + aggregations: '[["count()",null,"count"],["uniq","os_build","uniq_os_build"]]', + conditions: '[]', + end: '"2018-07-10T01:18:04"', + fields: '["id","timestamp"]', + limit: '1000', + orderby: '"-timestamp"', + projects: '[8]', + start: '"2018-06-26T01:18:04"', + }); }); });