fix(metadata): Disable updates to metadata #3376

joshmarnold · 2023-07-25T14:42:57Z

Summary

There is an option in the Admin Console that allows an admin to restrict which users have the ability to create cascading metadata. At some point in the not-so-distant past (sometime in the last 6-12 months as of July 2023) the feature was broken. Users who previously did not have the ability to create cascading metadata could now do so.

Background/History

When folder level metadata was introduced, it seems like it was intended to be synonymous with cascading metadata. That is not the case today. It’s true that most folder level metadata is cascaded, but it’s not the only use case.

It also appears that with the way the feature was built, it not only restricted cascading metadata, but restricted ALL metadata adds/edits to folders. I believe this was a side effect of the previously mentioned belief that folder = cascade.

An important note to keep in mind with all of this is that the feature was built at the UI layer. Meaning, any user with edit capabilities, irrespective of the Admin Console option, could add/edit/delete folder level metadata or apply/remove cascading metadata via the API. In simpler terms, we put a roadblock in place, but this was NOT a security control.

The intent behind this feature is to allow web users to be editors of folders but not allow them to add cascading metadata to those folders.

Solution Requirements

There are 4 options an admin can select for this feature. Those are: “Enable for all managed users”, “Disable for all managed users”, “Enable for select users”, and “Enable for everyone except select users”. To make things simple, users in the following requirements will be denoted as “allowed users” or “disallowed users” regardless of how the status was obtained. We’ll also assume that all users have Editor permissions to the folder in question.

Scenario	Can View Metadata on a folder	Can apply a Metadata template to a folder*	Can edit/delete Metadata on a folder*	Can enable/disable Cascade Metadata	Can see if folder* Metadata is being cascaded
Allowed user - No Metadata on folder	YES	YES	YES	n/a	n/a
Allowed user - Metadata on folder (no cascade)	YES	YES	YES	YES	YES
Allowed user - Metadata on folder with cascade	YES	YES	YES	YES	YES
Disallowed user - No Metadata on folder	YES	YES	YES	n/a	n/a
Disallowed user - Metadata on folder (no cascade)	YES	YES	YES	NO	YES
Disallowed user - Metadata on folder with cascade	YES	YES	NO	NO	YES

JChan106 · 2023-07-25T22:00:13Z

src/features/metadata-instance-editor/Instance.js

