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
8 changed files with 172 additions and 9 deletions

View File

@ -6,6 +6,16 @@
"table": "util.notificationAcl" "table": "util.notificationAcl"
} }
}, },
"properties":{
"notificationFk": {
"id": true,
"type": "number"
},
"roleFk":{
"id": true,
"type": "number"
}
},
"relations": { "relations": {
"notification": { "notification": {
"type": "belongsTo", "type": "belongsTo",

View File

@ -0,0 +1,62 @@
const UserError = require('vn-loopback/util/user-error');
module.exports = Self => {
Self.observe('before save', async function(ctx) {
const models = Self.app.models;
const userId = ctx.options.accessToken.userId;
const user = await ctx.instance.userFk;
const modifiedUser = await getUserToModify(null, user, models);
if (userId != modifiedUser.id && userId != modifiedUser.bossFk)
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
throw new UserError('You dont have permission to modify this user');
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*
});
Self.remoteMethod('deleteNotification', {
description: 'Deletes a notification subscription',
accepts: [
{
pau marked this conversation as resolved Outdated
Outdated
Review

Cambiar descripción

Cambiar descripción
arg: 'ctx',
type: 'object',
http: {source: 'context'}
},
{
arg: 'notificationId',
type: 'number',
required: true
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)
},
],
returns: {
type: 'object',
root: true
},
http: {
verb: 'POST',
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
path: '/deleteNotification'
}
});
Self.deleteNotification = async function(ctx, notificationId) {
const models = Self.app.models;
const user = ctx.req.accessToken.userId;
const modifiedUser = await getUserToModify(notificationId, null, models);
if (user != modifiedUser.id && user != modifiedUser.bossFk)
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
throw new UserError('You dont have permission to modify this 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); ```
await models.NotificationSubscription.destroyById(notificationId);
};
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; ```
async function getUserToModify(notificationId, userFk, models) {
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?
let userToModify = userFk;
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 } });
if (notificationId) {
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.
const subscription = await models.NotificationSubscription.findById(notificationId);
pau marked this conversation as resolved
Review

Pq per defecte els dos parametres son null?

Pq per defecte els dos parametres son null?
userToModify = subscription.userFk;
}
return await models.Worker.findOne({
fields: ['id', 'bossFk'],
where: {
id: userToModify
}
});
}
};

View File

@ -7,15 +7,18 @@
} }
}, },
"properties": { "properties": {
"notificationFk": { "id": {
"type": "number", "type": "number",
"id": true, "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": { "userFk": {
"type": "number", "type": "number",
"id": true, "description": "Foreign key to Account"
"description": "Identifier"
} }
}, },
"relations": { "relations": {

View File

@ -0,0 +1,74 @@
const models = require('vn-loopback/server/server').models;
pau marked this conversation as resolved Outdated
Outdated
Review

const models = require('vn-loopback/server/server').models;

const models = require('vn-loopback/server/server').models;
describe('loopback model NotificationSubscription', () => {
it('Should fail to delete a notification if the user is not editing itself or a subordinate', async() => {
pau marked this conversation as resolved
Review

Poner un caso en el que te deje si eres tu mismo y otro que te deje si es tu jefe, te hará falta transaccionar la ruta para poder deshacer el delete

Poner un caso en el que te deje si eres tu mismo y otro que te deje si es tu jefe, te hará falta transaccionar la ruta para poder deshacer el delete
const tx = await models.NotificationSubscription.beginTransaction({});
try {
const options = {transaction: tx};
const user = 9;
const notificationSubscriptionId = 2;
const ctx = {req: {accessToken: {userId: user}}};
const notification = await models.NotificationSubscription.findById(notificationSubscriptionId);
let error;
try {
await models.NotificationSubscription.deleteNotification(ctx, notification.id, options);
} catch (e) {
error = e;
}
expect(error.message).toContain('You dont have permission to modify this user');
await tx.rollback();
} catch (e) {
await tx.rollback();
throw e;
}
});
it('Should delete a notification if the user is editing itself', async() => {
const tx = await models.NotificationSubscription.beginTransaction({});
try {
const options = {transaction: tx};
const user = 9;
const notificationSubscriptionId = 4;
const ctx = {req: {accessToken: {userId: user}}};
const notification = await models.NotificationSubscription.findById(notificationSubscriptionId);
await models.NotificationSubscription.deleteNotification(ctx, notification.id, options);
const deletedNotification = await models.NotificationSubscription.findById(notificationSubscriptionId);
expect(deletedNotification).toBeNull();
await tx.rollback();
} catch (e) {
await tx.rollback();
throw e;
}
});
it('Should delete a notification if the user is editing a subordinate', async() => {
const tx = await models.NotificationSubscription.beginTransaction({});
try {
const options = {transaction: tx};
const user = 9;
const notificationSubscriptionId = 5;
const ctx = {req: {accessToken: {userId: user}}};
const notification = await models.NotificationSubscription.findById(notificationSubscriptionId);
await models.NotificationSubscription.deleteNotification(ctx, notification.id, options);
const deletedNotification = await models.NotificationSubscription.findById(notificationSubscriptionId);
expect(deletedNotification).toBeNull();
await tx.rollback();
} catch (e) {
await tx.rollback();
throw e;
}
});
});

View File

@ -0,0 +1,4 @@
INSERT INTO `salix`.`ACL` (model,property,accessType,principalId)
VALUES
('NotificationSubscription','*','*','employee'),
pau marked this conversation as resolved Outdated
Outdated
Review

INSERT INTO salix.ACL (model,property,accessType,principalId)
VALUES ('NotificationAcl','*','READ','employee');

INSERT INTO `salix`.`ACL` (model,property,accessType,principalId) VALUES ('NotificationAcl','*','READ','employee');
('NotificationAcl','*','READ','employee');

View File

@ -0,0 +1,4 @@
ALTER TABLE
`util`.`notificationSubscription`
ADD
CONSTRAINT `notificationSubscription_UN` UNIQUE KEY (`notificationFk`, `userFk`);

View File

@ -0,0 +1,7 @@
ALTER TABLE `util`.`notificationSubscription`
ADD `id` int(11) auto_increment NULL,
DROP PRIMARY KEY,
ADD CONSTRAINT PRIMARY KEY (`id`);
ALTER TABLE `util`.`notificationSubscription`
ADD KEY `notificationSubscription_ibfk_1` (`notificationFk`);

View File

@ -1954,10 +1954,6 @@ INSERT INTO `vn`.`workerBusinessType` (`id`, `name`, `isFullTime`, `isPermanent`
(100, 'INDEFINIDO A TIEMPO COMPLETO', 1, 1, 1), (100, 'INDEFINIDO A TIEMPO COMPLETO', 1, 1, 1),
(109, 'CONVERSION DE TEMPORAL EN INDEFINIDO T.COMPLETO', 1, 1, 1); (109, 'CONVERSION DE TEMPORAL EN INDEFINIDO T.COMPLETO', 1, 1, 1);
INSERT INTO `vn`.`businessCategory` (`id`, `description`, `rate`)
VALUES
(1, 'basic employee', 1);
UPDATE `vn`.`business` b UPDATE `vn`.`business` b
SET `rate` = 7, SET `rate` = 7,
`workerBusinessCategoryFk` = 1, `workerBusinessCategoryFk` = 1,
@ -2705,7 +2701,10 @@ INSERT INTO `util`.`notificationSubscription` (`notificationFk`, `userFk`)
VALUES VALUES
(1, 1109), (1, 1109),
(1, 1110), (1, 1110),
(3, 1109); (3, 1109),
(1,9),
(1,3);
INSERT INTO `vn`.`routeConfig` (`id`, `defaultWorkCenterFk`) INSERT INTO `vn`.`routeConfig` (`id`, `defaultWorkCenterFk`)
VALUES VALUES