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
Contributor
No description provided.
pau added 6 commits 2022-12-27 09:12:10 +00:00
gitea/salix/pipeline/head There was a failure building this commit Details
dfdf3d68bf
changes needed for the worker Notification manager to work
gitea/salix/pipeline/head There was a failure building this commit Details
3e08650b6f
acls for the notifications
gitea/salix/pipeline/head There was a failure building this commit Details
074763f633
rename files
gitea/salix/pipeline/head There was a failure building this commit Details
aee7cd113e
add template strings to sql
gitea/salix/pipeline/head There was a failure building this commit Details
c9468990b7
Merge branch 'dev' into 4797-lilium-worker-notifications
gitea/salix/pipeline/head This commit looks good Details
ada3e6f175
update sql folder
pau requested review from alexm 2022-12-27 09:12:46 +00:00
alexm requested changes 2022-12-27 09:18:54 +00:00
@ -0,0 +37,4 @@
myOptions.transaction = tx;
}
Member

Poner validacion del front aqui tambien

Poner validacion del front aqui tambien
pau marked this conversation as resolved
@ -9,7 +9,6 @@
"properties": {
"notificationFk": {
"type": "number",
"id": true,
Member

Revisar que se tenga que quitar o dejar sin

Revisar que se tenga que quitar o dejar sin
Author
Contributor

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
pau marked this conversation as resolved
pau added 3 commits 2022-12-27 13:32:38 +00:00
pau requested review from alexm 2022-12-27 13:33:18 +00:00
alexm requested changes 2022-12-27 13:50:34 +00:00
@ -0,0 +7,4 @@
let modifiedUser = await models.Worker.findOne({
fields: ['id', 'bossFk'],
where: {
id: ctx.instance.userFk
Member

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
pau added 1 commit 2022-12-27 13:59:37 +00:00
gitea/salix/pipeline/head This commit looks good Details
0a602892e6
changed let to const and removed duplicate code
pau requested review from alexm 2022-12-29 06:37:28 +00:00
pau added 1 commit 2022-12-29 06:37:36 +00:00
alexm requested changes 2023-01-02 07:09:01 +00:00
@ -0,0 +14,4 @@
});
Self.remoteMethod('deleteNotification', {
description: 'Gets the current user data',
Member

Cambiar descripción

Cambiar descripción
pau marked this conversation as resolved
@ -0,0 +44,4 @@
}
});
Self.deleteNotification = async function(ctx) {
Member

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
@ -0,0 +46,4 @@
Self.deleteNotification = async function(ctx) {
const models = Self.app.models;
const user = await ctx.args.authorId;
Member

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
alexm requested review from joan 2023-01-02 07:09:16 +00:00
joan requested changes 2023-01-02 14:03:14 +00:00
@ -0,0 +22,4 @@
http: {source: 'context'}
},
{
arg: 'userId',
Contributor

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)
pau marked this conversation as resolved
@ -0,0 +30,4 @@
type: 'number'
},
{
arg: 'authorId',
Contributor

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

Quitar el parámetro authorId que lo tienes en el context
pau marked this conversation as resolved
@ -0,0 +48,4 @@
const models = Self.app.models;
const user = await ctx.args.authorId;
const notificationId = await ctx.args.notificationId;
const userId = await ctx.args.userId;
Contributor

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

Además de lo que comenta alex, quitar el await.
pau marked this conversation as resolved
pau added 1 commit 2023-01-03 07:13:08 +00:00
gitea/salix/pipeline/head There was a failure building this commit Details
a55d007655
requested changes
pau added 1 commit 2023-01-03 12:33:30 +00:00
pau added 1 commit 2023-01-05 08:53:07 +00:00
pau added 2 commits 2023-01-09 07:43:32 +00:00
pau requested review from joan 2023-01-09 07:46:45 +00:00
pau requested review from alexm 2023-01-09 07:46:46 +00:00
alexm reviewed 2023-01-09 08:36:35 +00:00
@ -0,0 +8,4 @@
const modifiedUser = await getUserToModify(null, user, models);
if (userId == modifiedUser.id || userId == modifiedUser.bossFk)
return;
Member

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*
pau marked this conversation as resolved
@ -19726,9 +19726,10 @@ 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,
Member

Minúscula

Minúscula
pau marked this conversation as resolved
@ -19729,3 +19730,3 @@
`notificationFk` int(11) NOT NULL,
`userFk` int(10) unsigned NOT NULL,
PRIMARY KEY (`notificationFk`,`userFk`),
PRIMARY KEY (`Id`),
Member

Minúscula

Minúscula
pau marked this conversation as resolved
@ -19730,3 +19731,3 @@
`userFk` int(10) unsigned NOT NULL,
PRIMARY KEY (`notificationFk`,`userFk`),
PRIMARY KEY (`Id`),
KEY `notificationSubscription_ibfk_2` (`userFk`),
Member

Hace falta añadir KEY notificationSubscription_ibfk_1 (notificationFk) ?

Hace falta añadir `KEY `notificationSubscription_ibfk_1` (`notificationFk`)` ?
pau marked this conversation as resolved
alexm reviewed 2023-01-09 08:40:07 +00:00
@ -0,0 +49,4 @@
throw new UserError('You dont have permission to modify this user');
};
async function getUserToModify(notificationId = null, userFk = null, models) {
Member

Pq per defecte els dos parametres son null?

Pq per defecte els dos parametres son null?
pau marked this conversation as resolved
alexm requested changes 2023-01-09 08:40:50 +00:00
@ -0,0 +42,4 @@
const user = ctx.req.accessToken.userId;
const modifiedUser = await getUserToModify(notificationId, null, models);
if (user == modifiedUser.id || modifiedUser.bossFk == user) {
Member

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); ```
pau marked this conversation as resolved
alexm requested changes 2023-01-09 08:44:44 +00:00
@ -0,0 +40,4 @@
Self.deleteNotification = async function(ctx, notificationId) {
const models = Self.app.models;
const user = ctx.req.accessToken.userId;
const modifiedUser = await getUserToModify(notificationId, null, models);
Member

No estas comprobando el usuario

No estas comprobando el usuario
Author
Contributor

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
pau marked this conversation as resolved
joan refused to review 2023-01-12 07:26:21 +00:00
pau added 1 commit 2023-01-18 06:41:35 +00:00
gitea/salix/pipeline/head Something is wrong with the build of this commit Details
cc806cc08f
requested changes
pau requested review from alexm 2023-01-18 06:43:40 +00:00
pau added 1 commit 2023-01-18 06:43:52 +00:00
alexm reviewed 2023-01-18 06:59:51 +00:00
@ -0,0 +47,4 @@
};
async function getUserToModify(notificationId, userFk, models) {
if (notificationId != null) {
Member

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
alexm requested changes 2023-01-18 07:00:53 +00:00
alexm left a comment
Member

Provar refactor

Provar refactor
pau added 2 commits 2023-01-18 08:06:26 +00:00
pau added 1 commit 2023-01-18 11:16:58 +00:00
gitea/salix/pipeline/head This commit looks good Details
a425d54bf6
add back test
pau added 1 commit 2023-01-19 06:08:23 +00:00
gitea/salix/pipeline/head There was a failure building this commit Details
7884957acd
Merge branch 'dev' into 4797-lilium-worker-notifications
pau added 1 commit 2023-01-23 06:05:25 +00:00
gitea/salix/pipeline/head There was a failure building this commit Details
8445c89070
Merge branch 'dev' into 4797-lilium-worker-notifications
pau requested review from alexm 2023-01-23 06:05:43 +00:00
alexm requested changes 2023-01-23 06:46:49 +00:00
@ -0,0 +1,22 @@
const app = require('vn-loopback/server/server');
Member

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

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

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
pau marked this conversation as resolved
@ -0,0 +1,4 @@
INSERT INTO `salix`.`ACL` (model,property,accessType,principalId)
Member

Cambiar a la versión 230401

Cambiar a la versión 230401
pau marked this conversation as resolved
@ -19726,9 +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,
Member

Esto no debería estar en el changes también???

Esto no debería estar en el changes también???
pau marked this conversation as resolved
pau added 2 commits 2023-01-24 07:20:59 +00:00
pau added 1 commit 2023-01-24 09:11:24 +00:00
gitea/salix/pipeline/head There was a failure building this commit Details
c13bffcc57
refs #4797 @1h requested changes
pau added 1 commit 2023-01-24 09:15:45 +00:00
gitea/salix/pipeline/head This commit looks good Details
a2150b8b08
change test to adhere to the fixtures
pau requested review from alexm 2023-01-25 06:26:21 +00:00
alexm requested changes 2023-01-25 06:40:58 +00:00
@ -0,0 +1,4 @@
INSERT INTO `salix`.`ACL` (model,property,accessType,principalId)
VALUES ('NotificationSubscription','*','*','employee');
INSERT INTO `salix`.`ACL` (model,property,accessType,principalId)
Member

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

INSERT INTO `salix`.`ACL` (model,property,accessType,principalId) VALUES ('NotificationAcl','*','READ','employee');
pau marked this conversation as resolved
@ -0,0 +1,10 @@
CREATE OR REPLACE TABLE `util`.`notificationSubscription` (
Member

Has contemplat que se borren tots els registres de la taula?

Has contemplat que se borren tots els registres de la taula?
pau marked this conversation as resolved
@ -0,0 +2,4 @@
`id` int(11) NOT NULL AUTO_INCREMENT,
`notificationFk` int(11) NOT NULL,
`userFk` int(10) unsigned NOT NULL,
PRIMARY KEY (`id`),
Member

Posali una UNIQUE_KEY pq al llevar PRIMARY KEY (notificationFk,userFk), el mateix usuari pot estar dos vegades en la matiexa notificació i no deuria

Posali una UNIQUE_KEY pq al llevar *PRIMARY KEY (`notificationFk`,`userFk`)*, el mateix usuari pot estar dos vegades en la matiexa notificació i no deuria
pau marked this conversation as resolved
pau added 1 commit 2023-01-26 07:41:22 +00:00
gitea/salix/pipeline/head This commit looks good Details
3184dcd39d
requested changes
pau requested review from alexm 2023-01-26 07:51:58 +00:00
pau added 1 commit 2023-01-30 08:04:07 +00:00
pau added 1 commit 2023-01-31 11:13:22 +00:00
alexm added 1 commit 2023-02-01 07:00:16 +00:00
alexm requested changes 2023-02-01 07:05:02 +00:00
@ -0,0 +1,4 @@
ALTER TABLE
Member

deuria anar en la 230401

deuria anar en la 230401
pau marked this conversation as resolved
@ -0,0 +1,4 @@
ALTER TABLE util.notificationSubscription
ADD Id int(11) auto_increment NULL,
Member

id

**id**
pau marked this conversation as resolved
@ -0,0 +1,4 @@
ALTER TABLE util.notificationSubscription
ADD Id int(11) auto_increment NULL,
DROP PRIMARY KEY,
ADD CONSTRAINT PRIMARY KEY (Id);
Member

id

**id**
pau marked this conversation as resolved
pau added 2 commits 2023-02-01 07:42:29 +00:00
pau requested review from alexm 2023-02-01 07:43:15 +00:00
alexm added 1 commit 2023-02-01 07:49:03 +00:00
gitea/salix/pipeline/head This commit looks good Details
56f7794d87
typo
alexm approved these changes 2023-02-01 07:49:25 +00:00
pau requested review from joan 2023-02-01 07:57:05 +00:00
alexm added 1 commit 2023-02-01 08:00:59 +00:00
gitea/salix/pipeline/head There was a failure building this commit Details
39d4b53872
fix: sql fail in real structure...
joan added 1 commit 2023-02-02 07:38:16 +00:00
joan approved these changes 2023-02-02 07:39:57 +00:00
pau added 1 commit 2023-02-02 08:35:17 +00:00
pau merged commit 6807d8d5b1 into dev 2023-02-02 08:53:29 +00:00
pau deleted branch 4797-lilium-worker-notifications 2023-02-02 08:53:29 +00:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: verdnatura/salix#1229
No description provided.