Skip to content

Commit

Permalink
Re-enable web push for github push events
Browse files Browse the repository at this point in the history
  • Loading branch information
mikebroberts committed Nov 20, 2024
1 parent 1db9d19 commit ab82175
Show file tree
Hide file tree
Showing 6 changed files with 95 additions and 36 deletions.
9 changes: 9 additions & 0 deletions src/app/domain/github/githubPush.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { GithubPush } from '../types/GithubPush'
import { executeAndCatchConditionalCheckFailed } from '../entityStore/entityStoreOperationSupport'
import { sendToEventBridge } from '../../outboundInterfaces/eventBridgeBus'
import { saveLatestPushes } from './githubLatestPushesPerRef'
import { GithubUserId } from '../types/GithubUserId'
import { getUserIdsForAccount } from './githubMembership'

export async function processPushes(appState: AppState, pushes: GithubPush[], publishNotifications: boolean) {
if (pushes.length > 0) {
Expand Down Expand Up @@ -38,3 +40,10 @@ async function savePushes(appState: AppState, pushes: GithubPush[]) {
export function latestCommitInPush(push: Pick<GithubPush, 'commits'>) {
return push.commits[push.commits.length - 1]
}

export async function getRelatedMemberIdsForPush(
appState: AppState,
push: GithubPush
): Promise<GithubUserId[]> {
return getUserIdsForAccount(appState, push.accountId)
}
38 changes: 38 additions & 0 deletions src/app/domain/user/userNotifyable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { loadCalculatedUserSettingsOrUseDefaults } from './calculatedUserSetting
import { UserScopeReferenceData } from '../types/UserScopeReferenceData'
import { GithubWorkflowRunEvent } from '../types/GithubWorkflowRunEvent'
import { GithubWorkflowSummary } from '../types/GithubSummaries'
import { GithubRepoKey } from '../types/GithubKeys'

export async function filterWorkflowNotifyEnabled(
appState: AppState,
Expand Down Expand Up @@ -45,3 +46,40 @@ async function getWorkflowNotifyEnabledForUser(
}
return yesNotify
}

export async function filterRepoNotifyEnabled(
appState: AppState,
refData: UserScopeReferenceData,
userIds: GithubUserId[],
repo: GithubRepoKey
): Promise<GithubUserId[]> {
const enabledUserIds: GithubUserId[] = []
for (const userId of userIds) {
if (
// TODO - don't do this! This is a hack while all users have same visibility to
// avoid a huge amount of queries for large accounts
// What we want instead is some caching for underlying ref data objects
await getRepoNotifyEnabledForUser(appState, repo, {
...refData,
userId
})
) {
enabledUserIds.push(userId)
}
}
return enabledUserIds
}

async function getRepoNotifyEnabledForUser(
appState: AppState,
repo: GithubRepoKey,
refData: UserScopeReferenceData
) {
const userSettings = await loadCalculatedUserSettingsOrUseDefaults(appState, refData)
const yesNotify = userSettings.github.accounts[repo.accountId]?.repos[repo.repoId].notify
if (yesNotify === undefined) {
logger.warn(`No calculated user notify setting for repo`, { repo, userSettings })
return false
}
return yesNotify
}
43 changes: 25 additions & 18 deletions src/app/domain/webPush/cicadaEventWebPushPublisher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@ import {
} from '../github/githubWorkflowRunEvent'
import { isCicadaEventBridgeDetail } from '../../outboundInterfaces/eventBridgeBus'
import { CicadaWebNotification } from '../../outboundInterfaces/webPushWrapper'
import { filterWorkflowNotifyEnabled } from '../user/userNotifyable'
import { filterRepoNotifyEnabled, filterWorkflowNotifyEnabled } from '../user/userNotifyable'

import { loadUserScopeReferenceData } from '../github/userScopeReferenceData'
import { isGithubPush } from '../types/GithubPush'
import { getRelatedMemberIdsForPush } from '../github/githubPush'

// TOEventually - these are going to create a lot of queries for subscription lookup for large organizations
// May be better to have one table / index for this.
Expand Down Expand Up @@ -54,20 +56,25 @@ export function generateRunEventNotification(
}
}

// TOEventually - add this back now that notification preferences added, maybe
// export async function handleNewPush(appState: AppState, eventDetail: unknown) {
// if (!isCicadaEventBridgeDetail(eventDetail) || !isGithubPush(eventDetail.data)) {
// logger.error(
// `Event detail for detail-type ${EVENTBRIDGE_DETAIL_TYPES.GITHUB_NEW_PUSH} was not of expected format`,
// { commit: eventDetail }
// )
// return
// }
//
// const push = eventDetail.data
//
// await publishToSubscriptionsForUsers(appState, await getRelatedMemberIdsForPush(appState, push), {
// title: 'New Push',
// body: `New push to ${push.repoName}:${push.ref} by ${push.actor.login}`
// })
// }
export async function handleNewPush(appState: AppState, eventDetail: unknown) {
if (!isCicadaEventBridgeDetail(eventDetail) || !isGithubPush(eventDetail.data)) {
logger.error(
`Event detail for detail-type ${EVENTBRIDGE_DETAIL_TYPES.GITHUB_NEW_PUSH} was not of expected format`,
{ commit: eventDetail }
)
return
}

const push = eventDetail.data
const userIds = await getRelatedMemberIdsForPush(appState, push)
if (userIds.length === 0) return
// TODO - this is a hack to avoid terrible performance - see todo in filterWorkflowNotifyEnabled
const refData = await loadUserScopeReferenceData(appState, userIds[0])
const notifyEnabledUserIds = await filterRepoNotifyEnabled(appState, refData, userIds, push)

await publishToSubscriptionsForUsers(appState, notifyEnabledUserIds, {
title: 'New Push',
body: `New push to ${push.repoName}:${push.ref} by ${push.actor.userName}`,
data: { url: push.repoUrl ?? '' }
})
}
19 changes: 7 additions & 12 deletions src/app/domain/webPush/webPushEventBridgeEventProcessor.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
import { AppState } from '../../environment/AppState'
import { EventBridgeEvent } from 'aws-lambda'
import { EVENTBRIDGE_DETAIL_TYPES, EventBridgeDetailType } from '../../../multipleContexts/eventBridge'
import {
EVENTBRIDGE_DETAIL_TYPES,
isWebPushEventBridgeDetailType,
WebPushEventBridgeDetailType
} from '../../../multipleContexts/eventBridge'
import { logger } from '../../util/logging'
import { handleNewWorkflowRunEvent } from './cicadaEventWebPushPublisher'
import { handleNewPush, handleNewWorkflowRunEvent } from './cicadaEventWebPushPublisher'
import { handleWebPushTest } from './webPushUserTest'

export async function processEventBridgeWebPushEvent(
Expand All @@ -18,18 +22,9 @@ export async function processEventBridgeWebPushEvent(
await webPushEventProcessors[detailType](appState, event.detail)
}

// We don't necessarily listen for all event bridge events
type WebPushEventBridgeDetailType = Extract<
EventBridgeDetailType,
'GithubNewWorkflowRunEvent' | 'WebPushTest'
>

function isWebPushEventBridgeDetailType(x: unknown): x is WebPushEventBridgeDetailType {
return typeof x === 'string' && (x === 'GithubNewWorkflowRunEvent' || x === 'WebPushTest')
}

const webPushEventProcessors: Record<WebPushEventBridgeDetailType, WebPushEventProcessor> = {
[EVENTBRIDGE_DETAIL_TYPES.GITHUB_NEW_WORKFLOW_RUN_EVENT]: handleNewWorkflowRunEvent,
[EVENTBRIDGE_DETAIL_TYPES.GITHUB_NEW_PUSH]: handleNewPush,
[EVENTBRIDGE_DETAIL_TYPES.WEB_PUSH_TEST]: handleWebPushTest
}

Expand Down
9 changes: 3 additions & 6 deletions src/cdk/stacks/main/userFacingWeb.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { Duration } from 'aws-cdk-lib'
import { grantLambdaFunctionPermissionToPutEvents } from '../../support/eventbridge'
import { Rule } from 'aws-cdk-lib/aws-events'
import * as targets from 'aws-cdk-lib/aws-events-targets'
import { EVENTBRIDGE_DETAIL_TYPES } from '../../../multipleContexts/eventBridge'
import { WEBPUSH_EVENTBRIDGE_DETAIL_TYPES } from '../../../multipleContexts/eventBridge'
import { IdentitySource, LambdaIntegration, RequestAuthorizer, RestApi } from 'aws-cdk-lib/aws-apigateway'
import { HttpMethod } from 'aws-cdk-lib/aws-apigatewayv2'
import { MainStackProps } from './mainStackProps'
Expand Down Expand Up @@ -102,11 +102,8 @@ function defineWebPushPublisher(scope: Construct, props: UserFacingWebEndpointsP
description: `Send Web Push notifications`,
eventPattern: {
source: [props.appName],
detailType: [
EVENTBRIDGE_DETAIL_TYPES.GITHUB_NEW_WORKFLOW_RUN_EVENT,
EVENTBRIDGE_DETAIL_TYPES.GITHUB_NEW_PUSH,
EVENTBRIDGE_DETAIL_TYPES.WEB_PUSH_TEST
]
// Need Array.from here otherwise a type error
detailType: Array.from(WEBPUSH_EVENTBRIDGE_DETAIL_TYPES)
},
targets: [new targets.LambdaFunction(lambdaFunction)]
})
Expand Down
13 changes: 13 additions & 0 deletions src/multipleContexts/eventBridge.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// See comment at WebPushEventBridgeDetailTypes if adding more types here

export const EVENTBRIDGE_DETAIL_TYPES = {
GITHUB_NEW_PUSH: 'GithubNewPush',
GITHUB_NEW_WORKFLOW_RUN_EVENT: 'GithubNewWorkflowRunEvent',
Expand All @@ -13,3 +14,15 @@ export type EventBridgeDetailType = (typeof EVENTBRIDGE_DETAIL_TYPES)[keyof type
export function isEventBridgeDetailType(x: unknown): x is EventBridgeDetailType {
return typeof x === 'string' && Object.values(EVENTBRIDGE_DETAIL_TYPES).includes(x as EventBridgeDetailType)
}

export const WEBPUSH_EVENTBRIDGE_DETAIL_TYPES = [
EVENTBRIDGE_DETAIL_TYPES.GITHUB_NEW_PUSH,
EVENTBRIDGE_DETAIL_TYPES.GITHUB_NEW_WORKFLOW_RUN_EVENT,
EVENTBRIDGE_DETAIL_TYPES.WEB_PUSH_TEST
] as const

export type WebPushEventBridgeDetailType = (typeof WEBPUSH_EVENTBRIDGE_DETAIL_TYPES)[number]

export function isWebPushEventBridgeDetailType(x: unknown): x is WebPushEventBridgeDetailType {
return typeof x === 'string' && WEBPUSH_EVENTBRIDGE_DETAIL_TYPES.includes(x as WebPushEventBridgeDetailType)
}

0 comments on commit ab82175

Please sign in to comment.