verdnatura/salix
verdnatura
/
salix
3
4
Fork 0
Code Issues Pull Requests 57 Releases Wiki Activity

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
Conversation 0 Commits 37 Files Changed 8 +11 -24
2 changed files with 11 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit a55d007655 - Show all commits

20
back/models/notificationSubscription.js
View File

@ -14,7 +14,7 @@ module.exports = Self => {
});
Self.remoteMethod('deleteNotification', {
description: 'Gets the current user data',
description: 'Deletes a notification subscription',
pau marked this conversation as resolved Outdated
alexm commented 2023-01-02 07:03:23 +00:00
Outdated
Review
Copy Link

Cambiar descripción

Cambiar descripción
accepts: [
{
arg: 'ctx',
@ -22,15 +22,11 @@ module.exports = Self => {
http: {source: 'context'}
},
{
arg: 'userId',
arg: 'userFk',
pau marked this conversation as resolved Outdated
joan commented 2023-01-02 14:01:01 +00:00
Outdated
Review
Copy Link

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: 'string'
},
{
arg: 'notificationId',
type: 'number'
},
{
arg: 'authorId',
arg: 'notificationFk',
type: 'number'
}
],
@ -44,18 +40,16 @@ module.exports = Self => {
}
});
Self.deleteNotification = async function(ctx) {
Self.deleteNotification = async function(ctx, userFk, notificationFk) {
pau marked this conversation as resolved Outdated
alexm commented 2023-01-09 08:43:53 +00:00
Outdated
Review
Copy Link

No estas comprobando el usuario

No estas comprobando el usuario
pau commented 2023-01-18 06:40:48 +00:00
Outdated
Review
Copy Link

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
const models = Self.app.models;
const user = await ctx.args.authorId;
const notificationId = await ctx.args.notificationId;
const userId = await ctx.args.userId;
const modifiedUser = await getUserToModify(userId, models);
const user = ctx.req.accessToken.userId;
pau marked this conversation as resolved Outdated
alexm commented 2023-01-09 08:40:24 +00:00
Outdated
Review
Copy Link

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 modifiedUser = await getUserToModify(userFk, models);
pau marked this conversation as resolved
alexm commented 2023-01-02 07:08:03 +00:00
Review
Copy Link

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; ```
if (user == modifiedUser.id || modifiedUser.bossFk == user) {
const query = `DELETE FROM util.notificationSubscription
pau marked this conversation as resolved Outdated
alexm commented 2023-01-02 07:06:40 +00:00
Outdated
Review
Copy Link

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?
WHERE notificationFk = ? AND userFk = ?`;
pau marked this conversation as resolved Outdated
alexm commented 2023-01-18 06:59:51 +00:00
Outdated
Review
Copy Link

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
joan commented 2023-01-02 14:00:12 +00:00
Outdated
Review
Copy Link

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

Además de lo que comenta alex, quitar el await.
await Self.rawSql(query, [notificationId, userId]);
await Self.rawSql(query, [notificationFk, userFk]);
pau marked this conversation as resolved
alexm commented 2023-01-09 08:40:07 +00:00
Review
Copy Link

Pq per defecte els dos parametres son null?

Pq per defecte els dos parametres son null?
return;
} else

15
db/changes/230201/00-acl_notifications.sql
View File

@ -1,11 +1,4 @@
INSERT INTO
`salix`.`ACL` (
`model`,
`property`,
`accessType`,
`permission`,
`principalId`
)
VALUES
('NotificationSubscription', '*', '*', 'employee'),
('NotificationAcl', '*', '*', 'employee');
INSERT INTO `salix`.`ACL` (model,property,accessType,principalId)
pau marked this conversation as resolved Outdated
alexm commented 2023-01-23 06:45:44 +00:00
Outdated
Review
Copy Link

Cambiar a la versión 230401

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