feat(server): Add FindByIntegrations for wokrspace in accountusecase #75
Conversation
WalkthroughThe changes introduce new Integration identifier types and aliases in the domain packages. Specifically, a new type for lists of integrations is declared alongside corresponding type aliases in the user and workspace packages. Additionally, new methods named Changes
Poem
✨ Finishing Touches
🪧 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: 2
🧹 Nitpick comments (1)
account/accountusecase/accountrepo/workspace.go (1)
16-17: Enhance the method comment for better documentation.While the method signature is well-designed and consistent with other Find* methods, consider enhancing the comment to include more details about the method's behavior, such as:
- What happens if the list is empty?
- Are the results ordered in any specific way?
- Are duplicate workspaces filtered out if they match multiple integration IDs?
- // FindByIntegrations finds workspace list based on integrations IDs + // FindByIntegrations returns a list of workspaces that have any of the specified integration IDs. + // The results are ordered by workspace ID. If the input list is empty, an empty list is returned. + // If a workspace matches multiple integration IDs, it appears only once in the result.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
account/accountdomain/id.go(1 hunks)account/accountdomain/user/id.go(1 hunks)account/accountdomain/workspace/id.go(1 hunks)account/accountinfrastructure/accountmemory/workspace.go(2 hunks)account/accountinfrastructure/accountmongo/workspace.go(1 hunks)account/accountusecase/accountrepo/workspace.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CI
🔇 Additional comments (5)
account/accountdomain/user/id.go (1)
11-12: LGTM! The new type aliases follow the established pattern.The additions of
IntegrationIDandIntegrationIDListtype aliases are consistent with the existing type alias declarations and naming conventions in the file.account/accountdomain/workspace/id.go (1)
10-10: LGTM! The new type alias follows the established pattern.The addition of
IntegrationIDListtype alias is consistent with the existing type alias declarations and naming conventions in the file.account/accountdomain/id.go (1)
21-21: LGTM! The new type declaration follows the established pattern.The addition of
IntegrationIDListtype is consistent with the existing list type declarations and naming conventions in the file.account/accountinfrastructure/accountmemory/workspace.go (1)
58-59: LGTM: Method signature and documentation.The method signature and documentation align well with the PR objective of finding workspaces by integration IDs.
account/accountinfrastructure/accountmongo/workspace.go (1)
61-62: LGTM: Method signature and documentation.The method signature and documentation align well with the PR objective of finding workspaces by integration IDs.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #75 +/- ##
========================================
Coverage 47.66% 47.67%
========================================
Files 97 101 +4
Lines 9323 9681 +358
========================================
+ Hits 4444 4615 +171
- Misses 4561 4738 +177
- Partials 318 328 +10 ☔ View full report in Codecov by Sentry. |
|
LGTM, don't for get to write test 🙏🏻 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
account/accountinfrastructure/accountmemory/workspace.go (2)
6-6: Use a consistent package name for imports.You're importing the
slicespackage twice with different aliases. This can lead to confusion and maintenance issues. Use one consistent import approach.- slices0 "slices" + "slices"Then update the usage in the FindByIntegrations method accordingly:
- return slices0.ContainsFunc(ids, value.Members().HasIntegration) + return slices.ContainsFunc(ids, value.Members().HasIntegration)
59-76: Implementation looks good but could be optimized with early returns.The implementation correctly finds workspaces by integration IDs. However, we could make the code more readable with an early return when ids is empty.
func (r *Workspace) FindByIntegrations(_ context.Context, ids workspace.IntegrationIDList) (workspace.List, error) { if r.err != nil { return nil, r.err } if len(ids) == 0 { - return nil, nil + return workspace.List{}, nil } res := r.data.FindAll(func(key workspace.ID, value *workspace.Workspace) bool { return slices0.ContainsFunc(ids, value.Members().HasIntegration) }) slices.SortFunc(res, func(a, b *workspace.Workspace) int { return a.ID().Compare(b.ID()) }) return res, nil }This returns an empty list instead of nil for consistency with other methods and provides better type safety.
account/accountinfrastructure/accountmemory/workspace_test.go (1)
115-117: Consider adding test for error handling.Your tests cover the happy paths well, but there's no test for when the repository returns an error. Consider adding a test case that sets an error using
SetWorkspaceErrorto ensure error propagation works correctly.You could add an additional test case similar to other tests in this file:
// After existing tests t.Run("Error", func(t *testing.T) { wantErr := errors.New("test") SetWorkspaceError(r, wantErr) out, err := r.FindByIntegrations(ctx, workspace.IntegrationIDList{i1}) assert.Same(t, wantErr, err) assert.Nil(t, out) })account/accountinfrastructure/accountmongo/workspace_test.go (1)
209-209: Typo in test case name.There's a typo in the test case name: "succes" should be "success".
- Name: "succes find multiple workspaces", + Name: "success find multiple workspaces",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
account/accountinfrastructure/accountmemory/workspace.go(2 hunks)account/accountinfrastructure/accountmemory/workspace_test.go(1 hunks)account/accountinfrastructure/accountmongo/workspace_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CI
🔇 Additional comments (3)
account/accountinfrastructure/accountmemory/workspace.go (1)
70-70:❓ Verification inconclusive
Verify the integration ID lookup logic is correct.
This implementation correctly uses
ContainsFuncto check if any integration ID in the provided list matches with the workspace members' integrations.
🏁 Script executed:
#!/bin/bash # Check Members().HasIntegration implementation to ensure it works as expected rg -A 3 'func \(\w+ Members\) HasIntegration' ./account/accountdomain/workspace/Length of output: 82
Attention: Manual Verification Needed for Integration Lookup Implementation
The automated search did not locate an implementation of
Members().HasIntegrationin the expected directory. Please verify manually that:
- The
HasIntegrationmethod exists (it might be implemented elsewhere in the codebase) and behaves as expected.- The logic using
slices0.ContainsFunc(ids, value.Members().HasIntegration)correctly checks for matching integration IDs.account/accountinfrastructure/accountmemory/workspace_test.go (1)
73-122: Test cases are comprehensive and well-structured.The test covers multiple scenarios including:
- Finding a workspace by a single integration ID
- Finding multiple workspaces by multiple integration IDs
- Handling empty integration ID lists
This provides good test coverage for the new functionality.
account/accountinfrastructure/accountmongo/workspace_test.go (1)
252-253:❓ Verification inconclusive
Verify MongoDB implementation is correct.
The test assertion is always expecting an empty result (
Len(tt, got, 0), regardless of the test case. This suggests there might be an issue with either the test or the MongoDB implementation ofFindByIntegrations.
🏁 Script executed:
#!/bin/bash # Description: Check the implementation of FindByIntegrations in the MongoDB repository # Check the MongoDB implementation rg -A 15 'func \(r \*Workspace\) FindByIntegrations' ./account/accountinfrastructure/accountmongo/Length of output: 1487
Action: Confirm expected behavior for FindByIntegrations
After reviewing the implementation of
FindByIntegrationsinaccount/accountinfrastructure/accountmongo/workspace.go, it’s clear that the function is designed to return anilslice when provided an empty list of integration IDs—which in Go evaluates to a length of 0. This behavior explains why the test usesassert.NoError(tt, err) assert.Len(tt, got, 0)and passes for the empty integration scenario.
To determine if this is correct overall, please verify whether non-empty integration IDs should produce a non-empty result. If so, additional test cases are needed to check:
- That passing valid integration IDs correctly constructs a MongoDB
$orquery.- That the underlying
findmethod retrieves matching records when available.If empty-input behavior is all that’s required, then the current test setup is consistent with the implementation.
jasonkarel
force-pushed
the
feat-server/add-find-by-integrations
branch
from
deadafa to
d0878ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
account/accountinfrastructure/accountmemory/workspace.go (2)
6-6: Consider renaming the slices import alias for clarity.You're importing the standard library "slices" package with an alias "slices0" while also importing "golang.org/x/exp/slices" without an alias. This could be confusing to readers. Consider using more descriptive aliases like "stdslices" and "expslices" to clearly differentiate between the two.
- slices0 "slices" + stdslices "slices"
70-70: Update slices0 reference if you rename the import.If you rename the slices import alias as suggested earlier, don't forget to update this reference as well.
- return slices0.ContainsFunc(ids, value.Members().HasIntegration) + return stdslices.ContainsFunc(ids, value.Members().HasIntegration)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
account/accountdomain/id.go(1 hunks)account/accountdomain/user/id.go(1 hunks)account/accountdomain/workspace/id.go(1 hunks)account/accountinfrastructure/accountmemory/workspace.go(2 hunks)account/accountinfrastructure/accountmemory/workspace_test.go(1 hunks)account/accountinfrastructure/accountmongo/workspace.go(1 hunks)account/accountinfrastructure/accountmongo/workspace_test.go(1 hunks)account/accountusecase/accountrepo/workspace.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- account/accountdomain/workspace/id.go
- account/accountdomain/user/id.go
- account/accountdomain/id.go
- account/accountinfrastructure/accountmongo/workspace.go
- account/accountinfrastructure/accountmongo/workspace_test.go
- account/accountusecase/accountrepo/workspace.go
🔇 Additional comments (3)
account/accountinfrastructure/accountmemory/workspace.go (1)
59-76: Implementation looks good!The new
FindByIntegrationsmethod is well-implemented with:
- Proper error handling
- Empty list check
- Efficient filtering using
ContainsFunc- Result sorting for consistent output
The use of
ContainsFuncis an elegant functional approach that correctly checks if any of the provided integration IDs match with the workspace members.account/accountinfrastructure/accountmemory/workspace_test.go (2)
73-122: Well-structured test with good coverage.The test for
FindByIntegrationsis comprehensive and covers all important scenarios:
- Finding by a single integration ID
- Finding by multiple integration IDs
- Handling empty integration ID lists
The table-driven approach makes the test easy to understand and maintain. Good job setting up the test data and assertions.
116-116: Consider the implications of parallel test execution.You're using
t.Parallel()which runs test cases in parallel. This is generally good for performance, but be cautious when tests share resources. In this case, the workspace repository is shared across test cases, so there's a potential for race conditions.Since you're not modifying the repository during tests, it's likely safe, but keep this in mind for future test modifications.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
account/accountinfrastructure/accountmongo/workspace_test.go (1)
253-253:⚠️ Potential issueFix incorrect assertion in integration test.
The test has an incorrect assertion that always expects zero results, regardless of the test case's expectations. This will cause false positives when the test should actually fail.
got, err := repo.FindByIntegrations(ctx, tc.Input) assert.NoError(tt, err) - assert.Len(tt, got, 0) + assert.Equal(tt, len(tc.want), len(got)) + + // Sort both slices to ensure consistent comparison order + if len(got) > 0 && len(tc.want) > 0 { + assert.Equal(tt, tc.want[0].ID(), got[0].ID(), "Workspace IDs should match") + assert.Equal(tt, tc.want[0].Name(), got[0].Name(), "Workspace names should match") + }The assertion should compare the expected and actual results instead of always asserting zero length.
🧹 Nitpick comments (3)
account/accountinfrastructure/accountmongo/workspace_test.go (3)
208-208: Fix typo in test case name.There's a typo in the test case name: "succes" should be "success".
- Name: "succes find multiple workspaces", + Name: "success find multiple workspaces",
242-242: Usett.Parallel()instead oft.Parallel().For consistency with other test functions in this file, use
ttinstead oftfor the parallel test execution.- t.Parallel() + tt.Parallel()
230-230: Use an explicit non-existent integration ID value.For test clarity, consider using a more explicit variable for the non-existent integration ID instead of creating it inline.
+ nonExistentID := workspace.NewIntegrationID() + tests := []struct { // ... existing struct fields }{ // ... existing test cases { Name: "success find no workspaces", - Input: accountdomain.IntegrationIDList{workspace.NewIntegrationID()}, + Input: accountdomain.IntegrationIDList{nonExistentID}, want: workspace.List{}, wantErr: nil, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
account/accountinfrastructure/accountmongo/workspace_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
account/accountinfrastructure/accountmongo/workspace_test.go
[error] 251-251: TestWorkspace_FindByIntegrations failed due to panic: invalid ID.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
account/accountinfrastructure/accountmongo/workspace_test.go (2)
251-253:⚠️ Potential issueFix "invalid ID" panic and incorrect assertion.
This test has two critical issues:
- It's failing with a panic on line 251 due to an "invalid ID"
- The assertion on line 253 always expects 0 results, regardless of the test case expectations
got, err := repo.FindByIntegrations(ctx, tc.Input) assert.NoError(tt, err) - assert.Len(tt, got, 0) + assert.Equal(tt, len(tc.want), len(got)) + + // Sort both slices to ensure consistent comparison order + if len(got) > 0 && len(tc.want) > 0 { + assert.Equal(tt, tc.want[0].ID(), got[0].ID(), "Workspace IDs should match") + assert.Equal(tt, tc.want[0].Name(), got[0].Name(), "Workspace names should match") + }The "invalid ID" panic indicates a problem with how integration IDs are handled in the FindByIntegrations method. This could be resolved by adding proper validation in the repository implementation to handle invalid IDs gracefully instead of panicking.
🧰 Tools
🪛 GitHub Actions: CI
[error] 251-251: TestWorkspace_FindByIntegrations failed due to panic: invalid ID.
194-199: 🛠️ Refactor suggestionAdd validation for IntegrationID creation.
The test is failing with a panic on line 251 due to an "invalid ID". This may be related to how integration IDs are created and initialized. Consider adding validation to ensure that all integration IDs are properly initialized before using them.
func TestWorkspace_FindByIntegrations(t *testing.T) { i1 := workspace.NewIntegrationID() i2 := workspace.NewIntegrationID() + // Ensure IDs are valid + assert.NotEmpty(t, i1.String(), "Integration ID should not be empty") + assert.NotEmpty(t, i2.String(), "Integration ID should not be empty") ws1 := workspace.New().NewID().Name("hoge").Integrations(map[workspace.IntegrationID]workspace.Member{i1: {}}).MustBuild() ws2 := workspace.New().NewID().Name("foo").Integrations(map[workspace.IntegrationID]workspace.Member{i2: {}}).MustBuild()Additionally, consider reviewing the
FindByIntegrationsimplementation in the repository to ensure it properly handles invalid IDs without panicking.
🧹 Nitpick comments (1)
account/accountinfrastructure/accountmongo/workspace_test.go (1)
208-208: Fix typo in test case name.There's a typo in the test case name: "succes" should be "success".
- Name: "succes find multiple workspaces", + Name: "success find multiple workspaces",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
account/accountinfrastructure/accountmongo/workspace_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
account/accountinfrastructure/accountmongo/workspace_test.go
[error] 251-251: TestWorkspace_FindByIntegrations failed due to panic: invalid ID.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
account/accountinfrastructure/accountmongo/workspace_test.go (1)
258-260:⚠️ Potential issueFix incorrect assertion in integration test.
The test has an incorrect assertion that always expects zero results, regardless of the test case's expectations. This will cause false positives when the test should actually fail.
got, err := repo.FindByIntegrations(ctx, tc.Input) assert.NoError(tt, err) - assert.Len(tt, got, 0) + assert.Equal(tt, len(tc.want), len(got)) + + // Sort both slices to ensure consistent comparison order + if len(got) > 0 && len(tc.want) > 0 { + assert.Equal(tt, tc.want[0].ID(), got[0].ID(), "Workspace IDs should match") + assert.Equal(tt, tc.want[0].Name(), got[0].Name(), "Workspace names should match") + }
🧹 Nitpick comments (2)
account/accountinfrastructure/accountmongo/workspace_test.go (2)
215-215: Fix typo in test case name.There's a typo in the test case name: "succes" should be "success".
- Name: "succes find multiple workspaces", + Name: "success find multiple workspaces",
251-264: Improve test cleanup with defer.The test cleanup is only executed if the test reaches line 262. If there's a panic or early return before that, the cleanup won't happen. Use a defer statement to ensure cleanup happens even if the test fails.
client := mongox.NewClientWithDatabase(init(t)) repo := NewWorkspace(client) ctx := context.Background() err := repo.SaveAll(ctx, tc.data) assert.NoError(tt, err) + // Ensure cleanup happens even if test fails + defer func() { + err := repo.RemoveAll(ctx, accountdomain.WorkspaceIDList{ws1.ID(), ws2.ID()}) + assert.NoError(tt, err) + }() + got, err := repo.FindByIntegrations(ctx, tc.Input) assert.NoError(tt, err) assert.Len(tt, got, 0) - err = repo.RemoveAll(ctx, accountdomain.WorkspaceIDList{ws1.ID(), ws2.ID()}) - assert.NoError(t, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
account/accountinfrastructure/accountmongo/workspace.go(1 hunks)account/accountinfrastructure/accountmongo/workspace_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- account/accountinfrastructure/accountmongo/workspace.go
🔇 Additional comments (2)
account/accountinfrastructure/accountmongo/workspace_test.go (2)
249-249: Fix inconsistent test parallelization.The test is using
t.Parallel()instead oftt.Parallel()which is inconsistent with the pattern used in other test functions (see lines 44, 93, 142).- t.Parallel() + tt.Parallel()
262-263: Fix inconsistent test variable usage.The test is using
tfor assertion instead ofttwhich is inconsistent with the rest of the test function.err = repo.RemoveAll(ctx, accountdomain.WorkspaceIDList{ws1.ID(), ws2.ID()}) - assert.NoError(t, err) + assert.NoError(tt, err)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
account/accountinfrastructure/accountmongo/workspace_test.go (1)
215-215: Fix typo in test nameThere's a typo in the test name: "succes" should be "success".
- Name: "succes find multiple workspaces", + Name: "success find multiple workspaces",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
account/accountinfrastructure/accountmongo/workspace_test.go(1 hunks)
🔇 Additional comments (4)
account/accountinfrastructure/accountmongo/workspace_test.go (4)
249-249: Fix inconsistent test parallelizationThe test is using
t.Parallel()instead oftt.Parallel()which is inconsistent with the pattern used in other test functions (see lines 44, 93, 142).- t.Parallel() + tt.Parallel()
258-260: Good implementation of assertion logicThe assertion logic correctly checks the length of results against the expected length, which resolves a previous issue where tests would pass incorrectly.
268-269: Fix inconsistent test variable usageThe test is using
tfor assertion instead ofttwhich is inconsistent with the rest of the test function.err = repo.RemoveAll(ctx, accountdomain.WorkspaceIDList{ws1.ID(), ws2.ID()}) - assert.NoError(t, err) + assert.NoError(tt, err)
194-272: Add use case layer tests for FindByIntegrationsWhile the repository implementation is well-tested, there are no tests for the use case layer implementation of FindByIntegrations. Per PR requirements and previous comments, tests should be added for the use case layer as well.
#!/bin/bash # Check if there are tests for the accountusecase implementation of FindByIntegrations # Look for any test files that might test the accountusecase echo "Searching for account use case test files..." fd "test.*\.go" --full-path account/accountusecase/ # Look for any FindByIntegrations implementations in the use case layer echo "Searching for FindByIntegrations implementations in use case layer..." rg "func.*FindByIntegrations" account/accountusecase/ -A 3
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
account/accountinfrastructure/accountmongo/workspace_test.go (1)
215-215: Fix typo in test case name.There's a minor typo in the test case name: "succes" should be "success".
- Name: "succes find multiple workspaces", + Name: "success find multiple workspaces",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
account/accountinfrastructure/accountmongo/workspace_test.go(1 hunks)
🔇 Additional comments (4)
account/accountinfrastructure/accountmongo/workspace_test.go (4)
247-247: Fix inconsistent test parallelization.The
t.Parallel()call is incorrectly placed outside the test run function. This is inconsistent with the pattern used in other test functions (see lines 44, 93, 142).- t.Parallel() t.Run(tc.Name, func(tt *testing.T) { + tt.Parallel()
267-268: Fix inconsistent test variable usage.The test is using
tfor assertion instead ofttwhich is inconsistent with the rest of the test function.err = repo.RemoveAll(ctx, accountdomain.WorkspaceIDList{ws1.ID(), ws2.ID()}) - assert.NoError(t, err) + assert.NoError(tt, err)
236-239: Add validation for invalid integration IDsThe test checks for non-existent integration IDs but doesn't validate the behavior with invalid IDs. Given the previous "invalid ID" panics, add a test case that explicitly handles invalid integration IDs.
{ Name: "success find no workspaces", Input: accountdomain.IntegrationIDList{workspace.NewIntegrationID()}, want: workspace.List{}, wantErr: nil, }, + { + Name: "handle invalid integration IDs gracefully", + Input: accountdomain.IntegrationIDList{workspace.IntegrationID{}}, // Empty/invalid ID + data: workspace.List{ws1, ws2}, + want: workspace.List{}, + wantErr: nil, + },
194-271: Add Use Case Layer Tests for FindByIntegrationsThe repository tests for FindByIntegrations look solid, but there are no test cases exercising the FindByIntegrations functionality at the use case layer. To ensure full test coverage as described in the PR, please add tests for the use case level that validate this feature.
- Verify that the use case layer indeed exposes the FindByIntegrations behavior.
- Add appropriate test cases (e.g. in
account/accountusecase/accountinteractor/workspace_test.go) to cover various integration scenarios.#!/bin/bash # Check if there are tests for the accountusecase implementation of FindByIntegrations # Look for use case tests and potential implementation echo "Searching for account use case test files..." fd "workspace_test\.go" --full-path account/accountusecase/ echo "Searching for FindByIntegrations implementations in use case layer..." rg "func.*FindByIntegrations" account/accountusecase/ -A 3
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
account/accountinfrastructure/accountmongo/workspace_test.go (2)
215-215: Fix typo in test case nameThere's a typo in the test case name: "succes" should be "success".
- Name: "succes find multiple workspaces", + Name: "success find multiple workspaces",
256-263: Add a verification step to ensure the returned workspaces match the integrationsThe test checks the ID and name of returned workspaces but doesn't verify that the returned workspaces actually contain the integration IDs we searched for. Add a verification step to ensure the correct workspaces are returned.
got, err := repo.FindByIntegrations(ctx, tc.Input) assert.NoError(tt, err) assert.Len(tt, got, len(tc.want)) for k, ws := range got { if ws != nil { assert.Equal(tt, tc.want[k].ID(), ws.ID()) assert.Equal(tt, tc.want[k].Name(), ws.Name()) + // Verify the workspace contains at least one of the input integration IDs + if len(tc.Input) > 0 { + found := false + for _, id := range tc.Input { + if _, ok := ws.Integrations()[id]; ok { + found = true + break + } + } + assert.True(tt, found, "Workspace should contain at least one of the input integration IDs") + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
account/accountinfrastructure/accountmongo/workspace_test.go(1 hunks)
🔇 Additional comments (2)
account/accountinfrastructure/accountmongo/workspace_test.go (2)
236-239: Add validation for invalid integration IDsThe test checks for non-existent integration IDs but doesn't validate the behavior with invalid IDs. Given the previous "invalid ID" panics, add a test case that explicitly handles invalid integration IDs.
{ Name: "success find no workspaces", Input: accountdomain.IntegrationIDList{workspace.NewIntegrationID()}, want: workspace.List{}, wantErr: nil, }, + { + Name: "handle invalid integration IDs gracefully", + Input: accountdomain.IntegrationIDList{workspace.IntegrationID{}}, // Empty/invalid ID + data: workspace.List{ws1, ws2}, + want: workspace.List{}, + wantErr: nil, + },
267-267: Fix inconsistent test variable usageThe test is using
tfor assertion instead oftt, which is inconsistent with the rest of the test function.err = repo.RemoveAll(ctx, accountdomain.WorkspaceIDList{ws1.ID(), ws2.ID()}) - assert.NoError(t, err) + assert.NoError(tt, err)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
account/accountinfrastructure/accountmongo/workspace_test.go (3)
215-215: Fix typo in test case name.There's a typo in the test case name: "succes" should be "success".
- Name: "succes find multiple workspaces", + Name: "success find multiple workspaces",
236-240: Add test case for invalid integration IDs.The test checks for non-existent integration IDs but doesn't validate the behavior with invalid IDs. Given the previous "invalid ID" panics mentioned in past review comments, it's important to explicitly test this scenario.
{ Name: "success find no workspaces", Input: accountdomain.IntegrationIDList{workspace.NewIntegrationID()}, want: workspace.List{}, wantErr: nil, }, + { + Name: "handle invalid integration IDs gracefully", + Input: accountdomain.IntegrationIDList{workspace.IntegrationID{}}, // Empty/invalid ID + data: workspace.List{ws1, ws2}, + want: workspace.List{}, + wantErr: nil, + },
248-265: Add sorting before comparison in tests.When comparing multiple workspaces, the order of results may not be guaranteed. Add sorting to both expected and actual results before comparison to ensure a consistent test.
got, err := repo.FindByIntegrations(ctx, tc.Input) assert.NoError(tt, err) assert.Len(tt, got, len(tc.want)) + + // Sort both slices to ensure consistent comparison order + sortWorkspaces := func(ws workspace.List) workspace.List { + if len(ws) < 2 { + return ws + } + // Sort by ID for consistent order + sort.Slice(ws, func(i, j int) bool { + return ws[i].ID().String() < ws[j].ID().String() + }) + return ws + } + + sortedGot := sortWorkspaces(got) + sortedWant := sortWorkspaces(tc.want) + for k, ws := range got { if ws != nil { - assert.Equal(tt, tc.want[k].ID(), ws.ID()) - assert.Equal(tt, tc.want[k].Name(), ws.Name()) + assert.Equal(tt, sortedWant[k].ID(), sortedGot[k].ID()) + assert.Equal(tt, sortedWant[k].Name(), sortedGot[k].Name()) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
account/accountinfrastructure/accountmongo/workspace_test.go(1 hunks)
🔇 Additional comments (1)
account/accountinfrastructure/accountmongo/workspace_test.go (1)
266-267: Fix inconsistent test variable usage.The test is using
tfor assertion instead oftt, which is inconsistent with the rest of the test function.err = repo.RemoveAll(ctx, accountdomain.WorkspaceIDList{ws1.ID(), ws2.ID()}) - assert.NoError(t, err) + assert.NoError(tt, err)
Add FindByIntegrations for wokrspace in accountusecase to list wrokspaces based on integration IDs
Summary by CodeRabbit
New Features
Tests
FindByIntegrationsmethod for both memory and MongoDB implementations.