Skip to content

Event link app#54

Open
Lili00ani wants to merge 69 commits intorocketacademy:mainfrom
Lili00ani:main
Open

Event link app#54
Lili00ani wants to merge 69 commits intorocketacademy:mainfrom
Lili00ani:main

Conversation

@Lili00ani
Copy link

No description provided.

@@ -1 +1,25 @@
node_modules/ No newline at end of file
Copy link

Choose a reason for hiding this comment

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

no readme file

Comment on lines +30 to +37
(async () => {
try {
await db.sequelize.authenticate();
console.log("Database connection has been established successfully.");
} catch (error) {
console.error("Unable to connect to the database:", error);
}
})();
Copy link

Choose a reason for hiding this comment

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

this should be handled in the db index file, not here

Comment on lines +26 to +38
// type: DataTypes.INTEGER,
// references: {
// model: "users",
// key: "id",
// },
// },
// event_id: {
// type: DataTypes.INTEGER,
// references: {
// model: "events",
// key: "id",
// },
// },
Copy link

Choose a reason for hiding this comment

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

by now you should know that this is not clean code for a review :)

this.hasMany(models.event);
}
}
Admin.init(
Copy link

Choose a reason for hiding this comment

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

what's the PK?

this.hasMany(models.event);
}
}
Category.init(
Copy link

Choose a reason for hiding this comment

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

what's the pk? Please make sure your models are always a representation of your DB table

}
}

async getAllStatuses(req, res) {
Copy link

Choose a reason for hiding this comment

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

Same here. Why does status logic sit in categories?

Comment on lines +42 to +45
{ model: this.adminModel, as: "admin" },
{ model: this.venueModel, as: "venue" },
{ model: this.languageModel, as: "language" },
{ model: this.categoryModel, as: "category" },
Copy link

Choose a reason for hiding this comment

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

why the need for all these aliases?

Comment on lines +81 to +83
output = await this.model.findAll(queryOptions);
} else {
output = await this.model.findAll(queryOptions);
Copy link

Choose a reason for hiding this comment

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

Output will always be the result of the same query. So why not just use a constant for output, without the if/else statement?

...
if (keyword !== "all") {
        queryOptions.where = {
          title: { [Op.iLike]: `%${keyword}%` },
        };
}

const output = await this.model.findAll(queryOptions);

image_link,
} = req.body;
try {
const newEvent = await this.model.create({
Copy link

Choose a reason for hiding this comment

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

since you do not run any validation (which you should), why not just do this.model.create(req.body) ?

async insertOne(req, res) {
const { postal_code, address, lat, lng, country } = req.body;
try {
let venue = await this.model.findOne({
Copy link

Choose a reason for hiding this comment

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

We can use this.model.findOrCreate for this function. Though it seems a bit wrong to do that as well. Your function is trying to insert one, not to find one. So you should try to decide what this should do, including your route.

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