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

feat(uaa-logging): Log UAA Parity data #3629

Merged
merged 14 commits into from
Sep 24, 2024
78 changes: 49 additions & 29 deletions src/api/Feed.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ import type {
Tasks,
ThreadedComments as ThreadedCommentsType,
} from '../common/types/feed';
import { collapseFeedState } from '../elements/content-sidebar/activity-feed/activity-feed/activityFeedUtils';
Copy link
Contributor

Choose a reason for hiding this comment

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

i think it's odd for an api file to be importing from a component utility file


const TASK_NEW_INITIAL_STATUS = TASK_NEW_NOT_STARTED;

Expand Down Expand Up @@ -550,6 +551,7 @@ class Feed extends Base {
shouldShowVersions?: boolean,
shouldUseUAA?: boolean,
} = {},
logAPIParity?: Function,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: function header definition

): void {
const { id, permissions = {} } = file;
const cachedItems = this.getCachedItems(id);
Expand All @@ -571,23 +573,16 @@ class Feed extends Base {
this.errorCallback = onError;

// Using the UAA File Activities endpoint replaces the need for these calls
const annotationsPromise =
!shouldUseUAA && shouldShowAnnotations
? this.fetchAnnotations(permissions, shouldShowReplies)
: Promise.resolve();
const annotationsPromise = shouldShowAnnotations
? this.fetchAnnotations(permissions, shouldShowReplies)
: Promise.resolve();
const commentsPromise = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: commentsPromise could be updated to match the ternary pattern of the others?

if (shouldUseUAA) {
return Promise.resolve();
}

return shouldShowReplies ? this.fetchThreadedComments(permissions) : this.fetchComments(permissions);
};
const tasksPromise = !shouldUseUAA && shouldShowTasks ? this.fetchTasksNew() : Promise.resolve();
const appActivityPromise =
!shouldUseUAA && shouldShowAppActivity ? this.fetchAppActivity(permissions) : Promise.resolve();
const versionsPromise = !shouldUseUAA && shouldShowVersions ? this.fetchVersions() : Promise.resolve();
const currentVersionPromise =
!shouldUseUAA && shouldShowVersions ? this.fetchCurrentVersion() : Promise.resolve();
const tasksPromise = shouldShowTasks ? this.fetchTasksNew() : Promise.resolve();
const appActivityPromise = shouldShowAppActivity ? this.fetchAppActivity(permissions) : Promise.resolve();
const versionsPromise = shouldShowVersions ? this.fetchVersions() : Promise.resolve();
const currentVersionPromise = shouldShowVersions ? this.fetchCurrentVersion() : Promise.resolve();

const annotationActivityType =
shouldShowAnnotations && permissions[PERMISSION_CAN_VIEW_ANNOTATIONS]
Expand Down Expand Up @@ -622,29 +617,54 @@ class Feed extends Base {
}
};

const v2Promises = [
versionsPromise,
currentVersionPromise,
commentsPromise(),
tasksPromise,
appActivityPromise,
annotationsPromise,
];

const fetchV2FeedItems = async promises => {
return Promise.all(promises).then(
([versions: ?FileVersions, currentVersion: ?BoxItemVersion, ...feedItems]) => {
const versionsWithCurrent = currentVersion
? this.versionsAPI.addCurrentVersion(currentVersion, versions, this.file)
: undefined;
return sortFeedItems(versionsWithCurrent, ...feedItems);
},
);
};

const compareV2AndUaaFeedItems = async (uaaFeedItems, uaaResponse) => {
Comment on lines +629 to +640
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think you need async in either function since await is not being used

fetchV2FeedItems(v2Promises).then(v2FeedItems => {
const transformedV2FeedItems = collapseFeedState(v2FeedItems);
const transformedUAAFeedItems = collapseFeedState(uaaFeedItems);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: inconsistency of uaa between this variable transformedUAAFeedItems and the function name compareV2AndUaaFeedItems


if (logAPIParity) {
logAPIParity({
uaaResponse,
uaaFeedItems: transformedUAAFeedItems,
v2FeedItems: transformedV2FeedItems,
});
}
});
};

