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 #1141

Closed
wants to merge 3 commits into from
Closed
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
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

Validate and sanitize seriesName before saving

When adding seriesName to the formData, ensure that the value is properly validated and sanitized. This includes trimming whitespace, limiting the length, and preventing invalid characters. This will help maintain data integrity and prevent potential issues with series management.

Consider adding validation rules using zod or another validation library to enforce these constraints on seriesName.

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

Ensure series update handles series removal correctly

Currently, the seriesUpdate mutation is called only when formData.seriesName exists. This means that if a user removes the series name from a post, the seriesUpdate function is not invoked, and the series association may not be properly removed from the post.

To handle series removal correctly, consider calling seriesUpdate regardless of whether formData.seriesName exists. The mutation can then handle adding, updating, or removing the series association based on the value of seriesName.

Apply this diff to adjust the logic:

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

Ensure that the seriesUpdate mutation on the server side appropriately handles cases where seriesName is undefined or an empty string, removing the series association when necessary.

📝 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 = false;
try {
await seriesUpdate({ postId, seriesName: formData.seriesName });
seriesUpdateSuccess = true;
} catch (error) {
toast.error("Error updating series.");
Sentry.captureException(error);
}
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 +577 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 prevent case-sensitivity issues

Requiring users to type the series name exactly as before due to case sensitivity can lead to inconsistencies and user errors. To enhance user experience and ensure consistent linkage of related articles, consider normalizing series names by converting them to a standard case (e.g., all lowercase) before saving and when querying.

This approach will help prevent issues where articles are not linked correctly because of differences in capitalization, improving the reliability of the series feature.

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

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
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

@coderabbitai coderabbitai bot Oct 17, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle case when post is not found

If the post with the provided postId does not exist, currentPost will be null. This will cause the subsequent check if (currentPost?.userId !== ctx.session.user.id) to incorrectly throw a FORBIDDEN error. It's more appropriate to throw a NOT_FOUND error to indicate that the post does not exist.

Apply the following diff to check if the post exists before proceeding:

+                if (!currentPost) {
+                    throw new TRPCError({ code: 'NOT_FOUND' });
+                }
                 if (currentPost.userId !== ctx.session.user.id) {
                     throw new TRPCError({
                       code: "FORBIDDEN",
                     });
                 }
📝 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 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),
});
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),
});
if (!currentPost) {
throw new TRPCError({ code: 'NOT_FOUND' });
}
if (currentPost.userId !== ctx.session.user.id) {
throw new TRPCError({
code: "FORBIDDEN",
});
}

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 second commit

Copy link
Contributor

Choose a reason for hiding this comment

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

Rate Limit Exceeded

@RangerCreaky have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 36 seconds before sending another message.


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),
})

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!)
)
})
// 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));
}
// 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 +101 to +127
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 17, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add input validation for seriesName

To prevent potential issues with empty or invalid series names, consider adding input validation to ensure that seriesName is a non-empty string when provided.

Apply the following diff to include validation:

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

Committable suggestion was skipped due to low confidence.

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 second 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 Thanks for addressing the input validation for seriesName in your second commit!

(_/)
(•ᴗ•)
/🥕


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

})
})
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()
Comment on lines +50 to +53
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 17, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add default value and auto-update logic for updatedAt field

The updatedAt field lacks a default value and auto-update logic, which could lead to null values or inaccurate timestamps. To maintain consistency with other tables and ensure accurate timestamps, consider adding these.

Apply this diff to update the updatedAt field:

 updatedAt: timestamp("updatedAt", {
   precision: 3,
   withTimezone: true
 }).notNull()
+  .$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()
updatedAt: timestamp("updatedAt", {
precision: 3,
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.

done

Copy link
Contributor

Choose a reason for hiding this comment

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

@RangerCreaky, great to hear that you've made the change!

(^_^)/


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

.$onUpdate(() => new Date())
.default(sql`CURRENT_TIMESTAMP`),
})
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 17, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Define relations for the series table

Adding a relations definition for the series table will enable you to define how it interacts with other tables, maintaining consistency with the existing schema design.

Consider adding the following relations:

export const seriesRelations = relations(series, ({ one, many }) => ({
  posts: many(post),
  user: one(user, {
    fields: [series.userId],
    references: [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 second 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 adding the seriesRelations definition in your second commit! This ensures the series table interacts correctly with other tables.

(_/)
( ^.^)
(")(")


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


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