GS Onboarding (backend and frontend) by Jiatao7 · Pull Request #10 · UWOrbital/GS-Onboarding

Jiatao7 · 2025-01-26T03:49:57Z

No description provided.

Yarik-Popov

Overall good start on the backend. Ill review the frontend later. Some changes are needed

Yarik-Popov · 2025-01-26T15:41:14Z

backend/api/endpoints/command.py

+    num_commands = len(items)
+
+    command = Command (
+        id=1+num_commands,


You dont need this as its error prone. Let the orm handle the auto increment

Yarik-Popov · 2025-01-26T15:42:38Z

backend/api/endpoints/command.py

+    try:
+        query = select(Command).where(Command.id == id)
+        command = db.exec(query).one()
+        db.delete(command)
+        db.commit()
+
+        query2 = select(Command).where(Command.id != id)
+        items = db.exec(query2).all()
+        return {"data": items}
+    except:
+        raise HTTPException(status_code=404, detail="Item does not exist")


The orm will not raise an exception if an item doesnt exist. It will return None and you should check against it

Yarik-Popov · 2025-01-26T15:42:58Z

backend/api/middlewares/logger_middleware.py

 from fastapi import Request, Response
 from starlette.middleware.base import BaseHTTPMiddleware

+from ...utils.logging import logger


Use absolute imports

Yarik-Popov · 2025-01-26T15:44:20Z

backend/data/data_models.py

        # TODO: (Member) Implement this method
-        return self
-
+        if (self.params == None and self.format == None):


Dont compare using == against None use is None instead. Also you dont need the extra () for if and elif, while statements

Yarik-Popov · 2025-01-26T15:45:34Z

backend/data/data_models.py

+        if (self.params == None and self.format == None):
+            return self
+        elif (self.params == None or self.format == None):
+            raise(ValueError)


The proper syntax is raise ValueError() as your case throws the class and not the instance.

Yarik-Popov · 2025-01-26T15:45:42Z

backend/data/data_models.py

+        elif (len(self.params.split(",")) == len(self.format.split(","))):
+            return self
+        else:
+            raise(ValueError)


Same thing here

frontend/src/data/request.ts

Yarik-Popov · 2025-01-26T23:56:52Z

test/backend/test_api.py

        # TODO: Figure out a better way to check the times
-        assert result.get("created_on") 
-        assert result.get("updated_on")
+        created_on_dt = datetime.fromisoformat(result.get("created_on"))
+        updated_on_dt = datetime.fromisoformat(result.get("updated_on"))
+        assert isinstance(created_on_dt, datetime)
+        assert isinstance(updated_on_dt, datetime)


Thanks. Not a part of the onboarding but definitely something that can be improved. Ill link this in the task to improve the gs onboarding.

Yarik-Popov

Good step in the right direction. Looks like I forgot to submit my requested changes for the frontend earlier. Sorry about that but all the requested changes are here for both the frontend and backend.

Yarik-Popov · 2025-01-28T00:18:42Z

frontend/src/input/command_input.tsx

+    let paramsObject : { [key: string]: string } = {}
+    for(const param of parameters) {
+      paramsObject[param] = ""
+    }


Theres a bug if you input data into a text field then switch the type. The text field still has the data displayed but then you click submit it says missing parameter. It might be due to this and the fact that the input does have a value property

Yarik-Popov · 2025-01-28T00:19:45Z

frontend/src/input/command_input.tsx

+
+           {/* TODO: (Member) Add input handling here if the selected command has a param input*/}
+          {commandType && 
+            commandType.params?.split(",").map((param) => (<input id={param} placeholder={`Enter ${param}`} onChange={changeParam}/>))}


This should have a value property that is an individual param. It should fix the bug i described above

Yarik-Popov · 2025-01-28T00:20:42Z

frontend/src/input/command_input.tsx

+    setParams(paramsObject);
+  }
+
+  const changeParam = (e: any) => {


Use a specific type here insteas of any or if you dont know the type use unknown

Yarik-Popov · 2025-01-28T00:21:54Z

frontend/src/input/command_input.tsx

+  //console.log(`Command: `, commandType)
+  //console.log(`Parameters: `, params)
+
+  const changeCommandType = (e: any) => {


Use a specific type here instead of any. Or if you dont know the type, use unknown as it is safer

Yarik-Popov · 2025-01-28T00:22:43Z

frontend/src/input/command_input.tsx

+  /*
+          <input value={params} onChange={(e) => setParams(e.target.value)}/>
+          {// Use parameters of command type to output input boxes}
+          { .params.map(param => (<input id={} value={}>{cmd.name}</option>))}
+  */
+  //console.log(`Command: `, commandType)
+  //console.log(`Parameters: `, params)


Do not leave code commented out. If its not needed remove it. This also applies to line 52

Yarik-Popov · 2025-02-01T22:39:12Z

backend/data/data_models.py

+        elif len(self.params.split(",")) == len(self.format.split(",")):
+            return self
+        else:
+            raise ValueError()


Add a descriptive error message

frontend/src/input/command_input.tsx

Yarik-Popov · 2025-02-01T22:44:20Z

frontend/src/input/command_input.tsx

+
+      setCommandType(data.data[0])
+
+      const parameters = commandType?.params?.split(",") || []


commandType will be null on the first render but not sure if it matters here

Yarik-Popov · 2025-02-01T22:45:02Z

frontend/src/input/command_input.tsx

  // TODO: (Member) Setup state and useEffect calls here
+  const [mainCommands, setMainCommands] = useState<MainCommandResponse[]>([])
+  const [commandType, setCommandType] = useState<MainCommandResponse | null>(null)
+  const [params, setParams] = useState<{[key: string] : string}>({})


Probably use a custom type instead of {[key: string]: string} maybe look into StringMap

Yarik-Popov · 2025-02-01T22:46:58Z

frontend/src/input/command_input.tsx

+    const paramsList = []
+    for (const param of allParams) {
+      if (!params[param]) {
+        alert("Parameter missing")


From a UX perspective probably best to indicate which parameter is missing

Yarik-Popov

Backend lgtm. Some changes are needed on the frontend.

Yarik-Popov · 2025-02-03T01:31:01Z

frontend/src/input/command_input.tsx

+      if(!data.data) {
+        alert("Error occured. Please try again later.")
+      }


This code will never run because an empty array in Typescript is truthy. So !data.data will always be false. Also add a return at the end of the if block. You should be comparing against the length

Yarik-Popov · 2025-02-03T01:41:21Z

frontend/src/input/command_input.tsx

+    const allParams = commandType?.params?.split(",") || [];
+    const paramsList = []
+
+    let sendAlert = false


The sendAlert variable is redundant you can just use the length of the missingParams array to check if there is something missing

Yarik-Popov · 2025-02-03T01:49:06Z

frontend/src/input/command_input.tsx

 const CommandInput = () => {
  // TODO: (Member) Setup state and useEffect calls here
+  const [mainCommands, setMainCommands] = useState<MainCommandResponse[]>([])
+  const [commandType, setCommandType] = useState<MainCommandResponse>({id: 0, name: "", params: null, format: null, data_size: 0, total_size: 0})


I think having null as the default value and checking against it in the code would probably be better than having a default object.

Yarik-Popov · 2025-02-03T01:50:19Z

frontend/src/input/command_input.tsx

+  }
+
+  const changeCommandType = (e: React.ChangeEvent<HTMLSelectElement>) => {
+    const type : MainCommandResponse = mainCommands.find(cmd => cmd.id == +e.target.value) || {id: 0, name: "", params: null, format: null, data_size: 0, total_size: 0}


Just let type be undefined and show an alert that an error occurred and return from the function. Also, probably not a good idea to use type as a variable name as it is a soft keyword in Typescript

Yarik-Popov

Do not use loose equals. Should be last set of changes

Yarik-Popov · 2025-02-04T00:42:03Z

frontend/src/input/command_input.tsx

+  useEffect(() => {
+    const setMainCommandsFn = async () => {
+      const data = await getMainCommands()
+      if(data.data.length == 0) {


Do not use double equals == in comparisons. Use ===

Because == is loosely equal. Example:
0 == [] // true
0 == '0' //true
But:
[] == '0' // false

Here's even more cursed things about javascript:
0 < null // false
0 > null // false
0 == null // false
But
0 <= null // true
0 >= null // true
But if you were to replace all of the null above with undefined, they would all be false

You can disregard the last 2 comments in the thread, they are just me showing you some trivia about js and why we use Typescript

Yarik-Popov · 2025-02-04T00:42:24Z

frontend/src/input/command_input.tsx

+      }
+      paramsList.push(params.get(param))
+    }
+    if(missingParams.length != 0) {


Do not use != in comparisons. Use !== instead

Yarik-Popov · 2025-02-04T03:26:30Z

Lgtm. Dm me your email and ill add u to the notion

Jiatao7 added 8 commits January 24, 2025 17:49

Add data validation for commands

6240c7b

implement create command

c463b13

implement delete command

dcd195b

add logger middleware

bde659e

finish onboarding for backend

dafe1d4

implement delete command

cb2bfe6

implement create command

957b4cc

fix create command

2fa2963

Yarik-Popov requested changes Jan 26, 2025

View reviewed changes

Yarik-Popov reviewed Jan 26, 2025

View reviewed changes

make requested changes

1ff2486

Jiatao7 requested a review from Yarik-Popov February 1, 2025 21:10

Yarik-Popov requested changes Feb 1, 2025

View reviewed changes

Jiatao7 added 4 commits February 1, 2025 19:28

complete new changes for backend

db6bb39

complete new changes for frontend

2822edb

complete new changes for frontend

cc0b376

add parameters missing to alert

1f2e9a2

Jiatao7 requested a review from Yarik-Popov February 3, 2025 00:05

Yarik-Popov requested changes Feb 3, 2025

View reviewed changes

Jiatao7 added 3 commits February 3, 2025 13:19

finish frontend changes

fe95383

finish frontend changes

4a5fc21

minor change

54c6b5f

Jiatao7 requested a review from Yarik-Popov February 3, 2025 18:42

Yarik-Popov requested changes Feb 4, 2025

View reviewed changes

make requested changes

b8fae64

Jiatao7 requested a review from Yarik-Popov February 4, 2025 01:09

Yarik-Popov closed this Feb 4, 2025


		setCommandType(data.data[0])

		const parameters = commandType?.params?.split(",") \|\| []

Conversation

Jiatao7 commented Jan 26, 2025

Uh oh!

Yarik-Popov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yarik-Popov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yarik-Popov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yarik-Popov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yarik-Popov commented Feb 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants