verdnatura/salix
verdnatura
/
salix
3
5
Fork 0
Code Issues Pull Requests 49 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 +8 -10
2 changed files with 8 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit cc806cc08f - Show all commits

13
back/models/notificationSubscription.js
View File

@ -7,9 +7,7 @@ module.exports = Self => {
const user = await ctx.instance.userFk;
const modifiedUser = await getUserToModify(null, user, models);
if (userId == modifiedUser.id || userId == modifiedUser.bossFk)
return;
else
if (userId != modifiedUser.id && userId != modifiedUser.bossFk)
pau marked this conversation as resolved Outdated
alexm commented 2022-12-27 13:50:30 +00:00
Outdated
Review
Copy Link

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
alexm commented 2023-01-09 08:36:35 +00:00
Review
Copy Link

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*
});
@ -42,14 +40,13 @@ module.exports = Self => {
const user = ctx.req.accessToken.userId;
const modifiedUser = await getUserToModify(notificationId, null, models);
if (user == modifiedUser.id || modifiedUser.bossFk == user) {
await models.NotificationSubscription.destroyById(notificationId);
return;
} else
if (user != modifiedUser.id && user != modifiedUser.bossFk)
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
throw new UserError('You dont have permission to modify this user');
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); ```
await models.NotificationSubscription.destroyById(notificationId);
};
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; ```
async function getUserToModify(notificationId = null, userFk = null, models) {
async function getUserToModify(notificationId, userFk, models) {
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?
if (notificationId != null) {
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 } });
const subscription = await models.NotificationSubscription.findById(notificationId);
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.
const user = await subscription.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?

5
db/dump/structure.sql
View File

@ -19726,10 +19726,11 @@ 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,
`id` int(11) NOT NULL AUTO_INCREMENT,
pau marked this conversation as resolved Outdated
alexm commented 2023-01-09 08:31:44 +00:00
Outdated
Review
Copy Link

Minúscula

Minúscula
alexm commented 2023-01-23 06:46:36 +00:00
Outdated
Review
Copy Link

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 (`Id`),
PRIMARY KEY (`id`),
pau marked this conversation as resolved Outdated
alexm commented 2023-01-09 08:31:46 +00:00
Outdated
Review
Copy Link

Minúscula

Minúscula
KEY `notificationSubscription_ibfk_1` (`notificationFk`),
pau marked this conversation as resolved Outdated
alexm commented 2023-01-09 08:33:25 +00:00
Outdated
Review
Copy Link

Hace falta añadir KEY notificationSubscription_ibfk_1 (notificationFk) ?

Hace falta añadir `KEY `notificationSubscription_ibfk_1` (`notificationFk`)` ?
KEY `notificationSubscription_ibfk_2` (`userFk`),
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