feat(content-sidebar): disable version modifications for archive file #3637

michalkowalczyk-box · 2024-09-06T08:29:04Z

Disables changing current version and deleting/restoring previous based on if previewed file is in archive, which is determined by existence of instance of global metadata template archivedItemTemplate. We need to use this approach temporarily, because permission settings for files that block this behavior on backend will not be publicly available in near future.

This metadata field is used to display date in #3625.

psatala-box · 2024-09-06T10:06:11Z

src/elements/content-sidebar/versions/VersionsItem.js

    const showDownload = can_download && !isDeleted && isDownloadable;
-    const showPromote = can_upload && !isDeleted && !isCurrent;
+    const showPromote = can_upload && !isDeleted && !shouldDisablePromoteAndDelete && !isCurrent;
    const showRestore = can_delete && isDeleted;


Should we also not allow restoring deleted versions?

They're restored in archiving process, but maybe this could be disabled just in case too

psatala-box · 2024-09-06T10:10:59Z

src/elements/content-sidebar/versions/__tests__/VersionsItem.test.js

+            ${{ can_delete: true, can_download: true, can_preview: true, can_upload: true }}
+            ${{ can_delete: true, can_download: true, can_preview: true, can_upload: false }}
+            ${{ can_delete: true, can_download: true, can_preview: false, can_upload: true }}
+            ${{ can_delete: true, can_download: false, can_preview: true, can_upload: true }}
+            ${{ can_delete: false, can_download: true, can_preview: true, can_upload: true }}
+            ${{ can_delete: true, can_download: true, can_preview: false, can_upload: false }}
+            ${{ can_delete: true, can_download: false, can_preview: true, can_upload: false }}
+            ${{ can_delete: false, can_download: true, can_preview: false, can_upload: true }}
+            ${{ can_delete: false, can_download: false, can_preview: true, can_upload: true }}


Is there any reason you test those 9 combinations instead of all 16? Same for the last test

Here, some of those have to be tested in test below, since none of actions are available and we don't render component. Rest didn't have affected permission, but I'll add them just in case

psatala-box

Looks good, a couple nitpicks about the tests

psatala-box · 2024-09-06T12:47:13Z

src/elements/content-sidebar/versions/__tests__/VersionsItem.test.js

