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
4 changed files with 83 additions and 60 deletions
Showing only changes of commit ee0cd2174f - Show all commits

View File

@ -1,56 +0,0 @@
module.exports = Self => {
Self.remoteMethod('deleteSubscription', {
description: 'delete a notification subscription',
accessType: 'WRITE',
accepts: [
{
arg: 'notificationId',
type: 'string',
required: true
},
{
arg: 'userId',
type: 'string',
required: true
}
],
returns: {
type: 'object',
root: true
},
http: {
path: `/deleteSubscription`,
verb: 'POST'
}
});
Self.deleteSubscription = async(notificationId, userId, options) => {
const myOptions = {};
let tx;
if (typeof options == 'object')
Object.assign(myOptions, options);
if (!myOptions.transaction) {
tx = await Self.beginTransaction({});
myOptions.transaction = tx;
}
try {
const query = `DELETE FROM util.notificationSubscription
WHERE notificationFk = ? AND userFk = ?`;
await Self.rawSql(query, [notificationId, userId], myOptions);
if (tx) await tx.commit();
return {success: true};
} catch (error) {
if (tx) await tx.rollback();
throw error;
}
};
};

View File

@ -1,3 +1,73 @@
const UserError = require('vn-loopback/util/user-error');
module.exports = Self => {
require('../methods/notification/deleteSubcription')(Self);
Self.observe('before save', async function(ctx) {
let models = Self.app.models;
let userId = ctx.options.accessToken.userId;
let modifiedUser = await models.Worker.findOne({
fields: ['id', 'bossFk'],
where: {
id: ctx.instance.userFk
pau marked this conversation as resolved Outdated
Outdated
Review

Pasar a funcion la parte de comprobar el usuario, ya que se repite en el before y en la ruta de delete

Pasar a funcion la parte de comprobar el usuario, ya que se repite en el before y en la ruta de delete
}
pau marked this conversation as resolved
Review

Te ahorres fer if else, fent:

        if (userId != modifiedUser.id && userId != modifiedUser.bossFk)
            throw new UserError('You dont have permission to modify this user');

I ns si faria falta ficar return al final o no

Te ahorres fer if else, fent: ``` if (userId != modifiedUser.id && userId != modifiedUser.bossFk) throw new UserError('You dont have permission to modify this user'); ``` *I ns si faria falta ficar return al final o no*
});
if (userId == modifiedUser.id || userId == modifiedUser.bossFk)
return;
else
throw new UserError('You dont have permission to modify this user');
pau marked this conversation as resolved Outdated
Outdated
Review

Cambiar descripción

Cambiar descripción
});
Self.remoteMethod('deleteNotification', {
description: 'Gets the current user data',
accepts: [
{
arg: 'ctx',
type: 'object',
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)
http: {source: 'context'}
},
{
arg: 'userId',
type: 'string'
},
{
arg: 'notificationId',
pau marked this conversation as resolved
Review

Quitar el parámetro authorId que lo tienes en el context

Quitar el parámetro authorId que lo tienes en el context
type: 'number'
},
{
arg: 'authorId',
type: 'number'
}
],
returns: {
type: 'object',
root: true
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
},
http: {
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); ```
verb: 'POST',
path: '/deleteNotification'
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; ```
}
});
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 } });
Self.deleteNotification = async function(ctx) {
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.
let models = Self.app.models;
pau marked this conversation as resolved
Review

Pq per defecte els dos parametres son null?

Pq per defecte els dos parametres son null?
let user = await ctx.args.authorId;
let notificationId = await ctx.args.notificationId;
let userId = await ctx.args.userId;
let modifiedUser = await models.Worker.findOne({
fields: ['id', 'bossFk'],
where: {
id: ctx.args.userId
}
});
if (user == modifiedUser.id || modifiedUser.bossFk == user) {
const query = `DELETE FROM util.notificationSubscription
WHERE notificationFk = ? AND userFk = ?`;
await Self.rawSql(query, [notificationId, userId]);
return;
} else
throw new UserError('You dont have permission to modify this user');
};
};

View File

@ -9,6 +9,7 @@
"properties": {
"notificationFk": {
"type": "number",
"id": true,
"description": "Identifier"
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
},
"userFk": {

View File

@ -1,3 +1,11 @@
VALUES
('NotificationSubscription','*','*','employee'),
('NotificationAcl','*','*','employee');
INSERT INTO
pau marked this conversation as resolved Outdated
Outdated
Review

Cambiar a la versión 230401

Cambiar a la versión 230401
`salix`.`ACL` (
`model`,
`property`,
`accessType`,
`permission`,
`principalId`
)
VALUES
('NotificationSubscription', '*', '*', 'employee'),
('NotificationAcl', '*', '*', 'employee');