chore: minor cleanup for memberships #4581
Conversation
| @@ -352,7 +352,7 @@ export class MembershipResource extends BaseResource<MembershipModel> { | |||
| } | |||
|
|
|||
| // If the membership is not terminated, we update the role in place. | |||
| // TODO(@fontanierh): check if we want to terminate + create a new membership with new role instead ? | |||
| // We do not historicize the roles. | |||
There was a problem hiding this comment.
Why don't we want to historicize the roles?
There was a problem hiding this comment.
Is there a good reason to do it at the moment? It creates more rows in the db so if we dont use we might as well skip
There was a problem hiding this comment.
I don't think those rows in the db would be costly 😄
I think it has value to know who had which role at a given moment, for example when debugging if you try to understand why a user was able to do something at some point while he's now only "member". Or if at some point we want to bill differently depending on the role but we're not there yet.
Truth is I "feel like it's cleaner" to have a new membership when you're promoted, but that's not a good enough reason to push for it 😄
Approving as is!
There was a problem hiding this comment.
You convinced me ! But I won't trust the last minute change on memberships resource so won't do in this PR. Might do the change whenever I have a minute !
Co-authored-by: Henry Fontanier <[email protected]>
Description
there's no "revoked" membership role anymore, so absence of membership alone determines if the auth
RoleTypeshould be "none"Risk
Deploy Plan