if (shouldUseUAA) {
fileActivitiesPromise.then(response => {
if (!response) {
return;
}

const parsedFeedItems = getParsedFileActivitiesResponse(response);
handleFeedItems(parsedFeedItems);
const uaaFeedItems = getParsedFileActivitiesResponse(response);
compareV2AndUaaFeedItems(uaaFeedItems, response);
handleFeedItems(uaaFeedItems);
});
} else {
Promise.all([
versionsPromise,
currentVersionPromise,
commentsPromise(),
tasksPromise,
appActivityPromise,
annotationsPromise,
]).then(([versions: ?FileVersions, currentVersion: ?BoxItemVersion, ...feedItems]) => {
const versionsWithCurrent = currentVersion
? this.versionsAPI.addCurrentVersion(currentVersion, versions, this.file)
: undefined;
const sortedFeedItems = sortFeedItems(versionsWithCurrent, ...feedItems);
handleFeedItems(sortedFeedItems);
fetchV2FeedItems(v2Promises).then(v2FeedItems => {
handleFeedItems(v2FeedItems);
});
}
}
Expand Down
5 changes: 2 additions & 3 deletions src/api/__tests__/Feed.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,7 @@ describe('api/Feed', () => {
});
});

test('should use the file activities api if shouldUseUAA is true', done => {
test('should use the file activities api if shouldUseUAA is true and shadow call v2 APIs', done => {
feed.feedItems(file, false, successCb, errorCb, errorCb, {
shouldUseUAA: true,
shouldShowAnnotations: true,
Expand All @@ -627,8 +627,7 @@ describe('api/Feed', () => {
],
true,
);
expect(feed.fetchComments).not.toBeCalled();
expect(feed.fetchThreadedComments).not.toBeCalled();
expect(feed.fetchThreadedComments).toBeCalled();
done();
});
});
Expand Down
4 changes: 3 additions & 1 deletion src/common/types/logging.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@ import {
METRIC_TYPE_PREVIEW,
METRIC_TYPE_ELEMENTS_LOAD_METRIC,
METRIC_TYPE_ELEMENTS_PERFORMANCE_METRIC,
METRIC_TYPE_UAA_PARITY_METRIC,
} from '../../constants';

type MetricType =
| typeof METRIC_TYPE_PREVIEW
| typeof METRIC_TYPE_ELEMENTS_LOAD_METRIC
| typeof METRIC_TYPE_ELEMENTS_PERFORMANCE_METRIC;
| typeof METRIC_TYPE_ELEMENTS_PERFORMANCE_METRIC
| typeof METRIC_TYPE_UAA_PARITY_METRIC;

type ElementsLoadMetricData = {
endMarkName: string,
Expand Down
1 change: 1 addition & 0 deletions src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,7 @@ export const ORIGIN_OPEN_WITH: 'open_with' = 'open_with';
export const METRIC_TYPE_PREVIEW: 'preview_metric' = 'preview_metric';
export const METRIC_TYPE_ELEMENTS_PERFORMANCE_METRIC: 'elements_performance_metric' = 'elements_performance_metric';
export const METRIC_TYPE_ELEMENTS_LOAD_METRIC: 'elements_load_metric' = 'elements_load_metric';
export const METRIC_TYPE_UAA_PARITY_METRIC = 'uaa_parity_metric';

/* ------------------ Error Keys ---------------------- */
export const IS_ERROR_DISPLAYED = 'isErrorDisplayed'; // used to determine if user will see some error state or message
Expand Down
22 changes: 17 additions & 5 deletions src/elements/common/logger/Logger.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ import noop from 'lodash/noop';
import { v4 as uuidv4 } from 'uuid';
import { isMarkSupported } from '../../../utils/performance';
import { EVENT_DATA_READY, EVENT_JS_READY } from './constants';
import { METRIC_TYPE_PREVIEW, METRIC_TYPE_ELEMENTS_LOAD_METRIC } from '../../../constants';
import {
METRIC_TYPE_PREVIEW,
METRIC_TYPE_ELEMENTS_LOAD_METRIC,
METRIC_TYPE_UAA_PARITY_METRIC,
} from '../../../constants';
import type { ElementOrigin } from '../flowTypes';
import type { MetricType, ElementsLoadMetricData, LoggerProps } from '../../../common/types/logging';

Expand Down Expand Up @@ -123,10 +127,18 @@ class Logger extends React.Component<Props> {
*/
handlePreviewMetric = (data: Object) => {
const { onMetric } = this.props;
onMetric({
...data,
type: METRIC_TYPE_PREVIEW,
});

if (data.type === METRIC_TYPE_UAA_PARITY_METRIC) {
onMetric({
...data,
type: METRIC_TYPE_UAA_PARITY_METRIC,
});
} else {
onMetric({
...data,
type: METRIC_TYPE_PREVIEW,
});
}
};

/**
Expand Down
18 changes: 18 additions & 0 deletions src/elements/content-sidebar/ActivitySidebar.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import {
ORIGIN_ACTIVITY_SIDEBAR,
SIDEBAR_VIEW_ACTIVITY,
TASK_COMPLETION_RULE_ALL,
METRIC_TYPE_UAA_PARITY_METRIC,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this list is alphasorted

} from '../../constants';
import type {
TaskCompletionRule,
Expand Down Expand Up @@ -736,6 +737,7 @@ class ActivitySidebar extends React.PureComponent<Props, State> {
shouldShowVersions,
shouldUseUAA,
},
shouldUseUAA ? this.logAPIParity : undefined,
);
}

Expand Down Expand Up @@ -795,6 +797,22 @@ class ActivitySidebar extends React.PureComponent<Props, State> {
this.setState({ feedItems, activityFeedError: undefined });
};

/**
* Logs diff between UAA and v2 API data
*
* @param {{}[]} responseParity array of aggragated responses from UAA and v2
* @param {{}} parsedDataParity parsed data from UAA and v2
* @return {void}
*/
logAPIParity = (parityData: { uaaFeedItems: FeedItems, v2FeedItems: FeedItems }): void => {
const { logger } = this.props;

logger.onPreviewMetric({
parityData,
type: METRIC_TYPE_UAA_PARITY_METRIC,
});
};

/**
* Handles a failed feed item fetch
*
Expand Down
13 changes: 5 additions & 8 deletions src/elements/content-sidebar/__tests__/ActivitySidebar.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -730,6 +730,7 @@ describe('elements/content-sidebar/ActivitySidebar', () => {
instance.errorCallback = jest.fn();
instance.fetchFeedItemsErrorCallback = jest.fn();
instance.fetchFeedItemsSuccessCallback = jest.fn();
instance.logAPIParity = jest.fn();

instance.fetchFeedItems();
expect(feedAPI.feedItems).toHaveBeenCalledWith(
Expand All @@ -746,6 +747,7 @@ describe('elements/content-sidebar/ActivitySidebar', () => {
shouldShowVersions: expectedVersions,
shouldUseUAA: expectedUseUAA,
},
expectedUseUAA ? instance.logAPIParity : undefined,
);
},
);
Expand Down Expand Up @@ -779,6 +781,7 @@ describe('elements/content-sidebar/ActivitySidebar', () => {
shouldShowVersions: true,
shouldUseUAA: false,
},
undefined,
);
});
});
Expand Down Expand Up @@ -1519,14 +1522,8 @@ describe('elements/content-sidebar/ActivitySidebar', () => {
`(
'should filter feed items of type "comment" or "annotation" based on status equal to $status',
({ status, expected }) => {
const {
annotationOpen,
annotationResolved,
commentOpen,
commentResolved,
taskItem,
versionItem,
} = cloneDeep(filterableActivityFeedItems);
const { annotationOpen, annotationResolved, commentOpen, commentResolved, taskItem, versionItem } =
cloneDeep(filterableActivityFeedItems);
const wrapper = getWrapper();
const instance = wrapper.instance();
instance.setState({
Expand Down
Loading