Skip to content
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

Feature/ Add series to link related articles #1146

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 47 additions & 3 deletions app/(app)/create/[[...paramsArr]]/_client.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,13 @@ const Create = () => {
isSuccess,
} = api.post.create.useMutation();

const { mutate: seriesUpdate, status: seriesStatus } = api.series.update.useMutation({
onError(error) {
toast.error("Error updating series");
Sentry.captureException(error);
}
});

// TODO get rid of this for standard get post
// Should be allowed get draft post through regular mechanism if you own it
const {
Expand Down Expand Up @@ -215,6 +222,7 @@ const Create = () => {
tags,
canonicalUrl: data.canonicalUrl || undefined,
excerpt: data.excerpt || removeMarkdown(data.body, {}).substring(0, 155),
seriesName: data.seriesName || undefined
};
return formData;
};
Expand All @@ -226,8 +234,30 @@ const Create = () => {
if (!formData.id) {
await create({ ...formData });
} else {
await save({ ...formData, id: postId });
toast.success("Saved");
let saveSuccess = false;
try {
await save({ ...formData, id: postId });
saveSuccess = true;
} catch (error) {
toast.error("Error saving post.");
Sentry.captureException(error);
}

let seriesUpdateSuccess = false;
try {
if(formData?.seriesName){
await seriesUpdate({ postId, seriesName: formData.seriesName });
}
seriesUpdateSuccess = true;
} catch (error) {
toast.error("Error updating series.");
Sentry.captureException(error);
}

if(saveSuccess && seriesUpdateSuccess){
toast.success("Saved");
}

Comment on lines +237 to +260
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix logical error in series update success handling

There is a logical issue in the way seriesUpdateSuccess is initialized and evaluated. If formData.seriesName is undefined, the seriesUpdate function is not called, but seriesUpdateSuccess is still set to true. This may prevent the success toast from displaying even when the post is saved successfully.

To ensure that the success toast appears appropriately, initialize seriesUpdateSuccess to true before the try block, and only set it to false if an error occurs during the series update. This way, if seriesUpdate is not called due to the absence of seriesName, seriesUpdateSuccess remains true.

Apply this diff to fix the issue:

-          let seriesUpdateSuccess = false;
+          let seriesUpdateSuccess = true;
           try {
             if(formData?.seriesName){
               await seriesUpdate({ postId, seriesName: formData.seriesName });
             }
-            seriesUpdateSuccess = true;
           } catch (error) {
             toast.error("Error updating series.");
             Sentry.captureException(error);
+            seriesUpdateSuccess = false;
           }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let saveSuccess = false;
try {
await save({ ...formData, id: postId });
saveSuccess = true;
} catch (error) {
toast.error("Error saving post.");
Sentry.captureException(error);
}
let seriesUpdateSuccess = false;
try {
if(formData?.seriesName){
await seriesUpdate({ postId, seriesName: formData.seriesName });
}
seriesUpdateSuccess = true;
} catch (error) {
toast.error("Error updating series.");
Sentry.captureException(error);
}
if(saveSuccess && seriesUpdateSuccess){
toast.success("Saved");
}
let saveSuccess = false;
try {
await save({ ...formData, id: postId });
saveSuccess = true;
} catch (error) {
toast.error("Error saving post.");
Sentry.captureException(error);
}
let seriesUpdateSuccess = true;
try {
if(formData?.seriesName){
await seriesUpdate({ postId, seriesName: formData.seriesName });
}
} catch (error) {
toast.error("Error updating series.");
Sentry.captureException(error);
seriesUpdateSuccess = false;
}
if(saveSuccess && seriesUpdateSuccess){
toast.success("Saved");
}

setSavedTime(
new Date().toLocaleString(undefined, {
dateStyle: "medium",
Expand Down Expand Up @@ -539,10 +569,24 @@ const Create = () => {
{copied ? "Copied" : "Copy Link"}
</div>
</button>
<p className="mt-2 text-sm text-neutral-600 dark:text-neutral-400">
<p className="mt-2 mb-2 text-sm text-neutral-600 dark:text-neutral-400">
Share this link with others to preview your
draft. Anyone with the link can view your draft.
</p>

<label htmlFor="seriesName">
Series Name
</label>
<input
id="seriesName"
type="text"
placeholder="The name of my series"
defaultValue={data?.series?.title || ""}
{...register("seriesName")}
/>
<p className="mt-2 text-sm text-neutral-600 dark:text-neutral-400">
This text is case-sensitive so make sure you type it exactly as you did in previous articles to ensure they are connected
</p>
Comment on lines +572 to +589
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Normalize series names to be case-insensitive

The series name field is currently case-sensitive, which may lead to user errors if the series name is not typed exactly the same in each article. This could result in related articles not being linked correctly.

Consider normalizing series names by converting them to lowercase (or a consistent case) when storing and comparing them in the database. This will enhance the user experience by ensuring that articles are correctly linked in a series, regardless of the case used when entering the series name.

Apply this diff to handle case normalization on the client side:

           const formData = {
             ...data,
             tags,
             canonicalUrl: data.canonicalUrl || undefined,
             excerpt: data.excerpt || removeMarkdown(data.body, {}).substring(0, 155),
-            seriesName: data.seriesName || undefined
+            seriesName: data.seriesName ? data.seriesName.toLowerCase() : undefined
           };

Ensure that the server-side logic also handles series names in a case-insensitive manner.

Committable suggestion was skipped due to low confidence.

</DisclosurePanel>
</>
)}
Expand Down
16 changes: 16 additions & 0 deletions drizzle/0011_add_series_update_post.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
-- Create Series table
CREATE TABLE IF NOT EXISTS "Series" (
"id" SERIAL PRIMARY KEY,
"title" TEXT NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we change this to "name"

"description" TEXT,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is used yet so we can delete it for the moment until we have a UI to update this

"userId" TEXT NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be inferred from the post itself

"createdAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL,
"updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL
);
-- Update Post table to add seriesId column
ALTER TABLE "Post"
ADD COLUMN "seriesId" INTEGER
ADD CONSTRAINT fk_post_series
FOREIGN KEY ("seriesId")
REFERENCES "Series" ("id")
ON DELETE SET NULL;
2 changes: 2 additions & 0 deletions schema/post.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export const SavePostSchema = z.object({
canonicalUrl: z.optional(z.string().trim().url()),
tags: z.string().array().max(5).optional(),
published: z.string().datetime().optional(),
seriesName: z.string().trim().optional()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be seriesId ?

In the drizzle migration you added a FK constraint for seriesId but I only see seriesName on the post schema

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @JohnAllenTech , Sorry I was away for the past 2 days, so couldn't reply

You are right, the foreign key is seriesId. However, the as of my knowledge, this schema (savePostSchema) is the input for the update Procedure in the post router

` update: protectedProcedure
.input(SavePostSchema)
.mutation(async ({ input, ctx }) => {
const { id, body, title, excerpt, canonicalUrl, tags = [] } = input;

  const currentPost = await ctx.db.query.post.findFirst({
    where: (posts, { eq }) => eq(posts.id, id),
  });

`
This method takes the seriesName as input, and creates a new record in the series. and updates the seriesId in the posts table.

The code below has the schema for the post table which has the seriesId
(db/schema.ts)
.references(() => user.id, { onDelete: "cascade", onUpdate: "cascade" }), showComments: boolean("showComments").default(true).notNull(), likes: integer("likes").default(0).notNull(), seriesId: integer("seriesId").references(() => series.id, { onDelete: "set null", onUpdate: "cascade" }),

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also had the same question as @JohnAllenTech since the series name may not be unique and we could have collisions while trying to find the relevant series. I think this needs to be seriesId.

});

export const PublishPostSchema = z.object({
Expand All @@ -50,6 +51,7 @@ export const ConfirmPostSchema = z.object({
.optional(),
canonicalUrl: z.string().trim().url().optional(),
tags: z.string().array().max(5).optional(),
seriesName: z.string().trim().optional()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, seriesId

});

export const DeletePostSchema = z.object({
Expand Down
6 changes: 6 additions & 0 deletions schema/series.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import z from "zod";

export const UpdateSeriesSchema = z.object({
postId: z.string(),
seriesName: z.string().trim().optional()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't unique so could cause issues when we join.

});
3 changes: 3 additions & 0 deletions server/api/router/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import { adminRouter } from "./admin";
import { reportRouter } from "./report";
import { tagRouter } from "./tag";

import { seriesRouter } from "./series";

export const appRouter = createTRPCRouter({
post: postRouter,
profile: profileRouter,
Expand All @@ -16,6 +18,7 @@ export const appRouter = createTRPCRouter({
admin: adminRouter,
report: reportRouter,
tag: tagRouter,
series: seriesRouter
});

// export type definition of API
Expand Down
30 changes: 24 additions & 6 deletions server/api/router/post.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
GetLimitSidePosts,
} from "../../../schema/post";
import { removeMarkdown } from "../../../utils/removeMarkdown";
import { bookmark, like, post, post_tag, tag, user } from "@/server/db/schema";
import { bookmark, like, post, post_tag, tag, user, series } from "@/server/db/schema";
import {
and,
eq,
Expand Down Expand Up @@ -187,12 +187,29 @@ export const postRouter = createTRPCRouter({
});
}

const [deletedPost] = await ctx.db
.delete(post)
.where(eq(post.id, id))
.returning();
const deletedPost = await ctx.db.transaction(async (tx) => {
const [deletedPost] = await tx
.delete(post)
.where(eq(post.id, id))
.returning();

if(deletedPost.seriesId){
// check is there is any other post with the current seriesId
const anotherPostInThisSeries = await tx.query.post.findFirst({
where: (post, { eq }) =>
eq(post.seriesId, deletedPost.seriesId!)
})
// if another post with the same seriesId is present, then do nothing
// else remove the series from the series table
if(!anotherPostInThisSeries){
await tx.delete(series).where(eq(series.id, deletedPost.seriesId));
}
}

return deletedPost;
});

return deletedPost;
return deletedPost;
}),
like: protectedProcedure
.input(LikePostSchema)
Expand Down Expand Up @@ -428,6 +445,7 @@ export const postRouter = createTRPCRouter({
where: (posts, { eq }) => eq(posts.id, id),
with: {
tags: { with: { tag: true } },
series: true
},
});

Expand Down
129 changes: 129 additions & 0 deletions server/api/router/series.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
import { TRPCError } from "@trpc/server";
import { createTRPCRouter, protectedProcedure } from "../trpc";
import { series, post } from "@/server/db/schema";
import { UpdateSeriesSchema } from "@/schema/series";
import {eq} from "drizzle-orm";
export const seriesRouter = createTRPCRouter({
update: protectedProcedure
.input(UpdateSeriesSchema)
.mutation(async ({input, ctx}) => {
const {postId, seriesName} = input;

if (seriesName && seriesName.trim() === "") {
throw new TRPCError({ code: 'BAD_REQUEST', message: 'Series name cannot be empty' });
}

const currentPost = await ctx.db.query.post.findFirst({
columns: {
id: true,
seriesId: true,
userId: true
},
with: {
series: {
columns: {
id: true,
title: true
},
},
},
where: (post, { eq }) => eq(post.id, postId),
});
Comment on lines +16 to +31
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Enhance postId Validation with Specific Constraints

Currently, postId is validated as a string. To further mitigate security risks, please implement more specific validation rules (e.g., ensure postId follows a UUID format if applicable).

🔗 Analysis chain

Validate postId and Handle Potential Security Risks

While fetching currentPost, ensure that postId is validated to prevent SQL injection or unauthorized access. Although TRPC and the ORM handle parameterization, additional validation enhances security.

Run the following script to check the validation of postId:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that `UpdateSeriesSchema` properly validates `postId`

# Test: Check the schema definition for `postId` validation
rg --type typescript "UpdateSeriesSchema"

Length of output: 76


Script:

#!/bin/bash
# Description: Verify that `UpdateSeriesSchema` properly validates `postId`

# Test: Check the schema definition for `postId` validation without specifying file type
rg "UpdateSeriesSchema"

# Alternatively, restrict the search to TypeScript files using a glob pattern
rg "UpdateSeriesSchema" --glob "*.ts" --glob "*.tsx"

Length of output: 490


Script:

#!/bin/bash
# Description: Extract the definition of `UpdateSeriesSchema` to verify `postId` validation

# Extract lines surrounding the `UpdateSeriesSchema` definition in schema/series.ts
rg "export const UpdateSeriesSchema" schema/series.ts -A 10 -B 5

Length of output: 206


if (!currentPost) {
throw new TRPCError({ code: 'NOT_FOUND' });
}
if (currentPost?.userId !== ctx.session.user.id) {
throw new TRPCError({
code: "FORBIDDEN",
});
}
const createNewSeries = async (seriesTitle: string) => {
// check if a series with that name already exists
// or else create a new one
return await ctx.db.transaction(async (tx) => {
let seriesId : number;
const currSeries = await tx.query.series.findFirst({
columns: {
id: true
},
where: (series, { eq }) => eq(series.title, seriesTitle),
})
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Prevent Cross-User Series Linking by Including userId in Series Query

Currently, when checking for an existing series, the query filters only by series.title. This could allow a user to link their post to a series owned by another user if the series titles match.

Apply this diff to include userId in the where clause to ensure the series belongs to the current user:

- where: (series, { eq }) => eq(series.title, seriesTitle),
+ where: (series, { eq, and }) => and(
+   eq(series.title, seriesTitle),
+   eq(series.userId, ctx.session.user.id)
+ ),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const currSeries = await tx.query.series.findFirst({
columns: {
id: true
},
where: (series, { eq }) => eq(series.title, seriesTitle),
})
const currSeries = await tx.query.series.findFirst({
columns: {
id: true
},
where: (series, { eq, and }) => and(
eq(series.title, seriesTitle),
eq(series.userId, ctx.session.user.id)
),
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in the latest commit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RangerCreaky, glad to hear that the issue has been resolved! 🎉

(^o^/)
(='.'=)
(")_(")


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!


if(!currSeries){
const [newSeries] = await tx.insert(series).values({
title: seriesTitle,
userId: ctx.session.user.id,
updatedAt: new Date()
}).returning();

seriesId = newSeries.id;
}
else{
seriesId = currSeries.id;
}
// update that series id in the current post
await tx
.update(post)
.set({
seriesId: seriesId
})
.where(eq(post.id, currentPost.id));
})

}

const unlinkSeries = async (seriesId: number) => {
// Check if the user has added a another post with the same series id previously
return await ctx.db.transaction(async (tx) =>{
const anotherPostInThisSeries = await tx.query.post.findFirst({
where: (post, { eq, and, ne }) =>
and (
ne(post.id, currentPost.id),
eq(post.seriesId, currentPost.seriesId!)
)
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix Incorrect Reference to currentPost.seriesId in unlinkSeries Function

In the unlinkSeries function, the condition uses currentPost.seriesId!, which may not align with the seriesId parameter passed to the function. This could lead to unexpected behavior if currentPost.seriesId has changed.

Apply this diff to use the seriesId parameter for accurate matching:

- eq(post.seriesId, currentPost.seriesId!)
+ eq(post.seriesId, seriesId)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
where: (post, { eq, and, ne }) =>
and (
ne(post.id, currentPost.id),
eq(post.seriesId, currentPost.seriesId!)
)
where: (post, { eq, and, ne }) =>
and (
ne(post.id, currentPost.id),
eq(post.seriesId, seriesId)
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in the latest commit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RangerCreaky, thank you for fixing the issue! 😊

(_/)
(•_•)
/ 🐰


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

})
// if another post with the same seriesId is present, then do nothing
// else remove the series from the series table
if(!anotherPostInThisSeries){
await tx.delete(series).where(eq(series.id, seriesId));
}
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure Only the User's Series is Deleted

When deleting a series, the current code removes any series matching the seriesId, regardless of ownership. This could inadvertently delete another user's series with the same ID.

Modify the where clause to include userId, ensuring only the current user's series is deleted:

- await tx.delete(series).where(eq(series.id, seriesId));
+ await tx.delete(series).where(
+   and(
+     eq(series.id, seriesId),
+     eq(series.userId, ctx.session.user.id)
+   )
+ );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await tx.delete(series).where(eq(series.id, seriesId));
}
await tx.delete(series).where(
and(
eq(series.id, seriesId),
eq(series.userId, ctx.session.user.id)
)
);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the seriesId is being picked from the currentPost , so this comparison is not necessary in my opinion. Had the comparison been done based on series title, then comparing the user id would have been important.

However, this check is alse added in the latest commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RangerCreaky, thank you for addressing the issue and adding the check in your latest commit. I appreciate your attention to detail.


✏️ Learnings added
Learnt from: RangerCreaky
PR: codu-code/codu#1146
File: server/api/router/series.ts:0-0
Timestamp: 2024-10-18T04:13:45.764Z
Learning: In the `seriesRouter` in `server/api/router/series.ts`, when deleting a series using the `seriesId` from `currentPost`, additional `userId` checks may not be necessary because unauthorized access to `currentPost` is already prevented.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

// update that series id in the current post
await tx
.update(post)
.set({
seriesId: null
})
.where(eq(post.id, currentPost.id));
})
}

if(seriesName){
// check if the current post is already linked to a series
if(currentPost?.seriesId){
// check if the series title is same as the current series name
// then we do nothing
if(currentPost?.series?.title !== seriesName){
// then the user has updated the series name in this particular edit
// Check if there is another post with the same title, else delete the series
// and create a new post with the new series name
// and update that new series id in the post
await unlinkSeries(currentPost.seriesId);
await createNewSeries(seriesName);
}
}
else{
// the current post is not yet linked to a seriesId
// so create a new series and put that Id in the post
await createNewSeries(seriesName);
}
}
else{
// either the user has not added the series Name (We do nothing)
// or while editing the post, the user has removed the series name
if(currentPost.seriesId !== null){
await unlinkSeries(currentPost.seriesId);
}
}
Comment on lines +109 to +135
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify Conditional Logic for Better Readability

The nested if-else statements can be refactored for clarity and maintainability. Simplifying the conditions will make the code easier to understand and reduce potential errors.

Consider restructuring the logic as follows:

 if (seriesName) {
   if (currentPost.seriesId) {
     if (currentPost.series.title !== seriesName) {
       await unlinkSeries(currentPost.seriesId);
       await createNewSeries(seriesName);
     }
     // No action needed if titles match
   } else {
     await createNewSeries(seriesName);
   }
 } else if (currentPost.seriesId !== null) {
   await unlinkSeries(currentPost.seriesId);
 }
 // No action needed if seriesName is not provided and no seriesId is linked
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if(seriesName){
// check if the current post is already linked to a series
if(currentPost?.seriesId){
// check if the series title is same as the current series name
// then we do nothing
if(currentPost?.series?.title !== seriesName){
// then the user has updated the series name in this particular edit
// Check if there is another post with the same title, else delete the series
// and create a new post with the new series name
// and update that new series id in the post
await unlinkSeries(currentPost.seriesId);
await createNewSeries(seriesName);
}
}
else{
// the current post is not yet linked to a seriesId
// so create a new series and put that Id in the post
await createNewSeries(seriesName);
}
}
else{
// either the user has not added the series Name (We do nothing)
// or while editing the post, the user has removed the series name
if(currentPost.seriesId !== null){
await unlinkSeries(currentPost.seriesId);
}
}
if (seriesName) {
if (currentPost.seriesId) {
if (currentPost.series.title !== seriesName) {
await unlinkSeries(currentPost.seriesId);
await createNewSeries(seriesName);
}
// No action needed if titles match
} else {
await createNewSeries(seriesName);
}
} else if (currentPost.seriesId !== null) {
await unlinkSeries(currentPost.seriesId);
}
// No action needed if seriesName is not provided and no seriesId is linked

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes suggested are to remove the comments
However, keeping it for now, for the ease of the reviewers

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RangerCreaky, understood. Keeping the comments for the ease of reviewers makes sense.

(_/)
(•︵•)
('')('')


✏️ Learnings added
Learnt from: RangerCreaky
PR: codu-code/codu#1146
File: server/api/router/series.ts:109-135
Timestamp: 2024-10-18T04:18:28.426Z
Learning: In the TypeScript file `server/api/router/series.ts`, when comments are included to aid reviewers, avoid suggesting their removal.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

})
})
30 changes: 30 additions & 0 deletions server/db/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,26 @@ export const sessionRelations = relations(session, ({ one }) => ({
}),
}));

export const series = pgTable("Series", {
id: serial("id").primaryKey(),
title: text("title").notNull(),
description: text("description"),
userId: text("userId").notNull().references(() => user.id, { onDelete: "cascade", onUpdate: "cascade" }),
createdAt: timestamp("createdAt", {
precision: 3,
mode: "string",
withTimezone: true,
})
.default(sql`CURRENT_TIMESTAMP`)
.notNull(),
updatedAt: timestamp("updatedAt", {
precision: 3,
withTimezone: true
}).notNull()
.$onUpdate(() => new Date())
.default(sql`CURRENT_TIMESTAMP`),
Comment on lines +50 to +55
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure consistency in 'updatedAt' timestamp configuration

In the series table, the updatedAt field is missing the mode: "string" option and uses .$onUpdate(() => new Date()), whereas other tables like post use mode: "string" and .$onUpdate(() => new Date().toISOString()). For consistency and to prevent potential type mismatches, consider adding mode: "string" and using .$onUpdate(() => new Date().toISOString()).

Apply this diff to ensure consistency:

 updatedAt: timestamp("updatedAt", {
   precision: 3,
+  mode: "string",
   withTimezone: true
 }).notNull()
-  .$onUpdate(() => new Date())
+  .$onUpdate(() => new Date().toISOString())
   .default(sql`CURRENT_TIMESTAMP`),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
updatedAt: timestamp("updatedAt", {
precision: 3,
withTimezone: true
}).notNull()
.$onUpdate(() => new Date())
.default(sql`CURRENT_TIMESTAMP`),
updatedAt: timestamp("updatedAt", {
precision: 3,
mode: "string",
withTimezone: true
}).notNull()
.$onUpdate(() => new Date().toISOString())
.default(sql`CURRENT_TIMESTAMP`),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping it as in the date time format, as mentioned in the issue description

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RangerCreaky, understood. We'll keep the datetime format as per the issue description.

(_/)
( •_•)
/


✏️ Learnings added
Learnt from: RangerCreaky
PR: codu-code/codu#1146
File: server/db/schema.ts:50-55
Timestamp: 2024-10-18T04:16:22.358Z
Learning: In `server/db/schema.ts`, for the `series` table, the `updatedAt` field uses `.$onUpdate(() => new Date())` without `mode: "string"`, as per the issue description. This is intentional, and we should not suggest changing it for consistency with other tables.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

})

export const account = pgTable(
"account",
{
Expand Down Expand Up @@ -149,6 +169,7 @@ export const post = pgTable(
.references(() => user.id, { onDelete: "cascade", onUpdate: "cascade" }),
showComments: boolean("showComments").default(true).notNull(),
likes: integer("likes").default(0).notNull(),
seriesId: integer("seriesId").references(() => series.id, { onDelete: "set null", onUpdate: "cascade" }),
},
(table) => {
return {
Expand All @@ -168,6 +189,7 @@ export const postRelations = relations(post, ({ one, many }) => ({
notifications: many(notification),
user: one(user, { fields: [post.userId], references: [user.id] }),
tags: many(post_tag),
series: one(series,{ fields: [post.seriesId], references: [series.id] }),
}));

export const user = pgTable(
Expand Down Expand Up @@ -273,6 +295,14 @@ export const bookmark = pgTable(
},
);

export const seriesRelations = relations(series, ({ one, many }) => ({
posts: many(post),
user: one(user, {
fields: [series.userId],
references: [user.id],
}),
}));

export const bookmarkRelations = relations(bookmark, ({ one, many }) => ({
post: one(post, { fields: [bookmark.postId], references: [post.id] }),
user: one(user, { fields: [bookmark.userId], references: [user.id] }),
Expand Down
Loading