Add admins/owner on group create #76
Conversation
| owner_id | ||
| } | ||
| // if the owner is the calling user (default), they should have been added to the | ||
| // admins list by `add_as_admin`. If they aren't, it will error later on. |
There was a problem hiding this comment.
what does this mean "will error later on"?
There was a problem hiding this comment.
The validation for it is a few lines below it. I was trying to show that the case is accounted for, even though it's not right here. Should I call this out differently?
| user_ids_and_keys: HashMap<UserId, PublicKey>, | ||
| ) -> Result< | ||
| ( | ||
| Vec<(UserId, PublicKey, TransformKey)>, |
There was a problem hiding this comment.
I think generating the transform keys in this function is not good separation of concerns. You could argue that if the number of members is huge there could be a performance reason to mix this in here so that you don't have to go over the list again, but I don't see that as a legitimate concern.
There was a problem hiding this comment.
I see this function as getting everything ready to make request::GroupMembers and request::GroupAdmins, so generating the TransformKeys here seems to fulfill that purpose to me. Are you thinking of it more as just a way to re-partition the admins and members?
There was a problem hiding this comment.
Maybe we should rename the function to note that, or add a comment to it? Really it wouldn't be that bad to have this return the 2 vecs and feed the members vec back into another function which would generate the transform keys. At the same time, this does break it up more than it was, which is incremental improvement.
I'll let you and clint figure out what needs to be done. 🏃♂️
There was a problem hiding this comment.
I still do think it would be cleaner to do the transform key generation as a separate step, but this solution isn't bad. I think the function signature makes it clear that transform keys are also being computed in here, but maybe putting a small doc comment on this function is a good idea anyway.
| user_ids_and_keys: HashMap<UserId, PublicKey>, | ||
| ) -> Result< | ||
| ( | ||
| Vec<(UserId, PublicKey, TransformKey)>, |
There was a problem hiding this comment.
Maybe we should rename the function to note that, or add a comment to it? Really it wouldn't be that bad to have this return the 2 vecs and feed the members vec back into another function which would generate the transform keys. At the same time, this does break it up more than it was, which is incremental improvement.
I'll let you and clint figure out what needs to be done. 🏃♂️
| user_ids_and_keys: HashMap<UserId, PublicKey>, | ||
| ) -> Result< | ||
| ( | ||
| Vec<(UserId, PublicKey, TransformKey)>, |
There was a problem hiding this comment.
I still do think it would be cleaner to do the transform key generation as a separate step, but this solution isn't bad. I think the function signature makes it clear that transform keys are also being computed in here, but maybe putting a small doc comment on this function is a good idea anyway.
add_as_admin,admins, andownertoGroupCreateOpts.GroupCreateOpts::standardize()return a newGroupCreateOptsStdthat is destructured bygroup_create.ownerandadmins.ownerto group response structs.?because better error messages were needed during testing.TODO: