Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: minor cleanup for memberships #4581

Merged
merged 1 commit into from
Apr 5, 2024
Merged

chore: minor cleanup for memberships #4581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 9 additions & 21 deletions front/lib/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,28 +129,16 @@ export class Authenticator {

if (user && workspace) {
[role, subscription, flags] = await Promise.all([
(async (): Promise<RoleType> => {
const membership =
await MembershipResource.getActiveMembershipOfUserInWorkspace({
user: renderUserType(user),
workspace: renderLightWorkspaceType({ workspace }),
});
return membership &&
// TODO(@fontanierh): get rid of the check ?
["admin", "builder", "user"].includes(membership.role)
? (membership.role as RoleType)
: "none";
})(),
MembershipResource.getActiveMembershipOfUserInWorkspace({
user: renderUserType(user),
workspace: renderLightWorkspaceType({ workspace }),
}).then((m) => m?.role ?? "none"),
subscriptionForWorkspace(workspace.sId),
(async () => {
return (
await FeatureFlag.findAll({
where: {
workspaceId: workspace.id,
},
})
).map((flag) => flag.name);
})(),
FeatureFlag.findAll({
where: {
workspaceId: workspace.id,
},
}).then((flags) => flags.map((flag) => flag.name)),
]);
}

Expand Down
2 changes: 1 addition & 1 deletion front/lib/resources/membership_resource.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we want to historicize the roles?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 !

if (!membership.endAt) {
if (membership.role === newRole) {
return new Err({ type: "already_on_role" });
Expand Down
Loading