Code review #66
Code review #66
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| if(err){ | ||
| return res.send("Please login again") | ||
| } | ||
| console.log(decoded) |
There was a problem hiding this comment.
suggestion: don't print the decoded info in the console. It may also contain some other user information.
|
|
||
| const TimerSchema = mongoose.Schema({ | ||
|
|
||
| taskId : String , |
There was a problem hiding this comment.
timer schema should have only task id and time info
it should not have, taskName, userId
those data you can be retreived from TaskModel
no data duplication
|
|
||
| const UserSchema = mongoose.Schema({ | ||
| email :{type : String, required : true}, | ||
| password : {type : String, required : true}, |
There was a problem hiding this comment.
how userId is referred to in TimerSchema and TaskSchema
as there is no userId preset in UserSchema?
userId is a JWT token?
| @@ -1 +0,0 @@ | |||
| eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6Imtpc2FuYmFkd2FpazM1NEBnbWFpbC5jb20iLCJVc2VySWQiOiI2MzBkZjM4ZTUzYWYxY2Y4MjY5YmRjZWQiLCJpYXQiOjE2NjE4NTg3MDJ9.Ag0yc1wAOJMg95OZ12Aru8U4VC-6Jw-i07luo7xfTGg | |||
There was a problem hiding this comment.
don't keep any tokens in repo
| return res.status(200).send({message : "project updated successfully", newProject}); | ||
|
|
||
| }else{ | ||
| return res.status(401).send("you are not authorised to update project") |
There was a problem hiding this comment.
while sending an error message, keep the same format
{message : "you are not authorized to update project"}
| return res.send({message : " new project added"}) | ||
| }) | ||
| ProjectController.get("/", async(req,res)=>{ | ||
| const {user_id} = req.body; |
There was a problem hiding this comment.
why is anyone allowed to see any project?
user can see the projects that are created by him or his team
I think the same auth logic in the patch can be added here as well
| return res.status(200).send({message : "project deleted successfully"}); | ||
|
|
||
| }else{ | ||
| return res.status(401).send("you are not authorised to update project") |
There was a problem hiding this comment.
while sending an error message, keep the same format
you are not authorised to delete project
| let email = profile._json.email; | ||
| const user = new UserModel({ | ||
| email, | ||
| password: uuidv4(), |
There was a problem hiding this comment.
any reason, why the password is stored as uuid?
| const UserId = req.user._id; | ||
| const token = jwt.sign({email, UserId}, process.env.SECRET_KEY) | ||
| console.log('token:', token) | ||
| localStorage.setItem("googleToken", token); |
There was a problem hiding this comment.
why the user JWT token is stored in the local storage of the server?
|
|
||
|
|
||
| const app = express(); | ||
| app.use(cors()) |
There was a problem hiding this comment.
var corsOptions = {
origin: 'http://localhost:8080',
optionsSuccessStatus: 200 // For legacy browser support
}
app.use(cors(corsOptions));`
cors only allow required sites, not all sites
| @@ -1,25 +0,0 @@ | |||
| { | |||
| "short_name": "React App", | |||
| "name": "Create React App Sample", | |||
There was a problem hiding this comment.
suggestion: some meaningful name can be given
| <Flex style={{justifyContent:"space-between",marginTop:"1vw",fontSize:"16px"}}> <Text>Plan:</Text><Text>Basic</Text></Flex> | ||
| <Flex style={{justifyContent:"space-between",marginTop:"1vw",fontSize:"16px"}}> <Text>Billing cycle:</Text><RadioGroup onChange={setValue} value={value}> | ||
| <Stack direction='row'> | ||
| <Radio value='1' colorScheme='green'>Annual</Radio><Button style={{backgroundColor:"#ffa500",color:"white"}}>10% off</Button> |
There was a problem hiding this comment.
some meaning full string can be given in value instead of 1
| <ImgDiv> | ||
| <img | ||
| src="https://cdn-m.timecamp.com/img/greenbranding/features/img-home-features.png" | ||
| alt="" |
There was a problem hiding this comment.
dont keep alt as empty, but any chance, if image image is not loading, alt text will be visible
| margin-bottom: 1rem; | ||
| } | ||
| `; | ||
| const XYZ = styled.div` |
There was a problem hiding this comment.
what is this XYZ?
| text-align: left; | ||
| `; | ||
| const H2 = styled.h2` | ||
| padding-right: 2rem !important; |
There was a problem hiding this comment.
using important is bad practice
https://stackoverflow.com/questions/3706819/what-are-the-implications-of-using-important-in-css
Nav bar:
pricing is not taking to the appropriate div
footer:
if any URL doesn't exist, it is good practice to show a 404
sign up:
no loader
no error message for an existing user trying to create an account
phone number validation is missing
suggestion: as soon as toast comes, redirect to project instead of waiting
unable to create a project in the deployed environment - no error message
Estimated hours Estimated fee validation has to be added, if any wrong data is entered server get hangs (wired issue)
post update not going back to the task page
your tasks
alignment is not correct edit, delete is going to the end
delete API is always saying not authorized
the timer is not persisted in DB?
review comments: https://drive.google.com/file/d/1MFbv7_qlvs2zinGtqAPosMqBphKPap6h/view?usp=sharing