Migrate get evaluations query to Drizzle on API Frontend #160

haikalvidya · 2024-11-05T17:17:33Z

closed #81

Important

Migrated evaluation fetching logic to Drizzle ORM on the frontend, removing old SQLX-based backend implementation.

Backend Removal:
- Removed get_evaluation_datapoint function from evaluations.rs.
- Removed get_evaluation_datapoint service from main.rs.
Frontend Migration:
- Deleted route.ts for datapoints under frontend/app/api/projects/[projectId]/evaluations/[evaluationId].
- Updated route.ts for evaluations to use Drizzle ORM for fetching evaluations and results.
- Updated page.tsx to fetch evaluation info and evaluations using new API endpoints.
API Changes:
- Added Drizzle ORM queries in route.ts for fetching evaluations and results.
- Added user membership check in route.ts for evaluations.
- Updated get_evaluations to handle groupId filtering in route.ts.

^{This description was created by}^{for f9ebdd8. It will automatically update as commits are pushed.}

CLAassistant · 2024-11-05T17:17:39Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ haikalvidya
✅ dinmukhamedm
❌ Vidya Haikal

Vidya Haikal seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

dinmukhamedm

LGTM overall, though left small comments. Regarding the deletion you mentioned: it is fine to only delete the unused frontend code. If you feel like setting up rust and building app-server is a little too much, we will remove the redundant backend code ourselves

frontend/app/api/projects/[projectId]/evaluations/[evaluationId]/route.ts

dinmukhamedm · 2024-11-05T21:44:06Z

frontend/app/api/projects/[projectId]/evaluations/route.ts

-    filters: [],
-    baseQuery,
-    orderBy: desc(sql`created_at`),
-  });
+    return Response.json(result);
+  } else {
+    const baseQuery = db.$with('base').as(db.select().from(evaluations));

-  return Response.json(result);
+    const result = await paginatedGet<any, Evaluation>({
+      table: evaluations,
+      baseFilters: [eq(sql`project_id`, projectId)],
+      filters: [],
+      baseQuery,
+      orderBy: desc(sql`created_at`)
+    });


we've simplified paginated get a bit, sorry about that. If you could pull/merge the latest dev into your branch? It now looks like this:

const result = await paginatedGet<any, Evaluation>({ table: evaluations, filters: [eq(evaluations.projectId, projectId)], orderBy: desc(evaluations.createdAt), });

frontend/app/api/projects/[projectId]/evaluations/route.ts

frontend/app/api/projects/[projectId]/evaluations/[evaluationId]/route.ts

# Conflicts: # frontend/app/api/projects/[projectId]/evaluations/route.ts

dinmukhamedm · 2024-11-06T17:33:18Z

Thanks @haikalvidya, amazing work! This looks good to me. Let me know once/if you've tested this

ellipsis-dev

❌ Changes requested. Reviewed everything up to f9ebdd8 in 1 minute and 7 seconds

More details

Looked at 368 lines of code in 7 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. frontend/app/api/projects/[projectId]/evaluations/route.ts:69

Draft comment:
The DELETE function does not execute the delete operation. You need to call .execute() to perform the deletion. Consider adding:

      .execute()

Reason this comment was not posted:
Comment was on unchanged code.

Workflow ID: wflow_Y0siwNPKFaxaIofL

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev · 2024-11-08T18:00:50Z

frontend/app/api/projects/[projectId]/evaluations/route.ts

-    filters: [eq(evaluations.projectId, projectId)],
-    orderBy: desc(evaluations.createdAt),
-  });
+  if (groupId && !groupId.trim()) {


The condition if (groupId && !groupId.trim()) seems incorrect. It should check if groupId is not empty after trimming. Consider changing it to:

Suggested change

if (groupId && !groupId.trim()) {

if (groupId && groupId.trim())

haikalvidya · 2024-11-09T08:47:56Z

Hey @dinmukhamedm sorry for late response, I haven't been able to test the changes yet due to some reasons (I DM'ed you about it on discord). However, I think the code is ready for review.

dinmukhamedm · 2024-11-09T21:28:14Z

frontend/app/project/[projectId]/evaluations/[evaluationId]/page.tsx

-      }
-    }
+  const evaluationInfoRes = await fetch(
+    `/api/projects/${params.projectId}/evaluations/${params.evaluationId}`


@haikalvidya sorry, hadn't realized at first, but this will neither compile nor we should keep it here. Let's move the logic from the routes directly to this file (as util functions) and remove the API routes if they are unused. Also, please address the Ellipsis comment

oohh, alright, will work on that today

Could you kindly review my latest commit again?, i have tested the changes, and it is running well

skull8888888

qq: did you test everything locally?

skull8888888 · 2024-11-10T09:13:10Z

frontend/app/project/[projectId]/evaluations/[evaluationId]/page.tsx

@@ -22,20 +26,82 @@ export default async function EvaluationPage({
    redirect('/sign-in');
  }


you can remove this (and its imports), middleware now protects all project/* and projects/* routes

i have updated the code upon this matter

dinmukhamedm

Excellent work! Thanks!

Migrate get evaluations query to Drizzle on API Frontend

dc9eb13

haikalvidya force-pushed the refac/migrate-evaluations-query-to-drizzle branch from 2f7bbb3 to dc9eb13 Compare November 5, 2024 17:21

dinmukhamedm reviewed Nov 5, 2024

View reviewed changes

dinmukhamedm reviewed Nov 6, 2024

View reviewed changes

frontend/app/api/projects/[projectId]/evaluations/[evaluationId]/route.ts Outdated Show resolved Hide resolved

Vidya Haikal and others added 2 commits November 6, 2024 17:11

Merge branch 'dev' into refac/migrate-evaluations-query-to-drizzle

0ffea34

# Conflicts: # frontend/app/api/projects/[projectId]/evaluations/route.ts

Change code to follow some feedbacks

8a0fdb3

haikalvidya force-pushed the refac/migrate-evaluations-query-to-drizzle branch from 8023955 to 8a0fdb3 Compare November 6, 2024 10:32

haikalvidya added 2 commits November 6, 2024 17:50

add with query get evaluation result

a7eaa4d

delete some unused code

f9ebdd8

haikalvidya marked this pull request as ready for review November 8, 2024 17:59

ellipsis-dev bot reviewed Nov 8, 2024

View reviewed changes

Merge branch 'dev' into refac/migrate-evaluations-query-to-drizzle

aa655c1

dinmukhamedm reviewed Nov 9, 2024

View reviewed changes

refac: move calling db to page file from api route

8f316df

skull8888888 requested changes Nov 10, 2024

View reviewed changes

delete unuse code on auth checking

dd72c87

dinmukhamedm approved these changes Nov 10, 2024

View reviewed changes

dinmukhamedm merged commit ee7dee6 into lmnr-ai:dev Nov 10, 2024
1 check was pending

dinmukhamedm mentioned this pull request Nov 10, 2024

Migrate get evaluations query to Drizzle #81

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate get evaluations query to Drizzle on API Frontend #160

Migrate get evaluations query to Drizzle on API Frontend #160

haikalvidya commented Nov 5, 2024 •

edited by ellipsis-dev bot

Loading

CLAassistant commented Nov 5, 2024 •

edited

Loading

dinmukhamedm left a comment

dinmukhamedm Nov 5, 2024

dinmukhamedm commented Nov 6, 2024

ellipsis-dev bot left a comment

ellipsis-dev bot Nov 8, 2024

haikalvidya commented Nov 9, 2024

dinmukhamedm Nov 9, 2024

haikalvidya Nov 10, 2024

haikalvidya Nov 10, 2024

skull8888888 left a comment

skull8888888 Nov 10, 2024

haikalvidya Nov 10, 2024

dinmukhamedm left a comment

	if (groupId && !groupId.trim()) {
	if (groupId && groupId.trim())

		@@ -22,20 +26,82 @@ export default async function EvaluationPage({
		redirect('/sign-in');
		}

Migrate get evaluations query to Drizzle on API Frontend #160

Migrate get evaluations query to Drizzle on API Frontend #160

Conversation

haikalvidya commented Nov 5, 2024 • edited by ellipsis-dev bot Loading

CLAassistant commented Nov 5, 2024 • edited Loading

dinmukhamedm left a comment

Choose a reason for hiding this comment

dinmukhamedm Nov 5, 2024

Choose a reason for hiding this comment

dinmukhamedm commented Nov 6, 2024

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot Nov 8, 2024

Choose a reason for hiding this comment

haikalvidya commented Nov 9, 2024

dinmukhamedm Nov 9, 2024

Choose a reason for hiding this comment

haikalvidya Nov 10, 2024

Choose a reason for hiding this comment

haikalvidya Nov 10, 2024

Choose a reason for hiding this comment

skull8888888 left a comment

Choose a reason for hiding this comment

skull8888888 Nov 10, 2024

Choose a reason for hiding this comment

haikalvidya Nov 10, 2024

Choose a reason for hiding this comment

dinmukhamedm left a comment

Choose a reason for hiding this comment

haikalvidya commented Nov 5, 2024 •

edited by ellipsis-dev bot

Loading

CLAassistant commented Nov 5, 2024 •

edited

Loading