4797-lilium-worker-notifications #1229

Merged
pau merged 37 commits from 4797-lilium-worker-notifications into dev 2023-02-02 08:53:29 +00:00
3 changed files with 33 additions and 25 deletions
Showing only changes of commit 24b8b172b9 - Show all commits

View File

@ -22,13 +22,10 @@ module.exports = Self => {
http: {source: 'context'}
},
{
arg: 'userFk',
type: 'string'
arg: 'notificationId',
pau marked this conversation as resolved Outdated
Outdated
Review

Renombrar a userFk. De todas formas, se podría evitar pasarle este parámetro, si en vez de especificar el id de la notificación genérica, se especificara el id de la notificación del usuario (Actualmente no existe el campo id autoincremental en la tabla notificationSubscription, pero creo que hubiese sido lo ideal)

Renombrar a userFk. De todas formas, se podría evitar pasarle este parámetro, si en vez de especificar el id de la notificación genérica, se especificara el id de la notificación del usuario (Actualmente no existe el campo id autoincremental en la tabla notificationSubscription, pero creo que hubiese sido lo ideal)
type: 'number',
required: true
},
{
arg: 'notificationFk',
type: 'number'
}
],
returns: {
type: 'object',
@ -40,28 +37,35 @@ module.exports = Self => {
}
});
Self.deleteNotification = async function(ctx, userFk, notificationFk) {
Self.deleteNotification = async function(ctx, notificationId) {
const models = Self.app.models;
const user = ctx.req.accessToken.userId;
const modifiedUser = await getUserToModify(userFk, models);
const modifiedUser = await getUserToModify(notificationId, models);
pau marked this conversation as resolved Outdated
Outdated
Review

No estas comprobando el usuario

No estas comprobando el usuario
Outdated
Review

No se comprueba el usuario porque se le pasa un id de notificationSubscription ya existente, por lo cual dentro del metodo getUserToModify se cumpliria el requisito del if-else y se realizaria la consulta en la que se sacaria a que usuario pertenece esa notificación

No se comprueba el usuario porque se le pasa un id de notificationSubscription ya existente, por lo cual dentro del metodo getUserToModify se cumpliria el requisito del if-else y se realizaria la consulta en la que se sacaria a que usuario pertenece esa notificación
if (user == modifiedUser.id || modifiedUser.bossFk == user) {
pau marked this conversation as resolved Outdated
Outdated
Review

Te ahorres fer if else, fent:

        if (user != modifiedUser.id && user != modifiedUser.bossFk)
            throw new UserError('You dont have permission to modify this user');
            
        await models.NotificationSubscription.destroyById(notificationId);   
        
Te ahorres fer if else, fent: ``` if (user != modifiedUser.id && user != modifiedUser.bossFk) throw new UserError('You dont have permission to modify this user'); await models.NotificationSubscription.destroyById(notificationId); ```
const query = `DELETE FROM util.notificationSubscription
WHERE notificationFk = ? AND userFk = ?`;
await Self.rawSql(query, [notificationFk, userFk]);
await models.NotificationSubscription.destroyById(notificationId);
return;
pau marked this conversation as resolved
Review

Self.deleteNotification = async function(ctx, userId, notificationId)

Y asi te ahorras poner

const notificationId = await ctx.args.notificationId;
const userId = await ctx.args.userId;
`Self.deleteNotification = async function(ctx, userId, notificationId)` Y asi te ahorras poner ``` const notificationId = await ctx.args.notificationId; const userId = await ctx.args.userId; ```
} else
throw new UserError('You dont have permission to modify this user');
pau marked this conversation as resolved Outdated
Outdated
Review

No seria mejor utilizar ctx.req.accessToken.userId y no pasarlo por parametro?

No seria mejor utilizar ctx.req.accessToken.userId y no pasarlo por parametro?
};
pau marked this conversation as resolved Outdated
Outdated
Review

Provar:
let userToCheck = userFk;
if (notificationId != null)
userToCheck = await models.NotificationSubscription.findById(notificationId).userFk;

return await models.Worker.findOne({
    fields: ['id', 'bossFk'],
    where: {
        id: userToCheck
    }
});
Provar: let userToCheck = userFk; if (notificationId != null) userToCheck = await models.NotificationSubscription.findById(notificationId).userFk; return await models.Worker.findOne({ fields: ['id', 'bossFk'], where: { id: userToCheck } });
pau marked this conversation as resolved Outdated
Outdated
Review

Además de lo que comenta alex, quitar el await.

Además de lo que comenta alex, quitar el await.
async function getUserToModify(user, models) {
return await models.Worker.findOne({
fields: ['id', 'bossFk'],
where: {
id: user
}
});
async function getUserToModify(notificationId = null, userFk = null, models) {
pau marked this conversation as resolved
Review

Pq per defecte els dos parametres son null?

Pq per defecte els dos parametres son null?
if (notificationId != null) {
const subscription = await models.NotificationSubscription.findById(notificationId);
const user = await subscription.userFk;
return await models.Worker.findOne({
fields: ['id', 'bossFk'],
where: {
id: user
}
});
} else {
return await models.Worker.findOne({
fields: ['id', 'bossFk'],
where: {
id: userFk
}
});
}
}
};

View File

@ -7,15 +7,18 @@
}
},
"properties": {
"notificationFk": {
"id": {
"type": "number",
"id": true,
pau marked this conversation as resolved Outdated
Outdated
Review

Revisar que se tenga que quitar o dejar sin

Revisar que se tenga que quitar o dejar sin
Outdated
Review

Estaba asi para poder borrar por id, cambiado para que ambos tengan id pues ya no se borra asi

Estaba asi para poder borrar por id, cambiado para que ambos tengan id pues ya no se borra asi
"description": "Identifier"
"description": "Primary key"
},
"notificationFk": {
"type": "number",
"description": "Foreign key to Notification"
},
"userFk": {
"type": "number",
"id": true,
"description": "Identifier"
"description": "Foreign key to Account"
}
},
"relations": {

View File

@ -19726,9 +19726,10 @@ DROP TABLE IF EXISTS `notificationSubscription`;
/*!40101 SET @saved_cs_client = @@character_set_client */;
/*!40101 SET character_set_client = utf8 */;
CREATE TABLE `notificationSubscription` (
`Id` int(11) NOT NULL AUTO_INCREMENT,
pau marked this conversation as resolved Outdated
Outdated
Review

Minúscula

Minúscula
Outdated
Review

Esto no debería estar en el changes también???

Esto no debería estar en el changes también???
`notificationFk` int(11) NOT NULL,
`userFk` int(10) unsigned NOT NULL,
PRIMARY KEY (`notificationFk`,`userFk`),
PRIMARY KEY (`Id`),
pau marked this conversation as resolved Outdated
Outdated
Review

Minúscula

Minúscula
KEY `notificationSubscription_ibfk_2` (`userFk`),
pau marked this conversation as resolved Outdated
Outdated
Review

Hace falta añadir KEY notificationSubscription_ibfk_1 (notificationFk) ?

Hace falta añadir `KEY `notificationSubscription_ibfk_1` (`notificationFk`)` ?
CONSTRAINT `notificationSubscription_ibfk_1` FOREIGN KEY (`notificationFk`) REFERENCES `notification` (`id`) ON DELETE CASCADE ON UPDATE CASCADE,
CONSTRAINT `notificationSubscription_ibfk_2` FOREIGN KEY (`userFk`) REFERENCES `account`.`user` (`id`) ON DELETE CASCADE ON UPDATE CASCADE