Skip to content

Commit

Permalink
ui: Refactor trackTableSort analytics service usage
Browse files Browse the repository at this point in the history
Initially, trackTableSort service was called directly from
React components and had an issue with correct tracking of
column names because `title` field has ReactNode type and
cannot be casted to String type for analytics payload.

With current change:
- trackTableSort service doesn't depend on interfaces defined on
representation level (decouples usage of service)
- trackTableSort service is called via sagas by watching appropriate
actions (again decouples service and representation layer)

Release note: None
  • Loading branch information
koorosh committed Aug 4, 2020
1 parent d7fa3bb commit b816b22
Show file tree
Hide file tree
Showing 12 changed files with 81 additions and 56 deletions.
2 changes: 1 addition & 1 deletion pkg/ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"cypress:update-snapshots": "yarn cypress run --env updateSnapshots=true --spec 'cypress/integration/**/*.visual.spec.ts'"
},
"dependencies": {
"@cockroachlabs/admin-ui-components": "^0.1.7",
"@cockroachlabs/admin-ui-components": "^0.1.9",
"analytics-node": "^3.4.0-beta.1",
"antd": "^3.25.2",
"babel-polyfill": "^6.26.0",
Expand Down
18 changes: 18 additions & 0 deletions pkg/ui/src/redux/analyticsActions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ import { PayloadAction } from "src/interfaces/action";

export const TRACK_STATEMENTS_SEARCH = "cockroachui/analytics/TRACK_STATEMENTS_SEARCH";
export const TRACK_STATEMENTS_PAGINATION = "cockroachui/analytics/TRACK_STATEMENTS_PAGINATION";
export const TRACK_TABLE_SORT = "cockroachui/analytics/TRACK_TABLE_SORT";

export interface TableSortActionPayload {
tableName: string;
columnName: string;
ascending?: boolean;
}

export function trackStatementsSearchAction(searchResults: number): PayloadAction<number> {
return {
Expand All @@ -26,3 +33,14 @@ export function trackStatementsPaginationAction(pageNum: number): PayloadAction<
payload: pageNum,
};
}

export function trackTableSortAction(tableName: string, columnName: string, ascending?: boolean): PayloadAction<TableSortActionPayload> {
return {
type: TRACK_TABLE_SORT,
payload: {
tableName,
columnName,
ascending,
},
};
}
21 changes: 19 additions & 2 deletions pkg/ui/src/redux/analyticsSagas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,19 @@ import {
DiagnosticsReportPayload,
OPEN_STATEMENT_DIAGNOSTICS_MODAL,
} from "src/redux/statements";
import { trackActivateDiagnostics, trackDiagnosticsModalOpen, trackPaginate, trackSearch } from "src/util/analytics";
import { TRACK_STATEMENTS_SEARCH, TRACK_STATEMENTS_PAGINATION } from "./analyticsActions";
import {
trackActivateDiagnostics,
trackDiagnosticsModalOpen,
trackPaginate,
trackSearch,
trackTableSort,
} from "src/util/analytics";
import {
TRACK_STATEMENTS_SEARCH,
TRACK_STATEMENTS_PAGINATION,
TRACK_TABLE_SORT,
TableSortActionPayload,
} from "./analyticsActions";

export function* trackActivateStatementsDiagnostics(action: PayloadAction<DiagnosticsReportPayload>) {
const { statementFingerprint } = action.payload;
Expand All @@ -36,11 +47,17 @@ export function* trackStatementsPagination(action: PayloadAction<number>) {
yield call(trackPaginate, action.payload);
}

export function* trackTableSortChange(action: PayloadAction<TableSortActionPayload>) {
const { tableName, columnName, ascending } = action.payload;
yield call(trackTableSort, tableName, columnName, ascending);
}

export function* analyticsSaga() {
yield all([
takeEvery(CREATE_STATEMENT_DIAGNOSTICS_REPORT, trackActivateStatementsDiagnostics),
takeEvery(OPEN_STATEMENT_DIAGNOSTICS_MODAL, trackOpenStatementsDiagnostics),
takeEvery(TRACK_STATEMENTS_SEARCH, trackStatementsSearch),
takeEvery(TRACK_STATEMENTS_PAGINATION, trackStatementsPagination),
takeEvery(TRACK_TABLE_SORT, trackTableSortChange),
]);
}
7 changes: 3 additions & 4 deletions pkg/ui/src/util/analytics/trackTableSort.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,9 @@ describe("trackTableSort", () => {
it("should send the correct payload", () => {
const spy = sandbox.spy();
const tableName = "Test table";
const column = { title: "test", cell: (x: number) => x };
const sortSetting = { sortKey: "whatever", ascending: true };
const title = "test";

track(spy)(tableName, column, sortSetting);
track(spy)(tableName, title, "asc");

const sent = spy.getCall(0).args[0];
const table = get(sent, "properties.tableName");
Expand All @@ -56,7 +55,7 @@ describe("trackTableSort", () => {
assert.isTrue(table === tableName, "Table name matches given table name");

assert.isTrue(isString(columnName), "Column name is a string");
assert.isTrue(column.title === columnName, "Column name matches given column name");
assert.isTrue(title === columnName, "Column name matches given column name");

assert.isTrue(isString(direction), "Sort direction is a string");
assert.isTrue(
Expand Down
20 changes: 8 additions & 12 deletions pkg/ui/src/util/analytics/trackTableSort.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,12 @@
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.
import { analytics } from "src/redux/analytics";
import { SortableColumn, SortSetting } from "src/views/shared/components/sortabletable";

export const track = (fn: Function) => (
name?: String,
col?: SortableColumn,
sortSetting?: SortSetting,
tableName = "",
columnName = "",
sortDirection: "asc" | "desc" = undefined,
) => {
const tableName = name || "";
const columnName = col && col.title || "";
const sortDirection = sortSetting && (sortSetting.ascending) ? "asc" : "desc";

fn({
event: "Table Sort",
properties: {
Expand All @@ -30,10 +25,11 @@ export const track = (fn: Function) => (
};

export default function trackTableSort(
name?: String,
col?: SortableColumn,
sortSetting?: SortSetting,
tableName?: string,
columnTitle?: string,
ascending?: boolean,
) {
const boundTrack = analytics.track.bind(analytics);
track(boundTrack)(name, col, sortSetting);
const sortDirection = ascending && ascending ? "asc" : "desc";
track(boundTrack)(tableName, columnTitle, sortDirection);
}
5 changes: 4 additions & 1 deletion pkg/ui/src/views/shared/components/sortabletable/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,10 @@ export class SortableTable extends React.Component<TableProps> {
if (!isUndefined(c.sortKey)) {
classes.push(cx("sort-table__cell--sortable"));
onClick = () => {
trackTableSort(className, c, sortSetting);
// TODO (koorosh): `title` field has ReactNode type isn't correct field to
// track column name. `SortableColumn` has to be imported from `@cockroachlabs/admin-ui-components`
// package which has extended field to track column name.
trackTableSort(className, c.title.toString(), sortSetting.ascending);
this.clickSort(c.sortKey);
};
if (c.sortKey === sortSetting.sortKey) {
Expand Down
14 changes: 9 additions & 5 deletions pkg/ui/src/views/statements/statements.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import Long from "long";
import moment from "moment";
import { RouteComponentProps } from "react-router-dom";
import * as H from "history";
import { merge } from "lodash";

import "src/protobufInit";
import * as protos from "src/js/protos";
Expand All @@ -21,6 +22,7 @@ import { appAttr, statementAttr } from "src/util/constants";
import { selectStatements, selectApps, selectTotalFingerprints, selectLastReset } from "./statementsPage";
import { selectStatement } from "./statementDetails";
import ISensitiveInfo = protos.cockroach.sql.ISensitiveInfo;
import { AdminUIState, createAdminUIStore } from "src/redux/state";

const INTERNAL_STATEMENT_PREFIX = "$ internal";

Expand Down Expand Up @@ -462,8 +464,9 @@ function makeEmptySensitiveInfo(): ISensitiveInfo {
};
}

function makeInvalidState() {
return {
function makeInvalidState(): AdminUIState {
const store = createAdminUIStore(H.createMemoryHistory());
return merge(store.getState(), {
cachedData: {
statements: {
inFlight: true,
Expand All @@ -474,11 +477,12 @@ function makeInvalidState() {
valid: false,
},
},
};
});
}

function makeStateWithStatementsAndLastReset(statements: CollectedStatementStatistics[], lastReset: number) {
return {
const store = createAdminUIStore(H.createMemoryHistory());
return merge(store.getState(), {
cachedData: {
statements: {
data: protos.cockroach.server.serverpb.StatementsResponse.fromObject({
Expand All @@ -497,7 +501,7 @@ function makeStateWithStatementsAndLastReset(statements: CollectedStatementStati
valid: false,
},
},
};
});
}

function makeStateWithStatements(statements: CollectedStatementStatistics[]) {
Expand Down
5 changes: 4 additions & 1 deletion pkg/ui/src/views/statements/statementsPage.fixture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

import { StatementsPageProps } from "./statementsPage";
import { StatementsPageProps } from "@cockroachlabs/admin-ui-components";
import { createMemoryHistory } from "history";
import Long from "long";
import * as protos from "src/js/protos";
Expand Down Expand Up @@ -392,6 +392,9 @@ const statementsPagePropsFixture: StatementsPageProps = {
dismissAlertMessage: () => {},
refreshStatementDiagnosticsRequests: (() => {}) as (typeof refreshStatementDiagnosticsRequests),
refreshStatements: (() => {}) as (typeof refreshStatements),
onActivateStatementDiagnostics: _ => {},
onDiagnosticsModalOpen: _ => {},
onPageChanged: _ => {},
};

export default statementsPagePropsFixture;
23 changes: 0 additions & 23 deletions pkg/ui/src/views/statements/statementsPage.stories.tsx

This file was deleted.

12 changes: 10 additions & 2 deletions pkg/ui/src/views/statements/statementsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ import { connect } from "react-redux";
import { createSelector } from "reselect";
import { RouteComponentProps, withRouter } from "react-router-dom";
import * as protos from "src/js/protos";
import { refreshStatementDiagnosticsRequests, refreshStatements } from "src/redux/apiReducers";
import {
refreshStatementDiagnosticsRequests,
refreshStatements,
} from "src/redux/apiReducers";
import { CachedDataReducerState } from "src/redux/cachedDataReducer";
import { AdminUIState } from "src/redux/state";
import { StatementsResponseMessage } from "src/util/api";
Expand All @@ -34,7 +37,11 @@ import { getMatchParamByName } from "src/util/query";

import { StatementsPage, AggregateStatistics } from "@cockroachlabs/admin-ui-components";
import { createOpenDiagnosticsModalAction, createStatementDiagnosticsReportAction } from "src/redux/statements";
import { trackStatementsPaginationAction, trackStatementsSearchAction } from "src/redux/analyticsActions";
import {
trackStatementsPaginationAction,
trackStatementsSearchAction,
trackTableSortAction,
} from "src/redux/analyticsActions";

type ICollectedStatementStatistics = protos.cockroach.server.serverpb.StatementsResponse.ICollectedStatementStatistics;

Expand Down Expand Up @@ -177,6 +184,7 @@ const StatementsPageConnected = withRouter(connect(
onDiagnosticsModalOpen: createOpenDiagnosticsModalAction,
onSearchComplete: (results: AggregateStatistics[]) => trackStatementsSearchAction(results.length),
onPageChanged: trackStatementsPaginationAction,
onSortingChange: trackTableSortAction,
},
)(StatementsPage));

Expand Down
8 changes: 4 additions & 4 deletions pkg/ui/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1806,10 +1806,10 @@
lodash "^4.17.13"
to-fast-properties "^2.0.0"

"@cockroachlabs/admin-ui-components@^0.1.7":
version "0.1.7"
resolved "https://registry.yarnpkg.com/@cockroachlabs/admin-ui-components/-/admin-ui-components-0.1.7.tgz#8d85668b1e72840cf1dabbb2721eb819246489a6"
integrity sha512-le5IF2KqM2ZXJnC7a86UfZvyWQBJqJcYHFo9EnPkL9hFkKTxNeNY6PJh60d+dfffga/FZJd/aT3tbNvaLTopMw==
"@cockroachlabs/admin-ui-components@^0.1.9":
version "0.1.9"
resolved "https://registry.yarnpkg.com/@cockroachlabs/admin-ui-components/-/admin-ui-components-0.1.9.tgz#e43a2508bc258e368d0c50c5ecf5d5c7fcf72c53"
integrity sha512-qWp4K0ChHHrNQYVCMhZxX5Ugwqjgog6htBm24d7KWmiHtZmEeqRzowuiO1I6289LvRk885yQJouD9XRe+vqjXA==
dependencies:
"@cockroachlabs/icons" "^0.2.2"
"@popperjs/core" "^2.4.0"
Expand Down

0 comments on commit b816b22

Please sign in to comment.