Refactored account retrieval to use AccountView#505
Conversation
|
""" WalkthroughThe changes update several components related to account handling and lookup processes. The expected HTTP status for non-existent accounts is corrected from 500 to 404, and an assertion ensuring that error codes are below 500 has been added in the test step definitions. The logic for retrieving account details is refactored to use a newly introduced Possibly related PRs
Suggested labels
✨ 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 (
|
allouis
left a comment
allouis
left a comment
There was a problem hiding this comment.
Overall structure good, except I think the ViewContext should change from Site -> Account
src/http/api/account.ts
Outdated
| * Create a handler to handle a request for an account | ||
| * | ||
| * @param accountService Account service instance | ||
| * @param db Database client |
There was a problem hiding this comment.
Are these param comments useful to you?
There was a problem hiding this comment.
not particularly no, just habit and i get a nice typescript description. Maybe we only include them when extra context is needed? (i.e @param db Database client that can only read and not write)
There was a problem hiding this comment.
Yeah I agree - that's a nice rule
| } | ||
|
|
||
| // Return response | ||
| return new Response(JSON.stringify(accountDto), { |
There was a problem hiding this comment.
We don't check for the accountDto being null here - I think we wanna 404 in that case probs
src/http/api/views/account.view.ts
Outdated
| * and determine if that account is following / followed by the account | ||
| * being viewed | ||
| */ | ||
| site?: Site; |
There was a problem hiding this comment.
I think we want context to be an Account rather than Site - otherwise we're leaking the idea of the default account for a site into this view.
If we pass in an account rather than a site, then when we eventually support multiple accounts for a site, the controller can just pass in like ctx.get('authenticateAccount') or something like this?
There was a problem hiding this comment.
Yeh i went with this so we don't have to double query the default account - If you are requesting your own profile, we cant just return this account, we still need to go via the view to then get the rest of the data needed (so 2 queries for similar data). Maybe not a big deal?
There was a problem hiding this comment.
Ah I see - that's a good point - but I do think we're going to want to be attaching the current account to the context pretty soon, as we have calls to getDefaultAccount everywhere and it would be nice to move that to middleware so we have an easy time migrating to multiple accounts.
| const actor = await lookupObject(fedifyContext, apId); | ||
|
|
||
| if (actor === null) { | ||
| throw new Error(`Could not find Actor ${apId}`); | ||
| } | ||
|
|
||
| if (!isActor(actor)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
I think this might just be a refactor/following some existing pattern - but I still think it's good to confirm, and change if we need to.
Why do we throw if actor is null, but we return null if it's not an actor? In this context I think I would expect a 404 in both cases 🤔
There was a problem hiding this comment.
Yeh that was existing code, but I agree, in both cases it should 404
src/http/api/account.ts
Outdated
| accountRepository: KnexAccountRepository, | ||
| fedify: Federation<ContextData>, | ||
| db: Knex, | ||
| fedifyContextFactory: FedifyContextFactory, |
There was a problem hiding this comment.
Is there a reason we are passing db and fedifyContextFactory to this controller and then passing these two to the AccountView?
Instead of passing db and fedifyContextFactory to the controller and then into AccountView, can we just create the AccountView instance in app.ts and inject it into the controller?
There was a problem hiding this comment.
Hm, i guess i saw the view as something tied closely to the API request, so the controller has control over how this is constructed. Do you think it should just be passed in? cc @allouis
There was a problem hiding this comment.
Yeah, my thinking was — since we’re not using db or fedifyContextFactory directly in the controller, it felt a bit cleaner to move their usage up into app.ts and just pass in the AccountView.
But open to thoughts — happy to go either way depending on how we see it fitting into the overall structure.
There was a problem hiding this comment.
I agree with @vershwal here, keeping the DI in app.ts makes the signature simpler, and doesn't expose details to the controller - also should be easier to unit test?
no ref Refactored account retrieval to use `AccountView`
mike182uk
force-pushed
the
refactor-get-account-to-use-view
branch
from
419d03d to
aebe9fc
Compare
mike182uk
force-pushed
the
refactor-get-account-to-use-view
branch
from
ae8be27 to
fe3d513
Compare
|
|
||
| Scenario: Get non-existent account | ||
| When an authenticated "get" request is made to "/.ghost/activitypub/account/@nonexistent@fake-external-activitypub" | ||
| Then the request is rejected with a 500 |
There was a problem hiding this comment.
😬 Yeah I think we should never have an explicit 500 check here.
I think we should update that stepdef to throw an error if a 5xx error is passed as the expected status code - what do you think?
There was a problem hiding this comment.
Yeh I don't think it makes sense to expect an internal server error 😅 I'll add a check into the stepdef 👍
mike182uk
force-pushed
the
refactor-get-account-to-use-view
branch
from
7475290 to
4e57973
Compare
![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/lookup-helpers.ts (1)
75-79: Consider a more robust check for environment variable
Right now, the code strictly checks forprocess.env.ALLOW_PRIVATE_ADDRESS === 'true'. Many deployments represent boolean environment variables using uppercase'TRUE'or'1'. You may want to ensure a case-insensitive or more general check.Example update:
- process.env.ALLOW_PRIVATE_ADDRESS === 'true' + process.env.ALLOW_PRIVATE_ADDRESS?.toLowerCase() === 'true'src/http/api/account.ts (1)
27-30: Optional: centralize the 'me' keyword
If theCURRENT_USER_KEYWORDis used in more than one place in the codebase, consider defining it in a central constants file or config to avoid duplication and promote consistency.src/http/api/views/account.view.ts (2)
21-78: Review ofAccountViewconstructor &viewByIdmethod
- The constructor parameters
dbandfedifyContextFactoryare well-defined for database and Fediverse lookups.viewByIdcorrectly fetches the account and determines follow relationships.- You’ve provisioned
attachment: []with a TODO comment. If not truly needed, consider removing it to simplify theAccountDTO.- attachment: [], // TODO: I don't think we need this + // remove attachment if it’s no longer used
303-333:getActorCollectionCountmethod
Nicely handles each collection type, returning zero on error. Consider logging the error at a debug or info level if you need to troubleshoot Fediverse issues later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
features/account.feature(1 hunks)features/step_definitions/stepdefs.js(1 hunks)src/app.ts(3 hunks)src/http/api/account.ts(2 hunks)src/http/api/views/account.view.ts(1 hunks)src/lookup-helpers.ts(1 hunks)src/lookup-helpers.unit.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/http/api/views/account.view.ts (6)
src/activitypub/fedify-context.factory.ts (1)
FedifyContextFactory(4-24)src/http/api/types.ts (1)
AccountDTO(7-77)src/account/utils.ts (1)
getAccountHandle(77-79)src/helpers/html.ts (1)
sanitizeHtml(3-212)src/lookup-helpers.ts (1)
lookupAPIdByHandle(64-108)src/helpers/activitypub/actor.ts (1)
getHandle(60-64)
🔇 Additional comments (15)
features/account.feature (1)
23-23: Good update to expected HTTP status code!Changing from 500 to 404 for non-existent accounts is appropriate - a 404 status code correctly indicates "not found" rather than suggesting a server error occurred.
src/lookup-helpers.unit.test.ts (1)
43-45: LGTM: Test updated to match new function signature.The test now properly checks that
lookupWebFingeris called with the new options parameter that includesallowPrivateAddress: true.features/step_definitions/stepdefs.js (1)
1485-1488: Excellent enhancement to test validation!Adding an assertion to check that expected status codes are less than 500 ensures tests never expect server errors. This is a good practice that aligns with the conversation in the previous reviews and the changes in the feature file.
src/app.ts (3)
116-116: LGTM: Adding the new AccountView import.This is the first step in the refactoring to use AccountView for account-related operations.
271-271: LGTM: Initializing the AccountView class.Creating the instance with the required dependencies looks good.
964-964: Properly updated handler to use AccountView.The account handler now uses the new
accountViewinstead ofaccountServiceandfedify, streamlining the dependency chain for account retrieval operations.src/http/api/account.ts (3)
36-36: No issues with parameter replacement
ReplacingaccountServicewithaccountViewin the handler signature is consistent with the new approach of using theAccountViewclass for account retrieval.
45-45: Validate handle parameter
The logic checks if the handle is'me'or a valid handle. Returning 404 for invalid handles makes sense from a user perspective. If you need to distinguish between an invalid handle vs. a missing resource, you might also consider logging or returning a 400 in certain scenarios, but 404 is acceptable if that’s your desired behavior.Also applies to: 47-49
57-72: Clean separation of concerns
• Creating aviewContextwithrequestUserAccountclarifies who the requesting user is for the subsequent retrieval.
• Splitting logic between handling'me'and a specific handle is neat and straightforward.
• Returning404whenaccountDtoisnullis a clear indication of a non-existent account.src/http/api/views/account.view.ts (6)
1-20: Interface documentation and import structure look good
Having a dedicatedViewContextinterface is clean and adaptable for future expansions. MarkingrequestUserAccountas optional is acceptable, as long as you’re ready to handle the case when it's not present.
80-100:viewByHandlemethod
Clean approach to first get the AP ID via WebFinger, then callingviewByApId. If you see repeated patterns in these low-level lookups across the codebase, consider unifying them in a small helper routine.
102-157:viewByApIdmethod
Gracefully handles local versus remote accounts by delegating toviewByApIdRemoteif the local DB query is empty. This separation helps keep the code simpler.
159-236:viewByApIdRemotemethod
- Correct to ensure
actoris non-null andisActor(actor)is true before proceeding.- Uses
sanitizeHtmlto protect from HTML injection in remote bios.- The fallback returns
nullif the remote actor can’t be fetched or is invalid, which is consistent.
238-272:getAccountByQuerymethod
The joins and subqueries for counts are straightforward and read clearly. For large-scale deployments, watch for potential performance bottlenecks with multiple subselects. Some DB vendors can optimize these effectively, but consider verifying with an EXPLAIN plan if performance becomes an issue.If you suspect performance issues in production, you can run a query analyzer or sample load testing on these queries.
274-301:getRequestUserContextDatamethod
Implementing follow checks in separate queries is acceptable, but you could unify it in a single query if you find performance concerns. For now, readability is fine.
mike182uk
force-pushed
the
refactor-get-account-to-use-view
branch
from
b9cba7c to
b931ec9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/http/api/views/account.view.ts (4)
30-78: Consider extracting common account formatting logic.There's significant duplication between
viewByIdandviewByApIdmethods. Both methods have nearly identical logic for checking relationships and returning an AccountDTO.async viewById( id: number, context: ViewContext, ): Promise<AccountDTO | null> { const accountData = await this.getAccountByQuery( (qb: Knex.QueryBuilder) => qb.where('accounts.id', id), ); if (!accountData) { return null; } - let followedByMe = false; - let followsMe = false; - - if ( - context.requestUserAccount?.id && - // Don't check if the request user is following / followed by themselves - accountData.id !== context.requestUserAccount.id - ) { - ({ followedByMe, followsMe } = await this.getRequestUserContextData( - context.requestUserAccount.id, - accountData.id, - )); - } - - return { - id: accountData.id, - name: accountData.name, - handle: getAccountHandle( - new URL(accountData.ap_id).host, - accountData.username, - ), - bio: sanitizeHtml(accountData.bio || ''), - url: accountData.url, - avatarUrl: accountData.avatar_url || '', - bannerImageUrl: accountData.banner_image_url || '', - customFields: accountData.custom_fields - ? JSON.parse(accountData.custom_fields) - : {}, - postCount: accountData.post_count, - likedCount: accountData.like_count, - followingCount: accountData.following_count, - followerCount: accountData.follower_count, - followedByMe, - followsMe, - attachment: [], // TODO: I don't think we need this - }; + return this.formatAccountDTO(accountData, context); } +/** + * Format account data into a consistent AccountDTO object + */ +private async formatAccountDTO( + accountData: any, + context: ViewContext +): Promise<AccountDTO> { + let followedByMe = false; + let followsMe = false; + + if ( + context.requestUserAccount?.id && + // Don't check if the request user is following / followed by themselves + accountData.id !== context.requestUserAccount.id + ) { + ({ followedByMe, followsMe } = await this.getRequestUserContextData( + context.requestUserAccount.id, + accountData.id, + )); + } + + return { + id: accountData.id, + name: accountData.name, + handle: getAccountHandle( + new URL(accountData.ap_id).host, + accountData.username, + ), + bio: sanitizeHtml(accountData.bio || ''), + url: accountData.url, + avatarUrl: accountData.avatar_url || '', + bannerImageUrl: accountData.banner_image_url || '', + customFields: accountData.custom_fields + ? JSON.parse(accountData.custom_fields) + : {}, + postCount: accountData.post_count, + likedCount: accountData.like_count, + followingCount: accountData.following_count, + followerCount: accountData.follower_count, + followedByMe, + followsMe, + attachment: [], // TODO: I don't think we need this + }; +}Similar changes would be needed in the
viewByApIdmethod.Also applies to: 109-157
76-76: Address or remove the TODO comments.There are several TODO comments indicating attachments may not be needed. Either implement the feature, remove the property, or add a more detailed comment explaining why it's being kept as a placeholder.
Also applies to: 155-155, 234-234
274-301: Consider optimizing the follow relationship queries.The method makes two separate database queries to check follow relationships. These could be combined into a single query for better performance.
private async getRequestUserContextData( requestUserAccountId: number, retrievedAccountId: number, ) { - let followedByMe = false; - let followsMe = false; - - followedByMe = - ( - await this.db('follows') - .where('follower_id', requestUserAccountId) - .where('following_id', retrievedAccountId) - .first() - )?.id !== undefined; - - followsMe = - ( - await this.db('follows') - .where('following_id', requestUserAccountId) - .where('follower_id', retrievedAccountId) - .first() - )?.id !== undefined; + const [followResult, followedResult] = await Promise.all([ + this.db('follows') + .where('follower_id', requestUserAccountId) + .where('following_id', retrievedAccountId) + .first(), + this.db('follows') + .where('following_id', requestUserAccountId) + .where('follower_id', retrievedAccountId) + .first() + ]); + + const followedByMe = followResult?.id !== undefined; + const followsMe = followedResult?.id !== undefined; return { followedByMe, followsMe, }; }
303-331: Error handling masks potential issues in collection retrieval.While it's good to gracefully handle errors when retrieving collection counts, the current implementation silently converts all errors to zero counts. Consider adding logging to capture and monitor these errors.
try { const collection = await getCollection.bind(actor)(); return collection?.totalItems ?? 0; } catch (error) { + // Log error for monitoring but still gracefully continue + console.error(`Failed to get ${collectionName} count for actor ${actor.id}:`, error); return 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
features/account.feature(1 hunks)features/step_definitions/stepdefs.js(1 hunks)src/app.ts(3 hunks)src/http/api/account.ts(2 hunks)src/http/api/views/account.view.ts(1 hunks)src/lookup-helpers.ts(1 hunks)src/lookup-helpers.unit.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- features/account.feature
- src/lookup-helpers.unit.test.ts
- features/step_definitions/stepdefs.js
- src/lookup-helpers.ts
- src/http/api/account.ts
- src/app.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build, Test and Push
🔇 Additional comments (7)
src/http/api/views/account.view.ts (7)
14-19: Good implementation of ViewContext interface.The interface clearly defines the purpose of the request user account context, which aligns with the previous PR discussions about using Account instead of Site context.
21-25: Clean constructor with appropriate dependencies.The class follows dependency injection principles by accepting database and federation context dependencies through the constructor, which is good for testability.
87-100: Well-structured handle resolution method.The
viewByHandlemethod correctly leverages lookup helpers and has appropriate error handling by returningnullwhen no matching account is found, aligning with the consistent pattern discussed in previous reviews.
171-177: Consistent null handling for not-found cases.The implementation now correctly returns
nullin both cases when an actor isn't found or isn't an actor type, which matches the approach discussed in previous reviews for consistent error handling.
216-227: Good sanitization of user-generated content.The implementation properly sanitizes all user-generated content, including custom fields from external sources, which is important for security.
238-272: Well-structured database query with efficient joins.The
getAccountByQuerymethod properly joins the users table and uses subqueries to efficiently gather all the required data in a single database call. The use of raw SQL for aggregate counts is appropriate here for performance reasons.
1-332: Overall excellent implementation of the AccountView.The
AccountViewclass successfully centralizes account retrieval logic, providing a consistent interface for both local and remote account data. The implementation aligns with the PR objective to refactor account retrieval, and the code handles errors appropriately, sanitizes user data, and maintains consistent return types as discussed in previous reviews.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/http/api/views/account.view.integration.test.ts (2)
307-321: UsemockImplementationOnceto prevent test‑to‑test bleed‑through
vi.spyOn(AccountView.prototype, 'viewByApId').mockImplementation(...)installs a permanent stub untilvi.restoreAllMocks()in the nextbeforeEach.
If a new test is inserted before that call, the stub state may unexpectedly persist.
Limit the scope to this assertion by usingmockImplementationOnce:- const spy = vi - .spyOn(AccountView.prototype, 'viewByApId') - .mockImplementation(async (apId) => { + const spy = vi + .spyOn(AccountView.prototype, 'viewByApId') + .mockImplementationOnce(async (apId) => {
17-21: Expose both mocked helpers to the testsThe factory passed to the top‑level
vi.mock()only creates stubs forlookupAPIdByHandleandlookupObject.
lookupObjectis never used in this suite; if future tests rely on its behaviour you’ll need to supply an implementation.
Consider exporting typed helpers in a shared__mocks__/lookup-helpers.tsfile so the intent is explicit and DRY across test files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/http/api/__snapshots__/views/AccountView.viewByApId.internal-no-context.json(1 hunks)src/http/api/__snapshots__/views/AccountView.viewById.no-context.json(1 hunks)src/http/api/views/account.view.integration.test.ts(1 hunks)src/http/api/views/account.view.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/http/api/snapshots/views/AccountView.viewById.no-context.json
- src/http/api/snapshots/views/AccountView.viewByApId.internal-no-context.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/http/api/views/account.view.ts
| beforeEach(async () => { | ||
| await db.raw('SET FOREIGN_KEY_CHECKS = 0'); | ||
| await Promise.all([ | ||
| db('reposts').truncate(), | ||
| db('likes').truncate(), | ||
| db('posts').truncate(), | ||
| db('follows').truncate(), | ||
| db('accounts').truncate(), | ||
| db('users').truncate(), | ||
| db('sites').truncate(), | ||
| ]); | ||
| await db.raw('SET FOREIGN_KEY_CHECKS = 1'); | ||
|
|
||
| vi.restoreAllMocks(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Serialise truncation to reduce the risk of deadlocks
Running six TRUNCATE statements in parallel (Promise.all([...])) while foreign‑key checks are disabled is usually safe, but MySQL occasionally deadlocks on parallel DDL in rapid test loops.
It’s safer (and fast enough) to execute them sequentially:
- await Promise.all([
- db('reposts').truncate(),
- db('likes').truncate(),
- db('posts').truncate(),
- db('follows').truncate(),
- db('accounts').truncate(),
- db('users').truncate(),
- db('sites').truncate(),
- ]);
+ for (const table of [
+ 'reposts',
+ 'likes',
+ 'posts',
+ 'follows',
+ 'accounts',
+ 'users',
+ 'sites',
+ ]) {
+ // sequential truncate avoids occasional DDL deadlocks
+ // and keeps the intent crystal‑clear
+ await db(table).truncate();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| beforeEach(async () => { | |
| await db.raw('SET FOREIGN_KEY_CHECKS = 0'); | |
| await Promise.all([ | |
| db('reposts').truncate(), | |
| db('likes').truncate(), | |
| db('posts').truncate(), | |
| db('follows').truncate(), | |
| db('accounts').truncate(), | |
| db('users').truncate(), | |
| db('sites').truncate(), | |
| ]); | |
| await db.raw('SET FOREIGN_KEY_CHECKS = 1'); | |
| vi.restoreAllMocks(); | |
| }); | |
| beforeEach(async () => { | |
| await db.raw('SET FOREIGN_KEY_CHECKS = 0'); | |
| - await Promise.all([ | |
| - db('reposts').truncate(), | |
| - db('likes').truncate(), | |
| - db('posts').truncate(), | |
| - db('follows').truncate(), | |
| - db('accounts').truncate(), | |
| - db('users').truncate(), | |
| - db('sites').truncate(), | |
| - ]); | |
| + for (const table of [ | |
| + 'reposts', | |
| + 'likes', | |
| + 'posts', | |
| + 'follows', | |
| + 'accounts', | |
| + 'users', | |
| + 'sites', | |
| + ]) { | |
| + // sequential truncate avoids occasional DDL deadlocks | |
| + // and keeps the intent crystal‑clear | |
| + await db(table).truncate(); | |
| + } | |
| await db.raw('SET FOREIGN_KEY_CHECKS = 1'); | |
| vi.restoreAllMocks(); | |
| }); |
| accountView = new AccountView(db, fedifyContextFactory); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Close the Knex connection after the test suite to avoid open‑handle leaks
beforeAll creates a real Knex instance but the suite never tears it down. Vitest keeps the process alive when there are un‑closed DB handles, which can make the test runner hang or report “open handles” warnings on CI.
+ afterAll(async () => {
+ await db.destroy(); // closes pool & frees resources
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| accountView = new AccountView(db, fedifyContextFactory); | |
| }); | |
| accountView = new AccountView(db, fedifyContextFactory); | |
| }); | |
| afterAll(async () => { | |
| await db.destroy(); // closes pool & frees resources | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/http/api/account.ts (1)
122-129: Suggestion: Reuse the CURRENT_USER_KEYWORD constantFor consistency, consider using the
CURRENT_USER_KEYWORDconstant in other handlers instead of hardcoded 'me' strings.- if (handle === 'me') { + if (handle === CURRENT_USER_KEYWORD) {This applies to both places where 'me' is used as a hardcoded string.
Also applies to: 224-232
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
features/step_definitions/stepdefs.js(1 hunks)src/app.ts(3 hunks)src/http/api/account.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- features/step_definitions/stepdefs.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app.ts
🔇 Additional comments (6)
src/http/api/account.ts (6)
31-34: Good extraction of 'me' into a constantExtracting the 'me' value into a named constant
CURRENT_USER_KEYWORDimproves readability and maintainability.
39-42: Clean dependency injection with AccountViewThe refactored signature is simpler and follows the agreed-upon approach from previous discussions, injecting
AccountViewdirectly rather than passing its dependencies through the controller.
49-53: Improved error handling for invalid handlesProper validation of the handle parameter with a clear 404 response for invalid values.
61-63: Good use of view context patternCreating a view context object with the request user account improves code structure and makes dependencies explicit.
65-72: Simplified account lookup logicThe refactored code elegantly handles both the current user case and regular handle lookups using the AccountView, resulting in cleaner and more maintainable code.
74-76: Proper null checking for accountThe null check for accountDto with a 404 response addresses the previous review comment correctly.
allouis
left a comment
allouis
left a comment
There was a problem hiding this comment.
I think we should return Result instead of AccountDTO | null - but seeing as we're not throwing errors, this is fine for now, and we can use result when we need to throw/handle errors
3 participants
no ref
Refactored account retrieval to use
AccountView