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

Conversation

foysalit
Copy link
Contributor

@foysalit foysalit commented Feb 25, 2025

This PR adds handle and displayName columns to the member table and keeps those values in sync with member profile through a new daemon that runs every 24hrs by default.

This allows searching for team members using handle/display name.

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 👍

@foysalit foysalit marked this pull request as ready for review February 26, 2025 00:17

export type TeamServiceCreator = (db: Database) => TeamService
export type AppviewCreator = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactoring away from passing the whole AppContext since we want to be able to run this from both app and daemon.

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.

Since you take the opportunity to refactor, you should change this from being a method argument to a constructor argument:

class TeamService {
  constructor(
    protected readonly db: Database,
    protected readonly appviewAgent: AtpAgent,
    protected readonly appviewDid: string,
  ) {}

  async getProfiles(dids: string[]) {
  

    const {headers} = createAuthHeaders(this.appviewDid, method)
    const result = await this.appviewAgent(...)
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

(this is the same comment as above)

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

Copy link
Contributor

@matthieusieben matthieusieben left a comment

Choose a reason for hiding this comment

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

I think that a minor refactor here could make things a little more neat.

We should probably name the handle and displayName with a cached prefix to clearly indicate that these are not a real source of thruth.

@@ -11,6 +11,9 @@ describe('team management', () => {
beforeAll(async () => {
network = await TestNetwork.create({
dbPostgresSchema: 'ozone_team_test',
ozone: {
dbTeamProfileRefreshIntervalMs: 1000,
Copy link
Contributor

Choose a reason for hiding this comment

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

There probably need to be a "wait for 1 second since the member was added to the list" in the tests below here somewhere to prevent tests from being flaky on fast machines.

You could event change this interval to something smaller.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the information is fetched when the member is added, I don't think the refresh interval is ever really used (or perhaps I missed something). Could we disable it altogether? (Also—isn't it only used by the daemon and not ozone proper?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i believe the daemon processes are run when processAll() is called in test network and for this test, we are creating the members with handle and displayName null to kinda implicitly test that the background sync works.

const teamProfileSynchronizer = new TeamProfileSynchronizer(
backgroundQueue,
{
createAuthHeaders: (method: string) =>
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

Comment on lines 9 to 18
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.

const teamProfileSynchronizer = new TeamProfileSynchronizer(
backgroundQueue,
{
createAuthHeaders: (method: string) =>
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 👍

Comment on lines 234 to 241
const { count } = await this.db.db
.selectFrom('member')
.select(this.db.db.fn.count<number>('did').as('count'))
.executeTakeFirstOrThrow()

let lastDid = ''
// Max 25 profiles can be fetched at a time so let's pull 25 members at a time from the db and update their profile details
for (let i = 0; i < count; i += 25) {
Copy link
Collaborator

@devinivy devinivy Mar 4, 2025

Choose a reason for hiding this comment

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

Rather than fetching the count and relying on it not changing, you could alternately only use the lastDid to iterate through. I think it's sort of nice since you're already iterating by lastDid. It would look more like:

let lastDid = ''
do {
  // ...
  lastDid = dids.at(-1) || ''
} while (lastDid)

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.

@foysalit foysalit merged commit 18fbfa0 into main Mar 7, 2025
10 checks passed
@foysalit foysalit deleted the ozone-team-member-details branch March 7, 2025 23:00
@github-actions github-actions bot mentioned this pull request Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants