-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new columns to perfile table #305 #332
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Please just review my other comments and fix both Vitest and ESLint, which are failing.
app/models/profile.server.ts
Outdated
@@ -8,7 +8,9 @@ import type { | |||
PrismaClient, | |||
} from "@prisma/client"; | |||
import { Prisma } from "@prisma/client"; | |||
import { sql } from "kysely"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none of these changes from lines 11 to 13 seem necessary. ESLint says we are importing but not using those names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this file? Is it necessary?
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this file? Is it necessary?
app/models/profile.server.ts
Outdated
@@ -8,7 +8,9 @@ import type { | |||
PrismaClient, | |||
} from "@prisma/client"; | |||
import { Prisma } from "@prisma/client"; | |||
import { sql } from "kysely"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you tested these changes? You need to run
npm run migrate-profiles-lake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
app/lake.server.tsx
Outdated
@@ -41,6 +42,7 @@ export async function getActiveProfiles() { | |||
const query = `SELECT contact__wizeos_profile_id, contact__employee_number, contact__email, | |||
contact__first_name, contact__preferred_name, contact__last_name, | |||
contact__photo__url, | |||
contact__isBillable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one is incorrect? Right?
What does this PR do?
Add the column isBillable of type boolean with the default value true to the profiles table
Where should the reviewer start?
In the schema and in the Prisma migrations
How should this be manually tested?
With a query to the database