Skip to content

Conversation

@mike182uk
Copy link
Member

ref https://linear.app/ghost/issue/AP-901

Update the system to emit a account.followed event when an account is followed. This event is then picked up by the NotificationEventService which will insert a notification into the database for the account being followed.

@coderabbitai
Copy link

coderabbitai bot commented Mar 26, 2025

Walkthrough

This pull request introduces event-driven handling for account follow actions. A new event class, AccountFollowedEvent, has been added to encapsulate information when an account is followed, including methods to retrieve the involved accounts and to provide an event identifier. The AccountService is updated to emit the event after a successful follow record, ensuring that duplicate follow attempts do not trigger duplicate events. Integration tests are enhanced to verify both the emission of this event and proper handling of duplicate follow cases. Additionally, a new NotificationEventService has been added to listen for the AccountFollowedEvent and delegate follow notification creation to the NotificationService, which now includes a new method for creating follow notifications and an updated NotificationType enum. Unit tests for the NotificationEventService have also been introduced.

Possibly related PRs

  • Added wire-up for new notifications endpoint #422: The changes in the main PR, specifically the introduction of the AccountFollowedEvent class and its usage in the recordAccountFollow method, are directly related to the modifications in the retrieved PR, which includes the NotificationService that handles notifications for events like account following.
  • Added notifications service implementation #425: The changes in the main PR, specifically the introduction of the AccountFollowedEvent class and its usage in the recordAccountFollow method, are directly related to the modifications in the retrieved PR, which also deals with notifications, including the handling of follow events.

Suggested reviewers

  • allouis
  • vershwal
✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

import { AccountFollowedEvent } from 'account/account-followed.event';
import type { NotificationService } from 'notification/notification.service';

export class NotificationEventService {
Copy link
Member Author

Choose a reason for hiding this comment

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

The naming of this feels a little wonky but I wasn't sure what else to call it 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I don’t have a better name in mind right now either 😅 but happy to revisit if something clicks later.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (6)
src/notification/notification-event.service.unit.test.ts (3)

15-15: Remove unused variable.

The account variable is declared but never used in the test suite.

-    let account: Account;

36-53: LGTM! Good test coverage for event handling.

The test properly verifies that the notification service's createFollowNotification method is called with the correct parameters when an account follow event is emitted.

However, consider using more complete mock objects that match the Account interface instead of using type assertions.

-            const account = { id: 123 };
-            const followerAccount = { id: 456 };
+            // Create more complete mock objects that match the Account interface
+            const account: Account = { 
+                id: 123,
+                // Add other required Account properties
+            };
+            const followerAccount: Account = { 
+                id: 456,
+                // Add other required Account properties
+            };

             events.emit(
                 AccountFollowedEvent.getName(),
                 new AccountFollowedEvent(
-                    account as Account,
-                    followerAccount as Account,
+                    account,
+                    followerAccount,
                 ),
             );

17-29: Consider testing error handling scenarios.

The current test only verifies the happy path where notification creation succeeds. Consider adding tests for error scenarios to verify that errors from the notification service are properly handled.

You could add a test like this:

it('should handle errors when creating a follow notification', () => {
    // Setup notification service to throw an error
    const error = new Error('Failed to create notification');
    (notificationService.createFollowNotification as any).mockRejectedValue(error);
    
    // Create spy for console.error
    const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
    
    const account = { id: 123 };
    const followerAccount = { id: 456 };
    
    // Emit the event
    events.emit(
        AccountFollowedEvent.getName(),
        new AccountFollowedEvent(
            account as Account,
            followerAccount as Account,
        ),
    );
    
    // Verify error was handled correctly
    expect(consoleSpy).toHaveBeenCalledWith(
        'Failed to create follow notification:',
        error
    );
    
    // Clean up
    consoleSpy.mockRestore();
});
src/account/account.service.ts (1)

188-191: Event emission for successful follows.

The event is properly emitted after confirming a successful insertion, passing both accounts as event data.

Consider adding error handling around the event emission:

+try {
   await this.events.emitAsync(
       AccountFollowedEvent.getName(),
       new AccountFollowedEvent(followee, follower),
   );
+} catch (error) {
+   // Log the error but don't rethrow since the follow was already recorded
+   console.error('Failed to emit account.followed event:', error);
+}
src/notification/notification.service.integration.test.ts (1)

267-313: Well-structured test for follow notification creation.

The test properly verifies that a follow notification is created with the correct data in the database. The test setup is thorough and includes all necessary database preparations.

Consider adding a test case for the error scenario when a user is not found for the account:

it('should throw an error if user is not found for account', async () => {
    const notificationService = new NotificationService(client);
    
    const accountWithoutUser = {
        id: 999, // Non-existent account ID
    } as Account;
    
    const followerAccount = {
        id: 1,
    } as Account;
    
    await expect(
        notificationService.createFollowNotification(
            accountWithoutUser,
            followerAccount
        )
    ).rejects.toThrow('User not found for account: 999');
});
src/notification/notification.service.ts (1)

144-165: Well-implemented follow notification creation method.

The method correctly:

