Suhas Unni Onboarding - Backend & Frontend by suhasunni · Pull Request #13 · UWOrbital/GS-Onboarding

suhasunni · 2025-02-16T03:05:10Z

Purpose

Completed entire onboarding task.

New Changes

Testing

Passed 13/13 backend tests.

Outstanding Changes

Yarik-Popov

Overall good start. Some changes are needed to the backend and existing frontend code to improve it

Yarik-Popov · 2025-02-16T16:17:46Z

backend/api/endpoints/command.py

    """
    # TODO:(Member) Implement this endpoint
+    query = select(Command).where(Command.id == id)
+    removedItem = db.exec(query).first()


Our naming convention for the backend is the python naming convention so variables are in snake_case. So change it to removed_item

Yarik-Popov · 2025-02-16T16:18:55Z

backend/api/endpoints/command.py

+
+    if removedItem is None:
+        raise HTTPException(status_code=404, detail="Item does not exist.")
+    else:


No need for the else here as we will only execute the following code if there is no exception raised

Yarik-Popov · 2025-02-16T16:21:01Z

backend/api/middlewares/logger_middleware.py

 from fastapi import Request, Response
 from starlette.middleware.base import BaseHTTPMiddleware
+from datetime import datetime
+import time


Based on your import convention, you should only be importing what you need ie dont import eh entire module if you only need time.time()

backend/data/data_models.py

Yarik-Popov · 2025-02-16T16:25:41Z

frontend/src/display/command_api.ts

+  try 
+  {
+    const { data } = await axios.get<CommandListResponse>(`${API_URL}/commands/`);
+    return data;
+  } catch (error) 
+  {
+    console.error(error);
+    throw error;
+  }


The code here is redundant as the delete endpoint returns the data. Just use the returned result of the delete call

Yarik-Popov · 2025-02-16T16:28:28Z

package.json

Normally we dont add formatting to the onboarding repos but it might be something that we add when we switch to using Deno 2 on this repo. You dont need to change anything based on this comment

Yarik-Popov · 2025-02-16T16:30:53Z

frontend/src/display/table.tsx

+      deleteCommand(id);
+      const data = await getCommands();


Since the deleteCommand already returns all the data after deleting it just use that.

There might be a race condition in the code where it will execute the getCommands() before the deleteCommand finishes thus getting the old data as there is no await on the deleteCommand statement

Yarik-Popov

I added a comment explaining why the pytest gh action is failing

Yarik-Popov · 2025-02-16T16:34:10Z

backend/api/middlewares/logger_middleware.py

        @return Response from endpoint
        """
        # TODO:(Member) Finish implementing this method
+        logger.info(f'Date of request: {datetime.now().strftime('%A, %d. %B %Y %I: %M%p')}.')


Since the version of python used is 3.10, you cant have multiple ' in the same f string. This is a feature of 3.12

Yarik-Popov

Congrats on finishing the backend. You can dm me your email and ill add you to the notion if you want you can already start working on a backend task.

Thanks for including a video of the frontend working. Looks like the frontend has some build issues due to unused variables. Check the github action and remove them. Overall the frontend was done well. Some changes are needed to improve it

Yarik-Popov · 2025-02-20T01:05:02Z

