Skip to content

Patrick-HongYun-Project3-Backend#52

Open
patrickkok wants to merge 61 commits intorocketacademy:mainfrom
magiicloud:main
Open

Patrick-HongYun-Project3-Backend#52
patrickkok wants to merge 61 commits intorocketacademy:mainfrom
magiicloud:main

Conversation

@patrickkok
Copy link
Copy Markdown

No description provided.

magiicloud and others added 30 commits March 20, 2024 22:00
…migration, model, seeder. also added auth0 initial setup before controllers and routers
…Name and amended createdAt to created_at and updatedAts. rewrote router and controller classes. added scripts in packagejson.
Created add items controller and routes
Add controller method to add new building and associated rooms.
Added logic for adding new items and also auth backend
added new logic to add new items if it exist already
…emoved consumed column and adjusted the Cart_Line_Items table to below Room_Items table
magiicloud and others added 28 commits April 6, 2024 23:32
…ry as sequelize cannot handle complex equations
Add route and controller method to get room items
…a user is a part of, create buildingUser row when creating new building
No conflicting files, to remove commented code before presentation.
Show only relevant buildings to current user
amended the usercontroller migrations and seeder for retreival of use…
Only show items that is in the user's building
Comment thread README.md
@@ -1,3 +1,5 @@
# Rocket Academy Coding Bootcamp: Project 3 Backend
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could really provide a bit more info here guys. How about what the BE really does, and what about setting up the local db for dev environment, including environment variables necessary etc?

Comment thread Dockerfile
ENV PATH=/usr/local/node/bin:$PATH
ARG NODE_VERSION=16.15.1

RUN apt-get update; apt install -y curl python-is-python3 pkg-config build-essential && \
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do we really need python to build a node project? Seems a bit overkill. I am pretty sure there must be an easier way

Comment thread config/database.js
@@ -0,0 +1,18 @@
require("dotenv").config();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do we need this file if we got the .ts file?

Comment thread tsconfig.json
@@ -0,0 +1,111 @@
{
"compilerOptions": {
/* Visit https://aka.ms/tsconfig to read more about this file */
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you should clean this file up a bit

Comment thread index.ts
Comment on lines +36 to +40
app.use(checkJwt, itemsRouter);
app.use(checkJwt, buildingsRouter);
app.use(checkJwt, cartRouter);
app.use(checkJwt, dashRouter);
app.use(checkJwt, usersrouter);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

if you are using checkJwt on every router, why not just make a generic app.use(checkJwt), just like with cors? That would avoid repetition here

Comment on lines +5 to +21
async updateUser(req: Request, res: Response) {
const { email, name, id, photoUrl } = req.body;
try {
const output = await User.findOrCreate({
where: { email: email },
defaults: {
auth_id: id,
email: email,
name: name,
profile_img_url: photoUrl,
},
});
return res.json(output);
} catch (err) {
return res.status(400).json({ error: true, msg: (err as Error).message });
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since I only saw you use checkJwt, I almost would assume the following:

  1. I have an account on your app and can authenticate with my token to the BE request
  2. I can copy the token from my browser into postman and make a request to your BE
  3. I don't type in my email into the request body (which I can find by updating my own user profile), but another email
  4. I can update another user's profile

So, even if the user has a token, we should still make sure that the token and the updated User, are the same user. So we need to actually deconstruct the token to extract the email address of the user and compare to the request body. Just a small security loophole I can think of here


module.exports = {
async up(queryInterface: QueryInterface, Sequelize: typeof DataTypes) {
await queryInterface.createTable("Users", {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As previously stated, do multiple files per each database change. Lumping into one file, I would highly discourage

Comment thread db/models/Building.ts
modelName: "Building",
underscored: true,
})
export class Building extends Model<BuildingAttributes> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Where is your PK?

Comment thread db/models/Item.ts
tableName: "Items",
underscored: true,
})
export class Item extends Model<ItemAttributes> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

PK?

Comment thread db/models/Room.ts
modelName: "Room",
underscored: true,
})
export class Room extends Model<RoomAttributes> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

PK?

rooms.map(async (obj: RoomAttributes) => {
const newRoom = { ...obj };
newRoom["building_id"] = newBuilding.id;
await Room.create(newRoom, { transaction: t });
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you should collect the rooms into an array, and then run bulkCreate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants