Gamer activity #20
Gamer activity #20
Conversation
| database.Init() | ||
| defer database.Close() |
| } | ||
|
|
||
| // Close closes the database connection | ||
| func Close() error { |
There was a problem hiding this comment.
If this function no longer returns an error we should update the signature
| created_at DATE | ||
| created_at DATE, | ||
| membership_expiry_date DATE |
There was a problem hiding this comment.
We shouldn't be editing past migrations. Can you refactor this out to a new migration that uses ALTER TABLE?
| } | ||
|
|
||
| // Builds query for recent gamer activity with optional search | ||
| func BuildGamerActivityRecentQuery(limit, offset int, search string) (string, []interface{}) { |
There was a problem hiding this comment.
I don't know how I feel about this. Feels like we're one step away from sql injection
|
|
||
| type HTTPHandlerWithErr func(http.ResponseWriter, *http.Request) error | ||
|
|
||
| func Wrap(h HTTPHandlerWithErr) http.HandlerFunc { |
There was a problem hiding this comment.
Is this logging middleware? Consider renaming to be more clear
| } | ||
|
|
||
| // Ensures the request has Content-Type application/json | ||
| func RequireJSONContentType(r *http.Request) bool { |
There was a problem hiding this comment.
Nit: Should be IsJSONContentType or something adjacent to that
|
|
||
| /* | ||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| var response GenerateKeyResponse | ||
| if tc.rawBody != "" { | ||
| req, _ := http.NewRequest(tc.method, "/admin/generate-key", strings.NewReader(tc.rawBody)) | ||
| req.Header.Set("Content-Type", "application/json") | ||
| rr := httptest.NewRecorder() | ||
| handler := http.HandlerFunc(GenerateAPIKey) | ||
| handler.ServeHTTP(rr, req) | ||
| tests.AssertResponse(t, rr, tc.expectedStatus, nil) | ||
| } else { | ||
| tests.ExecuteTestRequest(t, http.HandlerFunc(GenerateAPIKey), tc.method, "/admin/generate-key", tc.body, tc.expectedStatus, &response) | ||
| } | ||
| if tc.expectKey { | ||
| if response.KeyID == "" { | ||
| t.Error("KeyID should not be empty") | ||
| } | ||
| if !strings.HasPrefix(response.APIKey, "api_") { | ||
| t.Error("API key should start with 'api_'") | ||
| } | ||
| if response.AppName != "test-app" { | ||
| t.Errorf("Expected app name 'test-app', got '%s'", response.AppName) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| */ |
| database.Init() | ||
| } | ||
|
|
||
| func SetupTestDBForTest(t *testing.T) { |
There was a problem hiding this comment.
Whats the reason for adding this function? It seems like you just moved out code to another function and calling the exact same thing.
| return query | ||
| } | ||
|
|
||
| // Builds query for recent gamer activity with optional search |
There was a problem hiding this comment.
I am usually against adding comments unless its for something that is hard to understand or you made a not so clear design choice. Your code should be eligible enough to read without comments.
|
This PR is way too big, it is very hard to review. In the future, if you see a PR getting too big, consider splitting up the work into separate PRs. The database and modelling and the test refactors should have been separate from the API implementation. Also, please try to stay in scope next time of the current issue. |
| mux.HandleFunc("/health", handlers.HealthCheck) | ||
| mux.HandleFunc("/db/ping", handlers.DatabasePing) | ||
| mux.HandleFunc("/admin/generate-key", handlers.GenerateAPIKey) | ||
| mux.HandleFunc("/activity/{student_number}", handlers.Wrap(ah.GetGamerActivityByStudent)) |
Gamer activity endpoint completed along with tests. Frontend might need to be adapted. Optional fields like EndedAt and ExecName are pointers and needs to be dereferenced.