Closed shpomp closed 3 years ago
@senner007 I realize now there is no way to add the owner of the board now, as a member with role 'owner'.
Should it be a separate endpoint, or just to pass the role as a parameter here? :
const addMember = async (userId, boardId, memberId) => {
if (
!Number.isInteger(Number(userId)) ||
!Number.isInteger(Number(boardId)) ||
!Number.isInteger(Number(memberId))
) {
throw new InvalidIdError('Id should be an integer');
}
await userIsOwner(userId, boardId);
const addNewMember = await knex('members').insert({
boardId,
userId: memberId,
role: 'basic',
});
return addNewMember;
};
(from members.controller)
role: 'basic',
}); return addNewMember; };
(from members.controller)
Yah, you just want to change the role: 'owner' right?
I'm thinking it makes sense to add the owner as a owner member when the board gets created, right?
@Midnighter What do you think?
Its working fine
Good :) I think you can easily finish your pr before this is merged
role: 'basic',
}); return addNewMember; };
(from members.controller)
Yah, you just want to change the role: 'owner' right?
yes. We just have no way of adding the owner with role 'owner' as a member to the member table now. Do we need it? If yes, there can be many ways to implement it. Which is the best? Passing the role as param would be the least changes 🙈
role: 'basic',
}); return addNewMember; };
(from members.controller)
Yah, you just want to change the role: 'owner' right?
yes. We just have no way of adding the owner with role 'owner' as a member to the member table now. Do we need it? If yes, there can be many ways to implement it. Which is the best? Passing the role as param would be the least changes 🙈
Ok then create a function with a parameter. When called here the parameter will be 'basic'. But also execute the function with the 'owner' parameter when the board is created
role: 'basic',
}); return addNewMember; };
(from members.controller)
Yah, you just want to change the role: 'owner' right?
yes. We just have no way of adding the owner with role 'owner' as a member to the member table now. Do we need it? If yes, there can be many ways to implement it. Which is the best? Passing the role as param would be the least changes 🙈
Ok then create a function with a parameter. When called here the parameter will be 'basic'. But also execute the function with the 'owner' parameter when the board is created
Diny suggested a very short and smart solution too:
if(userId===memberId){
const addNewMember = await knex('members').insert({
boardId,
userId: memberId,
role: 'owner',
});
return addNewMember;
}
else{
const addNewMember = await knex('members').insert({
boardId,
userId: memberId,
role: 'basic',
});
return addNewMember;
}
What you think about just that?
What you think about just that?
Fine by me if you don't mind adding the owner as member in the frontend also
What you think about just that?
Fine by me if you don't mind adding the owner as member in the frontend also
...and I would still prefer a function since you now have duplicated code
What you think about just that?
Fine by me if you don't mind adding the owner as member in the frontend also
...and I would still prefer a function since you now have duplicated code
ok! Just to make sure I don't misunderstand - by 'create a function', do you mean just to introduce the role parameter to addMember, right? I would do simply like this:
const addMember = async (userId, boardId, memberId, role) => {
if (
!Number.isInteger(Number(userId)) ||
!Number.isInteger(Number(boardId)) ||
!Number.isInteger(Number(memberId))
) {
throw new InvalidIdError('Id should be an integer');
}
if (role !== 'basic' || role !== 'owner') {
throw new IncorrectEntryError(`Role should be either 'basic' or 'owner'`);
}
await userIsOwner(userId, boardId);
const addNewMember = await knex('members').insert({
boardId,
userId: memberId,
role: role,
});
return addNewMember;
};
What you think about just that?
Fine by me if you don't mind adding the owner as member in the frontend also
...and I would still prefer a function since you now have duplicated code
ok! Just to make sure I don't misunderstand - by 'create a function', do you mean just to introduce the role parameter to addMembr, right? I would do simply like this:
const addMember = async (userId, boardId, memberId, role) => { if ( !Number.isInteger(Number(userId)) || !Number.isInteger(Number(boardId)) || !Number.isInteger(Number(memberId)) ) { throw new InvalidIdError('Id should be an integer'); } if (role !== 'basic' || role !== 'owner') { throw new IncorrectEntryError(`Role should be either 'basic' or 'owner'`); } await userIsOwner(userId, boardId); const addNewMember = await knex('members').insert({ boardId, userId: memberId, role: role, }); return addNewMember; };
That's also an option. Now the frontend has to know about the role parameter also. It might come in handy if more roles are added later
I would do simply like this:
const addMember = async (userId, boardId, memberId, role) => { if ( !Number.isInteger(Number(userId)) || !Number.isInteger(Number(boardId)) || !Number.isInteger(Number(memberId)) ) { throw new InvalidIdError('Id should be an integer'); } if (role !== 'basic' || role !== 'owner') { throw new IncorrectEntryError(`Role should be either 'basic' or 'owner'`); } await userIsOwner(userId, boardId); const addNewMember = await knex('members').insert({ boardId, userId: memberId, role: role, }); return addNewMember; };
@shpomp Please create an issue for this
I would do simply like this:
const addMember = async (userId, boardId, memberId, role) => { if ( !Number.isInteger(Number(userId)) || !Number.isInteger(Number(boardId)) || !Number.isInteger(Number(memberId)) ) { throw new InvalidIdError('Id should be an integer'); } if (role !== 'basic' || role !== 'owner') { throw new IncorrectEntryError(`Role should be either 'basic' or 'owner'`); } await userIsOwner(userId, boardId); const addNewMember = await knex('members').insert({ boardId, userId: memberId, role: role, }); return addNewMember; };
@shpomp Please create an issue for this
will do! I am working now 🧑🏭 , but will make it as soon as I get a moment
Description
userIsOwner function fix in users.controller was not designed right. Simplified the query and fixed it.
Fixes #243
How to test?
Try adding a member to the board
Checklist