backend/api/middlewares/cors_middleware.py

    """
    app.add_middleware(
        CORSMiddleware,
-        allow_origins=["http://localhost:5173"],


Did u get any cors issues when running the web client?

For me, this causes cors errors. Add your URL to the list and keep the original one

I did have some cors issues, changing the link fixed it for me, I'll add the original back to the list.

Where u accessing the frontend on localhost or via 127...?

frontend/src/display/table.tsx

Yarik-Popov · 2025-02-20T01:08:17Z

frontend/src/display/table.tsx

+      try {
+        const response  = await deleteCommand(id);
+        setCommands(response.data);
+      } catch (error) {
+        console.error("Error deleting item: ", error);
+        throw error;


Since this function is at the component level, it shouldnt throw an exception but display a user message instead that there was an error

Yarik-Popov · 2025-02-20T01:08:35Z

frontend/src/input/command_input.tsx

+  const [mainCommands, setMainCommands] = useState<MainCommandListResponse|null>(null);
+  const [selectedMainCommand, setSelectedMainCommand] = useState<MainCommandResponse|null>(null);
+  const [ commandParams, setCommandParams ] = useState<{[key:string]: string}|null>(null);
+  const [ listTrigger, setListTrigger ] = useState(0);


This variable is unused so remove it

Yarik-Popov · 2025-02-20T01:09:25Z

frontend/src/input/command_input.tsx

+        setMainCommands(response);
+      } catch (error) {
+        console.error("Error fetching main commands: ", error);
+        throw error;


You shouldnt be throwing here but display some sort of message to the user that there was an error

Yarik-Popov · 2025-02-20T01:10:18Z

frontend/src/input/command_input.tsx

+    if (mainCommands){
+      const response = mainCommands.data.find((command:MainCommandResponse) => (command.id.toString() === e.target.value));
+      //response could be null if not found
+      if (response) {
+        setSelectedMainCommand(response);
+        const newParams: {[key:string]: string} = {};
+        response.params?.split(',').forEach((param) => (newParams[param] = ""));
+        setCommandParams(newParams);
+      }


This is quite indented. U should use guard clauses. So invert the if statements and have an early return with a user error message

Yarik-Popov · 2025-02-20T01:11:09Z

frontend/src/input/command_input.tsx

+      name: selectedMainCommand.name,
+      params: Object.keys(commandParams).map((param) => commandParams[param]).join(","),
+      format: null,
+      // command_type IS part of CommandRequest in the backend but NOT the frontend model...is this intentional?


This is unintentional and will be dealt with later on when we improve the gs onboarding

frontend/src/input/input_api.ts

Yarik-Popov

CORS issues

Yarik-Popov · 2025-02-20T02:26:27Z

backend/api/middlewares/cors_middleware.py

    """
    app.add_middleware(
        CORSMiddleware,
-        allow_origins=["http://localhost:5173"],


For me, this causes cors errors. Add your URL to the list and keep the original one

Yarik-Popov

Should be last set of changes

Yarik-Popov · 2025-02-21T17:53:18Z

frontend/src/input/command_input.tsx

+    const fetchMainCommands = async() => {
+      try {
+        const response:MainCommandListResponse = await getMainCommands();
+        setMainCommands(response);


Set the selectedCommand here to be the response.data[0] id the response.data.length > 0. Also check that response isnt null before doing the length check

frontend/src/input/command_input.tsx

frontend/src/display/table.tsx

suhasunni added 8 commits January 30, 2025 22:02

test commit

5858c1f

implement validate_params_format

c16b79a

implement post and delete methods

0d9897c

implement LoggerMiddlerware

3f019be

fix bug in data_models

99beb7a

create deleteCommand function

724a733

implement handleDelete function

aec8cd8

revert command_input

cce4171

Yarik-Popov requested changes Feb 16, 2025

View reviewed changes

suhasunni added 4 commits February 17, 2025 11:01

implement requested changes

c9ba782

"implement requested changes"

80cd261

build everything except handleSubmit()

bce454e

implement CommandInput and fix logger formatting

3c6f690

suhasunni changed the title ~~Suhas Unni Onboarding - Backend~~ Suhas Unni Onboarding - Backend & Frontend Feb 19, 2025

Yarik-Popov requested changes Feb 20, 2025

View reviewed changes

finish frontend changes

b218a4c

Yarik-Popov requested changes Feb 21, 2025

View reviewed changes

suhasunni added 2 commits February 22, 2025 17:11

fix loading bug

191e6d1

check mainCommands length

9b6aa51

Yarik-Popov closed this Feb 22, 2025

Conversation

suhasunni commented Feb 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

New Changes

Testing

Outstanding Changes

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!

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!

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!

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!

Yarik-Popov left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

suhasunni commented Feb 16, 2025 •

edited

Loading