  1. Finds the user associated with the followed account
  2. Handles the error case when no user is found
  3. Creates a notification with the appropriate data

Consider adding transaction support for database operations to ensure atomicity:

 async createFollowNotification(account: Account, followerAccount: Account) {
-    const user = await this.db('users')
-        .where('account_id', account.id)
-        .select('id')
-        .first();
-
-    if (!user) {
-        throw new Error(`User not found for account: ${account.id}`);
-    }
-
-    await this.db('notifications').insert({
-        user_id: user.id,
-        account_id: followerAccount.id,
-        event_type: NotificationType.Follow,
-    });
+    return this.db.transaction(async (trx) => {
+        const user = await trx('users')
+            .where('account_id', account.id)
+            .select('id')
+            .first();
+
+        if (!user) {
+            throw new Error(`User not found for account: ${account.id}`);
+        }
+
+        await trx('notifications').insert({
+            user_id: user.id,
+            account_id: followerAccount.id,
+            event_type: NotificationType.Follow,
+        });
+    });
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d775402 and 201c80f.

📒 Files selected for processing (8)
  • src/account/account-followed.event.ts (1 hunks)
  • src/account/account.service.integration.test.ts (3 hunks)
  • src/account/account.service.ts (2 hunks)
  • src/app.ts (2 hunks)
  • src/notification/notification-event.service.ts (1 hunks)
  • src/notification/notification-event.service.unit.test.ts (1 hunks)
  • src/notification/notification.service.integration.test.ts (2 hunks)
  • src/notification/notification.service.ts (2 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
src/notification/notification-event.service.ts (2)
src/notification/notification.service.ts (1)
  • NotificationService (54-166)
src/account/account-followed.event.ts (1)
  • AccountFollowedEvent (3-20)
src/app.ts (1)
src/notification/notification-event.service.ts (1)
  • NotificationEventService (5-24)
src/notification/notification.service.integration.test.ts (1)
src/notification/notification.service.ts (1)
  • NotificationService (54-166)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Build, Test and Push
🔇 Additional comments (12)
src/app.ts (1)

279-284: LGTM! Good integration of the notification event service.

The NotificationEventService is correctly instantiated with the necessary dependencies and initialized, which will enable the account follow notifications to work properly.

src/account/account-followed.event.ts (1)

3-20: LGTM! Well-structured event class.

The AccountFollowedEvent class is well designed with clear responsibilities, proper encapsulation of data, and good accessor methods. The static getName() method provides a clean way to reference the event type.

src/account/account.service.ts (3)

18-18: Import looks good.

This import supports the new event emission feature for account follows.


176-183: Capturing insert count for follow record - good approach.

Storing the insert count allows you to conditionally emit events only when a follow is actually created, avoiding duplicate notifications.


184-186: Early return for duplicate follows prevents duplicate events.

This check ensures that when a follow already exists and no new row is inserted (insertCount = 0), the function returns early without emitting an event.

src/notification/notification.service.integration.test.ts (1)

7-8: Import additions are appropriate.

The imports provide the necessary types and enums for the new test cases.

src/notification/notification.service.ts (2)

3-3: Import for Account type is appropriate.

This import provides the type needed for the new method parameter.


6-11: Well-defined notification type enum.

The enum provides a clear structure for different notification types with Follow added as a new type.

src/account/account.service.integration.test.ts (4)

14-14: Import for new event class is appropriate.

This import supports testing the new event emission functionality.


51-51: Using real timers by default is a good practice.

Setting up the test environment with real timers helps ensure tests run in a predictable way, with fake timers used only for specific tests that need it.


279-304: Comprehensive test for account follow event emission.

This test effectively verifies that:

  1. The event is emitted when an account is followed
  2. The event contains the correct account and follower data
  3. The event is awaited correctly using vi.waitFor

306-330: Good test for duplicate follow event handling.

This test correctly verifies that duplicate follows don't trigger multiple events.
The use of fake timers and event counting is an effective approach.

Comment on lines +151 to +154
const user = await this.db('users')
.where('account_id', account.id)
.select('id')
.first();
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have access to the user from the Account so we have manually look this up

@@ -0,0 +1,20 @@
import type { Account } from 'account/types';
Copy link
Member Author

Choose a reason for hiding this comment

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

I know we wanted to get away from using the Account type, but it felt like too big a jump to do as part of this work

To use the Account entity AccountService.recordAccountFollow would need to be updated to accept Account entity instead of Account type

AccountService.recordAccountFollow is used in the Follow & Accept handlers to resolve the follower / followee. If one of these does not exist we call AccountService.createExternalAccount which returns Account type and we then pass this to AccountService.recordAccountFollow. This means we would need to update AccountService.createExternalAccount to return an Account entity, which is used in the Follow & Accept handlers as well as followAction and unfollowAction (and some tests).

I feel this migration would be more appropriate outside of the scope of this PR (i know this technically adds more work to migration) to keep the changeset small / focused on what it is actually doing.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

It’s always nicer to clean things up when we’re in the area and context loaded, but I had a look at the recordAccountFollow, createExternalAccount and related test, and yeah, this one’s definitely a bit too much to squeeze into this PR. Would’ve made it way harder to review/understand the main logic of this PR.

Makes sense to leave it out for now.
Just wondering — are we thinking of doing that migration all at once later, or more incrementally? Like maybe you plan to do a quick follow-up PR after this to clean it up?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeh i think it will probably be incremental. Once I've got this in, I'll make the changes noted above in a separate PR and ping you 👍

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (5)
src/notification/notification-event.service.unit.test.ts (2)

32-34: Consider removing vi.clearAllMocks() as it's redundant

Since you're using vi.useFakeTimers() in the beforeEach hook but not actually using timer functionality in this test, consider removing both the fake timers setup and the clearAllMocks call for cleaner test code.

 afterEach(() => {
-    vi.clearAllMocks();
 });

36-52: Consider adding cleanup for event listeners

While the test works correctly, it would be good practice to clean up event listeners after the test to prevent potential memory leaks in larger test suites.

 describe('handling an account follow', () => {
+    afterEach(() => {
+        events.removeAllListeners();
+    });
+
     it('should create a follow notification', () => {
         const account = { id: 123 };
         const followerAccount = { id: 456 };
src/notification/notification.service.integration.test.ts (1)

301-304: Consider testing error case for user not found

The implementation of createFollowNotification throws an error when no user is found for an account, but there's no test covering this error path. Consider adding a test for this scenario.

 it('should create a follow notification', async () => {
     // existing test code
 });

+it('should throw an error when user is not found for account', async () => {
+    const notificationService = new NotificationService(client);
+
+    const nonExistentAccount = {
+        id: 9999
+    } as Account;
+
+    const followerAccount = {
+        id: 8888
+    } as Account;
+
+    await expect(
+        notificationService.createFollowNotification(
+            nonExistentAccount,
+            followerAccount
+        )
+    ).rejects.toThrow(`User not found for account: ${nonExistentAccount.id}`);
+});
src/notification/notification.service.ts (1)

150-165: Consider returning the notification ID

The method currently doesn't return anything after creating the notification. Consider returning the ID of the created notification to make it more versatile for clients that might need to reference it.

 async createFollowNotification(account: Account, followerAccount: Account) {
     const user = await this.db('users')
         .where('account_id', account.id)
         .select('id')
         .first();

     if (!user) {
         throw new Error(`User not found for account: ${account.id}`);
     }

-    await this.db('notifications').insert({
+    const [notificationId] = await this.db('notifications').insert({
         user_id: user.id,
         account_id: followerAccount.id,
         event_type: NotificationType.Follow,
     });
+    
+    return notificationId;
 }
src/account/account.service.integration.test.ts (1)

279-304: Consider cleaning up event listeners

While the tests work correctly, it would be good practice to clean up event listeners after each test to prevent potential memory leaks in larger test suites.

 it('should emit an account.followed event', async () => {
     let accountFollowedEvent: AccountFollowedEvent | undefined;
+    const eventListener = (event) => {
+        accountFollowedEvent = event;
+    };
 
-    events.on(AccountFollowedEvent.getName(), (event) => {
-        accountFollowedEvent = event;
-    });
+    events.on(AccountFollowedEvent.getName(), eventListener);
 
     const account = await service.createInternalAccount(site, {
         ...internalAccountData,
         username: 'account',
     });
     const follower = await service.createInternalAccount(site, {
         ...internalAccountData,
         username: 'follower',
     });
 
     await service.recordAccountFollow(account, follower);
 
     await vi.waitFor(() => {
         return accountFollowedEvent !== undefined;
     });
 
     expect(accountFollowedEvent).toBeDefined();
     expect(accountFollowedEvent?.getAccount()).toBe(account);
     expect(accountFollowedEvent?.getFollower()).toBe(follower);
+    
+    events.off(AccountFollowedEvent.getName(), eventListener);
 });

Apply a similar pattern to the second test as well for consistency.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 201c80f and 7c5353d.

📒 Files selected for processing (8)
  • src/account/account-followed.event.ts (1 hunks)
  • src/account/account.service.integration.test.ts (3 hunks)
  • src/account/account.service.ts (2 hunks)
  • src/app.ts (2 hunks)
  • src/notification/notification-event.service.ts (1 hunks)
  • src/notification/notification-event.service.unit.test.ts (1 hunks)
  • src/notification/notification.service.integration.test.ts (2 hunks)
  • src/notification/notification.service.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/app.ts
  • src/account/account.service.ts
  • src/notification/notification-event.service.ts
  • src/account/account-followed.event.ts
🧰 Additional context used
🧬 Code Definitions (2)
src/notification/notification-event.service.unit.test.ts (3)
src/notification/notification.service.ts (1)
  • NotificationService (54-166)
src/notification/notification-event.service.ts (1)
  • NotificationEventService (5-24)
src/account/account-followed.event.ts (1)
  • AccountFollowedEvent (3-20)
src/notification/notification.service.integration.test.ts (1)
src/notification/notification.service.ts (1)
  • NotificationService (54-166)
🔇 Additional comments (10)
src/notification/notification-event.service.unit.test.ts (2)

17-24: Robust mock setup with jest spies

The test correctly sets up a mock NotificationService using Vitest's spy function to track if the createFollowNotification method is called.


1-9: Good organization of imports

The imports are well-organized, separating testing utilities, Node.js imports, and application imports with proper blank lines.

src/notification/notification.service.integration.test.ts (2)

7-8: Clean import of required types

Good job adding the imports for the Account type and NotificationType enum that are needed for the new test case.


267-313: Comprehensive test for notification creation

The new test suite for createFollowNotification is thorough and verifies all important aspects:

  1. Setting up the database with required records
  2. Calling the notification creation method
  3. Verifying the notification is correctly stored with the right properties

The test provides good coverage for this new feature.

src/notification/notification.service.ts (3)

6-11: Well-structured NotificationType enum

The NotificationType enum provides clear categorization of different notification types with numeric values. This will make it easier to extend with new notification types in the future.


144-165: Well-implemented notification creation method

The createFollowNotification method:

  1. Properly looks up the user associated with the account
  2. Includes appropriate error handling for when a user isn't found
  3. Correctly inserts the notification with the appropriate fields

The implementation is solid and follows best practices.


151-154: Good comment explaining the user lookup

The comment noting that you don't have direct access to the user from the Account is helpful for understanding why this lookup is necessary.

src/account/account.service.integration.test.ts (3)

14-15: Appropriate import for the AccountFollowedEvent

Good job importing the AccountFollowedEvent class which is used in the new tests.


279-304: Thorough test for event emission

The test for emitting the account.followed event is comprehensive:

  1. It sets up an event listener to capture the emitted event
  2. Creates the necessary accounts
  3. Executes the follow action
  4. Waits for the event to be emitted
  5. Verifies the event contains the correct account and follower information

This ensures the event emission works as expected.


306-330: Good test for duplicate follow handling

This test verifies that duplicate follow attempts don't trigger duplicate events, which is an important edge case to cover. The test correctly uses fake timers to control the timing of the event emission.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/notification/notification-event.service.unit.test.ts (2)

15-28: Consider removing unused timer mocks.

The test setup is good, but vi.useFakeTimers() is being used even though the tests don't appear to rely on any time-based functionality.

-    beforeEach(() => {
-        vi.useFakeTimers();
-
-        events = new EventEmitter();
-        notificationService = {
-            createFollowNotification: vi.fn(),
-        } as unknown as NotificationService;
-
-        notificationEventService = new NotificationEventService(
-            events,
-            notificationService,
-        );
-        notificationEventService.init();
-    });
+    beforeEach(() => {
+        events = new EventEmitter();
+        notificationService = {
+            createFollowNotification: vi.fn(),
+        } as unknown as NotificationService;
+
+        notificationEventService = new NotificationEventService(
+            events,
+            notificationService,
+        );
+        notificationEventService.init();
+    });

30-47: Add test for error handling.

The current test verifies the happy path, but it would be beneficial to add a test for error conditions when createFollowNotification fails.

Consider adding a test case like this:

it('should handle errors when creating a follow notification', async () => {
    const account = { id: 123 };
    const followerAccount = { id: 456 };
    const expectedError = new Error('User not found');
    
    notificationService.createFollowNotification.mockRejectedValueOnce(expectedError);
    
    // Using a promise to wait for the event to be processed
    await new Promise<void>((resolve) => {
        events.once('error', (error) => {
            expect(error).toBe(expectedError);
            resolve();
        });
        
        events.emit(
            AccountFollowedEvent.getName(),
            new AccountFollowedEvent(
                account as Account,
                followerAccount as Account,
            ),
        );
    });
    
    expect(
        notificationService.createFollowNotification,
    ).toHaveBeenCalledWith(account, followerAccount);
});
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7c5353d and 726325f.

📒 Files selected for processing (8)
  • src/account/account-followed.event.ts (1 hunks)
  • src/account/account.service.integration.test.ts (4 hunks)
  • src/account/account.service.ts (2 hunks)
  • src/app.ts (2 hunks)
  • src/notification/notification-event.service.ts (1 hunks)
  • src/notification/notification-event.service.unit.test.ts (1 hunks)
  • src/notification/notification.service.integration.test.ts (2 hunks)
  • src/notification/notification.service.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/app.ts
  • src/account/account-followed.event.ts
  • src/notification/notification-event.service.ts
  • src/account/account.service.integration.test.ts
  • src/account/account.service.ts
🧰 Additional context used
🧬 Code Definitions (2)
src/notification/notification-event.service.unit.test.ts (3)
src/notification/notification.service.ts (1)
  • NotificationService (54-166)
src/notification/notification-event.service.ts (1)
  • NotificationEventService (5-24)
src/account/account-followed.event.ts (1)
  • AccountFollowedEvent (3-20)
src/notification/notification.service.integration.test.ts (1)
src/notification/notification.service.ts (1)
  • NotificationService (54-166)
🔇 Additional comments (8)
src/notification/notification-event.service.unit.test.ts (2)

1-9: Clean import structure.

The imports are well-organized, separating testing utilities, Node.js modules, and application imports with appropriate spacing.


10-14: LGTM: Clear test suite structure.

The test suite is well-structured with clear variable declarations for all dependencies that will be tested.

src/notification/notification.service.ts (3)

3-3: Good import addition.

Import of the Account type is properly added to support the new follow notification functionality.


6-11: Well-structured enum for notification types.

The NotificationType enum clearly defines the different types of notifications the system can handle. Using numeric values for database storage is a good approach.


144-165: Well-implemented notification creation method.

The createFollowNotification method follows the same pattern as other methods in the service, including proper error handling when a user isn't found. The database operations are clean and consistent with the rest of the service.

We don't have access to the user from the Account so we have manually look this up

src/notification/notification.service.integration.test.ts (3)

7-8: Proper import additions.

The Account type and NotificationType enum are correctly imported to support the new tests.


268-312: Comprehensive happy path test.

The test thoroughly sets up the database state, calls the method under test, and verifies the results. The assertions are complete, checking the notification count, user ID, account ID, and event type.


314-331: Good error case test.

This test correctly verifies the error handling behavior when trying to create a notification for an account without an associated user. It matches the same error handling pattern used in other methods.

ref https://linear.app/ghost/issue/AP-901

Update the system to emit a `account.followed` event when an account is followed.
This event is then picked up by the `NotificationEventService` which will
insert a notification into the database for the account being followed.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
src/notification/notification.service.ts (2)

144-165: Add @throws to the method documentation.

The method properly implements notification creation for follow events, but the JSDoc comment should document that it can throw an error when the user is not found.

 /**
  * Create a notification for a follow event
  *
  * @param account The account that is being followed
  * @param followerAccount The account that is following
+ * @throws Error if the user is not found for the provided account
  */

160-164: Consider error handling for database operations.

The notification insert operation has no specific error handling. While propagating the error may be acceptable depending on your error handling strategy, consider adding a try/catch block if you need custom error handling or want to provide a more specific error message.

+    try {
         await this.db('notifications').insert({
             user_id: user.id,
             account_id: followerAccount.id,
             event_type: NotificationType.Follow,
         });
+    } catch (error) {
+        throw new Error(`Failed to create follow notification: ${error.message}`);
+    }
src/notification/notification.service.integration.test.ts (1)

293-299: Consider using more complete mock Account objects.

Type assertion is used to create minimal Account objects with only the id property. While this works for the current implementation that only uses the id, consider creating more complete mocks if other Account properties might be used in the future.

-    const account = {
-        id: accountId,
-    } as Account;
+    const account = {
+        id: accountId,
+        username: 'alice',
+        ap_id: 'https://alice.com/user/alice',
+        ap_inbox_url: 'https://alice.com/user/alice/inbox',
+    } as Account;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 726325f and 9fbdc07.

📒 Files selected for processing (8)
  • src/account/account-followed.event.ts (1 hunks)
  • src/account/account.service.integration.test.ts (4 hunks)
  • src/account/account.service.ts (2 hunks)
  • src/app.ts (2 hunks)
  • src/notification/notification-event.service.ts (1 hunks)
  • src/notification/notification-event.service.unit.test.ts (1 hunks)
  • src/notification/notification.service.integration.test.ts (2 hunks)
  • src/notification/notification.service.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
  • src/notification/notification-event.service.ts
  • src/notification/notification-event.service.unit.test.ts
  • src/app.ts
  • src/account/account.service.ts
  • src/account/account-followed.event.ts
  • src/account/account.service.integration.test.ts
🧰 Additional context used
🧬 Code Definitions (1)
src/notification/notification.service.integration.test.ts (1)
src/notification/notification.service.ts (1)
  • NotificationService (54-166)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Build, Test and Push
🔇 Additional comments (6)
src/notification/notification.service.ts (3)

3-3: Import for Account type is appropriate.

The addition of the Account type import is necessary for the new createFollowNotification method parameters. This keeps the code strongly typed and provides better intellisense support.


6-11: Well-structured enum for notification types.

The NotificationType enum is a good addition that replaces magic numbers with descriptive constants, improving code maintainability and readability. The enum values (1-4) match the existing event_type values used in the notification records.


151-154: User lookup from account is handled correctly.

The manual lookup for user from account is appropriate given the constraint mentioned in the previous review comment.

src/notification/notification.service.integration.test.ts (3)

7-8: Import statements correctly updated for new functionality.

The addition of the Account type and NotificationType imports supports the new test cases.


267-312: Well-structured test case for successful notification creation.

The test thoroughly verifies the notification creation functionality by:

  1. Setting up the necessary database records
  2. Creating mock Account objects
  3. Calling the createFollowNotification method
  4. Verifying the notification was inserted with correct values

This provides good coverage for the new functionality.


314-331: Error handling test case is appropriately implemented.

The test correctly verifies that an error is thrown with the appropriate message when attempting to create a notification for an account that doesn't have an associated user.

Copy link
Member

@vershwal vershwal left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -0,0 +1,20 @@
import type { Account } from 'account/types';
Copy link
Member

Choose a reason for hiding this comment

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

It’s always nicer to clean things up when we’re in the area and context loaded, but I had a look at the recordAccountFollow, createExternalAccount and related test, and yeah, this one’s definitely a bit too much to squeeze into this PR. Would’ve made it way harder to review/understand the main logic of this PR.

Makes sense to leave it out for now.
Just wondering — are we thinking of doing that migration all at once later, or more incrementally? Like maybe you plan to do a quick follow-up PR after this to clean it up?

import { AccountFollowedEvent } from 'account/account-followed.event';
import type { NotificationService } from 'notification/notification.service';

export class NotificationEventService {
Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I don’t have a better name in mind right now either 😅 but happy to revisit if something clicks later.

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