+            ${'restore'} | ${false}   | ${true}      | ${true}     | ${false}    | ${false}
+            ${'upload'}  | ${false}   | ${true}      | ${true}     | ${false}    | ${false}
+        `(
+            "should show actions correctly when the version's action is $action and unchangeableVersions feature is disabled",


Suggested change

"should show actions correctly when the version's action is $action and unchangeableVersions feature is disabled",

"should show actions correctly when the version's action is $action and unchangeableVersions feature is enabled",

psatala-box · 2024-09-06T12:47:54Z

src/elements/content-sidebar/versions/__tests__/VersionsItem.test.js

+                expect(button.prop('isDisabled')).toBe(!permissions.can_preview);
+                expect(actions.prop('showDelete')).toBe(false);
+                expect(actions.prop('showDownload')).toBe(permissions.can_download);
+                expect(actions.prop('showPromote')).toBe(false);
+                expect(actions.prop('showPreview')).toBe(permissions.can_preview);
+                expect(wrapper.find(ReadableTime)).toBeTruthy();


Could you check showRestore here as well?

It'll always be false here, regardless of feature, since it needs action to be 'delete'. If I set action to delete it'll be testable, but same will happen to delete. I decided to test it with action tests below.

I'll look if it can be done better after the weekend

psatala-box · 2024-09-06T12:48:09Z

src/elements/content-sidebar/versions/__tests__/VersionsItem.test.js

+                expect(button.prop('isDisabled')).toBe(!permissions.can_preview);
+                expect(actions.prop('showDelete')).toBe(permissions.can_delete);
+                expect(actions.prop('showDownload')).toBe(permissions.can_download);
+                expect(actions.prop('showPromote')).toBe(permissions.can_upload);
+                expect(actions.prop('showPreview')).toBe(permissions.can_preview);
+                expect(wrapper.find(ReadableTime)).toBeTruthy();


Could you check showRestore here as well?

psatala-box · 2024-09-06T12:48:43Z

src/elements/content-sidebar/versions/__tests__/VersionsItem.test.js

+            },
+        );
+
+        test("should show actions correctly when the version's action is $action and unchangeableVersions feature is disabled", () => {


Suggested change

test("should show actions correctly when the version's action is $action and unchangeableVersions feature is disabled", () => {

test("should not show actions when the version's action is delete and unchangeableVersions feature is enabled", () => {

jstoffan · 2024-09-09T15:15:56Z

src/elements/content-sidebar/versions/VersionsItem.js

-    const showPromote = can_upload && !isDeleted && !isCurrent;
-    const showRestore = can_delete && isDeleted;
+    const showPromote = can_upload && !shouldDisableVersionChanges && !isDeleted && !isCurrent;
+    const showRestore = can_delete && !shouldDisableVersionChanges && isDeleted;


We should use permissions to control whether a user has the ability to do something. That way, our API will enforce the same restrictions.

rafalmaksymiuk · 2024-10-03T08:33:17Z

src/elements/content-sidebar/versions/__tests__/VersionsItem.test.js

@@ -238,5 +238,111 @@ describe('elements/content-sidebar/versions/VersionsItem', () => {
            expect(wrapper.exists(VersionsItemRetention)).toBe(true);
            expect(wrapper.find(VersionsItemActions).prop('isRetained')).toBe(true);
        });
+


Can you please rewrite those test to use React Testing Library. Enzyme is a huge problem in EUA and we need to slowly convert existing tests instead of creating new ones.

Updated whole file to RTL

rafalmaksymiuk · 2024-10-03T08:35:08Z

src/elements/content-sidebar/versions/__tests__/VersionsSidebarContainer.test.js

@@ -263,6 +265,17 @@ describe('elements/content-sidebar/versions/VersionsSidebarContainer', () => {

            expect(wrapper.state('isWatermarked')).toBe(true);
        });
+
+        test('should set state with isArchiveFile if file is archive file', () => {


I feel this file is too complicated to switch whole of it from enzyme to RTL in scope of this PR. I have RTL implementation for new tests (added to tests this PR code changes), but I'm working on putting it alongside enzyme in this file. I'm having trouble, because some module mocks needed for RTL are breaking enzyme tests and vice versa

Moved this test case to new file using RTL

jstoffan · 2024-10-09T15:01:38Z

src/elements/content-sidebar/versions/VersionsSidebarContainer.js

@@ -43,6 +47,7 @@ type Props = {
 type State = {
    error?: MessageDescriptor,
    isLoading: boolean,
+    isArchiveFile: boolean,


Minor, but we should alphabetize props and object properties when possible.

jstoffan · 2024-10-09T15:02:16Z

src/elements/content-sidebar/versions/VersionsSidebarContainer.js

@@ -66,6 +71,7 @@ class VersionsSidebarContainer extends React.Component<Props, State> {
    props: Props;

    state: State = {
+        isArchiveFile: false,


Minor, but consider isArchived to stay consistent with isWatermarked.

jstoffan · 2024-10-09T15:02:57Z

src/elements/content-sidebar/versions/__tests__/VersionsItem.test.js

+                expect(actions.prop('showPreview')).toBe(showPreview);
+                expect(actions.prop('showRestore')).toBe(showRestore);
+                expect(wrapper.find(ReadableTime)).toBeTruthy();
+                expect(wrapper).toMatchSnapshot();


We generally want to avoid adding new snapshot tests, especially for larger components. Let's rely on explicit assertions, instead.

I've moved all the tests for this component to react-testing-library, so I removed snapshots completely there

…sidebar

jstoffan · 2024-10-09T15:52:09Z

i18n/en-US.properties

@@ -938,6 +938,12 @@ boxui.collaboratorAvatars.viewAdditionalPeopleText = View additional people
 boxui.collapsiblesidebar.collapseBtnLabel = Collapse
 # Aria label for toggle button that expands/collapses sidebar (collapsed state)
 boxui.collapsiblesidebar.expandBtnLabel = Expand
+# Content Answers submit input button text
+boxui.contentAnswers.ask = Ask


It looks like something went awry during the pre-commit hook.

Yep, it amended last commit for me and said to push with --no-verify, is this okay? Or should I revert changes for this file?

It was pre-push hook

Yeah, I would revert. Maybe @greg-in-a-box can comment on the automation here.

Reverted them, lint_test_build was failing on them https://github.com/box/box-ui-elements/runs/31305517831

…sidebar

psatala-box

Great job!

michalkowalczyk-box requested a review from a team as a code owner September 6, 2024 08:29

psatala-box reviewed Sep 6, 2024

View reviewed changes

jstoffan requested changes Sep 9, 2024

View reviewed changes

michalkowalczyk-box added 8 commits October 2, 2024 17:58

feat(content-sidebar): add unchangeable versions feature to VersionsItem

a6e56d9

feat(content-sidebar): create tests for unchangeableVersionsFeature

9fbdfc7

feat(content-sidebar): add more exhaustive test cases for permissions

ea10fee

feat(content-sidebar): block restore on unchangeableVersions enabled

08ab0b1

feat(content-sidebar): fix VersionsItem test names

c2c76f3

feat(content-sidebar): revert changes to VersionsItem

07843c6

feat(content-sidebar): update VersionsItem to use archive metadata

fc18385

feat(content-sidebar): implement archive fetch for versions

1dcc7c6

michalkowalczyk-box force-pushed the disable-version-modifications-for-content-sidebar branch from 8cc1644 to 1dcc7c6 Compare October 2, 2024 18:31

michalkowalczyk-box requested a review from a team as a code owner October 2, 2024 18:31

feat(content-sidebar): fix flow error

37e767c

michalkowalczyk-box requested review from psatala-box and jstoffan October 2, 2024 19:06

michalkowalczyk-box changed the title ~~feat(content-sidebar): disable version modifications for content sidebar~~ feat(content-sidebar): disable version modifications for archive file Oct 2, 2024

rafalmaksymiuk reviewed Oct 3, 2024

View reviewed changes

feat(content-sidebar): udpate VersionsItem tests to RTL

c0d4e86

jstoffan previously approved these changes Oct 9, 2024

View reviewed changes

michalkowalczyk-box dismissed jstoffan’s stale review via c0d4e86 October 9, 2024 15:17

michalkowalczyk-box added 2 commits October 9, 2024 17:21

Merge branch 'master' into disable-version-modifications-for-content-…

2ac32fb

…sidebar

feat(content-sidebar): updated prop name and order

56896b1

jstoffan reviewed Oct 9, 2024

View reviewed changes

feat(content-sidebar): reverted changes to en-US.properties

6aa0f8f

jstoffan previously approved these changes Oct 9, 2024

View reviewed changes

michalkowalczyk-box requested a review from rafalmaksymiuk October 9, 2024 16:28

feat(content-sidebar): moved tests to new file

df0645e

feat(content-sidebar): updated snapshot

4e21d1c

michalkowalczyk-box dismissed jstoffan’s stale review via 4e21d1c October 10, 2024 13:32

Merge branch 'master' into disable-version-modifications-for-content-…

da95adf

…sidebar

michalkowalczyk-box requested a review from jstoffan October 10, 2024 13:42

jstoffan approved these changes Oct 10, 2024

View reviewed changes

Merge branch 'master' into disable-version-modifications-for-content-…

140a7e1

…sidebar

psatala-box approved these changes Oct 11, 2024

View reviewed changes

michalkowalczyk-box added ready-to-merge and removed ready-to-merge labels Oct 11, 2024

rafalmaksymiuk approved these changes Oct 11, 2024

View reviewed changes

mergify bot merged commit e735c4c into box:master Oct 11, 2024
6 checks passed

michalkowalczyk-box deleted the disable-version-modifications-for-content-sidebar branch October 11, 2024 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(content-sidebar): disable version modifications for archive file #3637

feat(content-sidebar): disable version modifications for archive file #3637

michalkowalczyk-box commented Sep 6, 2024 •

edited

Loading

psatala-box Sep 6, 2024

michalkowalczyk-box Sep 6, 2024

psatala-box Sep 6, 2024

michalkowalczyk-box Sep 6, 2024

psatala-box left a comment

psatala-box Sep 6, 2024

psatala-box Sep 6, 2024

michalkowalczyk-box Sep 6, 2024

michalkowalczyk-box Sep 6, 2024

psatala-box Sep 6, 2024

psatala-box Sep 6, 2024

jstoffan Sep 9, 2024

rafalmaksymiuk Oct 3, 2024

michalkowalczyk-box Oct 9, 2024 •

edited

Loading

rafalmaksymiuk Oct 3, 2024

michalkowalczyk-box Oct 9, 2024 •

edited

Loading

michalkowalczyk-box Oct 10, 2024

jstoffan Oct 9, 2024

michalkowalczyk-box Oct 9, 2024

jstoffan Oct 9, 2024

michalkowalczyk-box Oct 9, 2024

jstoffan Oct 9, 2024

michalkowalczyk-box Oct 9, 2024

jstoffan Oct 9, 2024

michalkowalczyk-box Oct 9, 2024

michalkowalczyk-box Oct 9, 2024

jstoffan Oct 9, 2024

michalkowalczyk-box Oct 9, 2024 •

edited

Loading

psatala-box left a comment

	"should show actions correctly when the version's action is $action and unchangeableVersions feature is disabled",
	"should show actions correctly when the version's action is $action and unchangeableVersions feature is enabled",

	test("should show actions correctly when the version's action is $action and unchangeableVersions feature is disabled", () => {
	test("should not show actions when the version's action is delete and unchangeableVersions feature is enabled", () => {

feat(content-sidebar): disable version modifications for archive file #3637

feat(content-sidebar): disable version modifications for archive file #3637

Conversation

michalkowalczyk-box commented Sep 6, 2024 • edited Loading

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

psatala-box 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

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

michalkowalczyk-box Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalkowalczyk-box Oct 9, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalkowalczyk-box Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

psatala-box left a comment

Choose a reason for hiding this comment

michalkowalczyk-box commented Sep 6, 2024 •

edited

Loading

michalkowalczyk-box Oct 9, 2024 •

edited

Loading

michalkowalczyk-box Oct 9, 2024 •

edited

Loading

michalkowalczyk-box Oct 9, 2024 •

edited

Loading