Feature/metadata editing/restrict annotations #464

SeanLeRoy · 2025-03-07T22:44:24Z

Changes

Anya added a ton of metadata editing logic this is the last piece which just adds a password modal auth before the actual edit modal and also restricts some of the annotations by hard coding some locked down ones. Resolves #341 & resolves #343.

Additionally:

I randomly also made a common spinner component as part of this because it was bugging me having to implement it so many times.
Pushed some stuff into common use hooks
Tried to simplify some of the ways props were passed down to reduce render counts and enhance readability
Fixed the way getFiles worked (this is already on main but because this is merged into a different branch it shows as a diff)

Testing

Tested locally against staging and production (staging seems to recently have gotten a data change though making it difficult to test with)

TODO

Talk with users about what other programs we need to add and then add them

…-editing/restrict-annotations

SeanLeRoy · 2025-03-10T18:34:23Z

packages/core/components/Modal/EditMetadata/index.tsx

@@ -18,16 +18,23 @@ import styles from "./EditMetadata.module.css";
 // a more robust solution in the future such as login or a more secure method.
 // or find a way to get this information passed in via GH secrets to the packaged
 // web bundle
+// TODO: Actually lets try to request this from GH or something..?


TODO: Before merging - test putting this into a file we can request from the web or something at least to get it out of this level of hard-coded. (Maybe even labkey...)

The intent is to update these password quarterly, right? So at least extracting them seems smart.

kmitcham · 2025-03-10T23:07:54Z

packages/core/entity/AnnotationFormatter/index.ts

@@ -22,7 +23,7 @@ export const AnnotationTypeIdMap = {
    [AnnotationType.BOOLEAN]: 3,
    [AnnotationType.DATETIME]: 4,
    [AnnotationType.DROPDOWN]: 5,
-    // [AnnotationType.LOOKUP]: 6, // Not currently supported
+    [AnnotationType.LOOKUP]: 6, // Not currently supported


is this comment still valid?

aswallace

I'm only partway through reviewing, just adding comments so far for now

aswallace · 2025-03-10T22:59:05Z

packages/core/components/StatusMessage/index.tsx

@@ -16,6 +9,7 @@ import { interaction } from "../../state";
 import { StatusUpdate, ProcessStatus } from "../../state/interaction/actions";

 import styles from "./StatusMessage.module.css";
+import LoadingIcon from "../Icons/LoadingIcon";


import order thing

aswallace · 2025-03-10T23:04:50Z

packages/core/entity/Annotation/index.ts

@@ -81,6 +82,10 @@ export default class Annotation {
        return this.annotation.isOpenFileLink || false;
    }

+    public get isImmutable(): boolean {


wondering if would benefit from an explanatory comment here (e.g., that this is mainly to prevent editing on certain top level annotations)

aswallace · 2025-03-10T23:09:43Z

packages/core/hooks/useFileAccessContextMenu.ts

@@ -136,7 +136,7 @@ export default (filters?: FileFilter[], onDismiss?: () => void) => {
                        ],
                    },
                },
