Skip to content

Refactor/56 addapplicationsenabledmiddleware#67

Open
5sansiva wants to merge 14 commits intomainfrom
refactor/56-addapplicationsenabledmiddleware
Open

Refactor/56 addapplicationsenabledmiddleware#67
5sansiva wants to merge 14 commits intomainfrom
refactor/56-addapplicationsenabledmiddleware

Conversation

@5sansiva
Copy link
Copy Markdown
Collaborator

Summary
Refactor: Move ApplicationsEnabled to Settings and wire up middleware

Store Layer
Moved GetApplicationsEnabled / SetApplicationsEnabled from the Application interface → Settings interface in storage.go (these are global toggles, not application CRUD)
Moved implementations from ApplicationsStore (applications.go) → SettingsStore (settings.go)
Moved mock stubs from MockApplicationStore → MockSettingsStore in mock_store.go

API Layer
Wired ApplicationsEnabledMiddleware onto hacker mutation routes (PATCH /me, POST /me/submit, POST /me/resume-upload-url, DELETE /me/resume)
Added unguarded GET /applications/enabled route so the frontend can check if submissions are open
Super admins bypass the middleware check; all other users receive 403 when applications are disabled
Updated getApplicationsEnabled / setApplicationsEnabled handlers to call app.store.Settings.* instead of app.store.Application.*

Tests
Added TestApplicationsEnabledMiddleware covering:
Returns 403 when applications are disabled
Passes through with 200 when applications are enabled
Uses .Once() on mock expectations to prevent cross-test pollution

Misc
Fixed Swagger tags: admin → admin/applications, superadmin → superadmin/applications

@5sansiva 5sansiva requested review from anishalle and balebbae March 26, 2026 18:46
Copy link
Copy Markdown
Collaborator

@anishalle anishalle left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Copy Markdown
Collaborator

@balebbae balebbae left a comment

Choose a reason for hiding this comment

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

no more importing strconv 😭

query := `
SELECT value
FROM settings
WHERE key = 'applications_enabled'
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Use const and pass it in like the GetAdminScheduleEditEnabled look at line 29

defer cancel()

query := `
UPDATE settings
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Instead of update returning do insert ... on conflict do update. look at the other upsert patterns

return err
}

func (s *SettingsStore) GetApplicationsEnabled(ctx context.Context) (bool, error) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

dont return error when called should already show enabled. just return error look at the other set signature patterns

RETURNING value`

var value bool
err := s.db.QueryRowContext(ctx, query, strconv.FormatBool(enabled)).Scan(&value)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

don't use strconv use json.Marshal like the other setters. So remove the import as well.

//
// @Summary Set applications enabled status
// @Description Sets whether the application portal is currently open for submissions. Requires SuperAdmin privileges.
// @Tags superadmin
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

tags should be superadmin/settings

// @Security CookieAuth
// @Router /superadmin/settings/applications-enabled [put]
func (app *application) setApplicationsEnabled(w http.ResponseWriter, r *http.Request) {
enabled, err := strconv.ParseBool(r.URL.Query().Get("enabled"))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

use JSON body like the other sets. no strconv :) remove that import. Look at line 365-369 for reference

return
}

//NOTE: Following existing design pattern of Get response and Set response structs
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

remove this note

r.Route("/applications", func(r chi.Router) {
r.Get("/", app.listApplicationsHandler)
r.Get("/stats", app.getApplicationStatsHandler)
r.Get("/enabled", app.getApplicationsEnabled)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

remove this. this is a duplicate from line 170

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants