4797-lilium-worker-notifications #1229
|
@ -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",
|
||||||
|
|
|
@ -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
|
|||||||
|
throw new UserError('You dont have permission to modify this user');
|
||||||
pau marked this conversation as resolved
alexm
commented
Te ahorres fer if else, fent:
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
alexm
commented
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
joan
commented
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
joan
commented
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
alexm
commented
No estas comprobando el usuario No estas comprobando el usuario
pau
commented
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
alexm
commented
Te ahorres fer if else, fent:
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
alexm
commented
Y asi te ahorras poner
`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
alexm
commented
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
alexm
commented
Provar:
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
joan
commented
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
alexm
commented
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
|
||||||
|
}
|
||||||
|
});
|
||||||
|
}
|
||||||
|
};
|
|
@ -7,15 +7,18 @@
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
"properties": {
|
"properties": {
|
||||||
"notificationFk": {
|
"id": {
|
||||||
"type": "number",
|
"type": "number",
|
||||||
"id": true,
|
"id": true,
|
||||||
pau marked this conversation as resolved
Outdated
alexm
commented
Revisar que se tenga que quitar o dejar sin Revisar que se tenga que quitar o dejar sin
pau
commented
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": {
|
||||||
|
|
|
@ -0,0 +1,74 @@
|
||||||
|
const models = require('vn-loopback/server/server').models;
|
||||||
pau marked this conversation as resolved
Outdated
alexm
commented
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
alexm
commented
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;
|
||||||
|
}
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
|
@ -0,0 +1,4 @@
|
||||||
|
INSERT INTO `salix`.`ACL` (model,property,accessType,principalId)
|
||||||
|
VALUES
|
||||||
|
('NotificationSubscription','*','*','employee'),
|
||||||
pau marked this conversation as resolved
Outdated
alexm
commented
INSERT INTO INSERT INTO `salix`.`ACL` (model,property,accessType,principalId)
VALUES ('NotificationAcl','*','READ','employee');
|
|||||||
|
('NotificationAcl','*','READ','employee');
|
|
@ -0,0 +1,4 @@
|
||||||
|
ALTER TABLE
|
||||||
|
`util`.`notificationSubscription`
|
||||||
|
ADD
|
||||||
|
CONSTRAINT `notificationSubscription_UN` UNIQUE KEY (`notificationFk`, `userFk`);
|
|
@ -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`);
|
|
@ -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
|
||||||
|
|
Pasar a funcion la parte de comprobar el usuario, ya que se repite en el before y en la ruta de delete