-                ...(isQueryingAicsFms
+                ...(isQueryingAicsFms && isOnWeb


are we preventing editing on desktop?

Yea I was originally but I think I'll add it back and use LK or something to grab the passwords - I just didn't want the desktop to be a relic holding stale passwords

packages/core/components/StatusMessage/StatusMessage.module.css

aswallace · 2025-03-10T23:38:21Z

packages/core/services/AnnotationService/HttpAnnotationService/immutableAnnotationNames.ts

+    "Cell Line R And D",
+    "Cell Line Source",
+    "Cell Population Id",
+    ,


extra comma?

aswallace · 2025-03-10T23:40:01Z

packages/core/services/FileService/HttpFileService/index.ts

@@ -102,6 +102,9 @@ export default class HttpFileService extends HttpServiceBase implements FileServ
    public async getFiles(request: GetFilesRequest): Promise<FileDetail[]> {
        const { from, limit, fileSet } = request;

+        if (limit - from > 1000) {
+            throw new Error("uhh" + limit + " " + from);


still needed?

aswallace · 2025-03-10T23:41:17Z

packages/core/state/interaction/logics.ts

            filesSelected = await fileSet.fetchFileRange(0, totalFileCount);
        } else {
            filesSelected = await fileSelection.fetchAllDetails();
+            console.log("non-filters", filesSelected.length);


still needed?

BrianWhitneyAI · 2025-03-11T17:47:38Z

packages/core/components/Icons/LoadingIcon.tsx

+    invertColor?: boolean;
+    size?: SpinnerSize;
+
+    "data-testid"?: string;


what is this for?

This is a prop used by the testing components - generally we don't need to explicitly define it like this but in this case we do

BrianWhitneyAI · 2025-03-11T17:49:28Z

packages/core/components/Modal/EditMetadata/PasswordForm.tsx

+        infoMessage = (
+            <p className={styles.errorMessage}>
+                The files you have selected do not share the same program or do not have a program
+                at all.


"program at all" feels a little weird. maybe "program metadata tag"

aswallace

Looks good!! I couldn't fully test locally but tried out the pre-submit behavior and things work as expected

aswallace · 2025-03-11T22:52:08Z

packages/core/components/EditMetadata/ExistingAnnotationPathway.tsx

+        key: annotation.name,
+        text: annotation.displayName,
+        disabled: annotation.isImmutable,
+        title: annotation.isImmutable


I'm not seeing this tooltip show up for some reason

aswallace · 2025-03-11T22:53:13Z

packages/core/components/EditMetadata/ExistingAnnotationPathway.tsx

+        text: annotation.displayName,
+        disabled: annotation.isImmutable,
+        title: annotation.isImmutable
+            ? "This field cannot be edited because it is automatically created based on some heuristic"


unsure if wording of "based on some heuristic" is clear but I don't have a suggested alternative

aswallace · 2025-03-11T23:15:11Z

packages/core/components/EditMetadata/ExistingAnnotationPathway.tsx

+        // If it's a dropdown, we need to fetch the dropdown options
+        if (option?.data === AnnotationType.STRING) {
+            annotationService
+                .fetchAnnotationDetails(selectedFieldName)


Is this likely to ever take long (i.e., any chance we'd need a loading indicator on the UI side?)

aswallace · 2025-03-11T23:31:46Z

packages/core/components/Modal/EditMetadata/PasswordForm.module.css

@@ -0,0 +1,79 @@
+.error-message {
+    background-color: rgba(194, 48, 48, 0.5);


there are also the global variables --error-status-background-color and --error-status-text-color which are what we use for backgrounds/borders in the status messages

aswallace · 2025-03-11T23:33:24Z

packages/core/components/Modal/EditMetadata/PasswordForm.tsx

+            <>
+                <p>
+                    A password is required for editing or creating new metadata. Each password is
+                    specific to a program, only files associated with that program can be edited.


tiny nit: program; instead of program,
also in line 65

SeanLeRoy and others added 13 commits February 2, 2025 15:32

Add password form

81a3a18

Simplify fetch for file details

e9d980c

Clean up; Use hook for grabbing selection

4612a91

Simplify data flow

6913786

Add common loading icon styled correctly

b6821a5

Add dropdown and lookup as viable annotation types

0b746e8

Use loading icon

17a3f41

Bury details of immutability in backend specific context

867b21d

Expose error from fetching details

458fe62

Use the same component for loading icons

f292be9

WIP

5661047

Merge branch 'feature/metadata-editing/develop' into feature/metadata…

8e654df

…-editing/restrict-annotations

Merge fixes

36045a5

SeanLeRoy requested review from aswallace and BrianWhitneyAI March 7, 2025 22:44

SeanLeRoy self-assigned this Mar 7, 2025

This was linked to issues Mar 7, 2025

Restrict editable annotations #343

Open

Enable metadata editing for internal data sources #96

Open

SeanLeRoy removed a link to an issue Mar 7, 2025

Enable metadata editing for internal data sources #96

Open

SeanLeRoy requested a review from kmitcham March 7, 2025 22:48

This was linked to issues Mar 7, 2025

Add password logic to edit modal #341

Open

Enable metadata editing for internal data sources #96

Open

Add remaining programs

0382131

SeanLeRoy commented Mar 10, 2025

View reviewed changes

kmitcham reviewed Mar 10, 2025

View reviewed changes

aswallace reviewed Mar 10, 2025

View reviewed changes

BrianWhitneyAI approved these changes Mar 11, 2025

View reviewed changes

aswallace approved these changes Mar 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/metadata editing/restrict annotations #464

Feature/metadata editing/restrict annotations #464

SeanLeRoy commented Mar 7, 2025 •

edited

Loading

SeanLeRoy Mar 10, 2025 •

edited

Loading

kmitcham Mar 10, 2025

kmitcham Mar 10, 2025

aswallace left a comment

aswallace Mar 10, 2025

aswallace Mar 10, 2025

aswallace Mar 10, 2025

SeanLeRoy Mar 10, 2025

aswallace Mar 10, 2025

aswallace Mar 10, 2025

SeanLeRoy Mar 10, 2025

aswallace Mar 10, 2025

BrianWhitneyAI Mar 11, 2025

SeanLeRoy Mar 11, 2025

BrianWhitneyAI Mar 11, 2025

aswallace left a comment

aswallace Mar 11, 2025

aswallace Mar 11, 2025

aswallace Mar 11, 2025

aswallace Mar 11, 2025

aswallace Mar 11, 2025

		@@ -0,0 +1,79 @@
		.error-message {
		background-color: rgba(194, 48, 48, 0.5);

Feature/metadata editing/restrict annotations #464

Are you sure you want to change the base?

Feature/metadata editing/restrict annotations #464

Conversation

SeanLeRoy commented Mar 7, 2025 • edited Loading

Changes

Testing

TODO

SeanLeRoy Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aswallace 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aswallace 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

SeanLeRoy commented Mar 7, 2025 •

edited

Loading

SeanLeRoy Mar 10, 2025 •

edited

Loading