Feature - role emails #3920
Feature - role emails #3920
Conversation
services/logs2notifications/internal/handler/useraction_events_template.go
Outdated
Show resolved
Hide resolved
services/logs2notifications/internal/handler/useraction_events_template.go
Show resolved
Hide resolved
| */ | ||
| exports.up = function(knex) { | ||
| return knex.schema.alterTable('user', function(table) { | ||
| table.boolean('org_email_optin').notNullable().defaultTo(true); |
There was a problem hiding this comment.
Might be worth changing this up a bit now to make further options in the future, thinking things like
table.boolean('opt_email_org_role').notNullable().defaultTo(true); // when a users role within the organization management changes
table.boolean('opt_email_sshkey').notNullable().defaultTo(true); // when an ssh key is added or removed from the user
table.boolean('opt_email_group_role').notNullable().defaultTo(false); // when the users role within a group is changed. this should probably default to false because it could be quite noisy for some users
There was a problem hiding this comment.
Also note with the opt_email_group_role possible notifications, another reason for default false would be for some of AIO customers that may use other systems for managing users that could already send these notifications. having it as default opt out would be best. and then users would be able to opt in specifically for this.
| @@ -29,6 +29,7 @@ export interface User { | |||
| admin?: boolean; | |||
| organizationRole?: string; | |||
| platformRoles?: [string]; | |||
| emailOptIn?: boolean; | |||
There was a problem hiding this comment.
I'm wondering if this should be an interface, to allow for future expansion
interface IEmailNotifications {
organizationRoleChanges?: boolean;
sshKeyChanges?: boolean;
groupRoleChanges?: boolean;
}
export interface User {
...
emailNotifications?: IEmailNotifications;
}
|
|
||
| // Here we deal explicitly with a class of 'user_action' events | ||
| if notification.Meta.Level == "user_action" { | ||
| if notification.Meta.Event == "api:addAdminToOrganization" { |
There was a problem hiding this comment.
This currently only fires on if a user is added as a manager to an organization. What about removal (https://github.com/uselagoon/lagoon/blob/v2.25.0/services/api/src/resources/user/resolvers.ts#L409)
Additionally, it could be worth notifying on the following:
Would we want to notify users if their platform roles changed? Maybe? This would probably be non-optional though so we don't have to expose the option to adjust it to general users
- add platform role to user: https://github.com/uselagoon/lagoon/blob/v2.25.0/services/api/src/resources/user/resolvers.ts#L633
- remove platform role from user: https://github.com/uselagoon/lagoon/blob/v2.25.0/services/api/src/resources/user/resolvers.ts#L654
Would probably be good to notify when ssh keys are added or removed from an account, github does this, others probably do too
- add sshkey: https://github.com/uselagoon/lagoon/blob/v2.25.0/services/api/src/resources/sshKey/resolvers.ts#L97
- remove sshkey: https://github.com/uselagoon/lagoon/blob/v2.25.0/services/api/src/resources/sshKey/resolvers.ts#L272
Possible future notify if a group role was changed? These actual addUserToGroup and removeUserFromGroup resolvers may need new logic in them to check if the group/role are unchanged though, otherwise if you run the same command twice with the same inputs the action is still performed and the user would receive an email saying their role was changed to the same role.
| @@ -485,10 +487,23 @@ export const addAdminToOrganization: ResolverFn = async ( | |||
| details: `${user.email} role ${role}`, | |||
| }, | |||
| }; | |||
|
|
|||
| var addAdminToOrganizationEmail = true; | |||
There was a problem hiding this comment.
rather than have the payload flag being addAdminToOrganizationEmail would be more generic to have it be named sendUserEmail and then if the other optional email notifications are added in the future, the logic would be to determine which type of notification it is in the resolver and set this flag.
then the logs2notifications service useraction event handler would only need to check if sendUserEmail=true
| type addAdmintoOrganizationUser struct { | ||
| Id string `json:"id"` | ||
| Email string `json:"email"` | ||
| Organization int `json:"organization"` | ||
| Role string `json:"role"` | ||
| } | ||
|
|
||
| type addAdminToOrganizationResource struct { | ||
| Id int `json:"id"` | ||
| Type string `json:"type"` | ||
| Details string `json:"details"` // stores the org name | ||
| } | ||
|
|
||
| // There is also a third payload struct called "linkedResource" which we could use, but it seems redundant | ||
|
|
||
| type addAdminToOrganization struct { | ||
| Meta struct { | ||
| Payload struct { | ||
| AddAdminToOrganizationEmail bool `json:"addAdminToOrganizationEmail"` | ||
| User addAdmintoOrganizationUser `json:"user"` | ||
| Resource addAdminToOrganizationResource `json:"resource"` | ||
| } `json:"payload"` | ||
| } `json:"meta"` | ||
| } |
There was a problem hiding this comment.
When auditLogs were added, we set up a new data structure in the activity payload that contained information specific to this.
We should probably do something similar for the email notifications in the resolvers that would send emails with a new structure that contains this payload data. rather than relying on information that may or may not be present in all other resolvers.
In a previous comment I mentioned replacing addAdminToOrganizationEmail with sendUserEmail, but it could even be that if the new payload is in the message then send the email instead. No need for a specific flag then, as the injected payload could be "the flag"
General Checklist
Database Migrations
Description
This PR will add the ability to email users when their organization roles change.