-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat-#419: Added post editing functionality #443
feat-#419: Added post editing functionality #443
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hey @devsharmagit! Thanks for sticking to the guidelines! High five! 🙌🏻 |
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.
LGTM functionality wise, some reviews if you can address. That would be great :)
@@ -11,12 +11,12 @@ export const isAuthorMiddleware = async (req, res, next) => { | |||
} | |||
|
|||
console.log(post.authorId, userId); | |||
if (post.authorId.toString() !== userId) { | |||
if (post?.authorId?.toString() !== userId) { | |||
return res |
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.
Why do we need ?
here? isn't authorId mandatory for the post? ideally we are not supposed to use ?
return res | ||
.status(HTTP_STATUS.FORBIDDEN) | ||
.json({ message: RESPONSE_MESSAGES.POSTS.NOT_ALLOWED }); | ||
} | ||
next(); | ||
return next(); | ||
} catch (error) { |
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.
return next()? can we do both in one ? I don't think so it is redundant it should be either return or next()
<img | ||
alt="white" | ||
src={navigateBackWhiteIcon} | ||
className="active:scale-click h-5 w-10" | ||
onClick={() => navigate(-1)} | ||
/> | ||
|
||
{(post?.authorId === userData?._id || userData?.role === 'ADMIN') && ( | ||
<div className="flex gap-4"> |
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 is one way, but now that we have access token , please use it, instead of id
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.
here I am checking if the logged-in user is the author of the post or not.
I don't think it's possible to do with the access token
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.
@devsharmagit https://www.youtube.com/watch?v=Yh5Lil03tpI first 6mins give it a look. That's the reason we have introduced the access token, it will fail in backend because it is handled there, but i want even the frontend to have some intelligence
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.
@devsharmagit are you working on this? This PR looks very stale
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.
Can we aim to close this within couple of days, picking it
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 am not able to get the changes you are requesting here.
I am getting user data from the user state. why I use access token instead of user state?
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.
@siddharthisrani can you please help @devsharmagit
@devsharmagit please sync with @siddharthisrani and @Sukomal07 and close this
frontend/src/pages/edit-blog.tsx
Outdated
const navigate = useNavigate(); | ||
|
||
if (post?.authorId && userData?._id) { | ||
if (userData?.role === 'USER' && post?.authorId !== userData?._id) { |
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.
Logic looks very complicated and difficult to comprehend, can you simplify
@@ -67,7 +67,7 @@ function Signup() { | |||
<div className="flex-grow cursor-default bg-white py-4 dark:bg-dark-card"> | |||
<div className="m-4 mb-4 flex justify-center"> | |||
<div className="flex w-full items-center justify-center"> | |||
<h2 className="text-center text-lg font-bold text-black dark:text-dark-primary w-2/4 pl-2 sm:text-xl md:w-3/4 md:pl-48"> | |||
<h2 className="w-2/4 pl-2 text-center text-lg font-bold text-black dark:text-dark-primary sm:text-xl md:w-3/4 md:pl-48"> | |||
Sign up to WanderLust |
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.
remove sm:
wherever you have used
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 have not changed anything here
preettier just "w-2/4 pl-2" in front of classname
@@ -6,6 +6,7 @@ type Post = { | |||
timeOfPost: string; | |||
description: string; | |||
categories: string[]; | |||
authorId?: string; | |||
}; |
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.
authorId? -> authorId
Okay now I know why you used ?
For this if we require to change and refill the db we will
Let's not have ?
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.
some posts in db does not have authorId that's why it's being added optional
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.
@devsharmagit so said
For this if we require to change and refill the db we will
Let's add to everything and change the sample-posts.json wala file
pushed the requested changes |
@devsharmagit any progress ? |
@devsharmagit please see the attached video above in the comments, you'll get a good picture about stuffs and let's please try to close it this weekend, of required take help of @siddharthisrani he has recently worked on in this |
@devsharmagit acknowledgment please |
@devsharmagit you there ? |
how do i contact @siddharthisrani ? |
Figure out @devsharmagit 🙂 |
He might be in discord also |
@krishnaacharyaa |
Don't be dependent see the video shared and figure out... |
@krishnaacharyaa |
Cool hei tho fir, not sure why i saw it being taken from the localstorage. Sorry for the miss |
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.
Cool hei, thanks for the changes, let's check in the production, and if there is any changes, lets pick it up and fix it.
Summary
This PR completes the post editing functionality in the application.
Description
A separate form component is created and used to create two pages. Add blog page and edit blog page.
Images
2024-07-08.19-29-46.mp4
Issue(s) Addressed
Closes #419
Prerequisites