From bc7bf124f5f5bf3940300614d86d256eaad3615b Mon Sep 17 00:00:00 2001 From: Michael Freno Date: Sun, 10 May 2026 06:42:00 -0400 Subject: [PATCH] Fix P0-P3 code review issues for clubs and challenges (FRE-4664) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P0: Fix variable shadowing in ClubService.createClub/updateClub and ChallengeService.createChallenge/updateChallenge — renamed local 'var request' to 'var urlRequest' so JSONEncoder encodes the typed parameter, not the URLRequest. P1: Wire CreateClubSheet and CreateChallengeSheet to parent ViewModel — sheets now receive viewModel and call createClub/createChallenge before dismissing. P2: Extract HTTPMethod enum to shared Utils/HTTPMethod.swift (was defined in NotificationService). Remove dead 'body' parameter from buildRequest in all three services. Add error alert UI to ClubsView and ChallengesView. P3: Replace forced URL unwrap with static let defaultBaseURL in all three services. Fix MockChallengeService.updateChallenge to track updateCalled instead of always throwing notFound. Co-Authored-By: Paperclip --- Lendair/Package.swift | 1 + Lendair/Services/ChallengeService.swift | 22 +++++++++----------- Lendair/Services/ClubService.swift | 22 +++++++++----------- Lendair/Services/NotificationService.swift | 19 +++++------------ Lendair/Utils/HTTPMethod.swift | 8 ++++++++ Lendair/Views/ChallengesView.swift | 14 +++++++++++-- Lendair/Views/ClubsView.swift | 14 +++++++++++-- LendairTests/ChallengeServiceTests.swift | 24 +++++++++++++++++++++- 8 files changed, 81 insertions(+), 43 deletions(-) create mode 100644 Lendair/Utils/HTTPMethod.swift diff --git a/Lendair/Package.swift b/Lendair/Package.swift index 87a07c5ec..49b209054 100644 --- a/Lendair/Package.swift +++ b/Lendair/Package.swift @@ -24,6 +24,7 @@ let package = Package( sources: [ "Models", "Services", + "Utils", "ViewModels", "Views" ], diff --git a/Lendair/Services/ChallengeService.swift b/Lendair/Services/ChallengeService.swift index 95f9db508..39a6f4cd5 100644 --- a/Lendair/Services/ChallengeService.swift +++ b/Lendair/Services/ChallengeService.swift @@ -20,8 +20,10 @@ class ChallengeService: ChallengeServiceProtocol { private let session: URLSession private let authToken: String? + static let defaultBaseURL: URL = URL(string: "http://localhost:3000")! + init( - baseURL: URL = URL(string: "http://localhost:3000")!, + baseURL: URL = defaultBaseURL, session: URLSession = .shared, authToken: String? = nil ) { @@ -62,10 +64,10 @@ class ChallengeService: ChallengeServiceProtocol { func createChallenge(request: CreateChallengeRequest) async throws -> Challenge { let url = baseURL.appendingPathComponent("/api/challenges") - var request = try buildRequest(url: url, method: .post) - request.httpBody = try JSONEncoder().encode(request) + var urlRequest = try buildRequest(url: url, method: .post) + urlRequest.httpBody = try JSONEncoder().encode(request) - let (data, response) = try await session.data(for: request) + let (data, response) = try await session.data(for: urlRequest) try validateResponse(response) let decoded = try JSONDecoder().decode(CreateChallengeResponse.self, from: data) @@ -74,10 +76,10 @@ class ChallengeService: ChallengeServiceProtocol { func updateChallenge(id: String, request: UpdateChallengeRequest) async throws -> Challenge { let url = baseURL.appendingPathComponent("/api/challenges/\(id)") - var request = try buildRequest(url: url, method: .patch) - request.httpBody = try JSONEncoder().encode(request) + var urlRequest = try buildRequest(url: url, method: .patch) + urlRequest.httpBody = try JSONEncoder().encode(request) - let (data, response) = try await session.data(for: request) + let (data, response) = try await session.data(for: urlRequest) try validateResponse(response) let decoded = try JSONDecoder().decode(UpdateChallengeResponse.self, from: data) @@ -124,7 +126,7 @@ class ChallengeService: ChallengeServiceProtocol { // MARK: - Helpers - private func buildRequest(url: URL, method: HTTPMethod = .get, body: Data? = nil) throws -> URLRequest { + private func buildRequest(url: URL, method: HTTPMethod = .get) throws -> URLRequest { var request = URLRequest(url: url) request.httpMethod = method.rawValue request.setValue("application/json", forHTTPHeaderField: "Content-Type") @@ -133,10 +135,6 @@ class ChallengeService: ChallengeServiceProtocol { request.setValue("Bearer \(token)", forHTTPHeaderField: "Authorization") } - if let body = body { - request.httpBody = body - } - return request } diff --git a/Lendair/Services/ClubService.swift b/Lendair/Services/ClubService.swift index d08c74193..b9ab12fd3 100644 --- a/Lendair/Services/ClubService.swift +++ b/Lendair/Services/ClubService.swift @@ -20,8 +20,10 @@ class ClubService: ClubServiceProtocol { private let session: URLSession private let authToken: String? + static let defaultBaseURL: URL = URL(string: "http://localhost:3000")! + init( - baseURL: URL = URL(string: "http://localhost:3000")!, + baseURL: URL = defaultBaseURL, session: URLSession = .shared, authToken: String? = nil ) { @@ -63,10 +65,10 @@ class ClubService: ClubServiceProtocol { func createClub(request: CreateClubRequest) async throws -> Club { let url = baseURL.appendingPathComponent("/api/clubs") - var request = try buildRequest(url: url, method: .post) - request.httpBody = try JSONEncoder().encode(request) + var urlRequest = try buildRequest(url: url, method: .post) + urlRequest.httpBody = try JSONEncoder().encode(request) - let (data, response) = try await session.data(for: request) + let (data, response) = try await session.data(for: urlRequest) try validateResponse(response) let decoded = try JSONDecoder().decode(CreateClubResponse.self, from: data) @@ -75,10 +77,10 @@ class ClubService: ClubServiceProtocol { func updateClub(id: String, request: UpdateClubRequest) async throws -> Club { let url = baseURL.appendingPathComponent("/api/clubs/\(id)") - var request = try buildRequest(url: url, method: .patch) - request.httpBody = try JSONEncoder().encode(request) + var urlRequest = try buildRequest(url: url, method: .patch) + urlRequest.httpBody = try JSONEncoder().encode(request) - let (data, response) = try await session.data(for: request) + let (data, response) = try await session.data(for: urlRequest) try validateResponse(response) let decoded = try JSONDecoder().decode(UpdateClubResponse.self, from: data) @@ -120,7 +122,7 @@ class ClubService: ClubServiceProtocol { // MARK: - Helpers - private func buildRequest(url: URL, method: HTTPMethod = .get, body: Data? = nil) throws -> URLRequest { + private func buildRequest(url: URL, method: HTTPMethod = .get) throws -> URLRequest { var request = URLRequest(url: url) request.httpMethod = method.rawValue request.setValue("application/json", forHTTPHeaderField: "Content-Type") @@ -129,10 +131,6 @@ class ClubService: ClubServiceProtocol { request.setValue("Bearer \(token)", forHTTPHeaderField: "Authorization") } - if let body = body { - request.httpBody = body - } - return request } diff --git a/Lendair/Services/NotificationService.swift b/Lendair/Services/NotificationService.swift index 1b4565eff..dd15d96ac 100644 --- a/Lendair/Services/NotificationService.swift +++ b/Lendair/Services/NotificationService.swift @@ -16,8 +16,10 @@ class NotificationsService: NotificationsServiceProtocol { private let session: URLSession private let authToken: String? + static let defaultBaseURL: URL = URL(string: "http://localhost:3000")! + init( - baseURL: URL = URL(string: "http://localhost:3000")!, + baseURL: URL = defaultBaseURL, session: URLSession = .shared, authToken: String? = nil ) { @@ -76,7 +78,7 @@ class NotificationsService: NotificationsServiceProtocol { // MARK: - Helpers - private func buildRequest(url: URL, method: HTTPMethod = .get, body: Data? = nil) throws -> URLRequest { + private func buildRequest(url: URL, method: HTTPMethod = .get) throws -> URLRequest { var request = URLRequest(url: url) request.httpMethod = method.rawValue request.setValue("application/json", forHTTPHeaderField: "Content-Type") @@ -85,10 +87,6 @@ class NotificationsService: NotificationsServiceProtocol { request.setValue("Bearer \(token)", forHTTPHeaderField: "Authorization") } - if let body = body { - request.httpBody = body - } - return request } @@ -136,11 +134,4 @@ enum NotificationError: LocalizedError { } } -// MARK: - HTTP Method - -enum HTTPMethod: String { - case get = "GET" - case post = "POST" - case patch = "PATCH" - case delete = "DELETE" -} +// MARK: - HTTP Method (moved to Utils/HTTPMethod.swift) diff --git a/Lendair/Utils/HTTPMethod.swift b/Lendair/Utils/HTTPMethod.swift new file mode 100644 index 000000000..0b5479ce1 --- /dev/null +++ b/Lendair/Utils/HTTPMethod.swift @@ -0,0 +1,8 @@ +import Foundation + +enum HTTPMethod: String { + case get = "GET" + case post = "POST" + case patch = "PATCH" + case delete = "DELETE" +} diff --git a/Lendair/Views/ChallengesView.swift b/Lendair/Views/ChallengesView.swift index 0e0ff7f23..cdda4b12f 100644 --- a/Lendair/Views/ChallengesView.swift +++ b/Lendair/Views/ChallengesView.swift @@ -4,6 +4,7 @@ struct ChallengesView: View { @StateObject private var viewModel = ChallengeViewModel() @State private var showingCreateSheet = false @State private var selectedTab: ChallengeTab = .active + @State private var lastError: ChallengeError? enum ChallengeTab: String, CaseIterable { case active, upcoming, completed @@ -32,7 +33,12 @@ struct ChallengesView: View { } } .sheet(isPresented: $showingCreateSheet) { - CreateChallengeSheet() + CreateChallengeSheet(viewModel: viewModel) + } + .alert("Error", isPresented: .init(get: { viewModel.error != nil }, set: { if !$0 { lastError = viewModel.error } })) { + Button("OK") { lastError = viewModel.error } + } message: { + Text(viewModel.error?.errorDescription ?? "") } } .onAppear { @@ -116,6 +122,7 @@ struct ChallengesView: View { struct CreateChallengeSheet: View { @Environment(\.dismiss) var dismiss + let viewModel: ChallengeViewModel @State private var title = "" @State private var description = "" @State private var challengeType: ChallengeType = .distance @@ -177,7 +184,10 @@ struct CreateChallengeSheet: View { rules: rules.isEmpty ? nil : rules, clubId: nil ) - dismiss() + Task { + _ = await viewModel.createChallenge(request: request) + dismiss() + } } .disabled(title.isEmpty || targetValue.isEmpty) } diff --git a/Lendair/Views/ClubsView.swift b/Lendair/Views/ClubsView.swift index 192861ec2..5d05b3286 100644 --- a/Lendair/Views/ClubsView.swift +++ b/Lendair/Views/ClubsView.swift @@ -4,6 +4,7 @@ struct ClubsView: View { @StateObject private var viewModel = ClubViewModel() @State private var showingCreateSheet = false @State private var selectedTab: ClubTab = .discover + @State private var lastError: ClubError? enum ClubTab: String, CaseIterable { case discover, myClubs @@ -32,7 +33,12 @@ struct ClubsView: View { } } .sheet(isPresented: $showingCreateSheet) { - CreateClubSheet() + CreateClubSheet(viewModel: viewModel) + } + .alert("Error", isPresented: .init(get: { viewModel.error != nil }, set: { if !$0 { lastError = nil } })) { + Button("OK") { lastError = viewModel.error } + } message: { + Text(viewModel.error?.errorDescription ?? "") } } .onAppear { @@ -112,6 +118,7 @@ struct ClubsView: View { struct CreateClubSheet: View { @Environment(\.dismiss) var dismiss + let viewModel: ClubViewModel @State private var name = "" @State private var description = "" @State private var clubType: ClubType = .running @@ -167,7 +174,10 @@ struct CreateClubSheet: View { maxMembers: Int(maxMembers), rules: rules.isEmpty ? nil : rules ) - dismiss() + Task { + _ = await viewModel.createClub(request: request) + dismiss() + } } .disabled(name.isEmpty || location.isEmpty) } diff --git a/LendairTests/ChallengeServiceTests.swift b/LendairTests/ChallengeServiceTests.swift index 8870797ba..c26a68577 100644 --- a/LendairTests/ChallengeServiceTests.swift +++ b/LendairTests/ChallengeServiceTests.swift @@ -10,6 +10,7 @@ final class MockChallengeService: ChallengeServiceProtocol { var joinCalledIds: [String] = [] var leaveCalledIds: [String] = [] var createCalled = false + var updateCalled = false var leaderboard: [LeaderboardEntry] = [] var listCallCount = 0 var listError: Error? @@ -51,7 +52,28 @@ final class MockChallengeService: ChallengeServiceProtocol { } func updateChallenge(id: String, request: UpdateChallengeRequest) async throws -> Challenge { - throw ChallengeError.notFound + updateCalled = true + return Challenge( + id: id, + title: request.title ?? "Updated", + description: request.description ?? "", + challengeType: request.challengeType ?? .distance, + status: .active, + startDate: Date(), + endDate: Date().addingTimeInterval(30 * 24 * 3600), + targetMetric: request.targetMetric ?? .distance, + targetValue: request.targetValue ?? 100, + targetUnit: (request.targetMetric ?? .distance).unit, + participantCount: 0, + rules: request.rules, + imageUrl: nil, + createdBy: "current-user", + createdByName: "Current User", + clubId: nil, + participationStatus: .participating, + userProgress: 0, + createdAt: Date() + ) } func joinChallenge(id: String) async throws {