Skip to content

Togglefavorite#49

Open
donkatsucombo wants to merge 4 commits intomainfrom
togglefavorite
Open

Togglefavorite#49
donkatsucombo wants to merge 4 commits intomainfrom
togglefavorite

Conversation

@donkatsucombo
Copy link
Collaborator

Need to do some more research but basically learned this stuff from chatgpt so fingers crossed. Added a addFavorite and removeFavorite route along with its controllers and services

const { User } = require('./models');

module.exports = {
addFavorite(req, res) {
Copy link
Owner

Choose a reason for hiding this comment

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

Make sure to call the authentication method in authentication.js

module.exports = {
addFavorite(req, res) {
const { username } = req.body;
const { userId } = req.user;
Copy link
Owner

Choose a reason for hiding this comment

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

The userId is not provided by default in the request; you'd have to find a way to get the userId from the JWT subject (sub) claim from the request. A good place to do this would be in authentication.js


// Validate both user IDs exist
try {
User.findOne({ where: { username } })
Copy link
Owner

Choose a reason for hiding this comment

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

username is also not provided by default in the request; where: { userId } here

const { User } = require('./models');

module.exports = {
addFavorite(req, res) {
Copy link
Owner

Choose a reason for hiding this comment

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

May want to consider making this an async method due to all of the nesting of promises with .then(). Also consider refactoring a lot of the database access logic into the favorites_service.js file.

}

// Check if an entry in the Favorites table already exists
Favorite.findOne({ where: { user: userId, favorite: username } })
Copy link
Owner

Choose a reason for hiding this comment

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

Here, you try to access a favorite with user: userId, where userId is a GUID and favorite: username is a string username. This will fail, as Favorite.findOne() expects both user and favorite to be a GUID. First, you'll have to find the other favorite's userId via username with User.findOne({ where: { username } }), then do Favorite.findOne() with both userIds.

Favorite.create({ user: userId, favorite: username })
.then(() => {
// Return a 200 OK response
res.status(200).json({ message: 'Favorite created successfully' });
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
res.status(200).json({ message: 'Favorite created successfully' });
res.status(204);

})
.catch(error => {
console.error(error);
res.status(500).json({ error: 'Internal Server Error' });
Copy link
Owner

Choose a reason for hiding this comment

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

Too much nesting of try/catch. Will need to be more specific with status codes.

}
},

removeFavorite(req, res) {
Copy link
Owner

Choose a reason for hiding this comment

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

Same problems as above with addFavorite

return res.sendStatus(401); // Unauthorized
}

jwt.verify(token, 'YOUR_SECRET_KEY', (err, user) => {
Copy link
Owner

Choose a reason for hiding this comment

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

Replace YOUR_SECRET_KEY with an environment variable.

next();
});
}

Copy link
Owner

Choose a reason for hiding this comment

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

Will need to add a method to create a JWT token

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