Merge UI fixes and metadata standardizer #365

sanghoonio · 2024-08-05T21:15:32Z

Aside from some UI updates, I found that the patch update project API endpoint doesn't seem to let you remove schemas from projects. I disabled clearing from the schema dropdown for now so that you can't choose no schema when updating. What should be the proper formatting for pep_schema in the request body when the user is trying to remove current schema? It was set to empty string (""), which didn't work

…s from dropdown because it doesn

nleroy917 · 2024-08-06T13:47:52Z

Could we try null (client-side) or None (server-side)? Maybe undefined?

if (e.target.value === "") {
  setSchema(null) // or undefined
}

nleroy917 · 2024-08-06T14:42:17Z

Another idea is to check (server-side) if the pep_schema has been assigned to an empty string ("") and then appropriately set it to null or None

sanghoonio · 2024-08-06T14:52:34Z

I think it might have to be server side; I can set the onChange in schemas-databio-dropdown.tsx from

onChange={(newValue: SingleValue<{ label: string; value: string }>) => {
  onChange(newValue?.value || '');
}}

to

onChange={(newValue: SingleValue<{ label: string; value: string }>) => {
  onChange(newValue?.value || null ***or undefined***);
}}

and it still doesn't work. It doesn't error out, but the schema itself doesn't get dropped from the pep

nleroy917 · 2024-08-06T14:55:54Z

Yeah, I think somewhere it ignores undefined values. It's an all-encompassing function that updates whatever you give it...and doesn't try to update what you don't. So if you give it schema: undefined it will just be like, "oh cool you don't want to update the schema right now".

That's why I think that passing null or even the empty string is a good idea such that we can detect that on the server and handle it appropriately.

I might ask @khoroshevskyi what the best approach is here from a database perspective, but

nleroy917

This is AWESOME! Great job. I havnt pulled it locally so I haven't checked types... (it wont build unless types are correct), but yeah this is so great.

The endpoint could be a little more robust and I think my suggestions will get us 90% of the way there
Lets see what @saanikat says about AttrStandardizer
And make sure the types are all good

But this is awesome! Thanks for getting this done well

pephub/routers/api/v1/project.py

nleroy917 · 2024-08-19T12:54:13Z

pephub/routers/api/v1/project.py

+
+    model = AttrStandardizer(schema)
+
+    results = model.standardize(pep=path)


This seems a little inefficient, no? We are already on PEPhub, but it seems like the attribute standardized is getting it... from PEPhub? But we are already there so its making a request to itself.

This is probably an implementation detail in AttrStandardizer... so we should raise an issue there, but I really feel like we should be able to just pass a peppy.Project object to it directly

opened: databio/bedms#16

pephub/routers/api/v1/project.py

web/src/components/modals/standardize-metadata.tsx

nleroy917 · 2024-08-19T13:03:55Z

web/src/components/modals/standardize-metadata.tsx

+    if (data?.results) {
+      const defaultSelections = getDefaultSelections(data.results);
+      setWhereDuplicates(checkForDuplicates(defaultSelections));
+      console.log('Setting default selections:', defaultSelections);


nleroy917 · 2024-08-19T13:09:17Z

web/src/components/modals/standardize-metadata.tsx

+    return [[selectedValue], ...topValues, ...emptyRows];
+  }, [selectedValues, tabData]);
+
+  useEffect(() => {


I'm trying to think of ways to get rid of this useEffect... I might need to pull the code and look at it in detail

nleroy917 · 2024-08-19T13:14:31Z

web/src/hooks/queries/useStandardize.ts

+) => {
+  const session = useSession();
+  const query = useQuery({
+    queryKey: [namespace, project, tag],


Suggested change

queryKey: [namespace, project, tag],

queryKey: ['standardize', schema, namespace, project, tag],

Can we change the key for this so it gets cached properly?

nleroy917 · 2024-08-19T13:14:58Z

web/src/hooks/stores/useStandardizeModalStore.ts

+import { create } from 'zustand';
+
+type StandardizeModalStore = {
+  showStandardizeMetadataModal: boolean;
+  setShowStandardizeMetadataModal: (show: boolean) => void;
+};
+
+export const useStandardizeModalStore = create<StandardizeModalStore>((set) => ({
+  showStandardizeMetadataModal: false,
+  setShowStandardizeMetadataModal: (show) => set({ showStandardizeMetadataModal: show }),
+}));


Nice love this

khoroshevskyi

Just one big thing, that Nathan mentioned, don't call pephub from pephub api. Other things look good

khoroshevskyi · 2024-08-19T14:45:00Z

pephub/routers/api/v1/project.py

+
+    model = AttrStandardizer(schema)
+
+    results = model.standardize(pep=path)


pephub/routers/api/v1/project.py

…ore saving

sanghoonio · 2024-09-03T18:18:31Z

Can we merge this to dev today? I want to merge it all before adding the updated geofetch UI and forking from history features

nleroy917 · 2024-09-03T18:19:15Z

I can get a review by EOD

nleroy917

Ok, nothing struck as particularly problematic. I can pull it locally and check it out too and my environment will probably identify things too..

Really the biggest comments are those made in the python code on the server.

nleroy917 · 2024-09-05T11:59:55Z

pephub/routers/api/v1/project.py

 )
 from .helpers import verify_updated_project

+from attribute_standardizer.attr_standardizer_class import AttrStandardizer


I know this isnt pephub but I still wanna say I don't like the ergonomics of this import 🙃

I changed a little bit bedbs, so now you can do: from attribute_standardizer import AttrStandardizer

Can you push to bedms?

It is already there, I added it with input changes

nleroy917 · 2024-09-05T12:03:57Z

pephub/routers/api/v1/project.py

+    if schema == "":
+        raise HTTPException(
+            code=400,
+            detail="Schema is required! Available schemas are ENCODE and Fairtracks",
+        )
+        return {}


You can remove the return {} here since it's unreachable.

Also, this could be just a tad more robust with this kind of setup:

# const.py VALID_STANDARDIZE_SCHEMAS = ["ENCODE", "Fairtracks"] # route.py if schema not in VALID_STANDARDIZE_SCHEMAS: raise HTTPException( code=400, detail=f"Invalid schema provided: {schema}. Valid schemas are: {VALID_STANDARDIZE_SCHEMAS}" )

I think this variable (VALID_STANDARDIZE_SCHEMAS) should be imported from bedms, as we will be adding new models there

I think Saanika is adding a function to bedms that lists the existing schemas

nleroy917 · 2024-09-05T12:05:03Z

pephub/routers/api/v1/project.py

+    except Exception:
+        raise HTTPException(
+            code=400,
+            detail=f"Error standardizing PEP.",
+        )


This is probably fine for now, but we'd ideally want to capture specific Exceptions and dispatch accordingly. We might be sending a 400 back indicating a user error, when in fact it could be our fault and a 500 error (developer error)

Updated code to 500 for now

nleroy917 · 2024-09-05T12:06:29Z

pephub/routers/api/v1/project.py

+    response_model=StandardizerResponse,
+)
+async def get_standardized_cols(
+    pep: project = Depends(get_project),


What is project here?

pep: project = Depends(get_project),

Why is Project not capitalized? Should we by type annotating that as a peppy.Project object?

yeah, it should be peppy.Project

nleroy917 · 2024-09-05T12:08:23Z

web/src/api/project.ts

+) => {
+  const url = `${API_BASE}/projects/${namespace}/${name}/standardize?schema=${schema}&tag=${tag}`;
+  return axios
+    .post<StandardizeColsResponse>(url, { headers: { Authorization: `Bearer ${jwt}` } })


Suggested change

.post<StandardizeColsResponse>(url, { headers: { Authorization: `Bearer ${jwt}` } })

.post<StandardizeColsResponse>(url, { headers: { Authorization: `Bearer ${jwt || 'NO_AUTHORIZATION'}` } })

nleroy917 · 2024-09-05T12:10:17Z

web/src/components/forms/upload-schema-form.tsx

+type CombinedErrorMessageProps = {
+  errors: FieldErrors<POPInputs>;
+};
+
+const CombinedErrorMessage = (props: CombinedErrorMessageProps) => {
+  const { errors } = props;
+  const nameError = errors.name?.message;
+  let msg = null;
+
+  if (nameError == 'empty') {
+    msg = 'Project Name must not be empty.';
+  } else if (nameError == 'invalid') {
+    msg = "Project Name must contain only alphanumeric characters, '-', or '_'.";
+  }
+
+  if (nameError) {
+    return <p className="text-danger text-xs pt-1 mb-0">{msg}</p>;
+  }
+
+  return null;
+};
+


Second time I have seen this.... should we abstract it into its own component that can be imported?

nleroy917 · 2024-09-05T12:12:45Z

web/src/components/modals/standardize-metadata.tsx

+  setNewSamples: (samples: any[][]) => void;
+  resetStandardizedData: boolean;
+  setResetStandardizedData: (boolean) => void;
+


Remove space? 😀

If you havnt yet... getting prettier setup is such a blessing. It auto-formats the markup for you and its a great great great life-enhancer

nleroy917 · 2024-09-05T12:24:37Z

web/src/globals.css

@@ -688,3 +697,176 @@ body {
 .btn-dark.glow-button {
  --box-shadow-color: #343a4080;
 }
+
+.btn-group-vertical .btn-check:checked + .btn-outline-secondary {


No comments on the CSS, but you strike me as someone who would love tailwind...

https://tailwindcss.com/

sanghoonio added 3 commits August 5, 2024 13:56

deleting view that is selected now unselects the view selector

db93de2

rounded corner in schema select dropdown

057803a

added modal for no schema yellow button and prevented clearing schema…

7905ce6

…s from dropdown because it doesn

sanghoonio requested a review from khoroshevskyi August 5, 2024 21:16

missed closing div

15d7b14

sanghoonio and others added 8 commits August 12, 2024 17:12

metadata standardizer modal

25b6fe4

standardizer ui kinda works

9ecb06e

some changes to modal

fe4bbc3

more standardizer ui progress

84b4dd6

progress on standardizer api, trouble using hook to refetch it

862f891

small tweaks

a69d512

metadata standardizer api integration

3030166

black

eaca413

sanghoonio changed the title ~~Merge UI fixes for schemas, views~~ Merge UI fixes and metadata standardizer Aug 16, 2024

sanghoonio requested a review from nleroy917 August 16, 2024 20:59

nleroy917 requested changes Aug 19, 2024

View reviewed changes

khoroshevskyi requested changes Aug 19, 2024

View reviewed changes

sanghoonio added 9 commits August 19, 2024 12:15

fix type errors

b0c4273

api updates

269f172

black

720709e

fixed bugs caused by trying to standardize columns multiple times bef…

126ce7e

…ore saving

address #368, #369, #370, #374

5a7739c

fixed namespace bug and made ph_id read only #364

d11283c

add feedback and bug links

d1d57d4

update some colors and shadows

373e763

more colors

4a40bee

sanghoonio and others added 9 commits August 28, 2024 16:38

ui fixes

0812106

fix type errors

adab06a

view modal ui and some color changes

c2a1b5b

modal ui update

476b0c8

standardizer api debug

b985284

push api

b713d21

Fixed prj object in standardizer

637e2db

reset standardizer modal on save and fix namespace colors

4acb645

remove printlines

44a537f

sanghoonio requested review from nleroy917 and khoroshevskyi September 3, 2024 18:18

sanghoonio added 14 commits September 3, 2024 14:24

rearrange fork modal helper text

0283627

namespace ui updates

baee6da

namespace consistency

c47e93f

add pep modal ui update

b009019

update fork pep modal ui

df45f4a

ui polish

1e1f8e6

css trickery

2e612ed

missed button alignment in add pep modal

dee5419

border radius

c23d532

logo resize

1a351da

class changes

04f61b9

edit metadata modal update

fcadcc4

fixed namespace, name, and tag input checks not working in modals

36573cb

fix sampletable whitespace causing issues

de41801

nleroy917 approved these changes Sep 5, 2024

View reviewed changes

address pr comments

cdd444b

sanghoonio merged commit 015f4e0 into dev Sep 5, 2024
1 check passed


		model = AttrStandardizer(schema)

		results = model.standardize(pep=path)

	queryKey: [namespace, project, tag],
	queryKey: ['standardize', schema, namespace, project, tag],

	.post<StandardizeColsResponse>(url, { headers: { Authorization: `Bearer ${jwt}` } })
	.post<StandardizeColsResponse>(url, { headers: { Authorization: `Bearer ${jwt \|\| 'NO_AUTHORIZATION'}` } })

Merge UI fixes and metadata standardizer #365

Merge UI fixes and metadata standardizer #365

Conversation

sanghoonio commented Aug 5, 2024

nleroy917 commented Aug 6, 2024 • edited Loading

nleroy917 commented Aug 6, 2024

sanghoonio commented Aug 6, 2024

nleroy917 commented Aug 6, 2024

nleroy917 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

khoroshevskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanghoonio commented Sep 3, 2024

nleroy917 commented Sep 3, 2024

nleroy917 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

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

nleroy917 commented Aug 6, 2024 •

edited

Loading