Fix P0-P3 code review issues for clubs and challenges (FRE-4664)
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 <noreply@paperclip.ing>
This commit is contained in:
@@ -24,6 +24,7 @@ let package = Package(
|
|||||||
sources: [
|
sources: [
|
||||||
"Models",
|
"Models",
|
||||||
"Services",
|
"Services",
|
||||||
|
"Utils",
|
||||||
"ViewModels",
|
"ViewModels",
|
||||||
"Views"
|
"Views"
|
||||||
],
|
],
|
||||||
|
|||||||
@@ -20,8 +20,10 @@ class ChallengeService: ChallengeServiceProtocol {
|
|||||||
private let session: URLSession
|
private let session: URLSession
|
||||||
private let authToken: String?
|
private let authToken: String?
|
||||||
|
|
||||||
|
static let defaultBaseURL: URL = URL(string: "http://localhost:3000")!
|
||||||
|
|
||||||
init(
|
init(
|
||||||
baseURL: URL = URL(string: "http://localhost:3000")!,
|
baseURL: URL = defaultBaseURL,
|
||||||
session: URLSession = .shared,
|
session: URLSession = .shared,
|
||||||
authToken: String? = nil
|
authToken: String? = nil
|
||||||
) {
|
) {
|
||||||
@@ -62,10 +64,10 @@ class ChallengeService: ChallengeServiceProtocol {
|
|||||||
|
|
||||||
func createChallenge(request: CreateChallengeRequest) async throws -> Challenge {
|
func createChallenge(request: CreateChallengeRequest) async throws -> Challenge {
|
||||||
let url = baseURL.appendingPathComponent("/api/challenges")
|
let url = baseURL.appendingPathComponent("/api/challenges")
|
||||||
var request = try buildRequest(url: url, method: .post)
|
var urlRequest = try buildRequest(url: url, method: .post)
|
||||||
request.httpBody = try JSONEncoder().encode(request)
|
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)
|
try validateResponse(response)
|
||||||
|
|
||||||
let decoded = try JSONDecoder().decode(CreateChallengeResponse.self, from: data)
|
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 {
|
func updateChallenge(id: String, request: UpdateChallengeRequest) async throws -> Challenge {
|
||||||
let url = baseURL.appendingPathComponent("/api/challenges/\(id)")
|
let url = baseURL.appendingPathComponent("/api/challenges/\(id)")
|
||||||
var request = try buildRequest(url: url, method: .patch)
|
var urlRequest = try buildRequest(url: url, method: .patch)
|
||||||
request.httpBody = try JSONEncoder().encode(request)
|
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)
|
try validateResponse(response)
|
||||||
|
|
||||||
let decoded = try JSONDecoder().decode(UpdateChallengeResponse.self, from: data)
|
let decoded = try JSONDecoder().decode(UpdateChallengeResponse.self, from: data)
|
||||||
@@ -124,7 +126,7 @@ class ChallengeService: ChallengeServiceProtocol {
|
|||||||
|
|
||||||
// MARK: - Helpers
|
// 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)
|
var request = URLRequest(url: url)
|
||||||
request.httpMethod = method.rawValue
|
request.httpMethod = method.rawValue
|
||||||
request.setValue("application/json", forHTTPHeaderField: "Content-Type")
|
request.setValue("application/json", forHTTPHeaderField: "Content-Type")
|
||||||
@@ -133,10 +135,6 @@ class ChallengeService: ChallengeServiceProtocol {
|
|||||||
request.setValue("Bearer \(token)", forHTTPHeaderField: "Authorization")
|
request.setValue("Bearer \(token)", forHTTPHeaderField: "Authorization")
|
||||||
}
|
}
|
||||||
|
|
||||||
if let body = body {
|
|
||||||
request.httpBody = body
|
|
||||||
}
|
|
||||||
|
|
||||||
return request
|
return request
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -20,8 +20,10 @@ class ClubService: ClubServiceProtocol {
|
|||||||
private let session: URLSession
|
private let session: URLSession
|
||||||
private let authToken: String?
|
private let authToken: String?
|
||||||
|
|
||||||
|
static let defaultBaseURL: URL = URL(string: "http://localhost:3000")!
|
||||||
|
|
||||||
init(
|
init(
|
||||||
baseURL: URL = URL(string: "http://localhost:3000")!,
|
baseURL: URL = defaultBaseURL,
|
||||||
session: URLSession = .shared,
|
session: URLSession = .shared,
|
||||||
authToken: String? = nil
|
authToken: String? = nil
|
||||||
) {
|
) {
|
||||||
@@ -63,10 +65,10 @@ class ClubService: ClubServiceProtocol {
|
|||||||
|
|
||||||
func createClub(request: CreateClubRequest) async throws -> Club {
|
func createClub(request: CreateClubRequest) async throws -> Club {
|
||||||
let url = baseURL.appendingPathComponent("/api/clubs")
|
let url = baseURL.appendingPathComponent("/api/clubs")
|
||||||
var request = try buildRequest(url: url, method: .post)
|
var urlRequest = try buildRequest(url: url, method: .post)
|
||||||
request.httpBody = try JSONEncoder().encode(request)
|
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)
|
try validateResponse(response)
|
||||||
|
|
||||||
let decoded = try JSONDecoder().decode(CreateClubResponse.self, from: data)
|
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 {
|
func updateClub(id: String, request: UpdateClubRequest) async throws -> Club {
|
||||||
let url = baseURL.appendingPathComponent("/api/clubs/\(id)")
|
let url = baseURL.appendingPathComponent("/api/clubs/\(id)")
|
||||||
var request = try buildRequest(url: url, method: .patch)
|
var urlRequest = try buildRequest(url: url, method: .patch)
|
||||||
request.httpBody = try JSONEncoder().encode(request)
|
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)
|
try validateResponse(response)
|
||||||
|
|
||||||
let decoded = try JSONDecoder().decode(UpdateClubResponse.self, from: data)
|
let decoded = try JSONDecoder().decode(UpdateClubResponse.self, from: data)
|
||||||
@@ -120,7 +122,7 @@ class ClubService: ClubServiceProtocol {
|
|||||||
|
|
||||||
// MARK: - Helpers
|
// 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)
|
var request = URLRequest(url: url)
|
||||||
request.httpMethod = method.rawValue
|
request.httpMethod = method.rawValue
|
||||||
request.setValue("application/json", forHTTPHeaderField: "Content-Type")
|
request.setValue("application/json", forHTTPHeaderField: "Content-Type")
|
||||||
@@ -129,10 +131,6 @@ class ClubService: ClubServiceProtocol {
|
|||||||
request.setValue("Bearer \(token)", forHTTPHeaderField: "Authorization")
|
request.setValue("Bearer \(token)", forHTTPHeaderField: "Authorization")
|
||||||
}
|
}
|
||||||
|
|
||||||
if let body = body {
|
|
||||||
request.httpBody = body
|
|
||||||
}
|
|
||||||
|
|
||||||
return request
|
return request
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -16,8 +16,10 @@ class NotificationsService: NotificationsServiceProtocol {
|
|||||||
private let session: URLSession
|
private let session: URLSession
|
||||||
private let authToken: String?
|
private let authToken: String?
|
||||||
|
|
||||||
|
static let defaultBaseURL: URL = URL(string: "http://localhost:3000")!
|
||||||
|
|
||||||
init(
|
init(
|
||||||
baseURL: URL = URL(string: "http://localhost:3000")!,
|
baseURL: URL = defaultBaseURL,
|
||||||
session: URLSession = .shared,
|
session: URLSession = .shared,
|
||||||
authToken: String? = nil
|
authToken: String? = nil
|
||||||
) {
|
) {
|
||||||
@@ -76,7 +78,7 @@ class NotificationsService: NotificationsServiceProtocol {
|
|||||||
|
|
||||||
// MARK: - Helpers
|
// 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)
|
var request = URLRequest(url: url)
|
||||||
request.httpMethod = method.rawValue
|
request.httpMethod = method.rawValue
|
||||||
request.setValue("application/json", forHTTPHeaderField: "Content-Type")
|
request.setValue("application/json", forHTTPHeaderField: "Content-Type")
|
||||||
@@ -85,10 +87,6 @@ class NotificationsService: NotificationsServiceProtocol {
|
|||||||
request.setValue("Bearer \(token)", forHTTPHeaderField: "Authorization")
|
request.setValue("Bearer \(token)", forHTTPHeaderField: "Authorization")
|
||||||
}
|
}
|
||||||
|
|
||||||
if let body = body {
|
|
||||||
request.httpBody = body
|
|
||||||
}
|
|
||||||
|
|
||||||
return request
|
return request
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -136,11 +134,4 @@ enum NotificationError: LocalizedError {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// MARK: - HTTP Method
|
// MARK: - HTTP Method (moved to Utils/HTTPMethod.swift)
|
||||||
|
|
||||||
enum HTTPMethod: String {
|
|
||||||
case get = "GET"
|
|
||||||
case post = "POST"
|
|
||||||
case patch = "PATCH"
|
|
||||||
case delete = "DELETE"
|
|
||||||
}
|
|
||||||
|
|||||||
8
Lendair/Utils/HTTPMethod.swift
Normal file
8
Lendair/Utils/HTTPMethod.swift
Normal file
@@ -0,0 +1,8 @@
|
|||||||
|
import Foundation
|
||||||
|
|
||||||
|
enum HTTPMethod: String {
|
||||||
|
case get = "GET"
|
||||||
|
case post = "POST"
|
||||||
|
case patch = "PATCH"
|
||||||
|
case delete = "DELETE"
|
||||||
|
}
|
||||||
@@ -4,6 +4,7 @@ struct ChallengesView: View {
|
|||||||
@StateObject private var viewModel = ChallengeViewModel()
|
@StateObject private var viewModel = ChallengeViewModel()
|
||||||
@State private var showingCreateSheet = false
|
@State private var showingCreateSheet = false
|
||||||
@State private var selectedTab: ChallengeTab = .active
|
@State private var selectedTab: ChallengeTab = .active
|
||||||
|
@State private var lastError: ChallengeError?
|
||||||
|
|
||||||
enum ChallengeTab: String, CaseIterable {
|
enum ChallengeTab: String, CaseIterable {
|
||||||
case active, upcoming, completed
|
case active, upcoming, completed
|
||||||
@@ -32,7 +33,12 @@ struct ChallengesView: View {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
.sheet(isPresented: $showingCreateSheet) {
|
.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 {
|
.onAppear {
|
||||||
@@ -116,6 +122,7 @@ struct ChallengesView: View {
|
|||||||
|
|
||||||
struct CreateChallengeSheet: View {
|
struct CreateChallengeSheet: View {
|
||||||
@Environment(\.dismiss) var dismiss
|
@Environment(\.dismiss) var dismiss
|
||||||
|
let viewModel: ChallengeViewModel
|
||||||
@State private var title = ""
|
@State private var title = ""
|
||||||
@State private var description = ""
|
@State private var description = ""
|
||||||
@State private var challengeType: ChallengeType = .distance
|
@State private var challengeType: ChallengeType = .distance
|
||||||
@@ -177,8 +184,11 @@ struct CreateChallengeSheet: View {
|
|||||||
rules: rules.isEmpty ? nil : rules,
|
rules: rules.isEmpty ? nil : rules,
|
||||||
clubId: nil
|
clubId: nil
|
||||||
)
|
)
|
||||||
|
Task {
|
||||||
|
_ = await viewModel.createChallenge(request: request)
|
||||||
dismiss()
|
dismiss()
|
||||||
}
|
}
|
||||||
|
}
|
||||||
.disabled(title.isEmpty || targetValue.isEmpty)
|
.disabled(title.isEmpty || targetValue.isEmpty)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ struct ClubsView: View {
|
|||||||
@StateObject private var viewModel = ClubViewModel()
|
@StateObject private var viewModel = ClubViewModel()
|
||||||
@State private var showingCreateSheet = false
|
@State private var showingCreateSheet = false
|
||||||
@State private var selectedTab: ClubTab = .discover
|
@State private var selectedTab: ClubTab = .discover
|
||||||
|
@State private var lastError: ClubError?
|
||||||
|
|
||||||
enum ClubTab: String, CaseIterable {
|
enum ClubTab: String, CaseIterable {
|
||||||
case discover, myClubs
|
case discover, myClubs
|
||||||
@@ -32,7 +33,12 @@ struct ClubsView: View {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
.sheet(isPresented: $showingCreateSheet) {
|
.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 {
|
.onAppear {
|
||||||
@@ -112,6 +118,7 @@ struct ClubsView: View {
|
|||||||
|
|
||||||
struct CreateClubSheet: View {
|
struct CreateClubSheet: View {
|
||||||
@Environment(\.dismiss) var dismiss
|
@Environment(\.dismiss) var dismiss
|
||||||
|
let viewModel: ClubViewModel
|
||||||
@State private var name = ""
|
@State private var name = ""
|
||||||
@State private var description = ""
|
@State private var description = ""
|
||||||
@State private var clubType: ClubType = .running
|
@State private var clubType: ClubType = .running
|
||||||
@@ -167,8 +174,11 @@ struct CreateClubSheet: View {
|
|||||||
maxMembers: Int(maxMembers),
|
maxMembers: Int(maxMembers),
|
||||||
rules: rules.isEmpty ? nil : rules
|
rules: rules.isEmpty ? nil : rules
|
||||||
)
|
)
|
||||||
|
Task {
|
||||||
|
_ = await viewModel.createClub(request: request)
|
||||||
dismiss()
|
dismiss()
|
||||||
}
|
}
|
||||||
|
}
|
||||||
.disabled(name.isEmpty || location.isEmpty)
|
.disabled(name.isEmpty || location.isEmpty)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -10,6 +10,7 @@ final class MockChallengeService: ChallengeServiceProtocol {
|
|||||||
var joinCalledIds: [String] = []
|
var joinCalledIds: [String] = []
|
||||||
var leaveCalledIds: [String] = []
|
var leaveCalledIds: [String] = []
|
||||||
var createCalled = false
|
var createCalled = false
|
||||||
|
var updateCalled = false
|
||||||
var leaderboard: [LeaderboardEntry] = []
|
var leaderboard: [LeaderboardEntry] = []
|
||||||
var listCallCount = 0
|
var listCallCount = 0
|
||||||
var listError: Error?
|
var listError: Error?
|
||||||
@@ -51,7 +52,28 @@ final class MockChallengeService: ChallengeServiceProtocol {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func updateChallenge(id: String, request: UpdateChallengeRequest) async throws -> Challenge {
|
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 {
|
func joinChallenge(id: String) async throws {
|
||||||
|
|||||||
Reference in New Issue
Block a user