Develop#21
Merged
Merged
Conversation
If the QR code returns the dest url, visits to that URL cannot be tracked. Use endpoint url instead.
It looks terrible this is going away
…atch fix(user): relaxed user update permissions
…atch Feature/caps 61 profile update patch
Feature/event data
Add QR Codes, Links, and tracking
…o-sqlc-yaml Point sqlc.yaml to migrations
Previously pushed an edit to the generated sqlc.json instead of sqlc.yaml. Oops!
Tests still need to be updated
…g-organization-for-user-events Add and use event with orgids view
…ents Feature/caps 66 recc orgs and events
Reviewer's GuideIntroduces a dynamic links feature (CRUD, redirects, visit tracking, QR codes) backed by new DB schema and migrations; enriches events with titles and organization IDs; loosens auth on public collection endpoints; refines user/org role and membership behaviors; improves OAuth/dev environment handling, CORS, logging, CI/publish branches, and migrates tests/infra from schema.sql to migrations-based setup. Sequence diagram for dynamic link resolution and visit loggingsequenceDiagram
actor User
participant Browser
participant Router
participant Handler as LinksHandler
participant DB as Database
User->>Browser: Open /r/{endpoint_url}
Browser->>Router: GET /r/{endpoint_url}
Router->>Handler: ResolveLink(request)
Handler->>DB: GetLinkByEndpointURL(endpointUrl)
DB-->>Handler: Link(lid, dest_url, oid)
Note over Handler,DB: Start async logging of visit
Handler->>Handler: spawn goroutine
activate Handler
Handler->>DB: LogLinkVisit(lid, uid?)
DB-->>Handler: LinkVisit
deactivate Handler
Handler-->>Browser: 302 Found Location: dest_url
Browser->>User: Redirect to destination URL
ER diagram for new links schema and relationshipserDiagram
USERS {
UUID uid PK
TEXT first_name
TEXT last_name
TEXT personal_email
TEXT school_email
}
ORGANIZATIONS {
UUID oid PK
TEXT name
TEXT description
TEXT location
DATE date_created
DATE date_modified
}
LINKS {
UUID lid PK
TEXT endpoint_url
TEXT dest_url
UUID oid FK
TIMESTAMP created_at
}
LINK_VISITS {
UUID lvid PK
UUID lid FK
UUID uid FK
TIMESTAMP created_at
}
EVENTS {
UUID eid PK
TEXT title
TEXT location
TIMESTAMP event_time
TEXT description
DATE date_created
DATE date_modified
}
EVENTS_WITH_ORG_IDS {
UUID eid PK
TEXT title
TEXT location
TIMESTAMP event_time
TEXT description
DATE date_created
DATE date_modified
UUID_array org_ids
}
ORGANIZATIONS ||--o{ LINKS : owns
LINKS ||--o{ LINK_VISITS : has
USERS ||--o{ LINK_VISITS : generates
EVENTS ||--o{ EVENTS_WITH_ORG_IDS : projected_in_view
ORGANIZATIONS ||--o{ EVENTS_WITH_ORG_IDS : hosts
Updated class diagram for events and dynamic links DTOs and modelsclassDiagram
class CreateEventRequest {
string Title
string Location
time.Time EventTime
string Description
}
class UpdateEventRequest {
string* Title
string* Location
time.Time* EventTime
string* Description
}
class EventResponse {
uuid.UUID EID
[]uuid.UUID Organizations
string* Title
string* Location
time.Time* EventTime
string* Description
time.Time* DateCreated
time.Time* DateModified
}
class EventsWithOrgID {
uuid.UUID Eid
pgtype.Text Location
pgtype.Timestamp EventTime
pgtype.Text Description
pgtype.Date DateCreated
pgtype.Date DateModified
pgtype.Text Title
[]uuid.UUID OrgIds
}
class Event {
uuid.UUID Eid
pgtype.Text Location
pgtype.Timestamp EventTime
pgtype.Text Description
pgtype.Date DateCreated
pgtype.Date DateModified
pgtype.Text Title
}
class CreateLinkRequest {
string EndpointURL
string DestURL
uuid.UUID OrgID
}
class UpdateLinkRequest {
string* EndpointURL
string* DestURL
}
class LinkResponse {
uuid.UUID LID
string EndpointURL
string DestURL
uuid.UUID OrgID
time.Time* CreatedAt
}
class VisitCountResponse {
uuid.UUID LID
int64 Count
}
class Link {
uuid.UUID Lid
string EndpointUrl
string DestUrl
uuid.UUID Oid
pgtype.Timestamp CreatedAt
}
class LinkVisit {
uuid.UUID Lvid
uuid.UUID Lid
pgtype.UUID Uid
pgtype.Timestamp CreatedAt
}
CreateEventRequest ..> Event : creates
UpdateEventRequest ..> Event : updates
EventsWithOrgID --> EventResponse : maps_to
Link --> LinkResponse : maps_to
CreateLinkRequest ..> Link : creates
UpdateLinkRequest ..> Link : updates
LinkVisit ..> VisitCountResponse : aggregated_into
Flow diagram for link routing and handler interactionsflowchart LR
subgraph Client
B[Browser]
end
subgraph API
R[Router]
HLinks[LinksHandler]
HOrgs[OrganizationsHandler]
end
subgraph DB[Database]
QLinks["Link queries<br>CreateLink<br>GetLinkByLID<br>GetLinkByEndpointURL<br>UpdateLink<br>DeleteLink<br>ListLinksByOrg<br>LogLinkVisit<br>GetTotalVisits"]
end
B -- GET /r/{endpoint_url} --> R
R -- ResolveLink --> HLinks
HLinks -- GetLinkByEndpointURL --> QLinks
HLinks -- LogLinkVisit (async) --> QLinks
HLinks -- 302 redirect --> B
B -- GET /api/v1/links/{lid}/qrcode --> R
R -- auth middleware --> HLinks
HLinks -- GetLinkByLID --> QLinks
B -- GET /api/v1/organizations/{oid}/links --> R
R -- auth middleware --> HOrgs
HOrgs -- ListLinksByOrg --> QLinks
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 7 issues, and left some high level feedback:
- The QR code generation in GetQRCode hardcodes PUBLIC_LINK_ENDPOINT and even has a TODO about brittleness; consider deriving the public link base from getBaseURL/config instead so links work correctly across environments and hostnames.
- Schema definitions are now split between schema.sql and the new migrations (e.g., user_role dev enum and events_with_org_ids view); since sqlc is driven by migrations only, it may be less confusing to either align schema.sql with the migrations or clearly retire it to avoid future drift.
- The new isDev/getBaseURL/getOAuthRedirectURL helpers introduce several overlapping signals (Env, X-Dev-Host, X-Dev-Proto, forwarded host); it might be worth centralizing this into a single 'deployment mode' decision to make it easier to reason about which hosts/protos are trusted in each environment.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The QR code generation in GetQRCode hardcodes PUBLIC_LINK_ENDPOINT and even has a TODO about brittleness; consider deriving the public link base from getBaseURL/config instead so links work correctly across environments and hostnames.
- Schema definitions are now split between schema.sql and the new migrations (e.g., user_role dev enum and events_with_org_ids view); since sqlc is driven by migrations only, it may be less confusing to either align schema.sql with the migrations or clearly retire it to avoid future drift.
- The new isDev/getBaseURL/getOAuthRedirectURL helpers introduce several overlapping signals (Env, X-Dev-Host, X-Dev-Proto, forwarded host); it might be worth centralizing this into a single 'deployment mode' decision to make it easier to reason about which hosts/protos are trusted in each environment.
## Individual Comments
### Comment 1
<location path="internal/database/mocks/Querier.go" line_range="780-789" />
<code_context>
}
// UpdateEvent provides a mock function with given fields: ctx, arg
-func (_m *Querier) UpdateEvent(ctx context.Context, arg database.UpdateEventParams) (database.Event, error) {
+func (_m *Querier) UpdateEvent(ctx context.Context, arg database.UpdateEventParams) (database.EventsWithOrgID, error) {
ret := _m.Called(ctx, arg)
</code_context>
<issue_to_address>
**issue (bug_risk):** Mock UpdateEvent return type no longer matches the Querier interface signature.
The `Querier` interface in `internal/database/querier.go` still returns `Event`, but this mock now returns `EventsWithOrgID`, so code using `database.Querier` with this mock will not compile. Please either change the interface (and `queries.sql.go` implementation) to return `EventsWithOrgID`, or revert the mock to return `Event` to keep them consistent.
</issue_to_address>
### Comment 2
<location path="internal/handler/links.go" line_range="294-301" />
<code_context>
+ return
+ }
+
+ qrcOpts := []standard.ImageOption{
+ standard.WithBgColorRGBHex(QR_BG_COLOR),
+ standard.WithFgColorRGBHex(QR_FG_COLOR),
+ // standard.WithCircleShape(),
+ }
+
+ w.Header().Set("Content-Type", "image/png")
+ wr := standard.NewWithWriter(nopWriteCloser{w}, qrcOpts...)
+ if err != nil {
+ slog.Error("failed to create standard writer for QR code", "error", err)
</code_context>
<issue_to_address>
**issue (bug_risk):** Error handling in GetQRCode checks the wrong error variable for the writer creation.
`qrc, err := qrcode.NewWith(...)` initializes `err`, but `standard.NewWithWriter(...)` assigns only to `wr`. The subsequent `if err != nil` therefore reuses the earlier (already-checked) `err` instead of the writer creation error. Bind the error from `NewWithWriter`, e.g. `wr, err := standard.NewWithWriter(...)`, and check that so writer construction failures are handled correctly.
</issue_to_address>
### Comment 3
<location path="internal/handler/organizations_test.go" line_range="134" />
<code_context>
}
}
+
+func TestAddOrgMemberAllowsSelfJoin(t *testing.T) {
+ uid := uuid.New()
+ oid := uuid.New()
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for non‑self AddOrgMember and self‑promotion to admin to fully exercise the new auth logic
The new `TestAddOrgMemberAllowsSelfJoin` and `TestRemoveOrgMemberAuthorization` cover the relaxed self-join/remove rules well.
To fully exercise `AddOrgMember`’s auth logic, please also add:
- A test where an authenticated human tries to add a different user without being an org admin, expecting `403`.
- A test where a user self-joins with `IsAdmin: true`, verifying self-promotion is rejected with `403`.
- Optionally, a test for `AuthType == "bot"` to confirm bots can still add members regardless of `UID`.
These will cover the branch gated by `(req.UID != authenticatedUID || req.IsAdmin)` and fully validate the new authorization behavior.
Suggested implementation:
```golang
func TestAddOrgMemberAllowsSelfJoin(t *testing.T) {
uid := uuid.New()
oid := uuid.New()
mockQueries := mocks.NewQuerier(t)
mockQueries.On("AddOrgMember", mock.Anything, mock.MatchedBy(func(arg database.AddOrgMemberParams) bool {
return arg.Oid == oid && arg.Uid == uid && arg.IsAdmin.Valid && !arg.IsAdmin.Bool
})).Return(nil)
h := handler.New(mockQueries, &config.Config{})
r := chi.NewRouter()
// Assuming there is a common route-registration helper used elsewhere in this file.
// Replace with the actual route registration call used in other org tests if different.
h.RegisterOrgRoutes(r)
body, _ := json.Marshal(dto.AddMemberRequest{
UID: uid,
IsAdmin: false,
})
req := httptest.NewRequest(http.MethodPost, "/orgs/"+oid.String()+"/members", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
// Assuming there is an auth helper / context key used elsewhere in tests; mirror that usage.
authenticatedUID := uid
ctx := context.WithValue(req.Context(), handler.AuthenticatedUIDContextKey{}, authenticatedUID)
req = req.WithContext(ctx)
rr := httptest.NewRecorder()
r.ServeHTTP(rr, req)
assert.Equal(t, http.StatusOK, rr.Code)
mockQueries.AssertExpectations(t)
}
func TestAddOrgMemberRejectsNonAdminAddingDifferentUser(t *testing.T) {
authenticatedUID := uuid.New()
targetUID := uuid.New()
oid := uuid.New()
require.NotEqual(t, authenticatedUID, targetUID, "sanity check: authenticated user and target user must differ")
mockQueries := mocks.NewQuerier(t)
// Non-admin, non-self add should be rejected by auth layer before hitting DB.
// So we do NOT expect AddOrgMember to be called at all.
mockQueries.AssertNotCalled(t, "AddOrgMember", mock.Anything, mock.Anything)
h := handler.New(mockQueries, &config.Config{})
r := chi.NewRouter()
h.RegisterOrgRoutes(r)
body, _ := json.Marshal(dto.AddMemberRequest{
UID: targetUID,
IsAdmin: false,
})
req := httptest.NewRequest(http.MethodPost, "/orgs/"+oid.String()+"/members", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
ctx := context.WithValue(req.Context(), handler.AuthenticatedUIDContextKey{}, authenticatedUID)
req = req.WithContext(ctx)
rr := httptest.NewRecorder()
r.ServeHTTP(rr, req)
assert.Equal(t, http.StatusForbidden, rr.Code)
// No DB call should have been made.
mockQueries.AssertExpectations(t)
}
func TestAddOrgMemberRejectsSelfPromotionToAdmin(t *testing.T) {
uid := uuid.New()
oid := uuid.New()
mockQueries := mocks.NewQuerier(t)
// Self-promotion should be rejected by auth before DB layer, so no AddOrgMember calls.
mockQueries.AssertNotCalled(t, "AddOrgMember", mock.Anything, mock.Anything)
h := handler.New(mockQueries, &config.Config{})
r := chi.NewRouter()
h.RegisterOrgRoutes(r)
body, _ := json.Marshal(dto.AddMemberRequest{
UID: uid,
IsAdmin: true, // attempt self-promotion
})
req := httptest.NewRequest(http.MethodPost, "/orgs/"+oid.String()+"/members", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
ctx := context.WithValue(req.Context(), handler.AuthenticatedUIDContextKey{}, uid)
req = req.WithContext(ctx)
rr := httptest.NewRecorder()
r.ServeHTTP(rr, req)
assert.Equal(t, http.StatusForbidden, rr.Code)
mockQueries.AssertExpectations(t)
}
func TestAddOrgMemberBotCanAddAnyUser(t *testing.T) {
botUID := uuid.New()
targetUID := uuid.New()
oid := uuid.New()
mockQueries := mocks.NewQuerier(t)
mockQueries.On("AddOrgMember", mock.Anything, mock.MatchedBy(func(arg database.AddOrgMemberParams) bool {
return arg.Oid == oid &&
arg.Uid == targetUID &&
arg.IsAdmin.Valid &&
!arg.IsAdmin.Bool
})).Return(nil)
h := handler.New(mockQueries, &config.Config{})
r := chi.NewRouter()
h.RegisterOrgRoutes(r)
body, _ := json.Marshal(dto.AddMemberRequest{
UID: targetUID,
IsAdmin: false,
})
req := httptest.NewRequest(http.MethodPost, "/orgs/"+oid.String()+"/members", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
// Assuming bots are identified via an auth struct or context; adapt to actual implementation.
botAuthInfo := handler.AuthInfo{
UID: botUID,
AuthType: "bot",
}
ctx := context.WithValue(req.Context(), handler.AuthInfoContextKey{}, botAuthInfo)
req = req.WithContext(ctx)
rr := httptest.NewRecorder()
r.ServeHTTP(rr, req)
assert.Equal(t, http.StatusOK, rr.Code)
mockQueries.AssertExpectations(t)
```
The above changes assume several details that need to be aligned with your actual codebase:
1. **Route registration**
- Replace `h.RegisterOrgRoutes(r)` with the actual route-registration helper used elsewhere in `organizations_test.go` (e.g. `registerOrgRoutes(r, h)` or `handler.RegisterRoutes(r, h)`).
2. **Auth wiring**
- I introduced `handler.AuthenticatedUIDContextKey{}` and `handler.AuthInfoContextKey{}` plus a `handler.AuthInfo` struct as placeholders.
- Update these to the real context keys / auth struct you already use in this test file (e.g. `auth.ContextKeyUID`, `auth.ContextKeyAuthInfo`, `dto.AuthenticatedUser`, etc.).
- For the bot test, set `AuthType == "bot"` in whatever way your auth layer expects (could be a string field on the auth struct, a separate context key, or a claim).
3. **DTO and route shape**
- Confirm the request type is `dto.AddMemberRequest` and that it has fields `UID` and `IsAdmin`. If the org identifier is supplied in the body (e.g. `OrgID`) rather than path, add/set that field accordingly and adjust the URL path if necessary.
- If the endpoint path differs from `/orgs/{oid}/members`, update the `httptest.NewRequest` URLs to match.
4. **Mock expectations**
- In the “rejected” tests, I used `mockQueries.AssertNotCalled` to express that authorization happens before DB calls. If your handler still calls `AddOrgMember` and then returns `403`, adjust expectations to either:
- Set up `.On("AddOrgMember", ...)` and still assert `StatusForbidden`, or
- Use `AssertNumberOfCalls` with the actual expected count.
Once you line up the route registration and auth/context wiring with the existing tests in this file, these tests will exercise:
- Non-self, non-admin add → `403`.
- Self-promotion to admin → `403`.
- Bot adding arbitrary user → allowed, `200` (or your success code).
</issue_to_address>
### Comment 4
<location path="internal/handler/links_test.go" line_range="24" />
<code_context>
+ "github.com/stretchr/testify/mock"
+)
+
+func TestCreateLink(t *testing.T) {
+ uid := uuid.New()
+ oid := uuid.New()
</code_context>
<issue_to_address>
**suggestion (testing):** Broaden link handler unit tests to cover validation failures, unauthorized calls, and the remaining handlers
The current tests exercise the happy path and a simple forbidden case, but many key behaviours of the new links handlers aren’t covered yet. Please add unit tests for:
**CreateLink**
- `OrgID == uuid.Nil` → `400`.
- Missing/malformed JSON body → `400`.
- Missing user claims (`GetUserClaims` returns !ok) → `401`.
- `IsOrgAdmin` error mapped via `handleDBError`.
**UpdateLink**
- Non-admin updates → `403`.
- `GetLinkByLID` returns `sql.ErrNoRows` → `404`.
- `UpdateLink` failure returns an error status (not `200`).
- Payloads with only one of `EndpointURL` / `DestURL` set (to confirm `COALESCE` behaviour).
**ListOrgLinks / GetTotalVisits / GetQRCode**
- `ListOrgLinks`: empty vs non-empty list, and `GetLinksByOrg` DB error.
- `GetTotalVisits`: invalid `lid` → `400`, missing link → `404` (per DB behaviour), and success `200`.
- `GetQRCode`: invalid `lid` → `400`, not found → `404`, and QR generation failure (e.g. via bad `PUBLIC_LINK_ENDPOINT`) → `500`.
Table-driven tests across these handlers will better validate error and edge-case behaviour.
Suggested implementation:
```golang
"github.com/capyrpi/api/internal/dto"
"github.com/capyrpi/api/internal/handler"
"github.com/capyrpi/api/internal/middleware"
"github.com/go-chi/chi/v5"
"github.com/google/uuid"
"github.com/jackc/pgx/v5/pgtype"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
)
func TestCreateLink_ValidationAndAuthFailures(t *testing.T) {
uid := uuid.New()
oid := uuid.New()
type testCase struct {
name string
body string
setupMock func(q *mocks.Querier)
setupRequest func(r *http.Request)
expectedStatus int
}
tests := []testCase{
{
name: "OrgID_nil_returns_400",
body: `{
"org_id": "` + uuid.Nil.String() + `",
"endpoint_url": "promo",
"dest_url": "https://example.com"
}`,
setupMock: func(q *mocks.Querier) {
// No DB calls expected because validation fails before DB access.
},
setupRequest: func(r *http.Request) {
// Add valid user claims so the failure is specifically due to OrgID validation.
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusBadRequest,
},
{
name: "Malformed_JSON_body_returns_400",
body: `{"org_id": "not-a-valid-json"`, // deliberately malformed JSON
setupMock: func(q *mocks.Querier) {
// No DB calls expected because JSON decoding fails.
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusBadRequest,
},
{
name: "Missing_user_claims_returns_401",
body: `{
"org_id": "` + oid.String() + `",
"endpoint_url": "promo",
"dest_url": "https://example.com"
}`,
setupMock: func(q *mocks.Querier) {
// No DB calls because the handler should short-circuit on missing claims.
},
setupRequest: func(r *http.Request) {
// Intentionally do not put user claims into the context.
},
expectedStatus: http.StatusUnauthorized,
},
{
name: "IsOrgAdmin_DB_error_mapped_via_handleDBError",
body: `{
"org_id": "` + oid.String() + `",
"endpoint_url": "promo",
"dest_url": "https://example.com"
}`,
setupMock: func(q *mocks.Querier) {
q.On("IsOrgAdmin", mock.Anything, uid, oid).
Return(false, sql.ErrConnDone).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
r = r.WithContext(ctx)
},
// We expect the status that handleDBError maps sql.ErrConnDone to.
// If your handleDBError maps this to 500, adjust accordingly.
expectedStatus: http.StatusInternalServerError,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
q := &mocks.Querier{}
if tc.setupMock != nil {
tc.setupMock(q)
}
h := handler.CreateLink(q)
req := httptest.NewRequest(http.MethodPost, "/links", bytes.NewBufferString(tc.body))
req.Header.Set("Content-Type", "application/json")
if tc.setupRequest != nil {
tc.setupRequest(req)
}
rr := httptest.NewRecorder()
h.ServeHTTP(rr, req)
assert.Equal(t, tc.expectedStatus, rr.Code)
q.AssertExpectations(t)
})
}
}
func TestUpdateLink_AuthorizationAndDBFailures(t *testing.T) {
uid := uuid.New()
oid := uuid.New()
lid := uuid.New()
type testCase struct {
name string
body string
setupMock func(q *mocks.Querier)
setupRequest func(r *http.Request)
expectedStatus int
}
tests := []testCase{
{
name: "Non_admin_update_returns_403",
body: `{
"endpoint_url": "updated-endpoint",
"dest_url": "https://updated.example.com"
}`,
setupMock: func(q *mocks.Querier) {
q.On("IsOrgAdmin", mock.Anything, uid, oid).
Return(false, nil).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("lid", lid.String())
ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusForbidden,
},
{
name: "GetLinkByLID_sql_NoRows_returns_404",
body: `{}`,
setupMock: func(q *mocks.Querier) {
q.On("IsOrgAdmin", mock.Anything, uid, oid).
Return(true, nil).
Once()
q.On("GetLinkByLID", mock.Anything, lid).
Return(dto.Link{}, sql.ErrNoRows).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("lid", lid.String())
ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusNotFound,
},
{
name: "UpdateLink_failure_returns_non_200",
body: `{
"endpoint_url": "new-endpoint",
"dest_url": "https://new.example.com"
}`,
setupMock: func(q *mocks.Querier) {
q.On("IsOrgAdmin", mock.Anything, uid, oid).
Return(true, nil).
Once()
q.On("GetLinkByLID", mock.Anything, lid).
Return(dto.Link{
ID: lid,
OrgID: oid,
Endpoint: "old-endpoint",
DestURL: "https://old.example.com",
CreatedAt: time.Now(),
}, nil).
Once()
q.On("UpdateLink", mock.Anything, mock.Anything).
Return(dto.Link{}, sql.ErrConnDone).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("lid", lid.String())
ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusInternalServerError,
},
{
name: "COALESCE_behaviour_endpoint_only",
body: `{
"endpoint_url": "only-endpoint"
}`,
setupMock: func(q *mocks.Querier) {
q.On("IsOrgAdmin", mock.Anything, uid, oid).
Return(true, nil).
Once()
q.On("GetLinkByLID", mock.Anything, lid).
Return(dto.Link{
ID: lid,
OrgID: oid,
Endpoint: "old-endpoint",
DestURL: "https://old.example.com",
}, nil).
Once()
q.On("UpdateLink", mock.Anything, mock.MatchedBy(func(arg dto.UpdateLinkParams) bool {
// Endpoint is updated, DestURL stays unchanged (COALESCE semantics).
return arg.Endpoint == "only-endpoint" && arg.DestURL == "https://old.example.com"
})).
Return(dto.Link{}, nil).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("lid", lid.String())
ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusOK,
},
{
name: "COALESCE_behaviour_dest_only",
body: `{
"dest_url": "https://only.example.com"
}`,
setupMock: func(q *mocks.Querier) {
q.On("IsOrgAdmin", mock.Anything, uid, oid).
Return(true, nil).
Once()
q.On("GetLinkByLID", mock.Anything, lid).
Return(dto.Link{
ID: lid,
OrgID: oid,
Endpoint: "old-endpoint",
DestURL: "https://old.example.com",
}, nil).
Once()
q.On("UpdateLink", mock.Anything, mock.MatchedBy(func(arg dto.UpdateLinkParams) bool {
// DestURL updated, endpoint unchanged.
return arg.Endpoint == "old-endpoint" && arg.DestURL == "https://only.example.com"
})).
Return(dto.Link{}, nil).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("lid", lid.String())
ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusOK,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
q := &mocks.Querier{}
if tc.setupMock != nil {
tc.setupMock(q)
}
h := handler.UpdateLink(q)
req := httptest.NewRequest(http.MethodPatch, "/links/"+lid.String(), bytes.NewBufferString(tc.body))
req.Header.Set("Content-Type", "application/json")
if tc.setupRequest != nil {
tc.setupRequest(req)
}
rr := httptest.NewRecorder()
h.ServeHTTP(rr, req)
assert.Equal(t, tc.expectedStatus, rr.Code)
q.AssertExpectations(t)
})
}
}
func TestListOrgLinks_EmptyAndNonEmptyAndDBError(t *testing.T) {
uid := uuid.New()
oid := uuid.New()
type testCase struct {
name string
setupMock func(q *mocks.Querier)
setupRequest func(r *http.Request)
expectedStatus int
expectedCount int
}
tests := []testCase{
{
name: "Empty_list_returns_200_with_empty_payload",
setupMock: func(q *mocks.Querier) {
q.On("GetLinksByOrg", mock.Anything, oid).
Return([]dto.Link{}, nil).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("oid", oid.String())
ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusOK,
expectedCount: 0,
},
{
name: "Non_empty_list_returns_200",
setupMock: func(q *mocks.Querier) {
q.On("GetLinksByOrg", mock.Anything, oid).
Return([]dto.Link{
{ID: uuid.New(), OrgID: oid},
{ID: uuid.New(), OrgID: oid},
}, nil).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("oid", oid.String())
ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusOK,
expectedCount: 2,
},
{
name: "DB_error_returns_error_status",
setupMock: func(q *mocks.Querier) {
q.On("GetLinksByOrg", mock.Anything, oid).
Return(nil, sql.ErrConnDone).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("oid", oid.String())
ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusInternalServerError,
expectedCount: 0,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
q := &mocks.Querier{}
if tc.setupMock != nil {
tc.setupMock(q)
}
h := handler.ListOrgLinks(q)
req := httptest.NewRequest(http.MethodGet, "/orgs/"+oid.String()+"/links", nil)
if tc.setupRequest != nil {
tc.setupRequest(req)
}
rr := httptest.NewRecorder()
h.ServeHTTP(rr, req)
assert.Equal(t, tc.expectedStatus, rr.Code)
if tc.expectedStatus == http.StatusOK {
var resp []dto.Link
err := json.Unmarshal(rr.Body.Bytes(), &resp)
assert.NoError(t, err)
assert.Len(t, resp, tc.expectedCount)
}
q.AssertExpectations(t)
})
}
}
func TestGetTotalVisits_VariousOutcomes(t *testing.T) {
lid := uuid.New()
type testCase struct {
name string
lidParam string
setupMock func(q *mocks.Querier)
expectedStatus int
}
tests := []testCase{
{
name: "Invalid_lid_returns_400",
lidParam: "not-a-uuid",
setupMock: func(q *mocks.Querier) {},
expectedStatus: http.StatusBadRequest,
},
{
name: "Missing_link_returns_404",
lidParam: lid.String(),
setupMock: func(q *mocks.Querier) {
q.On("GetTotalVisitsByLID", mock.Anything, lid).
Return(int64(0), sql.ErrNoRows).
Once()
},
expectedStatus: http.StatusNotFound,
},
{
name: "Success_returns_200",
lidParam: lid.String(),
setupMock: func(q *mocks.Querier) {
q.On("GetTotalVisitsByLID", mock.Anything, lid).
Return(int64(42), nil).
Once()
},
expectedStatus: http.StatusOK,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
q := &mocks.Querier{}
if tc.setupMock != nil {
tc.setupMock(q)
}
h := handler.GetTotalVisits(q)
req := httptest.NewRequest(http.MethodGet, "/links/"+tc.lidParam+"/visits", nil)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("lid", tc.lidParam)
ctx := context.WithValue(req.Context(), chi.RouteCtxKey, chiCtx)
req = req.WithContext(ctx)
rr := httptest.NewRecorder()
h.ServeHTTP(rr, req)
assert.Equal(t, tc.expectedStatus, rr.Code)
q.AssertExpectations(t)
})
}
}
func TestGetQRCode_VariousOutcomes(t *testing.T) {
lid := uuid.New()
type testCase struct {
name string
lidParam string
publicLinkEnv string
setupMock func(q *mocks.Querier)
expectedStatus int
expectImageBody bool
}
tests := []testCase{
{
name: "Invalid_lid_returns_400",
lidParam: "not-a-uuid",
publicLinkEnv: "https://links.example.com",
setupMock: func(q *mocks.Querier) {},
expectedStatus: http.StatusBadRequest,
},
{
name: "Not_found_returns_404",
lidParam: lid.String(),
publicLinkEnv: "https://links.example.com",
setupMock: func(q *mocks.Querier) {
q.On("GetLinkByLID", mock.Anything, lid).
Return(dto.Link{}, sql.ErrNoRows).
Once()
},
expectedStatus: http.StatusNotFound,
},
{
name: "QR_generation_failure_returns_500",
lidParam: lid.String(),
publicLinkEnv: "://bad-url", // Deliberately malformed base URL
setupMock: func(q *mocks.Querier) {
q.On("GetLinkByLID", mock.Anything, lid).
Return(dto.Link{ID: lid, Endpoint: "promo"}, nil).
Once()
},
expectedStatus: http.StatusInternalServerError,
},
{
name: "Success_returns_200_with_image",
lidParam: lid.String(),
publicLinkEnv: "https://links.example.com",
setupMock: func(q *mocks.Querier) {
q.On("GetLinkByLID", mock.Anything, lid).
Return(dto.Link{ID: lid, Endpoint: "promo"}, nil).
Once()
},
expectedStatus: http.StatusOK,
expectImageBody: true,
},
}
origPublicEndpoint := os.Getenv("PUBLIC_LINK_ENDPOINT")
defer os.Setenv("PUBLIC_LINK_ENDPOINT", origPublicEndpoint)
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
os.Setenv("PUBLIC_LINK_ENDPOINT", tc.publicLinkEnv)
q := &mocks.Querier{}
if tc.setupMock != nil {
tc.setupMock(q)
}
h := handler.GetQRCode(q)
req := httptest.NewRequest(http.MethodGet, "/links/"+tc.lidParam+"/qr", nil)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("lid", tc.lidParam)
ctx := context.WithValue(req.Context(), chi.RouteCtxKey, chiCtx)
req = req.WithContext(ctx)
rr := httptest.NewRecorder()
h.ServeHTTP(rr, req)
assert.Equal(t, tc.expectedStatus, rr.Code)
if tc.expectImageBody {
assert.NotEmpty(t, rr.Body.Bytes())
assert.Equal(t, "image/png", rr.Header().Get("Content-Type"))
}
q.AssertExpectations(t)
})
}
}
func TestCreateLink(t *testing.T) {
uid := uuid.New()
oid := uuid.New()
lid := uuid.New()
tests := []struct {
name string
requestBody interface{}
setupMock func(*mocks.Querier)
setupContext func() context.Context
expectedStatus int
}{
{
```
1. These tests assume:
- `handler.CreateLink`, `handler.UpdateLink`, `handler.ListOrgLinks`, `handler.GetTotalVisits`, and `handler.GetQRCode` each return `http.HandlerFunc` and accept a `*mocks.Querier`-compatible DB interface as their only parameter.
- Context keys such as `middleware.UserIDContextKey` (or your actual user-claims key) are used to retrieve the authenticated user. Adjust the keys and claim types based on your code.
- DTOs such as `dto.Link` and `dto.UpdateLinkParams` exist with the fields referenced; if your parameter types differ, update the matchers accordingly (especially the `UpdateLink` COALESCE tests).
- `handleDBError` maps `sql.ErrConnDone` to `500`. If it maps to a different status, adjust the expected status codes in `IsOrgAdmin_DB_error_mapped_via_handleDBError`, `DB_error_returns_error_status`, and similar tests.
2. Ensure the following imports exist at the top of the file if they are not already present: `bytes`, `context`, `database/sql`, `encoding/json`, `net/http`, `net/http/httptest`, `os`, `testing`, `time`.
3. If your existing tests use a shared router or helper (e.g. `newTestServer` instead of calling handler constructors directly), adapt the `h := handler.XYZ(q)` / `h.ServeHTTP` lines to match that pattern.
4. The context wiring using `chi.NewRouteContext` assumes route params are read via `chi.URLParam`; if your handlers pull IDs differently, adjust the way the `lid` and `oid` values are passed into the request.
</issue_to_address>
### Comment 5
<location path="internal/router/router_auth_test.go" line_range="268" />
<code_context>
assert.Equal(t, http.StatusOK, res.Code)
}
+func TestPublicCollectionRoutesDoNotRequireAuth(t *testing.T) {
+ mockQueries := mocks.NewQuerier(t)
+ routerUnderTest := newTestRouter(mockQueries)
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert public collection routes on the /v1 prefix and add a negative case for /api/v1 events/orgs
Since the router also exposes public collection routes under `/v1` (without `/api`), please extend this test to cover `GET /v1/organizations` and `GET /v1/events` as unauthenticated. Also add a negative case such as unauthenticated `GET /api/v1/organizations/{oid}` to verify per-org endpoints remain protected, matching the behavior defined in `router.go`.
Suggested implementation:
```golang
func TestPublicCollectionRoutesDoNotRequireAuth(t *testing.T) {
mockQueries := mocks.NewQuerier(t)
routerUnderTest := newTestRouter(mockQueries)
mockQueries.On("ListOrganizations", mock.Anything, mock.MatchedBy(func(arg database.ListOrganizationsParams) bool {
return arg.Limit == 20 && arg.Offset == 0
})).Return([]database.Organization{}, nil).Twice()
mockQueries.On("ListEvents", mock.Anything, mock.MatchedBy(func(arg database.ListEventsParams) bool {
return arg.Limit == 20 && arg.Offset == 0
})).Return([]database.EventsWithOrgID{}, nil).Twice()
// /api/v1 collection endpoints should be public
orgReq := httptest.NewRequest(http.MethodGet, "/api/v1/organizations", nil)
orgRes := httptest.NewRecorder()
routerUnderTest.ServeHTTP(orgRes, orgReq)
assert.Equal(t, http.StatusOK, orgRes.Code)
eventsReq := httptest.NewRequest(http.MethodGet, "/api/v1/events", nil)
eventsRes := httptest.NewRecorder()
routerUnderTest.ServeHTTP(eventsRes, eventsReq)
assert.Equal(t, http.StatusOK, eventsRes.Code)
// /v1 collection endpoints should also be public
v1OrgReq := httptest.NewRequest(http.MethodGet, "/v1/organizations", nil)
v1OrgRes := httptest.NewRecorder()
routerUnderTest.ServeHTTP(v1OrgRes, v1OrgReq)
assert.Equal(t, http.StatusOK, v1OrgRes.Code)
v1EventsReq := httptest.NewRequest(http.MethodGet, "/v1/events", nil)
v1EventsRes := httptest.NewRecorder()
routerUnderTest.ServeHTTP(v1EventsRes, v1EventsReq)
assert.Equal(t, http.StatusOK, v1EventsRes.Code)
// Per-org endpoints should remain protected
protectedOrgReq := httptest.NewRequest(http.MethodGet, "/api/v1/organizations/some-org-id", nil)
protectedOrgRes := httptest.NewRecorder()
routerUnderTest.ServeHTTP(protectedOrgRes, protectedOrgReq)
assert.Equal(t, http.StatusUnauthorized, protectedOrgRes.Code)
```
1. If your auth middleware returns a different status code for unauthenticated access (e.g. 403 Forbidden instead of 401 Unauthorized), adjust the final `assert.Equal` accordingly.
2. If there were already assertions after the original `orgReq := ...` line in this test, remove them to avoid duplicated or conflicting checks, keeping only the expanded body above.
</issue_to_address>
### Comment 6
<location path="internal/handler/handler.go" line_range="34" />
<code_context>
}
}
+func (h *Handler) isDev(r *http.Request) bool {
+ // 1. Explicit dev/staging environment
+ if h.Config.Env == "development" || h.Config.Env == "staging" || h.Config.Env == "" {
</code_context>
<issue_to_address>
**issue (complexity):** Consider resolving dev mode, host, and protocol once per request into a small context struct and having all helper methods reuse it.
You can reduce the overlap and branching by resolving host/proto/dev **once** per request and reusing that everywhere.
### 1. Introduce a small request context helper
```go
type requestCtx struct {
host string // without port
proto string // "http" or "https"
isDev bool
}
func (h *Handler) resolveRequestCtx(r *http.Request) requestCtx {
// 1. Decide isDev once
fh := r.Header.Get("X-Forwarded-Host")
host := r.Host
if fh != "" {
host = fh
}
isDev := false
if h.Config.Env == "development" || h.Config.Env == "staging" || h.Config.Env == "" {
isDev = true
} else if r.Header.Get("X-Dev-Host") != "" {
isDev = true
} else if strings.HasPrefix(fh, "localhost") || strings.HasPrefix(fh, "127.0.0.1") {
isDev = true
} else if strings.HasPrefix(host, "dev.") ||
strings.HasPrefix(host, "localhost") ||
strings.HasPrefix(host, "127.0.0.1") {
isDev = true
}
// 2. Decide proto once (respect dev override)
proto := r.Header.Get("X-Dev-Proto")
if proto == "" {
proto = "http"
if r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https" {
proto = "https"
}
}
// 3. Normalize host (respect dev override)
if devHost := r.Header.Get("X-Dev-Host"); devHost != "" {
host = devHost
}
if strings.Contains(host, ":") {
host, _, _ = strings.Cut(host, ":")
}
return requestCtx{
host: host,
proto: proto,
isDev: isDev,
}
}
```
This keeps all your current rules (env, headers, forwarded host, TLS, dev overrides) in one place.
### 2. Rewrite the helpers to use this context
```go
func (h *Handler) getBaseURL(r *http.Request) string {
ctx := h.resolveRequestCtx(r)
return ctx.proto + "://" + ctx.host
}
func (h *Handler) getCookieDomain(r *http.Request) string {
ctx := h.resolveRequestCtx(r)
if ctx.isDev {
return ctx.host
}
return h.Config.Cookie.Domain
}
func (h *Handler) getOAuthRedirectURL(r *http.Request, providerRedirectURL string) string {
ctx := h.resolveRequestCtx(r)
if !ctx.isDev {
return ""
}
if !strings.Contains(providerRedirectURL, "://") {
return ""
}
parts := strings.SplitN(providerRedirectURL, "/", 4)
if len(parts) < 4 {
return ""
}
return ctx.proto + "://" + ctx.host + "/" + parts[3]
}
```
If you still want to keep `isDev` as a separate helper, make it a thin wrapper over `resolveRequestCtx`:
```go
func (h *Handler) isDev(r *http.Request) bool {
return h.resolveRequestCtx(r).isDev
}
```
This:
- Removes repeated header/host/proto logic from `isDev`, `getActualHost`, `getBaseURL`, `getCookieDomain`, and `getOAuthRedirectURL`.
- Centralizes precedence rules (env vs headers vs proto/TLS) for easier reasoning and testing.
- Keeps all existing behavior (dev headers, dynamic domains, etc.) while reducing branching and coupling.
</issue_to_address>
### Comment 7
<location path="internal/router/router.go" line_range="40" />
<code_context>
})
})
+
+ r.With(middleware.Auth(jwtSecret)).Post("/organizations", h.CreateOrganization)
+ r.With(middleware.Auth(jwtSecret)).Get("/organizations/{oid}", h.GetOrganization)
+ r.With(middleware.Auth(jwtSecret)).Put("/organizations/{oid}", h.UpdateOrganization)
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared authenticated org/event/link route trees and public list/link-resolution routes into reusable helpers to avoid duplication and fragmentation across router sections.
You can keep the new functionality while reducing the duplication and fragmentation by extracting the shared route trees into small helpers and reusing them.
### 1. Centralize authenticated org/event/link routes
Instead of repeating `r.With(middleware.Auth(jwtSecret))` for each path at the top level, factor them into helpers and call those from both `mountProtectedRoutes` and the top-level router:
```go
func mountProtectedOrgRoutes(r chi.Router, h *handler.Handler) {
r.Route("/organizations", func(r chi.Router) {
r.Post("/", h.CreateOrganization)
r.Get("/{oid}", h.GetOrganization)
r.Put("/{oid}", h.UpdateOrganization)
r.Delete("/{oid}", h.DeleteOrganization)
r.Get("/{oid}/members", h.ListOrgMembers)
r.Post("/{oid}/members", h.AddOrgMember)
r.Delete("/{oid}/members/{uid}", h.RemoveOrgMember)
r.Get("/{oid}/events", h.ListOrgEvents)
r.Get("/{oid}/links", h.ListOrgLinks)
})
}
func mountProtectedEventRoutes(r chi.Router, h *handler.Handler) {
r.Route("/events", func(r chi.Router) {
r.Post("/", h.CreateEvent)
r.Get("/org/{oid}", h.ListEventsByOrg)
r.Get("/{eid}", h.GetEvent)
r.Put("/{eid}", h.UpdateEvent)
r.Delete("/{eid}", h.DeleteEvent)
r.Get("/{eid}/registrations", h.ListEventRegistrations)
r.Post("/{eid}/register", h.RegisterForEvent)
r.Delete("/{eid}/register", h.UnregisterFromEvent)
})
}
func mountProtectedLinkRoutes(r chi.Router, h *handler.Handler) {
r.Route("/links", func(r chi.Router) {
r.Post("/", h.CreateLink)
r.Put("/{lid}", h.UpdateLink)
r.Get("/{lid}/visits", h.GetTotalVisits)
r.Get("/{lid}/qrcode", h.GetQRCode)
})
}
```
Use them in `mountProtectedRoutes`:
```go
func mountProtectedRoutes(r chi.Router, h *handler.Handler, jwtSecret string) {
r.Group(func(r chi.Router) {
r.Use(middleware.Auth(jwtSecret))
r.Route("/users", func(r chi.Router) {
// existing user routes...
})
mountProtectedOrgRoutes(r, h)
mountProtectedEventRoutes(r, h)
mountProtectedLinkRoutes(r, h)
r.Route("/bot/tokens", func(r chi.Router) {
// existing token routes...
})
})
}
```
And reuse the same helpers for the top-level non-versioned API:
```go
// keep behavior: top-level authenticated org/event routes
r.Group(func(r chi.Router) {
r.Use(middleware.Auth(jwtSecret))
mountProtectedOrgRoutes(r, h)
mountProtectedEventRoutes(r, h)
// if these top-level routes should include links as well:
mountProtectedLinkRoutes(r, h)
})
```
This removes duplicated per-path `With(middleware.Auth(...))` calls and keeps a single definition of each protected tree.
### 2. Reuse the same link routes in bot M2M section
Instead of redefining `/links` under the bot routes, reuse `mountProtectedLinkRoutes` inside the M2M group (bots already have appropriate auth):
```go
// inside bot group:
r.Group(func(r chi.Router) {
r.Use(middleware.M2MAuth(queries))
r.Get("/bot/me", h.GetBotMe)
// Organizations & events already here...
// Links (same handlers as protected human routes)
mountProtectedLinkRoutes(r, h)
})
```
That keeps link behavior consistent between human and bot routes.
### 3. Centralize public list routes
You now have public `ListOrganizations`/`ListEvents` registered in multiple places. Extract them into a helper and mount where needed:
```go
func mountPublicCollectionRoutes(r chi.Router, h *handler.Handler) {
r.Get("/organizations", h.ListOrganizations)
r.Get("/events", h.ListEvents)
}
```
Usage:
```go
// inside /api/v1
mountPublicCollectionRoutes(r, h)
// inside /v1
mountPublicCollectionRoutes(r, h)
```
If the non-versioned `/organizations` & `/events` are also meant to be public, reuse the same helper at the root instead of re-declaring.
### 4. Define link resolution path once
You currently register `/r/{endpoint_url}` twice and have a TODO about a global variable. Wrap it in a helper/constant:
```go
const LinkResolvePath = "/r/{endpoint_url}"
func mountLinkResolution(r chi.Router, h *handler.Handler) {
r.Get(LinkResolvePath, h.ResolveLink)
}
```
Then:
```go
// root
mountLinkResolution(r, h)
// under /api
r.Route("/api", func(r chi.Router) {
// ...
mountLinkResolution(r, h)
// ...
})
```
This keeps the dual entry points if desired, but avoids path drift and clarifies that they are intentionally the same route.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
-780
to
-789
| func (_m *Querier) UpdateEvent(ctx context.Context, arg database.UpdateEventParams) (database.Event, error) { | ||
| func (_m *Querier) UpdateEvent(ctx context.Context, arg database.UpdateEventParams) (database.EventsWithOrgID, error) { | ||
| ret := _m.Called(ctx, arg) | ||
|
|
||
| if len(ret) == 0 { | ||
| panic("no return value specified for UpdateEvent") | ||
| } | ||
|
|
||
| var r0 database.Event | ||
| var r0 database.EventsWithOrgID | ||
| var r1 error | ||
| if rf, ok := ret.Get(0).(func(context.Context, database.UpdateEventParams) (database.Event, error)); ok { |
There was a problem hiding this comment.
issue (bug_risk): Mock UpdateEvent return type no longer matches the Querier interface signature.
The Querier interface in internal/database/querier.go still returns Event, but this mock now returns EventsWithOrgID, so code using database.Querier with this mock will not compile. Please either change the interface (and queries.sql.go implementation) to return EventsWithOrgID, or revert the mock to return Event to keep them consistent.
Comment on lines
+294
to
+301
| qrcOpts := []standard.ImageOption{ | ||
| standard.WithBgColorRGBHex(QR_BG_COLOR), | ||
| standard.WithFgColorRGBHex(QR_FG_COLOR), | ||
| // standard.WithCircleShape(), | ||
| } | ||
|
|
||
| w.Header().Set("Content-Type", "image/png") | ||
| wr := standard.NewWithWriter(nopWriteCloser{w}, qrcOpts...) |
There was a problem hiding this comment.
issue (bug_risk): Error handling in GetQRCode checks the wrong error variable for the writer creation.
qrc, err := qrcode.NewWith(...) initializes err, but standard.NewWithWriter(...) assigns only to wr. The subsequent if err != nil therefore reuses the earlier (already-checked) err instead of the writer creation error. Bind the error from NewWithWriter, e.g. wr, err := standard.NewWithWriter(...), and check that so writer construction failures are handled correctly.
| } | ||
| } | ||
|
|
||
| func TestAddOrgMemberAllowsSelfJoin(t *testing.T) { |
There was a problem hiding this comment.
suggestion (testing): Add tests for non‑self AddOrgMember and self‑promotion to admin to fully exercise the new auth logic
The new TestAddOrgMemberAllowsSelfJoin and TestRemoveOrgMemberAuthorization cover the relaxed self-join/remove rules well.
To fully exercise AddOrgMember’s auth logic, please also add:
- A test where an authenticated human tries to add a different user without being an org admin, expecting
403. - A test where a user self-joins with
IsAdmin: true, verifying self-promotion is rejected with403. - Optionally, a test for
AuthType == "bot"to confirm bots can still add members regardless ofUID.
These will cover the branch gated by (req.UID != authenticatedUID || req.IsAdmin) and fully validate the new authorization behavior.
Suggested implementation:
func TestAddOrgMemberAllowsSelfJoin(t *testing.T) {
uid := uuid.New()
oid := uuid.New()
mockQueries := mocks.NewQuerier(t)
mockQueries.On("AddOrgMember", mock.Anything, mock.MatchedBy(func(arg database.AddOrgMemberParams) bool {
return arg.Oid == oid && arg.Uid == uid && arg.IsAdmin.Valid && !arg.IsAdmin.Bool
})).Return(nil)
h := handler.New(mockQueries, &config.Config{})
r := chi.NewRouter()
// Assuming there is a common route-registration helper used elsewhere in this file.
// Replace with the actual route registration call used in other org tests if different.
h.RegisterOrgRoutes(r)
body, _ := json.Marshal(dto.AddMemberRequest{
UID: uid,
IsAdmin: false,
})
req := httptest.NewRequest(http.MethodPost, "/orgs/"+oid.String()+"/members", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
// Assuming there is an auth helper / context key used elsewhere in tests; mirror that usage.
authenticatedUID := uid
ctx := context.WithValue(req.Context(), handler.AuthenticatedUIDContextKey{}, authenticatedUID)
req = req.WithContext(ctx)
rr := httptest.NewRecorder()
r.ServeHTTP(rr, req)
assert.Equal(t, http.StatusOK, rr.Code)
mockQueries.AssertExpectations(t)
}
func TestAddOrgMemberRejectsNonAdminAddingDifferentUser(t *testing.T) {
authenticatedUID := uuid.New()
targetUID := uuid.New()
oid := uuid.New()
require.NotEqual(t, authenticatedUID, targetUID, "sanity check: authenticated user and target user must differ")
mockQueries := mocks.NewQuerier(t)
// Non-admin, non-self add should be rejected by auth layer before hitting DB.
// So we do NOT expect AddOrgMember to be called at all.
mockQueries.AssertNotCalled(t, "AddOrgMember", mock.Anything, mock.Anything)
h := handler.New(mockQueries, &config.Config{})
r := chi.NewRouter()
h.RegisterOrgRoutes(r)
body, _ := json.Marshal(dto.AddMemberRequest{
UID: targetUID,
IsAdmin: false,
})
req := httptest.NewRequest(http.MethodPost, "/orgs/"+oid.String()+"/members", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
ctx := context.WithValue(req.Context(), handler.AuthenticatedUIDContextKey{}, authenticatedUID)
req = req.WithContext(ctx)
rr := httptest.NewRecorder()
r.ServeHTTP(rr, req)
assert.Equal(t, http.StatusForbidden, rr.Code)
// No DB call should have been made.
mockQueries.AssertExpectations(t)
}
func TestAddOrgMemberRejectsSelfPromotionToAdmin(t *testing.T) {
uid := uuid.New()
oid := uuid.New()
mockQueries := mocks.NewQuerier(t)
// Self-promotion should be rejected by auth before DB layer, so no AddOrgMember calls.
mockQueries.AssertNotCalled(t, "AddOrgMember", mock.Anything, mock.Anything)
h := handler.New(mockQueries, &config.Config{})
r := chi.NewRouter()
h.RegisterOrgRoutes(r)
body, _ := json.Marshal(dto.AddMemberRequest{
UID: uid,
IsAdmin: true, // attempt self-promotion
})
req := httptest.NewRequest(http.MethodPost, "/orgs/"+oid.String()+"/members", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
ctx := context.WithValue(req.Context(), handler.AuthenticatedUIDContextKey{}, uid)
req = req.WithContext(ctx)
rr := httptest.NewRecorder()
r.ServeHTTP(rr, req)
assert.Equal(t, http.StatusForbidden, rr.Code)
mockQueries.AssertExpectations(t)
}
func TestAddOrgMemberBotCanAddAnyUser(t *testing.T) {
botUID := uuid.New()
targetUID := uuid.New()
oid := uuid.New()
mockQueries := mocks.NewQuerier(t)
mockQueries.On("AddOrgMember", mock.Anything, mock.MatchedBy(func(arg database.AddOrgMemberParams) bool {
return arg.Oid == oid &&
arg.Uid == targetUID &&
arg.IsAdmin.Valid &&
!arg.IsAdmin.Bool
})).Return(nil)
h := handler.New(mockQueries, &config.Config{})
r := chi.NewRouter()
h.RegisterOrgRoutes(r)
body, _ := json.Marshal(dto.AddMemberRequest{
UID: targetUID,
IsAdmin: false,
})
req := httptest.NewRequest(http.MethodPost, "/orgs/"+oid.String()+"/members", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
// Assuming bots are identified via an auth struct or context; adapt to actual implementation.
botAuthInfo := handler.AuthInfo{
UID: botUID,
AuthType: "bot",
}
ctx := context.WithValue(req.Context(), handler.AuthInfoContextKey{}, botAuthInfo)
req = req.WithContext(ctx)
rr := httptest.NewRecorder()
r.ServeHTTP(rr, req)
assert.Equal(t, http.StatusOK, rr.Code)
mockQueries.AssertExpectations(t)The above changes assume several details that need to be aligned with your actual codebase:
-
Route registration
- Replace
h.RegisterOrgRoutes(r)with the actual route-registration helper used elsewhere inorganizations_test.go(e.g.registerOrgRoutes(r, h)orhandler.RegisterRoutes(r, h)).
- Replace
-
Auth wiring
- I introduced
handler.AuthenticatedUIDContextKey{}andhandler.AuthInfoContextKey{}plus ahandler.AuthInfostruct as placeholders. - Update these to the real context keys / auth struct you already use in this test file (e.g.
auth.ContextKeyUID,auth.ContextKeyAuthInfo,dto.AuthenticatedUser, etc.). - For the bot test, set
AuthType == "bot"in whatever way your auth layer expects (could be a string field on the auth struct, a separate context key, or a claim).
- I introduced
-
DTO and route shape
- Confirm the request type is
dto.AddMemberRequestand that it has fieldsUIDandIsAdmin. If the org identifier is supplied in the body (e.g.OrgID) rather than path, add/set that field accordingly and adjust the URL path if necessary. - If the endpoint path differs from
/orgs/{oid}/members, update thehttptest.NewRequestURLs to match.
- Confirm the request type is
-
Mock expectations
- In the “rejected” tests, I used
mockQueries.AssertNotCalledto express that authorization happens before DB calls. If your handler still callsAddOrgMemberand then returns403, adjust expectations to either:- Set up
.On("AddOrgMember", ...)and still assertStatusForbidden, or - Use
AssertNumberOfCallswith the actual expected count.
- Set up
- In the “rejected” tests, I used
Once you line up the route registration and auth/context wiring with the existing tests in this file, these tests will exercise:
- Non-self, non-admin add →
403. - Self-promotion to admin →
403. - Bot adding arbitrary user → allowed,
200(or your success code).
| "github.com/stretchr/testify/mock" | ||
| ) | ||
|
|
||
| func TestCreateLink(t *testing.T) { |
There was a problem hiding this comment.
suggestion (testing): Broaden link handler unit tests to cover validation failures, unauthorized calls, and the remaining handlers
The current tests exercise the happy path and a simple forbidden case, but many key behaviours of the new links handlers aren’t covered yet. Please add unit tests for:
CreateLink
OrgID == uuid.Nil→400.- Missing/malformed JSON body →
400. - Missing user claims (
GetUserClaimsreturns !ok) →401. IsOrgAdminerror mapped viahandleDBError.
UpdateLink
- Non-admin updates →
403. GetLinkByLIDreturnssql.ErrNoRows→404.UpdateLinkfailure returns an error status (not200).- Payloads with only one of
EndpointURL/DestURLset (to confirmCOALESCEbehaviour).
ListOrgLinks / GetTotalVisits / GetQRCode
ListOrgLinks: empty vs non-empty list, andGetLinksByOrgDB error.GetTotalVisits: invalidlid→400, missing link →404(per DB behaviour), and success200.GetQRCode: invalidlid→400, not found →404, and QR generation failure (e.g. via badPUBLIC_LINK_ENDPOINT) →500.
Table-driven tests across these handlers will better validate error and edge-case behaviour.
Suggested implementation:
"github.com/capyrpi/api/internal/dto"
"github.com/capyrpi/api/internal/handler"
"github.com/capyrpi/api/internal/middleware"
"github.com/go-chi/chi/v5"
"github.com/google/uuid"
"github.com/jackc/pgx/v5/pgtype"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
)
func TestCreateLink_ValidationAndAuthFailures(t *testing.T) {
uid := uuid.New()
oid := uuid.New()
type testCase struct {
name string
body string
setupMock func(q *mocks.Querier)
setupRequest func(r *http.Request)
expectedStatus int
}
tests := []testCase{
{
name: "OrgID_nil_returns_400",
body: `{
"org_id": "` + uuid.Nil.String() + `",
"endpoint_url": "promo",
"dest_url": "https://example.com"
}`,
setupMock: func(q *mocks.Querier) {
// No DB calls expected because validation fails before DB access.
},
setupRequest: func(r *http.Request) {
// Add valid user claims so the failure is specifically due to OrgID validation.
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusBadRequest,
},
{
name: "Malformed_JSON_body_returns_400",
body: `{"org_id": "not-a-valid-json"`, // deliberately malformed JSON
setupMock: func(q *mocks.Querier) {
// No DB calls expected because JSON decoding fails.
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusBadRequest,
},
{
name: "Missing_user_claims_returns_401",
body: `{
"org_id": "` + oid.String() + `",
"endpoint_url": "promo",
"dest_url": "https://example.com"
}`,
setupMock: func(q *mocks.Querier) {
// No DB calls because the handler should short-circuit on missing claims.
},
setupRequest: func(r *http.Request) {
// Intentionally do not put user claims into the context.
},
expectedStatus: http.StatusUnauthorized,
},
{
name: "IsOrgAdmin_DB_error_mapped_via_handleDBError",
body: `{
"org_id": "` + oid.String() + `",
"endpoint_url": "promo",
"dest_url": "https://example.com"
}`,
setupMock: func(q *mocks.Querier) {
q.On("IsOrgAdmin", mock.Anything, uid, oid).
Return(false, sql.ErrConnDone).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
r = r.WithContext(ctx)
},
// We expect the status that handleDBError maps sql.ErrConnDone to.
// If your handleDBError maps this to 500, adjust accordingly.
expectedStatus: http.StatusInternalServerError,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
q := &mocks.Querier{}
if tc.setupMock != nil {
tc.setupMock(q)
}
h := handler.CreateLink(q)
req := httptest.NewRequest(http.MethodPost, "/links", bytes.NewBufferString(tc.body))
req.Header.Set("Content-Type", "application/json")
if tc.setupRequest != nil {
tc.setupRequest(req)
}
rr := httptest.NewRecorder()
h.ServeHTTP(rr, req)
assert.Equal(t, tc.expectedStatus, rr.Code)
q.AssertExpectations(t)
})
}
}
func TestUpdateLink_AuthorizationAndDBFailures(t *testing.T) {
uid := uuid.New()
oid := uuid.New()
lid := uuid.New()
type testCase struct {
name string
body string
setupMock func(q *mocks.Querier)
setupRequest func(r *http.Request)
expectedStatus int
}
tests := []testCase{
{
name: "Non_admin_update_returns_403",
body: `{
"endpoint_url": "updated-endpoint",
"dest_url": "https://updated.example.com"
}`,
setupMock: func(q *mocks.Querier) {
q.On("IsOrgAdmin", mock.Anything, uid, oid).
Return(false, nil).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("lid", lid.String())
ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusForbidden,
},
{
name: "GetLinkByLID_sql_NoRows_returns_404",
body: `{}`,
setupMock: func(q *mocks.Querier) {
q.On("IsOrgAdmin", mock.Anything, uid, oid).
Return(true, nil).
Once()
q.On("GetLinkByLID", mock.Anything, lid).
Return(dto.Link{}, sql.ErrNoRows).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("lid", lid.String())
ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusNotFound,
},
{
name: "UpdateLink_failure_returns_non_200",
body: `{
"endpoint_url": "new-endpoint",
"dest_url": "https://new.example.com"
}`,
setupMock: func(q *mocks.Querier) {
q.On("IsOrgAdmin", mock.Anything, uid, oid).
Return(true, nil).
Once()
q.On("GetLinkByLID", mock.Anything, lid).
Return(dto.Link{
ID: lid,
OrgID: oid,
Endpoint: "old-endpoint",
DestURL: "https://old.example.com",
CreatedAt: time.Now(),
}, nil).
Once()
q.On("UpdateLink", mock.Anything, mock.Anything).
Return(dto.Link{}, sql.ErrConnDone).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("lid", lid.String())
ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusInternalServerError,
},
{
name: "COALESCE_behaviour_endpoint_only",
body: `{
"endpoint_url": "only-endpoint"
}`,
setupMock: func(q *mocks.Querier) {
q.On("IsOrgAdmin", mock.Anything, uid, oid).
Return(true, nil).
Once()
q.On("GetLinkByLID", mock.Anything, lid).
Return(dto.Link{
ID: lid,
OrgID: oid,
Endpoint: "old-endpoint",
DestURL: "https://old.example.com",
}, nil).
Once()
q.On("UpdateLink", mock.Anything, mock.MatchedBy(func(arg dto.UpdateLinkParams) bool {
// Endpoint is updated, DestURL stays unchanged (COALESCE semantics).
return arg.Endpoint == "only-endpoint" && arg.DestURL == "https://old.example.com"
})).
Return(dto.Link{}, nil).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("lid", lid.String())
ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusOK,
},
{
name: "COALESCE_behaviour_dest_only",
body: `{
"dest_url": "https://only.example.com"
}`,
setupMock: func(q *mocks.Querier) {
q.On("IsOrgAdmin", mock.Anything, uid, oid).
Return(true, nil).
Once()
q.On("GetLinkByLID", mock.Anything, lid).
Return(dto.Link{
ID: lid,
OrgID: oid,
Endpoint: "old-endpoint",
DestURL: "https://old.example.com",
}, nil).
Once()
q.On("UpdateLink", mock.Anything, mock.MatchedBy(func(arg dto.UpdateLinkParams) bool {
// DestURL updated, endpoint unchanged.
return arg.Endpoint == "old-endpoint" && arg.DestURL == "https://only.example.com"
})).
Return(dto.Link{}, nil).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("lid", lid.String())
ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusOK,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
q := &mocks.Querier{}
if tc.setupMock != nil {
tc.setupMock(q)
}
h := handler.UpdateLink(q)
req := httptest.NewRequest(http.MethodPatch, "/links/"+lid.String(), bytes.NewBufferString(tc.body))
req.Header.Set("Content-Type", "application/json")
if tc.setupRequest != nil {
tc.setupRequest(req)
}
rr := httptest.NewRecorder()
h.ServeHTTP(rr, req)
assert.Equal(t, tc.expectedStatus, rr.Code)
q.AssertExpectations(t)
})
}
}
func TestListOrgLinks_EmptyAndNonEmptyAndDBError(t *testing.T) {
uid := uuid.New()
oid := uuid.New()
type testCase struct {
name string
setupMock func(q *mocks.Querier)
setupRequest func(r *http.Request)
expectedStatus int
expectedCount int
}
tests := []testCase{
{
name: "Empty_list_returns_200_with_empty_payload",
setupMock: func(q *mocks.Querier) {
q.On("GetLinksByOrg", mock.Anything, oid).
Return([]dto.Link{}, nil).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("oid", oid.String())
ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusOK,
expectedCount: 0,
},
{
name: "Non_empty_list_returns_200",
setupMock: func(q *mocks.Querier) {
q.On("GetLinksByOrg", mock.Anything, oid).
Return([]dto.Link{
{ID: uuid.New(), OrgID: oid},
{ID: uuid.New(), OrgID: oid},
}, nil).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("oid", oid.String())
ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusOK,
expectedCount: 2,
},
{
name: "DB_error_returns_error_status",
setupMock: func(q *mocks.Querier) {
q.On("GetLinksByOrg", mock.Anything, oid).
Return(nil, sql.ErrConnDone).
Once()
},
setupRequest: func(r *http.Request) {
ctx := r.Context()
ctx = context.WithValue(ctx, middleware.UserIDContextKey{}, uid)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("oid", oid.String())
ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
r = r.WithContext(ctx)
},
expectedStatus: http.StatusInternalServerError,
expectedCount: 0,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
q := &mocks.Querier{}
if tc.setupMock != nil {
tc.setupMock(q)
}
h := handler.ListOrgLinks(q)
req := httptest.NewRequest(http.MethodGet, "/orgs/"+oid.String()+"/links", nil)
if tc.setupRequest != nil {
tc.setupRequest(req)
}
rr := httptest.NewRecorder()
h.ServeHTTP(rr, req)
assert.Equal(t, tc.expectedStatus, rr.Code)
if tc.expectedStatus == http.StatusOK {
var resp []dto.Link
err := json.Unmarshal(rr.Body.Bytes(), &resp)
assert.NoError(t, err)
assert.Len(t, resp, tc.expectedCount)
}
q.AssertExpectations(t)
})
}
}
func TestGetTotalVisits_VariousOutcomes(t *testing.T) {
lid := uuid.New()
type testCase struct {
name string
lidParam string
setupMock func(q *mocks.Querier)
expectedStatus int
}
tests := []testCase{
{
name: "Invalid_lid_returns_400",
lidParam: "not-a-uuid",
setupMock: func(q *mocks.Querier) {},
expectedStatus: http.StatusBadRequest,
},
{
name: "Missing_link_returns_404",
lidParam: lid.String(),
setupMock: func(q *mocks.Querier) {
q.On("GetTotalVisitsByLID", mock.Anything, lid).
Return(int64(0), sql.ErrNoRows).
Once()
},
expectedStatus: http.StatusNotFound,
},
{
name: "Success_returns_200",
lidParam: lid.String(),
setupMock: func(q *mocks.Querier) {
q.On("GetTotalVisitsByLID", mock.Anything, lid).
Return(int64(42), nil).
Once()
},
expectedStatus: http.StatusOK,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
q := &mocks.Querier{}
if tc.setupMock != nil {
tc.setupMock(q)
}
h := handler.GetTotalVisits(q)
req := httptest.NewRequest(http.MethodGet, "/links/"+tc.lidParam+"/visits", nil)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("lid", tc.lidParam)
ctx := context.WithValue(req.Context(), chi.RouteCtxKey, chiCtx)
req = req.WithContext(ctx)
rr := httptest.NewRecorder()
h.ServeHTTP(rr, req)
assert.Equal(t, tc.expectedStatus, rr.Code)
q.AssertExpectations(t)
})
}
}
func TestGetQRCode_VariousOutcomes(t *testing.T) {
lid := uuid.New()
type testCase struct {
name string
lidParam string
publicLinkEnv string
setupMock func(q *mocks.Querier)
expectedStatus int
expectImageBody bool
}
tests := []testCase{
{
name: "Invalid_lid_returns_400",
lidParam: "not-a-uuid",
publicLinkEnv: "https://links.example.com",
setupMock: func(q *mocks.Querier) {},
expectedStatus: http.StatusBadRequest,
},
{
name: "Not_found_returns_404",
lidParam: lid.String(),
publicLinkEnv: "https://links.example.com",
setupMock: func(q *mocks.Querier) {
q.On("GetLinkByLID", mock.Anything, lid).
Return(dto.Link{}, sql.ErrNoRows).
Once()
},
expectedStatus: http.StatusNotFound,
},
{
name: "QR_generation_failure_returns_500",
lidParam: lid.String(),
publicLinkEnv: "://bad-url", // Deliberately malformed base URL
setupMock: func(q *mocks.Querier) {
q.On("GetLinkByLID", mock.Anything, lid).
Return(dto.Link{ID: lid, Endpoint: "promo"}, nil).
Once()
},
expectedStatus: http.StatusInternalServerError,
},
{
name: "Success_returns_200_with_image",
lidParam: lid.String(),
publicLinkEnv: "https://links.example.com",
setupMock: func(q *mocks.Querier) {
q.On("GetLinkByLID", mock.Anything, lid).
Return(dto.Link{ID: lid, Endpoint: "promo"}, nil).
Once()
},
expectedStatus: http.StatusOK,
expectImageBody: true,
},
}
origPublicEndpoint := os.Getenv("PUBLIC_LINK_ENDPOINT")
defer os.Setenv("PUBLIC_LINK_ENDPOINT", origPublicEndpoint)
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
os.Setenv("PUBLIC_LINK_ENDPOINT", tc.publicLinkEnv)
q := &mocks.Querier{}
if tc.setupMock != nil {
tc.setupMock(q)
}
h := handler.GetQRCode(q)
req := httptest.NewRequest(http.MethodGet, "/links/"+tc.lidParam+"/qr", nil)
chiCtx := chi.NewRouteContext()
chiCtx.URLParams.Add("lid", tc.lidParam)
ctx := context.WithValue(req.Context(), chi.RouteCtxKey, chiCtx)
req = req.WithContext(ctx)
rr := httptest.NewRecorder()
h.ServeHTTP(rr, req)
assert.Equal(t, tc.expectedStatus, rr.Code)
if tc.expectImageBody {
assert.NotEmpty(t, rr.Body.Bytes())
assert.Equal(t, "image/png", rr.Header().Get("Content-Type"))
}
q.AssertExpectations(t)
})
}
}
func TestCreateLink(t *testing.T) {
uid := uuid.New()
oid := uuid.New()
lid := uuid.New()
tests := []struct {
name string
requestBody interface{}
setupMock func(*mocks.Querier)
setupContext func() context.Context
expectedStatus int
}{
{- These tests assume:
handler.CreateLink,handler.UpdateLink,handler.ListOrgLinks,handler.GetTotalVisits, andhandler.GetQRCodeeach returnhttp.HandlerFuncand accept a*mocks.Querier-compatible DB interface as their only parameter.- Context keys such as
middleware.UserIDContextKey(or your actual user-claims key) are used to retrieve the authenticated user. Adjust the keys and claim types based on your code. - DTOs such as
dto.Linkanddto.UpdateLinkParamsexist with the fields referenced; if your parameter types differ, update the matchers accordingly (especially theUpdateLinkCOALESCE tests). handleDBErrormapssql.ErrConnDoneto500. If it maps to a different status, adjust the expected status codes inIsOrgAdmin_DB_error_mapped_via_handleDBError,DB_error_returns_error_status, and similar tests.
- Ensure the following imports exist at the top of the file if they are not already present:
bytes,context,database/sql,encoding/json,net/http,net/http/httptest,os,testing,time. - If your existing tests use a shared router or helper (e.g.
newTestServerinstead of calling handler constructors directly), adapt theh := handler.XYZ(q)/h.ServeHTTPlines to match that pattern. - The context wiring using
chi.NewRouteContextassumes route params are read viachi.URLParam; if your handlers pull IDs differently, adjust the way thelidandoidvalues are passed into the request.
| assert.Equal(t, http.StatusOK, res.Code) | ||
| } | ||
|
|
||
| func TestPublicCollectionRoutesDoNotRequireAuth(t *testing.T) { |
There was a problem hiding this comment.
suggestion (testing): Also assert public collection routes on the /v1 prefix and add a negative case for /api/v1 events/orgs
Since the router also exposes public collection routes under /v1 (without /api), please extend this test to cover GET /v1/organizations and GET /v1/events as unauthenticated. Also add a negative case such as unauthenticated GET /api/v1/organizations/{oid} to verify per-org endpoints remain protected, matching the behavior defined in router.go.
Suggested implementation:
func TestPublicCollectionRoutesDoNotRequireAuth(t *testing.T) {
mockQueries := mocks.NewQuerier(t)
routerUnderTest := newTestRouter(mockQueries)
mockQueries.On("ListOrganizations", mock.Anything, mock.MatchedBy(func(arg database.ListOrganizationsParams) bool {
return arg.Limit == 20 && arg.Offset == 0
})).Return([]database.Organization{}, nil).Twice()
mockQueries.On("ListEvents", mock.Anything, mock.MatchedBy(func(arg database.ListEventsParams) bool {
return arg.Limit == 20 && arg.Offset == 0
})).Return([]database.EventsWithOrgID{}, nil).Twice()
// /api/v1 collection endpoints should be public
orgReq := httptest.NewRequest(http.MethodGet, "/api/v1/organizations", nil)
orgRes := httptest.NewRecorder()
routerUnderTest.ServeHTTP(orgRes, orgReq)
assert.Equal(t, http.StatusOK, orgRes.Code)
eventsReq := httptest.NewRequest(http.MethodGet, "/api/v1/events", nil)
eventsRes := httptest.NewRecorder()
routerUnderTest.ServeHTTP(eventsRes, eventsReq)
assert.Equal(t, http.StatusOK, eventsRes.Code)
// /v1 collection endpoints should also be public
v1OrgReq := httptest.NewRequest(http.MethodGet, "/v1/organizations", nil)
v1OrgRes := httptest.NewRecorder()
routerUnderTest.ServeHTTP(v1OrgRes, v1OrgReq)
assert.Equal(t, http.StatusOK, v1OrgRes.Code)
v1EventsReq := httptest.NewRequest(http.MethodGet, "/v1/events", nil)
v1EventsRes := httptest.NewRecorder()
routerUnderTest.ServeHTTP(v1EventsRes, v1EventsReq)
assert.Equal(t, http.StatusOK, v1EventsRes.Code)
// Per-org endpoints should remain protected
protectedOrgReq := httptest.NewRequest(http.MethodGet, "/api/v1/organizations/some-org-id", nil)
protectedOrgRes := httptest.NewRecorder()
routerUnderTest.ServeHTTP(protectedOrgRes, protectedOrgReq)
assert.Equal(t, http.StatusUnauthorized, protectedOrgRes.Code)- If your auth middleware returns a different status code for unauthenticated access (e.g. 403 Forbidden instead of 401 Unauthorized), adjust the final
assert.Equalaccordingly. - If there were already assertions after the original
orgReq := ...line in this test, remove them to avoid duplicated or conflicting checks, keeping only the expanded body above.
| } | ||
| } | ||
|
|
||
| func (h *Handler) isDev(r *http.Request) bool { |
There was a problem hiding this comment.
issue (complexity): Consider resolving dev mode, host, and protocol once per request into a small context struct and having all helper methods reuse it.
You can reduce the overlap and branching by resolving host/proto/dev once per request and reusing that everywhere.
1. Introduce a small request context helper
type requestCtx struct {
host string // without port
proto string // "http" or "https"
isDev bool
}
func (h *Handler) resolveRequestCtx(r *http.Request) requestCtx {
// 1. Decide isDev once
fh := r.Header.Get("X-Forwarded-Host")
host := r.Host
if fh != "" {
host = fh
}
isDev := false
if h.Config.Env == "development" || h.Config.Env == "staging" || h.Config.Env == "" {
isDev = true
} else if r.Header.Get("X-Dev-Host") != "" {
isDev = true
} else if strings.HasPrefix(fh, "localhost") || strings.HasPrefix(fh, "127.0.0.1") {
isDev = true
} else if strings.HasPrefix(host, "dev.") ||
strings.HasPrefix(host, "localhost") ||
strings.HasPrefix(host, "127.0.0.1") {
isDev = true
}
// 2. Decide proto once (respect dev override)
proto := r.Header.Get("X-Dev-Proto")
if proto == "" {
proto = "http"
if r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https" {
proto = "https"
}
}
// 3. Normalize host (respect dev override)
if devHost := r.Header.Get("X-Dev-Host"); devHost != "" {
host = devHost
}
if strings.Contains(host, ":") {
host, _, _ = strings.Cut(host, ":")
}
return requestCtx{
host: host,
proto: proto,
isDev: isDev,
}
}This keeps all your current rules (env, headers, forwarded host, TLS, dev overrides) in one place.
2. Rewrite the helpers to use this context
func (h *Handler) getBaseURL(r *http.Request) string {
ctx := h.resolveRequestCtx(r)
return ctx.proto + "://" + ctx.host
}
func (h *Handler) getCookieDomain(r *http.Request) string {
ctx := h.resolveRequestCtx(r)
if ctx.isDev {
return ctx.host
}
return h.Config.Cookie.Domain
}
func (h *Handler) getOAuthRedirectURL(r *http.Request, providerRedirectURL string) string {
ctx := h.resolveRequestCtx(r)
if !ctx.isDev {
return ""
}
if !strings.Contains(providerRedirectURL, "://") {
return ""
}
parts := strings.SplitN(providerRedirectURL, "/", 4)
if len(parts) < 4 {
return ""
}
return ctx.proto + "://" + ctx.host + "/" + parts[3]
}If you still want to keep isDev as a separate helper, make it a thin wrapper over resolveRequestCtx:
func (h *Handler) isDev(r *http.Request) bool {
return h.resolveRequestCtx(r).isDev
}This:
- Removes repeated header/host/proto logic from
isDev,getActualHost,getBaseURL,getCookieDomain, andgetOAuthRedirectURL. - Centralizes precedence rules (env vs headers vs proto/TLS) for easier reasoning and testing.
- Keeps all existing behavior (dev headers, dynamic domains, etc.) while reducing branching and coupling.
| }) | ||
| }) | ||
|
|
||
| r.With(middleware.Auth(jwtSecret)).Post("/organizations", h.CreateOrganization) |
There was a problem hiding this comment.
issue (complexity): Consider extracting shared authenticated org/event/link route trees and public list/link-resolution routes into reusable helpers to avoid duplication and fragmentation across router sections.
You can keep the new functionality while reducing the duplication and fragmentation by extracting the shared route trees into small helpers and reusing them.
1. Centralize authenticated org/event/link routes
Instead of repeating r.With(middleware.Auth(jwtSecret)) for each path at the top level, factor them into helpers and call those from both mountProtectedRoutes and the top-level router:
func mountProtectedOrgRoutes(r chi.Router, h *handler.Handler) {
r.Route("/organizations", func(r chi.Router) {
r.Post("/", h.CreateOrganization)
r.Get("/{oid}", h.GetOrganization)
r.Put("/{oid}", h.UpdateOrganization)
r.Delete("/{oid}", h.DeleteOrganization)
r.Get("/{oid}/members", h.ListOrgMembers)
r.Post("/{oid}/members", h.AddOrgMember)
r.Delete("/{oid}/members/{uid}", h.RemoveOrgMember)
r.Get("/{oid}/events", h.ListOrgEvents)
r.Get("/{oid}/links", h.ListOrgLinks)
})
}
func mountProtectedEventRoutes(r chi.Router, h *handler.Handler) {
r.Route("/events", func(r chi.Router) {
r.Post("/", h.CreateEvent)
r.Get("/org/{oid}", h.ListEventsByOrg)
r.Get("/{eid}", h.GetEvent)
r.Put("/{eid}", h.UpdateEvent)
r.Delete("/{eid}", h.DeleteEvent)
r.Get("/{eid}/registrations", h.ListEventRegistrations)
r.Post("/{eid}/register", h.RegisterForEvent)
r.Delete("/{eid}/register", h.UnregisterFromEvent)
})
}
func mountProtectedLinkRoutes(r chi.Router, h *handler.Handler) {
r.Route("/links", func(r chi.Router) {
r.Post("/", h.CreateLink)
r.Put("/{lid}", h.UpdateLink)
r.Get("/{lid}/visits", h.GetTotalVisits)
r.Get("/{lid}/qrcode", h.GetQRCode)
})
}Use them in mountProtectedRoutes:
func mountProtectedRoutes(r chi.Router, h *handler.Handler, jwtSecret string) {
r.Group(func(r chi.Router) {
r.Use(middleware.Auth(jwtSecret))
r.Route("/users", func(r chi.Router) {
// existing user routes...
})
mountProtectedOrgRoutes(r, h)
mountProtectedEventRoutes(r, h)
mountProtectedLinkRoutes(r, h)
r.Route("/bot/tokens", func(r chi.Router) {
// existing token routes...
})
})
}And reuse the same helpers for the top-level non-versioned API:
// keep behavior: top-level authenticated org/event routes
r.Group(func(r chi.Router) {
r.Use(middleware.Auth(jwtSecret))
mountProtectedOrgRoutes(r, h)
mountProtectedEventRoutes(r, h)
// if these top-level routes should include links as well:
mountProtectedLinkRoutes(r, h)
})This removes duplicated per-path With(middleware.Auth(...)) calls and keeps a single definition of each protected tree.
2. Reuse the same link routes in bot M2M section
Instead of redefining /links under the bot routes, reuse mountProtectedLinkRoutes inside the M2M group (bots already have appropriate auth):
// inside bot group:
r.Group(func(r chi.Router) {
r.Use(middleware.M2MAuth(queries))
r.Get("/bot/me", h.GetBotMe)
// Organizations & events already here...
// Links (same handlers as protected human routes)
mountProtectedLinkRoutes(r, h)
})That keeps link behavior consistent between human and bot routes.
3. Centralize public list routes
You now have public ListOrganizations/ListEvents registered in multiple places. Extract them into a helper and mount where needed:
func mountPublicCollectionRoutes(r chi.Router, h *handler.Handler) {
r.Get("/organizations", h.ListOrganizations)
r.Get("/events", h.ListEvents)
}Usage:
// inside /api/v1
mountPublicCollectionRoutes(r, h)
// inside /v1
mountPublicCollectionRoutes(r, h)If the non-versioned /organizations & /events are also meant to be public, reuse the same helper at the root instead of re-declaring.
4. Define link resolution path once
You currently register /r/{endpoint_url} twice and have a TODO about a global variable. Wrap it in a helper/constant:
const LinkResolvePath = "/r/{endpoint_url}"
func mountLinkResolution(r chi.Router, h *handler.Handler) {
r.Get(LinkResolvePath, h.ResolveLink)
}Then:
// root
mountLinkResolution(r, h)
// under /api
r.Route("/api", func(r chi.Router) {
// ...
mountLinkResolution(r, h)
// ...
})This keeps the dual entry points if desired, but avoids path drift and clarifies that they are intentionally the same route.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce dynamic organization-owned link management and improve event, auth, and environment handling across the API.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Tests: