feat(uaa-integration): Parse Versions Response from UAA #3385

JChan106 · 2023-08-11T22:14:05Z

The UAA response gives a Versions activity event that is different than the v2 Versions API that we are currently using. We must parse the response and add the needed fields that our UI components need to render Version events.

One big difference with this change is that the latest version events do not have promotion context, unlike the v2 API. So if the user has promoted a version and that version is the most recent version, they can only see that version was promoted by clicking the info button and looking at the version history. The team has had discussions around this and is okay with this tradeoff, we will look into adding the promotion context into the UAA response in the future.

UAA:
Multiple version events

Multiple upload collaborators

Version events with comments between

new file upload, no events

V2 Versions:
Multiple version events

Multiple upload collaborators

Version events with comments between

new file upload, no events

JChan106 · 2023-08-11T22:20:52Z

src/elements/content-sidebar/activity-feed/version/CollapsedVersion.js

@@ -83,9 +84,7 @@ const CollapsedVersion = (props: Props): React.Node => {
                        aria-label={intl.formatMessage(messages.getVersionInfo)}
                        className="bcs-Version-info"
                        data-resin-target={ACTIVITY_TARGETS.VERSION_CARD}
-                        onClick={() => {
-                            onInfo({ versions });


What's passed into this cb is only used when hasVersions prop is false and onVersionHistoryClick prop is passed into BUIE. There doesn't seem to be a standardized format, since the CollapsedVersion and Version components pass back different data. Because UAA doesn't have the versions array like the v2 api, I decided to use the end version of the event and match what was passed in the Version.js component (id and version_number). This cb is not used in EUA.

question: does the version_end always the represent current version even if a previous version was promoted?

patlm · 2023-08-14T22:32:46Z

src/elements/content-sidebar/activity-feed/version/Version.js

@@ -26,6 +26,7 @@ type Props = {
    id: string,
    modified_by: User,
    onInfo?: Function,
+    shouldUseUAA?: boolean,


I'm not seeing where this is being used here. Can it be removed?

patlm · 2023-08-14T22:40:58Z

src/elements/content-sidebar/activity-feed/version/Version.js

-                        onClick={() => {
-                            onInfo({ id, version_number });
-                        }}
+                        onClick={() => onInfo({ id, version_number })}


This change may un-intentionally be returning a value in the onClick when it wasn't before. I think it might be better to leave it in brackets or type the onInfo prop to show it returns void or undefined (same in CollapsedVersion).

patlm · 2023-08-14T22:43:06Z

src/elements/content-sidebar/activity-feed/version/CollapsedVersion.js

-                        onClick={() => {
-                            onInfo({ versions });
-                        }}
+                        onClick={() => onInfo(shouldUseUAA ? { id, version_number: version_end } : { versions })}


Might be returning a value now implicitly from the arrow function

patlm · 2023-08-14T23:19:17Z

src/api/Feed.js

+                        }
+                    }
+
+                    return versionsItem;


I was wondering if there would be a way to extract and organize some of this into separate functions to help reduce the length of the getParsedFileActivitiesResponse function? I'm also fine with leaving as-is if you don't think that would make sense here.

Yeah, thought about it, there isn't really benefits to splitting though since the logic isn't reusable, and the switch statements make it more readable as well.

patlm · 2023-08-15T16:44:34Z

src/elements/content-sidebar/activity-feed/activity-feed/__tests__/ActivityFeed.test.js

@@ -148,6 +148,15 @@ describe('elements/content-sidebar/ActivityFeed/activity-feed/ActivityFeed', ()
        expect(wrapper.find('EmptyState').exists()).toBe(true);
    });

+    test('should not render empty state when UAA is enabled and there is 1 version (current version from file) and the start and end version are the same', () => {


Was this meant to be "should render empty.." instead of "should not render"?

patlm

Looks good to me!

src/api/Feed.js

src/elements/content-sidebar/activity-feed/activity-feed/ActiveState.js

tjuanitas · 2023-08-18T02:40:29Z

src/elements/content-sidebar/activity-feed/version/CollapsedVersion.js

@@ -83,9 +84,7 @@ const CollapsedVersion = (props: Props): React.Node => {
                        aria-label={intl.formatMessage(messages.getVersionInfo)}
                        className="bcs-Version-info"
                        data-resin-target={ACTIVITY_TARGETS.VERSION_CARD}
-                        onClick={() => {
-                            onInfo({ versions });


question: does the version_end always the represent current version even if a previous version was promoted?

src/elements/content-sidebar/activity-feed/activity-feed/__tests__/ActivityFeed.test.js

src/elements/content-sidebar/activity-feed/activity-feed/ActivityFeed.js

src/api/__tests__/Feed.test.js

src/api/Feed.js

tjuanitas · 2023-08-18T03:29:21Z

src/api/Feed.js

+                            versionsItem.uploader_display_name = versionsItem.action_by[0].name;
+                        }
+
+                        versionsItem.action_by.map(collaborator => {


it's a little odd to use map without intending to use the returned array. I think this might be a good use case for reduce:

versionsItem.collaborators = versionsItem.action_by.reduce( ... , {});

Leaning for keeping it for now since we've used map in a few other place. Going through another round of manual testing for the logic change also is a big time sink.

semantically it feels a little wrong to use a map in this case over something like a forEach since the goal is to populate a separate object instead of using the returned array

with the reduce it could be:

versionsItem.collaborators = versionsItem.action_by.reduce((collaborators, collaborator) => ({ ...collaborators, [collaborator.id]: { ...collaborator } }), {});

although you could leave it as-is if the current implementation is easier to read

src/api/Feed.js

tjuanitas

lgtm!

tjuanitas · 2023-08-23T01:11:32Z

src/api/Feed.js

+                            versionsItem.uploader_display_name = versionsItem.action_by[0].name;
+                        }
+
+                        versionsItem.action_by.map(collaborator => {


semantically it feels a little wrong to use a map in this case over something like a forEach since the goal is to populate a separate object instead of using the returned array

with the reduce it could be:

versionsItem.collaborators = versionsItem.action_by.reduce((collaborators, collaborator) => ({ ...collaborators, [collaborator.id]: { ...collaborator } }), {});

although you could leave it as-is if the current implementation is easier to read

tjuanitas · 2023-08-23T01:15:40Z

src/constants.js

@@ -471,6 +471,11 @@ export const ACTIVITY_FILTER_OPTION_RESOLVED: 'resolved' = 'resolved';
 export const ACTIVITY_FILTER_OPTION_TASKS: 'tasks' = 'tasks';
 export const ACTIVITY_FILTER_OPTION_UNRESOLVED: 'open' = 'open';

+/* ------------------ File Activity Action Types ----------- */
+export const ACTION_TYPE_CREATED: 'created' = 'created';


I'm not sure if there's a concept of "action type" for other features but if there was we could change to FILE_ACTIVITY_ACTION_TYPE_[...] to be more specific

JChan106 requested review from a team as code owners August 11, 2023 22:14

JChan106 commented Aug 11, 2023

View reviewed changes

JChan106 force-pushed the COXP-11588 branch from 46dca16 to 85c34a4 Compare August 14, 2023 22:28

patlm reviewed Aug 14, 2023

View reviewed changes

patlm reviewed Aug 15, 2023

View reviewed changes

patlm previously approved these changes Aug 15, 2023

View reviewed changes

mickr reviewed Aug 15, 2023

View reviewed changes

src/api/Feed.js Outdated Show resolved Hide resolved

src/api/Feed.js Outdated Show resolved Hide resolved

src/elements/content-sidebar/activity-feed/activity-feed/ActiveState.js Show resolved Hide resolved

JChan106 dismissed patlm’s stale review via a99cfd8 August 16, 2023 19:17

JChan106 force-pushed the COXP-11588 branch from 2eb9601 to a99cfd8 Compare August 16, 2023 19:17

JChan106 requested a review from a team as a code owner August 16, 2023 19:17

JChan106 requested review from mickr and patlm August 16, 2023 19:49

mickr previously approved these changes Aug 17, 2023

View reviewed changes

tjuanitas reviewed Aug 18, 2023

View reviewed changes

JChan106 dismissed mickr’s stale review via d621379 August 22, 2023 21:15

JChan106 force-pushed the COXP-11588 branch from a99cfd8 to d621379 Compare August 22, 2023 21:15

JChan106 requested a review from tjuanitas August 22, 2023 23:18

JChan106 added 9 commits August 22, 2023 16:21

feat(uaa-integration): Use file activities for versions

632e7c7

feat(uaa-integration): Handle other version events

8c3e90b

feat(uaa-integration): Update onInfo

39aff49

feat(uaa-integration): Fix tests

9647e3e

feat(uaa-integration): Fix lint errors

7e028d1

feat(uaa-integration): Fix empty state, address PR comments

236fa65

feat(uaa-integration): fix tests

a13139f

feat(uaa-integration): address PR comments

e688330

feat(uaa-integration): Address PR comments

d6a3d3c

tjuanitas approved these changes Aug 23, 2023

View reviewed changes

JChan106 force-pushed the COXP-11588 branch from d621379 to d6a3d3c Compare August 23, 2023 19:42

mickr approved these changes Aug 23, 2023

View reviewed changes

patlm approved these changes Aug 23, 2023

View reviewed changes

JChan106 added the ready-to-merge label Aug 23, 2023

mergify bot merged commit 0fa730f into box:master Aug 23, 2023
6 checks passed

JChan106 deleted the COXP-11588 branch August 23, 2023 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(uaa-integration): Parse Versions Response from UAA #3385

feat(uaa-integration): Parse Versions Response from UAA #3385

JChan106 commented Aug 11, 2023 •

edited

JChan106 Aug 11, 2023

tjuanitas Aug 18, 2023

JChan106 Aug 22, 2023

patlm Aug 14, 2023

patlm Aug 14, 2023

patlm Aug 14, 2023

patlm Aug 14, 2023

JChan106 Aug 15, 2023

patlm Aug 15, 2023

JChan106 Aug 15, 2023

patlm left a comment

tjuanitas Aug 18, 2023

tjuanitas Aug 18, 2023

JChan106 Aug 22, 2023

tjuanitas Aug 23, 2023

tjuanitas left a comment

tjuanitas Aug 23, 2023

tjuanitas Aug 23, 2023

feat(uaa-integration): Parse Versions Response from UAA #3385

feat(uaa-integration): Parse Versions Response from UAA #3385

Conversation

JChan106 commented Aug 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patlm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjuanitas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JChan106 commented Aug 11, 2023 •

edited