Migrating modification of Accounts to Entity layer#527
Conversation
ref https://linear.app/ghost/issue/AP-1071 This allows us to move away from the `updateAccount` implementation in AccountService which writes directly to the DB and then emits an `account.updated` event. Instead we can use the entity system and follow the existing patterns
ref https://linear.app/ghost/issue/AP-1071 This gives us a way to modify the profile fields of an Account entity so that we can later save them and have the Update activity sent to federated servers.
WalkthroughThis change introduces a new event-driven mechanism for handling account profile updates. A new Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/account/account.repository.knex.integration.test.ts (2)
215-218: Consider testing partial updates.The test only verifies updating both name and bio. Consider also testing scenarios where only a subset of profile fields are updated to ensure partial updates work correctly.
- accountEntity.updateProfile({ - name: 'Updated Name', - bio: 'Updated Bio', - }); + // Test partial update + accountEntity.updateProfile({ + name: 'Updated Name', + }); + + // Later verify only name was updated + expect(updatedAccount.name).toBe('Updated Name'); + expect(updatedAccount.bio).toBe(account.bio);
229-231: Consider verifying event content more thoroughly.You're checking that the event contains the account entity, but not verifying the actual updated values within the account. Consider expanding this to validate the content of the account within the event.
// Verify that the event contains the account const event = emitSpy.mock.calls[0][1] as AccountUpdatedEvent; expect(event.getAccount()).toBe(accountEntity); + expect(event.getAccount().name).toBe('Updated Name'); + expect(event.getAccount().bio).toBe('Updated Bio');src/account/account.entity.ts (2)
101-117: Consider validating input in updateProfile method.The
updateProfilemethod currently accepts any values without validation. Consider adding basic validation to ensure data quality.updateProfile(params: ProfileUpdateParams): void { if (params.name !== undefined) { + // Optional validation for name (e.g., length, format) this._name = params.name; } if (params.bio !== undefined) { + // Optional validation for bio (e.g., length) this._bio = params.bio; } if (params.avatarUrl !== undefined) { + // Optional validation for avatarUrl (e.g., valid URL format if not null) this._avatarUrl = params.avatarUrl; } if (params.bannerImageUrl !== undefined) { + // Optional validation for bannerImageUrl (e.g., valid URL format if not null) this._bannerImageUrl = params.bannerImageUrl; } }
58-62: Consider grouping field initializations.The initialization of private fields is separate from their declaration. Consider initializing these fields directly in the declaration for better readability and to avoid potential issues if initialization is missed.
-private _name: string | null; -private _bio: string | null; -private _avatarUrl: URL | null; -private _bannerImageUrl: URL | null; +private _name: string | null = null; +private _bio: string | null = null; +private _avatarUrl: URL | null = null; +private _bannerImageUrl: URL | null = null; constructor( public readonly id: number | null, uuid: string | null, public readonly username: string, name: string | null, bio: string | null, avatarUrl: URL | null, bannerImageUrl: URL | null, private readonly site: AccountSite | null, apId: URL | null, url: URL | null, apFollowers: URL | null, ) { super(id); this._name = name; this._bio = bio; this._avatarUrl = avatarUrl; this._bannerImageUrl = bannerImageUrl;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/account/account-updated.event.ts(1 hunks)src/account/account.entity.ts(2 hunks)src/account/account.entity.unit.test.ts(1 hunks)src/account/account.repository.knex.integration.test.ts(3 hunks)src/account/account.repository.knex.ts(2 hunks)src/activitypub/fediverse-bridge.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/activitypub/fediverse-bridge.ts (1)
src/account/account-updated.event.ts (1)
AccountUpdatedEvent(3-13)
src/account/account.repository.knex.integration.test.ts (4)
src/core/events.ts (1)
AsyncEvents(3-17)src/account/account.repository.knex.ts (1)
KnexAccountRepository(9-142)src/account/account.entity.ts (1)
Account(32-188)src/account/account-updated.event.ts (1)
AccountUpdatedEvent(3-13)
src/account/account.repository.knex.ts (2)
src/account/account.entity.ts (1)
Account(32-188)src/account/account-updated.event.ts (1)
AccountUpdatedEvent(3-13)
🔇 Additional comments (11)
src/account/account-updated.event.ts (1)
1-13: Clean implementation for the new event class.This is a well-structured event class that encapsulates the updated Account instance. The static
getName()method provides a central place to define the event name, improving maintainability.The implementation follows good practices by:
- Using a private readonly field for immutability
- Providing a getter method for encapsulation
- Following the existing event pattern in the codebase
src/account/account.repository.knex.ts (2)
6-6: LGTM!The import is correctly added to support the new event emission.
15-35:Details
❓ Verification inconclusive
Well-structured implementation for account updates.
The new
savemethod correctly:
- Handles existing accounts only (with a clear error for new accounts)
- Updates the database with the account's current property values
- Emits the
AccountUpdatedEventafter the database update is completeThe method uses the getters introduced in the Account entity, maintaining proper encapsulation.
🏁 Script executed:
#!/bin/bash # Verify if there are existing uses of account updates that should be migrated to this new method rg -A 2 "UPDATE.*accounts" --glob "*.ts" | grep -v "save(account"Length of output: 67
Account Update Save Method Verified – Manual Check Advised
The updated
savemethod insrc/account/account.repository.knex.tscorrectly handles account updates by:
- Throwing an error for new accounts.
- Performing the update on the
accountstable with the current property values.- Emitting an
AccountUpdatedEventafter the update.A verification script was run to locate other direct update calls to the
accountstable in TypeScript files. No extra occurrences were identified. However, since the script’s output was empty, please manually verify that no direct update queries exist elsewhere in the codebase.src/account/account.entity.unit.test.ts (1)
71-247: Comprehensive test coverage for profile updates.The tests are thorough and well-structured, covering:
- Individual field updates (name, bio, avatarUrl, bannerImageUrl)
- Multiple field updates in a single operation
- Setting values to null
Each test properly verifies that:
- The specified fields are updated correctly
- Unspecified fields remain unchanged
This testing approach ensures the
updateProfilemethod works as expected across various scenarios.src/activitypub/fediverse-bridge.ts (3)
5-5: LGTM!The import is correctly added for the new event handling.
17-20: Good event registration pattern.The event handler registration follows the existing pattern in the code, maintaining consistency.
83-111:Details
✅ Verification successful
Well-implemented event handler with proper checks.
The new handler correctly:
- Extracts the account from the event
- Verifies it's an internal account (early return if not)
- Creates and sends an ActivityPub Update activity to the account's followers
The implementation properly uses the entity's properties (
apIdandapFollowers) rather than string URLs, aligning with the entity encapsulation changes.
🏁 Script executed:
#!/bin/bash # Verify that both event handlers (old and new) are being used and that there's no duplication rg -A 2 "account\.updated" --glob "*.ts" | grep -v "AccountUpdatedEvent"Length of output: 1713
Event Handler Update Verified – Code Looks Good
The new event handler in
src/activitypub/fediverse-bridge.tsis implemented correctly:
- It extracts the account from the event and exits early if the account isn’t internal.
- It builds and broadcasts an ActivityPub Update using the account’s encapsulated properties (
apIdandapFollowers).- Global database update and activity sending are handled as expected.
- Our grep-check confirms that the
account.updatedevent is registered only once across the codebase, indicating there’s no duplication.Note: Please verify that the event listener binding (
this.events.on('account.updated', this.handleAccountUpdate.bind(this));) correctly points to the new handler—either directly or via a proper alias tohandleAccountUpdatedEvent.src/account/account.repository.knex.integration.test.ts (1)
187-240: Well-structured integration test with good logical flow.This integration test effectively validates the new account profile update functionality. The test follows a clear structure:
- Sets up the repository and spies on events
- Retrieves an existing account and creates an entity
- Updates profile fields with the new
updateProfilemethod- Verifies both event emission and database updates
The test provides good coverage for the core functionality, validating both the event emission and persistence aspects.
src/account/account.entity.ts (3)
25-30: Good use of optional properties for partial updates.The
ProfileUpdateParamsinterface effectively uses TypeScript's optional properties to allow partial updates to the account profile, which is a clean approach for supporting selective field updates.
38-41: Good encapsulation of profile fields.Converting the profile fields from public properties to private fields with public getters improves encapsulation, making the Account entity more maintainable by controlling how these fields can be modified.
85-99: Clean implementation of getters.The getters for the profile fields are clean and straightforward, following best practices for encapsulation.
|
|
||
| export class AccountUpdatedEvent { | ||
| static getName(): string { | ||
| return 'account.updated.event'; |
There was a problem hiding this comment.
This could just be account.updated, but I think it would contradict the current event. Maybe something to clean up later?
There was a problem hiding this comment.
Yeah exactly, that name is already taken so I went with this - it's abstracted away in this method so easy to change later
| return this._bannerImageUrl; | ||
| } | ||
|
|
||
| updateProfile(params: ProfileUpdateParams): void { |
There was a problem hiding this comment.
Do we need to add username here? Or maybe that's coming as part of other changes(custom handles)?
There was a problem hiding this comment.
Yeah that will come as part of the username change work
| async save(account: Account): Promise<void> { | ||
| if (account.isNew) { | ||
| throw new Error( | ||
| 'Saving of new Accounts has not been implemented yet.', |
There was a problem hiding this comment.
We can rename this method to update and use it only for this purpose, and have a different implementation for save. My thinking is that this method is updating the account and sending update event.
There was a problem hiding this comment.
Our repository pattern uses a single save even for both update and saves, this means that callers of the code don't need to know if the entity they have a reference to is new or not.
We could change this if you think it's worth it but I think it needs to be part of a larger discussion and the PostRepository would need to be updated too
There was a problem hiding this comment.
In that case, it’s fine. I get that the single save method simplifies things in PostRepository, we can keep the same patter here.
|
|
||
| export class AccountUpdatedEvent { | ||
| static getName(): string { | ||
| return 'account.updated.event'; |
There was a problem hiding this comment.
Yeah exactly, that name is already taken so I went with this - it's abstracted away in this method so easy to change later
| return this._bannerImageUrl; | ||
| } | ||
|
|
||
| updateProfile(params: ProfileUpdateParams): void { |
There was a problem hiding this comment.
Yeah that will come as part of the username change work
| async save(account: Account): Promise<void> { | ||
| if (account.isNew) { | ||
| throw new Error( | ||
| 'Saving of new Accounts has not been implemented yet.', |
There was a problem hiding this comment.
Our repository pattern uses a single save even for both update and saves, this means that callers of the code don't need to know if the entity they have a reference to is new or not.
We could change this if you think it's worth it but I think it needs to be part of a larger discussion and the PostRepository would need to be updated too
2 participants
ref https://linear.app/ghost/issue/AP-1071
These are some building blocks for updating an Accounts profile fields