Added invitation CRUD #128
Conversation
📝 WalkthroughWalkthroughThe pull request implements a trip invites feature that enables users to create and share invite codes to join trips. It introduces database schema, models, repository operations, service logic for creating and joining invites, API endpoints, and comprehensive test coverage for the workflow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/internal/repository/repository.go (1)
23-34: 🧹 Nitpick | 🔵 TrivialMinor style inconsistency in repository initialization.
Other repositories are initialized with direct struct literals (e.g.,
&userRepository{db: db}), whileTripInviteuses a constructor function. This works correctly but creates a minor inconsistency. Consider aligning with the existing pattern for uniformity, or refactoring all repositories to use constructor functions for consistency.
🤖 Fix all issues with AI agents
In `@backend/internal/controllers/trips.go`:
- Around line 219-224: Don't ignore the error from c.BodyParser when parsing
CreateTripInviteRequest; instead, first check the raw request body via c.Body()
(or len(c.Body()) == 0) and only skip parsing for an empty body, otherwise call
c.BodyParser(&req) and if it returns an error return a 400/validation error
(rather than discarding it). Update the block around
BodyParser/validators.Validate so that for non-empty bodies a parse error is
propagated (e.g., return a fiber 400 with "invalid JSON") and only allow silent
defaulting when the body is truly empty before calling validators.Validate with
ctrl.validator.
In `@backend/internal/migrations/20260205035655_create_trip_invite.sql`:
- Around line 7-14: Remove the redundant non-unique index creation for the
`code` column: delete the `CREATE INDEX idx_trip_invites_code ON
trip_invites(code);` statement (the `code TEXT NOT NULL UNIQUE` already creates
a unique index) and update the down migration so it no longer tries to DROP
`idx_trip_invites_code` (or remove any DROP INDEX for that name) to keep up/down
symmetric; leave the `idx_trip_invites_trip_id` index and the `code` UNIQUE
constraint intact.
In `@backend/internal/models/trip_invites.go`:
- Line 15: The struct field ExpiresAt on the trip invite model uses
contradictory bun tags `nullzero` and `notnull`; remove the `nullzero` tag so
the bun tag becomes `notnull` (and any other intended options like `expires_at`)
to ensure the column is required. Locate the ExpiresAt field in the trip invite
model (symbol ExpiresAt in trip_invites.go), delete `nullzero` from its struct
tag, run tests and any schema/migration checks to ensure the DB column remains
non-nullable.
In `@backend/internal/services/membership.go`:
- Around line 106-122: Extract the duplicated conversion logic into a single
helper, e.g., create a function membershipFromDB(m
*models.MembershipDatabaseResponse) *models.Membership that maps UserID, TripID,
IsAdmin, BudgetMin, BudgetMax, Availability, CreatedAt and UpdatedAt; then
replace the inline conversions in methods that call s.Membership.Find and
AddMember (the blocks that build a models.Membership from
existingMembership/newMembership) to call membershipFromDB(existingMembership)
(or membershipFromDB(newMembership)) instead.
In `@backend/internal/services/trips.go`:
- Around line 265-271: The generateInviteCode function currently uses 6 random
bytes (48 bits); add a brief comment above generateInviteCode documenting the
approximate collision probability (2^48 possible codes ≈ 281 trillion
combinations) and noting the single-retry behavior in the invite-creation path
(the duplicate-check/retry logic that follows) is based on that low probability;
alternatively, if you expect high-volumes, increase entropy by changing the
function to use 8 bytes (64 bits) and update any dependent code/comments
accordingly (keep function name generateInviteCode and ensure callers that
validate duplicates remain compatible).
- Around line 296-306: The retry path after detecting errs.ErrDuplicate must
handle errors from generateInviteCode instead of discarding them: in the block
around s.TripInvite.Create and generateInviteCode, capture the error returned by
generateInviteCode, and if non-nil return it immediately (do not overwrite
invite.Code with an invalid value); only assign invite.Code and call
s.TripInvite.Create again when generateInviteCode succeeds. Ensure the
subsequent err checks still return on failure and reference the existing symbols
s.TripInvite.Create, generateInviteCode, invite.Code, created.
- Around line 308-314: The code currently calls os.Getenv("APP_PUBLIC_URL")
inside CreateTripInvite leading to repeated syscalls; add a field (e.g.,
appPublicURL string) to the service struct in the constructor or a config struct
so the value is read once, initialize s.appPublicURL when the service is
created, and replace the getenv call in CreateTripInvite with s.appPublicURL;
ensure you still trim trailing slashes and build the joinURL using created.Code
(and update any tests to inject the appPublicURL via the service
constructor/config).
In `@backend/internal/tests/invites_test.go`:
- Around line 95-129: The test inserts a TripInvite directly using
db.NewInsert() which can leave state behind; add a t.Cleanup callback
immediately after the insert that deletes the inserted record (use
db.NewDelete().Model for models.TripInvite and filter by invite.ID) to ensure
isolation and prevent flakiness; place the cleanup right after the call that
sets invite (i.e., after NewInsert().Model(invite).Exec) so the inserted invite
is removed after the test finishes.
| code TEXT NOT NULL UNIQUE, | ||
| expires_at TIMESTAMP WITH TIME ZONE NOT NULL, | ||
| is_revoked BOOLEAN NOT NULL DEFAULT false, | ||
| created_at TIMESTAMP WITH TIME ZONE DEFAULT now() | ||
| ); | ||
|
|
||
| CREATE INDEX idx_trip_invites_trip_id ON trip_invites(trip_id); | ||
| CREATE INDEX idx_trip_invites_code ON trip_invites(code); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Index on code is redundant with UNIQUE constraint.
Line 7 declares code TEXT NOT NULL UNIQUE, which automatically creates a unique index. Line 14 creates an additional index idx_trip_invites_code on the same column. This duplicate index wastes storage and slows writes.
Remove redundant index
CREATE INDEX idx_trip_invites_trip_id ON trip_invites(trip_id);
-CREATE INDEX idx_trip_invites_code ON trip_invites(code);
CREATE INDEX idx_trip_invites_created_by ON trip_invites(created_by);Also update the down migration:
DROP INDEX IF EXISTS idx_trip_invites_created_by;
-DROP INDEX IF EXISTS idx_trip_invites_code;
DROP INDEX IF EXISTS idx_trip_invites_trip_id;🤖 Prompt for AI Agents
In `@backend/internal/migrations/20260205035655_create_trip_invite.sql` around
lines 7 - 14, Remove the redundant non-unique index creation for the `code`
column: delete the `CREATE INDEX idx_trip_invites_code ON trip_invites(code);`
statement (the `code TEXT NOT NULL UNIQUE` already creates a unique index) and
update the down migration so it no longer tries to DROP `idx_trip_invites_code`
(or remove any DROP INDEX for that name) to keep up/down symmetric; leave the
`idx_trip_invites_trip_id` index and the `code` UNIQUE constraint intact.
| TripID uuid.UUID `bun:"trip_id,type:uuid,notnull" json:"trip_id"` | ||
| CreatedBy uuid.UUID `bun:"created_by,type:uuid,notnull" json:"created_by"` | ||
| Code string `bun:"code,notnull" json:"code"` | ||
| ExpiresAt time.Time `bun:"expires_at,nullzero,notnull" json:"expires_at"` |
There was a problem hiding this comment.
Contradictory bun tags: nullzero with notnull on expires_at.
The nullzero tag instructs bun to convert Go's zero time.Time to SQL NULL, while notnull declares the column cannot be NULL. If ExpiresAt is ever unset (zero value), the insert will fail at the database level. The service currently always sets this field, but this tag combination is error-prone for future maintainers.
Consider removing nullzero since ExpiresAt is required:
Proposed fix
- ExpiresAt time.Time `bun:"expires_at,nullzero,notnull" json:"expires_at"`
+ ExpiresAt time.Time `bun:"expires_at,notnull" json:"expires_at"`📝 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.
| ExpiresAt time.Time `bun:"expires_at,nullzero,notnull" json:"expires_at"` | |
| ExpiresAt time.Time `bun:"expires_at,notnull" json:"expires_at"` |
🤖 Prompt for AI Agents
In `@backend/internal/models/trip_invites.go` at line 15, The struct field
ExpiresAt on the trip invite model uses contradictory bun tags `nullzero` and
`notnull`; remove the `nullzero` tag so the bun tag becomes `notnull` (and any
other intended options like `expires_at`) to ensure the column is required.
Locate the ExpiresAt field in the trip invite model (symbol ExpiresAt in
trip_invites.go), delete `nullzero` from its struct tag, run tests and any
schema/migration checks to ensure the DB column remains non-nullable.
| // If already a member, return existing membership. | ||
| existingMembership, err := s.Membership.Find(ctx, userID, invite.TripID) | ||
| if err == nil { | ||
| return &models.Membership{ | ||
| UserID: existingMembership.UserID, | ||
| TripID: existingMembership.TripID, | ||
| IsAdmin: existingMembership.IsAdmin, | ||
| BudgetMin: existingMembership.BudgetMin, | ||
| BudgetMax: existingMembership.BudgetMax, | ||
| Availability: existingMembership.Availability, | ||
| CreatedAt: existingMembership.CreatedAt, | ||
| UpdatedAt: existingMembership.UpdatedAt, | ||
| }, nil | ||
| } | ||
| if !errors.Is(err, errs.ErrNotFound) { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting membership conversion to a helper.
The code that converts MembershipDatabaseResponse to Membership (lines 109-118, 143-152) is duplicated here and also appears in AddMember (lines 62-71). A helper method would reduce repetition.
Optional helper extraction
func toMembership(m *models.MembershipDatabaseResponse) *models.Membership {
return &models.Membership{
UserID: m.UserID,
TripID: m.TripID,
IsAdmin: m.IsAdmin,
BudgetMin: m.BudgetMin,
BudgetMax: m.BudgetMax,
Availability: m.Availability,
CreatedAt: m.CreatedAt,
UpdatedAt: m.UpdatedAt,
}
}Also applies to: 135-155
🤖 Prompt for AI Agents
In `@backend/internal/services/membership.go` around lines 106 - 122, Extract the
duplicated conversion logic into a single helper, e.g., create a function
membershipFromDB(m *models.MembershipDatabaseResponse) *models.Membership that
maps UserID, TripID, IsAdmin, BudgetMin, BudgetMax, Availability, CreatedAt and
UpdatedAt; then replace the inline conversions in methods that call
s.Membership.Find and AddMember (the blocks that build a models.Membership from
existingMembership/newMembership) to call membershipFromDB(existingMembership)
(or membershipFromDB(newMembership)) instead.
| created, err := s.TripInvite.Create(ctx, invite) | ||
| if err != nil { | ||
| if errors.Is(err, errs.ErrDuplicate) { | ||
| code, _ = generateInviteCode() | ||
| invite.Code = code | ||
| created, err = s.TripInvite.Create(ctx, invite) | ||
| } | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Error ignored when regenerating invite code on duplicate collision.
On line 299, if generateInviteCode() fails during the retry, the error is silently discarded. The code variable retains its previous (duplicate) value, causing the second Create call to fail with the same duplicate error.
Proposed fix
created, err := s.TripInvite.Create(ctx, invite)
if err != nil {
if errors.Is(err, errs.ErrDuplicate) {
- code, _ = generateInviteCode()
+ code, err = generateInviteCode()
+ if err != nil {
+ return nil, err
+ }
invite.Code = code
created, err = s.TripInvite.Create(ctx, invite)
}
if err != nil {
return nil, err
}
}📝 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.
| created, err := s.TripInvite.Create(ctx, invite) | |
| if err != nil { | |
| if errors.Is(err, errs.ErrDuplicate) { | |
| code, _ = generateInviteCode() | |
| invite.Code = code | |
| created, err = s.TripInvite.Create(ctx, invite) | |
| } | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| created, err := s.TripInvite.Create(ctx, invite) | |
| if err != nil { | |
| if errors.Is(err, errs.ErrDuplicate) { | |
| code, err = generateInviteCode() | |
| if err != nil { | |
| return nil, err | |
| } | |
| invite.Code = code | |
| created, err = s.TripInvite.Create(ctx, invite) | |
| } | |
| if err != nil { | |
| return nil, err | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@backend/internal/services/trips.go` around lines 296 - 306, The retry path
after detecting errs.ErrDuplicate must handle errors from generateInviteCode
instead of discarding them: in the block around s.TripInvite.Create and
generateInviteCode, capture the error returned by generateInviteCode, and if
non-nil return it immediately (do not overwrite invite.Code with an invalid
value); only assign invite.Code and call s.TripInvite.Create again when
generateInviteCode succeeds. Ensure the subsequent err checks still return on
failure and reference the existing symbols s.TripInvite.Create,
generateInviteCode, invite.Code, created.
| var joinURL *string | ||
| baseURL := os.Getenv("APP_PUBLIC_URL") | ||
| if baseURL != "" { | ||
| trimmed := strings.TrimRight(baseURL, "/") | ||
| u := trimmed + "/invites/" + created.Code | ||
| joinURL = &u | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid reading environment variable on every request.
os.Getenv("APP_PUBLIC_URL") is called on each invite creation. Inject this value through the service constructor or a config struct to avoid repeated syscalls and improve testability.
Proposed approach
type TripService struct {
*repository.Repository
fileService FileServiceInterface
publisher realtime.EventPublisher
+ appPublicURL string
}
-func NewTripService(repo *repository.Repository, fileService FileServiceInterface, publisher realtime.EventPublisher) TripServiceInterface {
+func NewTripService(repo *repository.Repository, fileService FileServiceInterface, publisher realtime.EventPublisher, appPublicURL string) TripServiceInterface {
return &TripService{
Repository: repo,
fileService: fileService,
publisher: publisher,
+ appPublicURL: appPublicURL,
}
}Then use s.appPublicURL instead of os.Getenv("APP_PUBLIC_URL") in CreateTripInvite.
🤖 Prompt for AI Agents
In `@backend/internal/services/trips.go` around lines 308 - 314, The code
currently calls os.Getenv("APP_PUBLIC_URL") inside CreateTripInvite leading to
repeated syscalls; add a field (e.g., appPublicURL string) to the service struct
in the constructor or a config struct so the value is read once, initialize
s.appPublicURL when the service is created, and replace the getenv call in
CreateTripInvite with s.appPublicURL; ensure you still trim trailing slashes and
build the joinURL using created.Code (and update any tests to inject the
appPublicURL via the service constructor/config).
| t.Run("expired invite returns 400", func(t *testing.T) { | ||
| app := fakes.GetSharedTestApp() | ||
| db := fakes.GetSharedDB() | ||
|
|
||
| owner := createUser(t, app) | ||
| trip := createTrip(t, app, owner) | ||
|
|
||
| code := "expired-" + uuid.NewString() | ||
| expired := time.Now().UTC().Add(-1 * time.Hour) | ||
|
|
||
| invite := &models.TripInvite{ | ||
| ID: uuid.New(), | ||
| TripID: uuid.MustParse(trip), | ||
| CreatedBy: uuid.MustParse(owner), | ||
| Code: code, | ||
| ExpiresAt: expired, | ||
| IsRevoked: false, | ||
| CreatedAt: time.Now().UTC(), | ||
| } | ||
|
|
||
| _, err := db.NewInsert().Model(invite).Exec(context.Background()) | ||
| require.NoError(t, err) | ||
|
|
||
| user := createUser(t, app) | ||
|
|
||
| testkit.New(t). | ||
| Request(testkit.Request{ | ||
| App: app, | ||
| Route: fmt.Sprintf("/api/v1/trip-invites/%s/join", code), | ||
| Method: testkit.POST, | ||
| UserID: &user, | ||
| }). | ||
| AssertStatus(http.StatusBadRequest). | ||
| AssertField("message", "invite link has expired") | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider test data cleanup for isolation.
Tests that directly insert data via db.NewInsert() (lines 115 and 151) may leave records in the shared test database. If other tests depend on specific invite counts or codes, this could cause flaky behavior. Consider adding cleanup in a t.Cleanup() callback or using unique code prefixes with a cleanup sweep.
Proposed cleanup pattern
// After inserting the invite
t.Cleanup(func() {
_, _ = db.NewDelete().Model((*models.TripInvite)(nil)).Where("id = ?", invite.ID).Exec(context.Background())
})🤖 Prompt for AI Agents
In `@backend/internal/tests/invites_test.go` around lines 95 - 129, The test
inserts a TripInvite directly using db.NewInsert() which can leave state behind;
add a t.Cleanup callback immediately after the insert that deletes the inserted
record (use db.NewDelete().Model for models.TripInvite and filter by invite.ID)
to ensure isolation and prevent flakiness; place the cleanup right after the
call that sets invite (i.e., after NewInsert().Model(invite).Exec) so the
inserted invite is removed after the test finishes.
| CreatedAt time.Time `bun:"created_at,nullzero" json:"created_at"` | ||
| } | ||
|
|
||
| func (TripInvite) TableName() string { |
There was a problem hiding this comment.
can we remove this, i don't think it's being used anywhere
There was a problem hiding this comment.
Which thing are you referring to? the created at or tablename()
in-mai-space
left a comment
in-mai-space
left a comment
There was a problem hiding this comment.
GREAT JOB @jleung40 🔥 few small nits and should be good to go!
| "context" | ||
|
|
||
| v4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4" | ||
| "github.com/aws/aws-sdk-go-v2/aws/signer/v4" |
There was a problem hiding this comment.
damn im not sure why this keeps popping up and disappearing in some prs. must be local linter settings. (no change needed)
|
Good call on the endpoint structuring! Sound logic to me. Just fix those nits, and you'll be good to merge my goat J |
3 participants
Description
Created the POST /trips/:id/invites and POST /trip-invites/:code/join. Notably, this differs from the original ticket instructions of creating a POST /trips/invites/:code/join endpoint. This is made as /trip-invites, since /trips/invites/:code/join was causing fiber to pick up the /invites part of the endpoint as an ID. Anyways, for the invite link url system to work, an env variable for APP_PUBLIC_URL needs to be set, as it's used in trips.go
How has this been tested?
Added tests in invite_tests.go
Checklist