@@ -569,6 +571,7 @@ class Instance extends React.PureComponent<Props, State> {
                        data-resin-target="metadata-instanceedit"
                        onClick={this.toggleIsEditing}
                        type="button"
+                        data-testid="metadata-instance-edit-button"


RTL doesn't recommend using extra test attributes unless necessary, do we need to add a testid to this button? Are we able to find the IconEdit and click that instead?

Now using the aria-label

src/features/metadata-instance-editor/Instance.js

tjuanitas

have changes been tested with ContentSidebar?
https://github.com/box/box-ui-elements/blob/master/src/elements/content-sidebar/MetadataSidebar.js

src/features/metadata-instance-editor/Instance.js

joshmarnold · 2023-07-28T13:05:49Z

have changes been tested with ContentSidebar? https://github.com/box/box-ui-elements/blob/master/src/elements/content-sidebar/MetadataSidebar.js

@tjuanitas I was not aware of MetadataSidebar and I haven't had the opportunity to look into it. I don't see its use in EUA to try to test it out. As of now, I can only report that the tests are passing, which is good. It seems similar to MetadataInstanceEditor in that they both render the Instances component. However, there is a difference, MetadataSidebar is not utilizing the isCascadingPolicyApplicable property, which would have rendered it incompatible with cascading policies. Due to my lack of knowledge about the intended purpose of the MetadataSidebar, I cannot definitively state what the expected behavior should be or whether any updates are necessary.

tjuanitas

I think you might be able to achieve the desired behavior in EUA using the editors prop that's being passed into MetadataInstanceEditor

for example, if the instance in the map has template.templateKey === TEMPLATE_CUSTOM_PROPERTIES, then you would omit the cascadePolicy property since that's not applied for custom instances. similarly, you could set the canEdit property based on isFolderMetadataCascadeEnabled / the split treatment.

this way when the values get to this Instance.js component, the props more accurately represent the desired behavior. e.g. the canEdit prop and the method this.canEdit should for the most part return the same value

src/features/metadata-instance-editor/Instance.js

TylerGauntlett · 2023-08-16T21:41:20Z

src/features/metadata-instance-editor/Instance.js

@@ -52,8 +52,9 @@ type Props = {
    hasError: boolean,
    id: string,
    intl: Object,
-    isCascadingPolicyApplicable?: boolean,
+    isCascadingPolicyApplicable: boolean,


I this intentional? metadata.js is still using isCascadingPolicyApplicable?: boolean,

TylerGauntlett · 2023-08-16T21:47:57Z

src/features/metadata-instance-editor/Instance.js

@@ -590,8 +581,7 @@ class Instance extends React.PureComponent<Props, State> {
            shouldShowCascadeOptions,
            isCascadingOverwritten,
        }: State = this.state;
-        const isProperties = template.templateKey === TEMPLATE_CUSTOM_PROPERTIES;
-        const isEditing = this.isEditing();


I would imagine the performance impact is minor but consider that you are now calling isEditing() n times instead of storing the result

agree, I think the original is cleaner too

tjuanitas

changes were confirmed to work with File metadata in the sidebar right?

overall implementation makes sense but left minor comments

src/features/metadata-instance-editor/__tests__/Instance.test.js

src/features/metadata-instance-editor/Instance.js

tjuanitas · 2023-08-17T20:25:39Z

src/features/metadata-instance-editor/Instance.js

@@ -590,8 +581,7 @@ class Instance extends React.PureComponent<Props, State> {
            shouldShowCascadeOptions,
            isCascadingOverwritten,
        }: State = this.state;
-        const isProperties = template.templateKey === TEMPLATE_CUSTOM_PROPERTIES;
-        const isEditing = this.isEditing();


agree, I think the original is cleaner too

tjuanitas · 2023-08-17T20:27:06Z

src/features/metadata-instance-editor/Instance.js

+
+        const cascadePolicyData = {
+            ...cascadePolicy,
+            isEnabled: isFolderInstance ? isCascadingEnabled : false,
+            overwrite: isCascadingOverwritten,
+        };
+
+        const clonedCurrentData = cloneDeep(currentData);
+        const JSONPatch = this.createJSONPatch(clonedCurrentData, originalData);
+        onSave(id, JSONPatch, cascadePolicyData, clonedCurrentData);


little confused on why this change is needed. in the original implementation, it was only passing cascadePolicyData when the policy was applicable. but now it's passing cascadePolicy in all cases?

Relevant EUW service: https://git.dev.box.net/AppServices/end-user-web/blob/25e47e8a54c6a19f666d15e3ddf3ac09c158807a/src/server/controllers/metadata/saveInstances.js

please reference this as I think it will help understand what's going on here

src/features/metadata-instance-editor/Instance.js

tjuanitas · 2023-08-17T20:32:38Z

src/features/metadata-instance-editor/__tests__/Instance.test.js

+                cascadePolicy={{
+                    id: 'hello',
+                }}
+                isCascadingPolicyApplicable


i see isFolderInstance is not added here, does the current implementation allow for the isCascadingPolicyApplicable and isFolderInstance props to work independently of each other? is that okay?

They're testing that the footer renders properly. Whether or not the footer renders doesn't depend on these variables.

tjuanitas · 2023-08-18T01:58:37Z

I also want to make sure the cases here are considered in the testing: https://opensource.box.com/box-ui-elements/#/Features?id=metadatainstanceeditor

i.e. showing the correct error states or messages

joshmarnold · 2023-08-24T14:50:43Z

Found a way to do this without changes to BUIE.

joshmarnold requested a review from a team as a code owner July 25, 2023 14:42

JChan106 reviewed Jul 25, 2023

View reviewed changes

joshmarnold requested a review from a team as a code owner July 26, 2023 14:32

joshmarnold requested a review from JChan106 July 26, 2023 14:35

tjuanitas reviewed Jul 26, 2023

View reviewed changes

joshmarnold requested a review from tjuanitas July 27, 2023 18:16

tjuanitas reviewed Jul 27, 2023

View reviewed changes

src/features/metadata-instance-editor/Instance.js Outdated Show resolved Hide resolved

tjuanitas reviewed Jul 28, 2023

View reviewed changes

src/features/metadata-instance-editor/Instance.js Outdated Show resolved Hide resolved

src/features/metadata-instance-editor/Instance.js Show resolved Hide resolved

src/features/metadata-instance-editor/Instance.js Outdated Show resolved Hide resolved

joshmarnold requested a review from tjuanitas July 28, 2023 13:32

tjuanitas reviewed Aug 4, 2023

View reviewed changes

src/features/metadata-instance-editor/Instance.js Outdated Show resolved Hide resolved

src/features/metadata-instance-editor/Instance.js Outdated Show resolved Hide resolved

src/features/metadata-instance-editor/Instance.js Outdated Show resolved Hide resolved

TylerGauntlett reviewed Aug 16, 2023

View reviewed changes

tjuanitas reviewed Aug 17, 2023

View reviewed changes

joshmarnold added 17 commits August 17, 2023 16:44

fix(metadata): Disable updates to metadata

54b4e38

fix(metadata): Disable updates to metadata

853d4d1

fix(metadata): Disable updates to metadata

b3bf5ad

fix(metadata): Disable updates to metadata

66ced6d

fix(metadata): Disable updates to metadata

bd22736

fix(metadata): Disable updates to metadata

b00d3e0

fix(metadata): Disable updates to metadata

ca8e8fc

fix(metadata): Disable updates to metadata

255ebf5

fix(metadata): Disable updates to metadata

3876b2e

fix(metadata): Disable updates to metadata

ec98e5a

fix(metadata): Disable updates to metadata

ae1efb8

fix(metadata): Disable updates to metadata

91062a8

fix(metadata): Disable updates to metadata

4a6a954

fix(metadata): Disable updates to metadata

e56de3b

fix(metadata): Disable updates to metadata

3c06a6a

fix(metadata): Disable updates to metadata

9d109ee

fix(metadata): Disable updates to metadata

496f857

joshmarnold added 2 commits August 17, 2023 16:44

fix(metadata): Disable updates to metadata

8166c3a

fix(metadata): Disable updates to metadata

8537652

joshmarnold and others added 3 commits August 21, 2023 10:49

Merge branch 'master' into WEBAPP-17450

1241fe7

fix(metadata): Disable updates to metadata

c826ee1

fix(metadata): Disable updates to metadata

f71d399

joshmarnold requested a review from tjuanitas August 21, 2023 17:00

joshmarnold and others added 5 commits August 22, 2023 11:28

fix(metadata): Disable updates to metadata

c55f430

fix(metadata): Disable updates to metadata

d1a4709

fix(metadata): Disable updates to metadata

b831683

Merge branch 'master' into WEBAPP-17450

8d8cbb7

fix(metadata): Disable updates to metadata

85586c4

joshmarnold closed this Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(metadata): Disable updates to metadata #3376

fix(metadata): Disable updates to metadata #3376

joshmarnold commented Jul 25, 2023 •

edited

Loading

JChan106 Jul 25, 2023

joshmarnold Jul 26, 2023

tjuanitas left a comment

joshmarnold commented Jul 28, 2023 •

edited

Loading

tjuanitas left a comment

TylerGauntlett Aug 16, 2023

TylerGauntlett Aug 16, 2023

tjuanitas Aug 17, 2023

tjuanitas left a comment

tjuanitas Aug 17, 2023

tjuanitas Aug 17, 2023

joshmarnold Aug 22, 2023

tjuanitas Aug 17, 2023

joshmarnold Aug 22, 2023

tjuanitas commented Aug 18, 2023

joshmarnold commented Aug 24, 2023

fix(metadata): Disable updates to metadata #3376

fix(metadata): Disable updates to metadata #3376

Conversation

joshmarnold commented Jul 25, 2023 • edited Loading

Summary

Background/History

Solution Requirements

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

joshmarnold commented Jul 28, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjuanitas commented Aug 18, 2023

joshmarnold commented Aug 24, 2023

joshmarnold commented Jul 25, 2023 •

edited

Loading

joshmarnold commented Jul 28, 2023 •

edited

Loading