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.

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

✨ Add team member handle and displayName #3587

Merged
merged 7 commits into from
Mar 7, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
3 changes: 3 additions & 0 deletions lexicons/tools/ozone/team/listMembers.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
"parameters": {
"type": "params",
"properties": {
"q": {
"type": "string"
},
"disabled": {
"type": "boolean"
},
Expand Down
3 changes: 3 additions & 0 deletions packages/api/src/client/lexicons.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14118,6 +14118,9 @@ export const schemaDict = {
parameters: {
type: 'params',
properties: {
q: {
type: 'string',
},
disabled: {
type: 'boolean',
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const is$typed = _is$typed,
const id = 'tools.ozone.team.listMembers'

export interface QueryParams {
q?: string
disabled?: boolean
roles?: string[]
limit?: number
Expand Down
6 changes: 6 additions & 0 deletions packages/dev-env/src/ozone.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,8 @@ export class TestOzone {
await this.ctx.teamService(this.ctx.db).create({
did,
disabled: false,
handle: null,
displayName: null,
lastUpdatedBy: this.ctx.cfg.service.did,
role: 'tools.ozone.team.defs#roleAdmin',
})
Expand All @@ -127,6 +129,8 @@ export class TestOzone {
await this.ctx.teamService(this.ctx.db).create({
did,
disabled: false,
handle: null,
displayName: null,
lastUpdatedBy: this.ctx.cfg.service.did,
role: 'tools.ozone.team.defs#roleModerator',
})
Expand All @@ -137,6 +141,8 @@ export class TestOzone {
await this.ctx.teamService(this.ctx.db).create({
did,
disabled: false,
handle: null,
displayName: null,
lastUpdatedBy: this.ctx.cfg.service.did,
role: 'tools.ozone.team.defs#roleTriage',
})
Expand Down
11 changes: 10 additions & 1 deletion packages/ozone/src/api/team/addMember.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,23 @@ export default function (server: Server, ctx: AppContext) {
)
}

const appviewCreator = {
agent: ctx.appviewAgent,
createAuthHeaders: ctx.appviewAuth,
}
const profiles = await teamService.getProfiles([did], appviewCreator)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be kind of nice if adding a member would still complete even if fresh profile information couldn't be fetched.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah teamService.getProfiles handles failures internally and won't throw error if fetching profile fails.

const profile = profiles.get(did)

const member = await teamService.create({
did,
handle: profile?.handle || null,
displayName: profile?.displayName || null,
disabled: false,
role: getMemberRole(role),
lastUpdatedBy:
access.type === 'admin_token' ? 'admin_token' : access.iss,
})
const memberView = await teamService.view([member], ctx)
const memberView = await teamService.view([member], appviewCreator)
return memberView[0]
})

Expand Down
5 changes: 4 additions & 1 deletion packages/ozone/src/api/team/listMembers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ export default function (server: Server, ctx: AppContext) {
encoding: 'application/json',
body: {
cursor,
members: await teamService.view(members, ctx),
members: await teamService.view(members, {
agent: ctx.appviewAgent,
createAuthHeaders: ctx.appviewAuth.bind(ctx),
}),
},
}
},
Expand Down
5 changes: 4 additions & 1 deletion packages/ozone/src/api/team/updateMember.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ export default function (server: Server, ctx: AppContext) {
lastUpdatedBy:
access.type === 'admin_token' ? 'admin_token' : access.iss,
})
const memberView = await teamService.view([updated], ctx)
const memberView = await teamService.view([updated], {
agent: ctx.appviewAgent,
createAuthHeaders: ctx.appviewAuth.bind(ctx),
})
return memberView[0]
})

Expand Down
2 changes: 2 additions & 0 deletions packages/ozone/src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export const envToCfg = (env: OzoneEnvironment): OzoneConfig => {
poolMaxUses: env.dbPoolMaxUses,
poolIdleTimeoutMs: env.dbPoolIdleTimeoutMs,
materializedViewRefreshIntervalMs: env.dbMaterializedViewRefreshIntervalMs,
teamProfileRefreshIntervalMs: env.dbTeamProfileRefreshIntervalMs,
}

assert(env.appviewUrl, 'appviewUrl is required')
Expand Down Expand Up @@ -124,6 +125,7 @@ export type DatabaseConfig = {
poolMaxUses?: number
poolIdleTimeoutMs?: number
materializedViewRefreshIntervalMs?: number
teamProfileRefreshIntervalMs?: number
}

export type AppviewConfig = {
Expand Down
4 changes: 4 additions & 0 deletions packages/ozone/src/config/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ export const readEnv = (): OzoneEnvironment => {
dbMaterializedViewRefreshIntervalMs: envInt(
'OZONE_DB_MATERIALIZED_VIEW_REFRESH_INTERVAL_MS',
),
dbTeamProfileRefreshIntervalMs: envInt(
'OZONE_DB_TEAM_PROFILE_REFRESH_INTERVAL_MS',
),
didPlcUrl: envStr('OZONE_DID_PLC_URL'),
didCacheStaleTTL: envInt('OZONE_DID_CACHE_STALE_TTL'),
didCacheMaxTTL: envInt('OZONE_DID_CACHE_MAX_TTL'),
Expand Down Expand Up @@ -57,6 +60,7 @@ export type OzoneEnvironment = {
dbPoolMaxUses?: number
dbPoolIdleTimeoutMs?: number
dbMaterializedViewRefreshIntervalMs?: number
dbTeamProfileRefreshIntervalMs?: number
didPlcUrl?: string
didCacheStaleTTL?: number
didCacheMaxTTL?: number
Expand Down
22 changes: 22 additions & 0 deletions packages/ozone/src/daemon/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ import { BackgroundQueue } from '../background'
import { OzoneConfig, OzoneSecrets } from '../config'
import { Database } from '../db'
import { ModerationService } from '../mod-service'
import { TeamService } from '../team'
import { getSigningKeyId } from '../util'
import { EventPusher } from './event-pusher'
import { EventReverser } from './event-reverser'
import { MaterializedViewRefresher } from './materialized-view-refresher'
import { TeamProfileSynchronizer } from './team-profile-synchronizer'

export type DaemonContextOptions = {
db: Database
Expand All @@ -20,6 +22,7 @@ export type DaemonContextOptions = {
eventPusher: EventPusher
eventReverser: EventReverser
materializedViewRefresher: MaterializedViewRefresher
teamProfileSynchronizer: TeamProfileSynchronizer
}

export class DaemonContext {
Expand Down Expand Up @@ -67,6 +70,17 @@ export class DaemonContext {
appviewAgent,
createAuthHeaders,
)
const teamService = TeamService.creator()
const teamProfileSynchronizer = new TeamProfileSynchronizer(
backgroundQueue,
{
createAuthHeaders: (method: string) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This reads a bit weird but basically can't come up with a better, more context aware name that would work elsewhere as well as here.

Copy link
Contributor

@matthieusieben matthieusieben Mar 4, 2025

Choose a reason for hiding this comment

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

It might make sense to pass the appviewAgent and appviewDid as argument to the new TeamService() constructor, then used as:

const options = createAuthHeaders(this.appviewDid, 'foo.bar')
this.agent.foo.bar(params, options)

See my other comment for a more detailed example

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that pretty well too 👍

createAuthHeaders(cfg.appview.did, method),
agent: appviewAgent,
},
teamService(db),
cfg.db.teamProfileRefreshIntervalMs,
)

const eventReverser = new EventReverser(db, modService)

Expand All @@ -83,6 +97,7 @@ export class DaemonContext {
eventPusher,
eventReverser,
materializedViewRefresher,
teamProfileSynchronizer,
...(overrides ?? {}),
})
}
Expand Down Expand Up @@ -111,16 +126,22 @@ export class DaemonContext {
return this.opts.materializedViewRefresher
}

get teamProfileSynchronizer(): TeamProfileSynchronizer {
return this.opts.teamProfileSynchronizer
}

async start() {
this.eventPusher.start()
this.eventReverser.start()
this.materializedViewRefresher.start()
this.teamProfileSynchronizer.start()
}

async processAll() {
// Sequential because the materialized view values depend on the events.
await this.eventPusher.processAll()
await this.materializedViewRefresher.run()
await this.teamProfileSynchronizer.run()
}

async destroy() {
Expand All @@ -129,6 +150,7 @@ export class DaemonContext {
this.eventReverser.destroy(),
this.eventPusher.destroy(),
this.materializedViewRefresher.destroy(),
this.teamProfileSynchronizer.destroy(),
])
} finally {
await this.backgroundQueue.destroy()
Expand Down
17 changes: 17 additions & 0 deletions packages/ozone/src/daemon/team-profile-synchronizer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { HOUR } from '@atproto/common'
import { BackgroundQueue, PeriodicBackgroundTask } from '../background'
import { AppviewCreator, TeamService } from '../team'

export class TeamProfileSynchronizer extends PeriodicBackgroundTask {
constructor(
backgroundQueue: BackgroundQueue,
appviewCreator: AppviewCreator,
teamService: TeamService,
interval = 24 * HOUR,
) {
super(backgroundQueue, interval, async (_, signal) => {
if (signal.aborted) return
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to check if the signal is aborted here. This check is only needed between tasks (if multiple statements, or during loops)

Suggested change
if (signal.aborted) return

await teamService.syncMemberProfiles(appviewCreator)
})
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { Kysely } from 'kysely'

export async function up(db: Kysely<unknown>): Promise<void> {
await db.schema.alterTable('member').addColumn('handle', 'text').execute()
await db.schema
.alterTable('member')
.addColumn('displayName', 'text')
.execute()
await db.schema
.createIndex('member_display_name_idx')
.on('member')
.column('displayName')
.execute()
await db.schema
.createIndex('member_handle_idx')
.on('member')
.column('handle')
.execute()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that these indexes can be used for the ilike queries, if that is their purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh right. I'm gonna go with trgm, i think i saw an example of that somewhere else.

}

export async function down(db: Kysely<unknown>): Promise<void> {
await db.schema.alterTable('member').dropColumn('handle').execute()
await db.schema.alterTable('member').dropColumn('displayName').execute()
}
1 change: 1 addition & 0 deletions packages/ozone/src/db/migrations/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,4 @@ export * as _20241220T144630860Z from './20241220T144630860Z-stats-materialized-
export * as _20250204T003647759Z from './20250204T003647759Z-add-subject-priority-score'
export * as _20250211T003647759Z from './20250211T003647759Z-add-reporter-stats-index'
export * as _20250211T132135150Z from './20250211T132135150Z-moderation-event-message-partial-idx'
export * as _20250221T132135150Z from './20250221T132135150Z-member-details'
2 changes: 2 additions & 0 deletions packages/ozone/src/db/schema/member.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ export interface Member {
| 'tools.ozone.team.defs#roleTriage'
| 'tools.ozone.team.defs#roleModerator'
disabled: Generated<boolean>
handle: string | null
displayName: string | null
createdAt: Date
updatedAt: Date
lastUpdatedBy: string
Expand Down
3 changes: 3 additions & 0 deletions packages/ozone/src/lexicon/lexicons.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14118,6 +14118,9 @@ export const schemaDict = {
parameters: {
type: 'params',
properties: {
q: {
type: 'string',
},
disabled: {
type: 'boolean',
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const is$typed = _is$typed,
const id = 'tools.ozone.team.listMembers'

export interface QueryParams {
q?: string
disabled?: boolean
roles?: string[]
limit: number
Expand Down
Loading