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

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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,
"description" TEXT,
"userId" TEXT NOT NULL,
"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" }),

});

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()
});

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()
});
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
137 changes: 137 additions & 0 deletions server/api/router/series.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
import { TRPCError } from "@trpc/server";
import { createTRPCRouter, protectedProcedure } from "../trpc";
import { series, post } from "@/server/db/schema";
import { UpdateSeriesSchema } from "@/schema/series";
import {eq, and} 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, and }) => and(
eq(series.title, seriesTitle),
eq(series.userId, ctx.session.user.id)
),
})

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, 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(
and(
eq(series.id, seriesId),
eq(series.userId, ctx.session.user.id)
)
);
}
// 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