Really appreciate the detailed writeup. This is exactly the kind of review I was hoping to find before committing to the upgrade.
", + contentRaw: String? = "preview raw", hasEditContext: Bool = true ) -> CommentDetail { CommentDetail( @@ -114,6 +120,7 @@ extension CommentDetail { postID: 10, parentID: parentID, contentHTML: contentHTML, + contentRaw: contentRaw, link: "https://example.com/?p=10#comment-\(id)", date: Date(timeIntervalSince1970: 1_700_000_000), status: status, diff --git a/Modules/Sources/WordPressComments/Models/CommentListItem.swift b/Modules/Sources/WordPressComments/Models/CommentListItem.swift index 11329227dbe2..41f8c3df90fc 100644 --- a/Modules/Sources/WordPressComments/Models/CommentListItem.swift +++ b/Modules/Sources/WordPressComments/Models/CommentListItem.swift @@ -18,9 +18,11 @@ struct CommentListItem: Identifiable, Equatable, Sendable { let authorName: String let avatarURL: URL? let postID: Int64 - let snippet: String + var snippet: String let date: Date var status: Status + /// The comment's permalink; nil when the server sends none. + let link: URL? init( id: Int64, @@ -29,7 +31,8 @@ struct CommentListItem: Identifiable, Equatable, Sendable { postID: Int64, snippet: String, date: Date, - status: Status + status: Status, + link: URL? ) { self.id = id self.authorName = authorName @@ -38,6 +41,7 @@ struct CommentListItem: Identifiable, Equatable, Sendable { self.snippet = snippet self.date = date self.status = status + self.link = link } init(comment: CommentWithViewContext) { @@ -48,6 +52,7 @@ struct CommentListItem: Identifiable, Equatable, Sendable { snippet = Self.snippet(fromHTML: comment.content.rendered) date = comment.dateGmt status = Status(comment.status) + link = comment.link.nonEmptyString().flatMap { URL(string: $0) } } /// Row-shaped projection of a fetched detail (used for the parent preview @@ -60,7 +65,8 @@ struct CommentListItem: Identifiable, Equatable, Sendable { postID: detail.postID, snippet: Self.snippet(fromHTML: detail.contentHTML), date: detail.date, - status: detail.status + status: detail.status, + link: detail.link ) } diff --git a/Modules/Sources/WordPressComments/Services/CommentsModerationCoordinator.swift b/Modules/Sources/WordPressComments/Services/CommentsModerationCoordinator.swift index 1734936e36fa..9a9c6ca8261a 100644 --- a/Modules/Sources/WordPressComments/Services/CommentsModerationCoordinator.swift +++ b/Modules/Sources/WordPressComments/Services/CommentsModerationCoordinator.swift @@ -126,6 +126,32 @@ final class CommentsModerationCoordinator { } } + /// Replaces the comment's content. Pessimistic and reconcile-free: a + /// thrown edit may still have landed (timeout-after-commit), but a retry + /// re-sends this user's content and comment edits are last-writer-wins, + /// matching wp-admin (which locks posts but not comments). Accepted + /// limitation; see the design doc. The response also carries the + /// authoritative server status; a status correction is emitted alongside + /// the content change when it disagrees with `comment.status`. + func editContent(on comment: CommentDetail, newContent: String) async throws -> CommentDetail { + try await holdingSlot(for: comment.id, waitingForSlot: true) { [weak self] in + guard let self else { throw CancellationError() } + let updated = try await self.service.updateContent(id: comment.id, content: newContent) + self.events.send( + .contentChanged(id: comment.id, contentHTML: updated.contentHTML, contentRaw: updated.contentRaw) + ) + // Editing content never changes status server-side, so this only + // fires when a concurrent moderator or plugin changed it while the + // editor was open; the correction keeps Reply gating and list-tab + // membership from going stale. + if updated.status != comment.status { + self.events.send(.statusChanged(id: comment.id, to: updated.status)) + } + self.tracker?.track(.edited(commentID: comment.id, postID: comment.postID)) + return updated + } + } + /// Broadcasts a status change the detail screen observed on load (its seed /// status disagreed with the fetched truth) without running a mutation, so /// loaded list tabs reconcile the corrected status in place. diff --git a/Modules/Sources/WordPressComments/Services/CommentsService.swift b/Modules/Sources/WordPressComments/Services/CommentsService.swift index dbc9c0cec08a..1dd73e5a158e 100644 --- a/Modules/Sources/WordPressComments/Services/CommentsService.swift +++ b/Modules/Sources/WordPressComments/Services/CommentsService.swift @@ -60,6 +60,9 @@ protocol CommentsServiceProtocol: Sendable { /// Creates a reply to `parentID` on `postID` and returns the created /// comment's detail. func createReply(postID: Int64, parentID: Int64, content: String) async throws -> CommentDetail + + /// Updates the comment's content and returns the updated detail. + func updateContent(id: Int64, content: String) async throws -> CommentDetail } /// Errors raised by `CommentsService` that don't originate from wordpress-rs. @@ -182,6 +185,14 @@ final class CommentsService: CommentsServiceProtocol { ) return CommentDetail(comment: response.data) } + + func updateContent(id: Int64, content: String) async throws -> CommentDetail { + let response = try await client.api.comments.update( + commentId: id, + params: CommentUpdateParams(content: content) + ) + return CommentDetail(comment: response.data) + } } extension WpApiError { diff --git a/Modules/Sources/WordPressComments/Services/CommentsTracker.swift b/Modules/Sources/WordPressComments/Services/CommentsTracker.swift index e5aace0718ab..dde86e0a618d 100644 --- a/Modules/Sources/WordPressComments/Services/CommentsTracker.swift +++ b/Modules/Sources/WordPressComments/Services/CommentsTracker.swift @@ -7,6 +7,12 @@ public enum CommentsTrackedEvent: Equatable, Sendable { // Permanent delete: legacy has no analytics event; deliberately untracked. /// A reply was successfully created, matching legacy's reply-sent event. case repliedTo(commentID: Int64, postID: Int64) + /// The content editor was opened for a comment, matching legacy's + /// edit-entry event. + case editorOpened(commentID: Int64, postID: Int64) + /// A comment's content was successfully edited, matching legacy's + /// edit-saved event. + case edited(commentID: Int64, postID: Int64) } public protocol CommentsTracker: Sendable { diff --git a/Modules/Sources/WordPressComments/Strings/Strings.swift b/Modules/Sources/WordPressComments/Strings/Strings.swift index 18f17a1f327d..bef2d9831d78 100644 --- a/Modules/Sources/WordPressComments/Strings/Strings.swift +++ b/Modules/Sources/WordPressComments/Strings/Strings.swift @@ -249,6 +249,12 @@ enum Strings { comment: "Title of the compose screen for replying to a comment" ) + static let composerEditTitle = NSLocalizedString( + "commentComposer.title.edit", + value: "Edit Comment", + comment: "Title of the compose screen for editing a comment" + ) + static let composerPlaceholder = NSLocalizedString( "commentComposer.placeholder", value: "Leave a reply…", @@ -261,6 +267,12 @@ enum Strings { comment: "Button label to send a new reply" ) + static let composerSave = NSLocalizedString( + "commentComposer.action.save", + value: "Save", + comment: "Button label to save changes to an edited comment" + ) + static let composerCancel = NSLocalizedString( "commentComposer.action.cancel", value: "Cancel", @@ -291,6 +303,12 @@ enum Strings { comment: "Button label to continue editing instead of discarding changes" ) + static let composerDiscardChanges = NSLocalizedString( + "commentComposer.action.discardChanges", + value: "Discard Changes", + comment: "Button label to discard unsaved changes to a comment" + ) + static let composerErrorClosed = NSLocalizedString( "commentComposer.error.closed", value: "Comments are closed for this post.", @@ -303,6 +321,12 @@ enum Strings { comment: "Error message shown when sending a reply fails" ) + static let composerErrorEditFailed = NSLocalizedString( + "commentComposer.error.editFailed", + value: "Failed to save changes.", + comment: "Error message shown when editing a comment fails" + ) + static let noticeReplySent = NSLocalizedString( "commentComposer.notice.replySent", value: "Reply sent.", @@ -326,4 +350,10 @@ enum Strings { value: "Reply", comment: "Button label to reply to a comment on the detail screen" ) + + static let detailEdit = NSLocalizedString( + "commentDetail.action.edit", + value: "Edit", + comment: "Button label to edit a comment on the detail screen" + ) } diff --git a/Modules/Sources/WordPressComments/ViewModels/CommentComposerViewModel.swift b/Modules/Sources/WordPressComments/ViewModels/CommentComposerViewModel.swift index 9fefad2834bc..9aae4dfb3775 100644 --- a/Modules/Sources/WordPressComments/ViewModels/CommentComposerViewModel.swift +++ b/Modules/Sources/WordPressComments/ViewModels/CommentComposerViewModel.swift @@ -2,17 +2,45 @@ import Foundation import WordPressAPIInternal import WordPressShared -/// Drives the reply composer sheet. Every mutation delegates to -/// `CommentsModerationCoordinator`. +/// Drives the reply/edit composer sheet. Every mutation delegates to +/// `CommentsModerationCoordinator`. `Identifiable` (by object identity) so the +/// detail screen can present it with `.sheet(item:)`. @MainActor -final class CommentComposerViewModel: ObservableObject { +final class CommentComposerViewModel: ObservableObject, Identifiable { enum Mode: Equatable { case reply(parent: CommentDetail) + case edit(comment: CommentDetail) + + /// The comment whose draft this composer owns. Only replies keep + /// drafts (legacy parity); edit mode never touches the store. + var draftCommentID: Int64? { + switch self { + case .reply(let parent): parent.id + case .edit: nil + } + } + + /// The text the composer opens with: the original content for an + /// edit, empty for a reply (the caller layers a restored draft over it). + var originalText: String { + switch self { + case .reply: "" + case .edit(let comment): comment.contentRaw ?? "" + } + } + + var failureMessage: String { + switch self { + case .reply: Strings.composerErrorReplyFailed + case .edit: Strings.composerErrorEditFailed + } + } } /// What the detail screen shows after the sheet dismisses. enum Outcome: Equatable { case replied(notice: String) + case edited } @Published var text: String @@ -20,27 +48,26 @@ final class CommentComposerViewModel: ObservableObject { @Published private(set) var errorMessage: String? let mode: Mode - /// The parent's author and one-line snippet, derived once (the sheet - /// re-renders on every keystroke). - let parentPreview: CommentListItem + /// Reply mode only: the parent's author and one-line snippet, derived + /// once (the sheet re-renders on every keystroke). + let parentPreview: CommentListItem? var title: String { switch mode { case .reply: Strings.composerReplyTitle + case .edit: Strings.composerEditTitle } } var sendButtonTitle: String { switch mode { case .reply: Strings.composerSend + case .edit: Strings.composerSave } } var canSend: Bool { - guard !trimmedText.isEmpty else { return false } - switch mode { - case .reply: return true - } + isDirty && !trimmedText.isEmpty } /// Reply mode only: sending will also approve the pending parent. @@ -50,11 +77,9 @@ final class CommentComposerViewModel: ObservableObject { } /// Whether cancelling should ask before discarding: a reply with any - /// non-blank text. + /// non-blank text, or an edit that differs from the original. var isDirty: Bool { - switch mode { - case .reply: !trimmedText.isEmpty - } + trimmedText != mode.originalText } private var trimmedText: String { @@ -79,6 +104,13 @@ final class CommentComposerViewModel: ObservableObject { case .reply(let parent): parentPreview = CommentListItem(detail: parent) text = draftStore.loadDraft(commentID: parent.id) ?? "" + case .edit(let comment): + parentPreview = nil + text = mode.originalText + // Matches legacy, which fires its edit-entry event when the + // editor opens. Reply mode has no equivalent: the coordinator + // tracks `.repliedTo` once the send succeeds. + tracker?.track(.editorOpened(commentID: comment.id, postID: comment.postID)) } } @@ -88,38 +120,40 @@ final class CommentComposerViewModel: ObservableObject { isSending = true defer { isSending = false } - let content = trimmedText - switch mode { - case .reply(let parent): - do { - let outcome = try await coordinator.reply(to: parent, content: content) + do { + switch mode { + case .reply(let parent): + let outcome = try await coordinator.reply(to: parent, content: trimmedText) draftStore.deleteDraft(commentID: parent.id) return .replied(notice: notice(for: outcome)) - } catch { - errorMessage = errorText(for: error) - return nil + case .edit(let comment): + _ = try await coordinator.editContent(on: comment, newContent: trimmedText) + return .edited } + } catch { + errorMessage = errorText(for: error) + return nil } } - /// Keeps the current text for the next time the composer opens on this - /// parent. + /// Reply mode only: keeps the current text for the next time the composer + /// opens on this parent. func saveDraft() { - guard case .reply(let parent) = mode else { return } - draftStore.saveDraft(text, commentID: parent.id) + guard let id = mode.draftCommentID else { return } + draftStore.saveDraft(text, commentID: id) } func deleteDraft() { - guard case .reply(let parent) = mode else { return } - draftStore.deleteDraft(commentID: parent.id) + guard let id = mode.draftCommentID else { return } + draftStore.deleteDraft(commentID: id) } /// Runs when the sheet closes. The blank exits (Cancel, swipe-down) skip /// the draft prompt, so a restored draft the user cleared is dropped here /// instead of coming back on the next open. func deleteDraftIfBlank() { - guard case .reply(let parent) = mode, trimmedText.isEmpty else { return } - draftStore.deleteDraft(commentID: parent.id) + guard let id = mode.draftCommentID, trimmedText.isEmpty else { return } + draftStore.deleteDraft(commentID: id) } /// Words the post-send notice: a duplicate confirms an earlier send @@ -137,11 +171,11 @@ final class CommentComposerViewModel: ObservableObject { /// Only comment_closed gets its own wording; every other failure (a /// duplicate never reaches here, the reply chain absorbs it) shows the - /// generic reply-failed message. + /// mode's generic message. private func errorText(for error: Error) -> String { if (error as? WpApiError)?.wpErrorCode == .CommentClosed { return Strings.composerErrorClosed } - return Strings.composerErrorReplyFailed + return mode.failureMessage } } diff --git a/Modules/Sources/WordPressComments/ViewModels/CommentDetailViewModel.swift b/Modules/Sources/WordPressComments/ViewModels/CommentDetailViewModel.swift index 85353eba6ed8..20346ae61175 100644 --- a/Modules/Sources/WordPressComments/ViewModels/CommentDetailViewModel.swift +++ b/Modules/Sources/WordPressComments/ViewModels/CommentDetailViewModel.swift @@ -111,6 +111,27 @@ final class CommentDetailViewModel: ObservableObject { showsReply && isToolbarEnabled } + /// Whether the Edit menu item renders: the toolbar's seed-or-fetched gate + /// (moderator, modeled status), so it appears with the toolbar rather than + /// after the whole load; `canEdit` enables it. + var showsEdit: Bool { + toolbarModel != .hidden + } + + /// The permalink to share, or nil. Only a publicly visible (approved) + /// comment is shareable. Sharing needs no authoritative truth, so the + /// seed's link serves until the fetch lands. + var shareLink: URL? { + guard header?.status == .approved else { return nil } + return loadedDetail?.link ?? seed?.link + } + + /// Edit context (and thus `contentRaw`) is already required by + /// `showsToolbar`; the modeled-status restriction comes from `showsEdit`. + var canEdit: Bool { + showsEdit && isToolbarEnabled + } + var trashConfirmation: TrashConfirmation { switch numberOfReplies { case .none: .generic @@ -130,12 +151,22 @@ final class CommentDetailViewModel: ObservableObject { } /// Presents the composer in reply mode. A no-op unless `canReply` (which - /// already covers "no mutation in flight" and "no composer already - /// presented"). + /// already covers "no mutation in flight" via `isToolbarEnabled`). func replyTapped() { - guard canReply, composer == nil, let detail = loadedDetail else { return } + guard canReply, let detail = loadedDetail else { return } + presentComposer(.reply(parent: detail)) + } + + /// Presents the composer in edit mode. Mirrors `replyTapped()`. + func editTapped() { + guard canEdit, let detail = loadedDetail else { return } + presentComposer(.edit(comment: detail)) + } + + private func presentComposer(_ mode: CommentComposerViewModel.Mode) { + guard composer == nil else { return } composer = CommentComposerViewModel( - mode: .reply(parent: detail), + mode: mode, coordinator: coordinator, draftStore: draftStore, tracker: tracker @@ -143,8 +174,9 @@ final class CommentDetailViewModel: ObservableObject { } /// Dismisses the composer sheet. A successful reply also posts its - /// notice. - func composerFinished(_ outcome: CommentComposerViewModel.Outcome) { + /// notice; a cancel (nil) or a successful edit needs none (the edited + /// content updates in place via the coordinator's `contentChanged` event). + func composerClosed(_ outcome: CommentComposerViewModel.Outcome?) { composer = nil if case .replied(let replyNotice) = outcome { noticePresenter?.present(title: replyNotice) @@ -191,9 +223,7 @@ final class CommentDetailViewModel: ObservableObject { // (e.g. the parent comment) hides this one, and a status change that // lands meanwhile must still correct the header/toolbar. Lives for the // VM's lifetime. - eventSubscription = coordinator.events - .filter { $0.commentID == commentID } - .sink { [weak self] in self?.handle($0) } + eventSubscription = coordinator.events.sink { [weak self] in self?.handle($0) } } func onAppear() async { @@ -296,14 +326,25 @@ final class CommentDetailViewModel: ObservableObject { // MARK: - Coordinator events + /// Routes every coordinator event: this comment's own events drive the + /// screen; the parent's content edits (made on the parent's own detail + /// screen) refresh the "In reply to" strip in place instead of showing a + /// stale snippet until the next full fetch. Everything else is ignored. private func handle(_ event: CommentChangeEvent) { + if event.commentID == parentPreview?.id { + if case .contentChanged(_, let contentHTML, _) = event { + parentPreview?.snippet = CommentListItem.snippet(fromHTML: contentHTML) + } + return + } + guard event.commentID == commentID else { return } switch event { case .statusChanged(_, let to): // A status change proves the comment exists at a known status. Clear // any prior terminal state (e.g. a delete that later proved false) // so the toolbar can re-enable, then apply the status. The loaded - // detail is the screen's status source of truth (header, pill, and - // toolbar model all read it). + // detail is the screen's status source of truth (header, pill, + // toolbar model, and Reply/Edit gating all read it). isDeleted = false if var detail = loadedDetail { detail.status = to @@ -313,6 +354,12 @@ final class CommentDetailViewModel: ObservableObject { // The comment is gone: a terminal state that turns the toolbar off // and dismisses the screen. isDeleted = true + case .contentChanged(_, let contentHTML, let contentRaw): + if var detail = loadedDetail { + detail.contentHTML = contentHTML + detail.contentRaw = contentRaw + content = .loaded(detail) + } case .replyCreated: // The reply is a different comment; this screen's own status is // corrected by the approve step's statusChanged event when diff --git a/Modules/Sources/WordPressComments/ViewModels/CommentsListViewModel.swift b/Modules/Sources/WordPressComments/ViewModels/CommentsListViewModel.swift index ca681776694f..0d20a0be931f 100644 --- a/Modules/Sources/WordPressComments/ViewModels/CommentsListViewModel.swift +++ b/Modules/Sources/WordPressComments/ViewModels/CommentsListViewModel.swift @@ -146,6 +146,15 @@ final class CommentsListViewModel: ObservableObject { // right away would cost one fetch per reply while the list is // hidden instead of one on return. markStale(reloadNow: false) + case .contentChanged(let id, let contentHTML, _): + if seenIDs.contains(id), let index = items.firstIndex(where: { $0.id == id }) { + items[index].snippet = CommentListItem.snippet(fromHTML: contentHTML) + } + // Invalidate in-flight fetches even when the row is absent: a + // page-one or load-more fetch captured before this edit could + // still deliver the row with its pre-edit snippet, and would + // otherwise pass its generation guard and land the stale data. + invalidateInFlightFetches() } } diff --git a/Modules/Sources/WordPressComments/Views/CommentRowView.swift b/Modules/Sources/WordPressComments/Views/CommentRowView.swift index 3fce673585f6..b8350c5c2d03 100644 --- a/Modules/Sources/WordPressComments/Views/CommentRowView.swift +++ b/Modules/Sources/WordPressComments/Views/CommentRowView.swift @@ -92,7 +92,8 @@ extension CommentListItem { postID: 1, snippet: "Really appreciate the detailed writeup, this is exactly the kind of review I was hoping to find.", date: Date(timeIntervalSince1970: 1_700_000_000), - status: status + status: status, + link: nil ) } } diff --git a/Modules/Sources/WordPressComments/Views/Detail/CommentComposerView.swift b/Modules/Sources/WordPressComments/Views/Detail/CommentComposerView.swift index 1de9ea1d8061..a718ea451977 100644 --- a/Modules/Sources/WordPressComments/Views/Detail/CommentComposerView.swift +++ b/Modules/Sources/WordPressComments/Views/Detail/CommentComposerView.swift @@ -1,11 +1,14 @@ import SwiftUI -/// The reply composer sheet, presented from the detail screen. Cancelling a -/// dirty reply asks whether to keep or discard its draft. +/// The reply/edit composer sheet. Presented from the detail screen for both +/// modes; the mode dictates which rows show (the parent snippet and approve +/// note appear only in reply mode) and what cancelling asks about (a reply +/// offers to keep or discard its draft; an edit only offers to discard). struct CommentComposerView: View { @ObservedObject var viewModel: CommentComposerViewModel - let onFinished: (CommentComposerViewModel.Outcome) -> Void - let onDismiss: () -> Void + /// Called once when the sheet should close: with the outcome after a + /// successful send, nil when the user cancelled. + let onClose: (CommentComposerViewModel.Outcome?) -> Void @FocusState private var editorFocused: Bool @State private var isCancelConfirmationPresented = false @@ -13,7 +16,9 @@ struct CommentComposerView: View { var body: some View { NavigationStack { VStack(alignment: .leading, spacing: 0) { - parentSnippet(viewModel.parentPreview) + if let parent = viewModel.parentPreview { + parentSnippet(parent) + } if viewModel.showsApproveNote { approveNote } @@ -95,7 +100,7 @@ struct CommentComposerView: View { Button(viewModel.sendButtonTitle) { Task { if let outcome = await viewModel.send() { - onFinished(outcome) + onClose(outcome) } } } @@ -104,23 +109,29 @@ struct CommentComposerView: View { } } - /// A dirty reply offers to keep or discard its draft. + /// A dirty reply offers to keep or discard its draft; a dirty edit only + /// offers to discard. @ViewBuilder private var cancelConfirmationActions: some View { - Button(Strings.composerSaveDraft) { - viewModel.saveDraft() - onDismiss() - } - Button(Strings.composerDeleteDraft, role: .destructive) { - viewModel.deleteDraft() - onDismiss() + switch viewModel.mode { + case .reply: + Button(Strings.composerSaveDraft) { + viewModel.saveDraft() + onClose(nil) + } + Button(Strings.composerDeleteDraft, role: .destructive) { + viewModel.deleteDraft() + onClose(nil) + } + case .edit: + Button(Strings.composerDiscardChanges, role: .destructive) { onClose(nil) } } Button(Strings.composerKeepEditing, role: .cancel) {} } private func handleCancel() { guard viewModel.isDirty else { - onDismiss() + onClose(nil) return } isCancelConfirmationPresented = true @@ -135,6 +146,16 @@ struct CommentComposerView: View { coordinator: coordinator, draftStore: PreviewCommentDraftStore() ) - return CommentComposerView(viewModel: viewModel, onFinished: { _ in }, onDismiss: {}) + return CommentComposerView(viewModel: viewModel, onClose: { _ in }) +} + +#Preview("Edit") { + let coordinator = CommentsModerationCoordinator(service: PreviewCommentsService()) + let viewModel = CommentComposerViewModel( + mode: .edit(comment: .preview()), + coordinator: coordinator, + draftStore: PreviewCommentDraftStore() + ) + return CommentComposerView(viewModel: viewModel, onClose: { _ in }) } #endif diff --git a/Modules/Sources/WordPressComments/Views/Detail/CommentDetailView.swift b/Modules/Sources/WordPressComments/Views/Detail/CommentDetailView.swift index 3e2ed1d2ad9e..0978c85acff4 100644 --- a/Modules/Sources/WordPressComments/Views/Detail/CommentDetailView.swift +++ b/Modules/Sources/WordPressComments/Views/Detail/CommentDetailView.swift @@ -42,17 +42,9 @@ struct CommentDetailView: View { .onChange(of: viewModel.isDeleted) { _, isDeleted in if isDeleted { dismiss() } } - .sheet( - isPresented: Binding(get: { viewModel.composer != nil }, set: { if !$0 { viewModel.composer = nil } }) - ) { - if let composer = viewModel.composer { - CommentComposerView( - viewModel: composer, - onFinished: { viewModel.composerFinished($0) }, - onDismiss: { viewModel.composer = nil } - ) + .sheet(item: $viewModel.composer) { composer in + CommentComposerView(viewModel: composer) { viewModel.composerClosed($0) } .presentationDetents([.large]) - } } } @@ -132,12 +124,16 @@ struct CommentDetailView: View { } } ToolbarItem(placement: .topBarTrailing) { - let link = viewModel.loadedDetail?.link + let shareLink = viewModel.shareLink let menuAction = viewModel.toolbarModel.menuAction - if link != nil || menuAction != nil { + if viewModel.showsEdit || shareLink != nil || menuAction != nil { Menu { - if let link { - ShareLink(item: link) + if viewModel.showsEdit { + Button(Strings.detailEdit, systemImage: "pencil") { viewModel.editTapped() } + .disabled(!viewModel.canEdit) + } + if let shareLink { + ShareLink(item: shareLink) } // The secondary moderation move shares the toolbar's // enablement so it can't fire on seed data or during a diff --git a/Modules/Sources/WordPressComments/Views/PreviewSupport.swift b/Modules/Sources/WordPressComments/Views/PreviewSupport.swift index e77e212f6d35..03ac77419480 100644 --- a/Modules/Sources/WordPressComments/Views/PreviewSupport.swift +++ b/Modules/Sources/WordPressComments/Views/PreviewSupport.swift @@ -29,6 +29,7 @@ final class PreviewCommentsService: CommentsServiceProtocol { func delete(id: Int64) async throws {} func numberOfReplies(for id: Int64) async throws -> Int { replyCount } func createReply(postID: Int64, parentID: Int64, content: String) async throws -> CommentDetail { .preview() } + func updateContent(id: Int64, content: String) async throws -> CommentDetail { .preview() } } struct PreviewCapabilities: CommentsCapabilitiesProtocol { diff --git a/Modules/Tests/WordPressCommentsTests/CommentComposerViewModelTests.swift b/Modules/Tests/WordPressCommentsTests/CommentComposerViewModelTests.swift index 5f87657af93c..b845c8166a67 100644 --- a/Modules/Tests/WordPressCommentsTests/CommentComposerViewModelTests.swift +++ b/Modules/Tests/WordPressCommentsTests/CommentComposerViewModelTests.swift @@ -4,12 +4,21 @@ import WordPressAPI import WordPressAPIInternal @testable import WordPressComments +/// Builds a composer over a fresh coordinator. `tracker` is shared by the +/// coordinator and the composer, as in production. @MainActor -private func makeCoordinator( +private func makeComposerVM( + mode: CommentComposerViewModel.Mode, service: FakeCommentsService = FakeCommentsService(), + store: FakeCommentDraftStore = FakeCommentDraftStore(), tracker: (any CommentsTracker)? = nil -) -> CommentsModerationCoordinator { - CommentsModerationCoordinator(service: service, tracker: tracker) +) -> CommentComposerViewModel { + CommentComposerViewModel( + mode: mode, + coordinator: CommentsModerationCoordinator(service: service, tracker: tracker), + draftStore: store, + tracker: tracker + ) } @MainActor @@ -21,37 +30,42 @@ struct CommentComposerViewModelTests { let store = FakeCommentDraftStore() store.preloadDraft("draft", commentID: 1) - let vm = CommentComposerViewModel( - mode: .reply(parent: makeDetail(id: 1)), - coordinator: makeCoordinator(), - draftStore: store - ) + let vm = makeComposerVM(mode: .reply(parent: makeDetail(id: 1)), store: store) #expect(vm.text == "draft") } - @Test func approveNoteShownOnlyForPendingReplyParent() { - let pendingVM = CommentComposerViewModel( - mode: .reply(parent: makeDetail(status: .hold)), - coordinator: makeCoordinator(), - draftStore: FakeCommentDraftStore() + @Test func editModeSeedsRawContentAndIgnoresDrafts() { + let store = FakeCommentDraftStore() + store.preloadDraft("should not be used", commentID: 1) + + let vm = makeComposerVM( + mode: .edit(comment: makeDetail(id: 1, editContext: true, content: "raw")), + store: store ) + + #expect(vm.text == "raw") + } + + @Test func editModeFallsBackToEmptyWithoutRaw() { + let vm = makeComposerVM(mode: .edit(comment: makeDetail(id: 1, editContext: false))) + + #expect(vm.text.isEmpty) + } + + @Test func approveNoteShownOnlyForPendingReplyParent() { + let pendingVM = makeComposerVM(mode: .reply(parent: makeDetail(status: .hold))) #expect(pendingVM.showsApproveNote) - let approvedVM = CommentComposerViewModel( - mode: .reply(parent: makeDetail(status: .approved)), - coordinator: makeCoordinator(), - draftStore: FakeCommentDraftStore() - ) + let approvedVM = makeComposerVM(mode: .reply(parent: makeDetail(status: .approved))) #expect(!approvedVM.showsApproveNote) + + let editVM = makeComposerVM(mode: .edit(comment: makeDetail(status: .hold, editContext: true))) + #expect(!editVM.showsApproveNote) } @Test func canSendRequiresNonEmptyTrimmedText() { - let vm = CommentComposerViewModel( - mode: .reply(parent: makeDetail()), - coordinator: makeCoordinator(), - draftStore: FakeCommentDraftStore() - ) + let vm = makeComposerVM(mode: .reply(parent: makeDetail())) vm.text = " \n" #expect(!vm.canSend) @@ -60,27 +74,31 @@ struct CommentComposerViewModelTests { #expect(vm.canSend) } + @Test func editCanSendRequiresChange() { + let vm = makeComposerVM(mode: .edit(comment: makeDetail(editContext: true, content: "raw"))) + + #expect(!vm.canSend) // text still equals the original raw content + + vm.text = "raw edited" + #expect(vm.canSend) + } + // MARK: - Send: reply - @Test func sendReplyPassesApproveParentForPendingParent() async { + @Test func sendReplyApprovesOnlyAPendingParent() async { let pendingService = FakeCommentsService() pendingService.createReplyResult = .success(makeDetail(id: 99, status: .approved)) pendingService.setStatusResult = .success(makeDetail(id: 1, status: .approved)) - let pendingVM = CommentComposerViewModel( - mode: .reply(parent: makeDetail(id: 1, status: .hold)), - coordinator: makeCoordinator(service: pendingService), - draftStore: FakeCommentDraftStore() - ) + let pendingVM = makeComposerVM(mode: .reply(parent: makeDetail(id: 1, status: .hold)), service: pendingService) pendingVM.text = "hi" _ = await pendingVM.send() #expect(pendingService.setStatusInvocations.map(\.status) == [.approved]) let approvedService = FakeCommentsService() approvedService.createReplyResult = .success(makeDetail(id: 99, status: .approved)) - let approvedVM = CommentComposerViewModel( + let approvedVM = makeComposerVM( mode: .reply(parent: makeDetail(id: 1, status: .approved)), - coordinator: makeCoordinator(service: approvedService), - draftStore: FakeCommentDraftStore() + service: approvedService ) approvedVM.text = "hi" _ = await approvedVM.send() @@ -93,10 +111,10 @@ struct CommentComposerViewModelTests { let spy = SpyCommentsTracker() let store = FakeCommentDraftStore() store.preloadDraft("draft", commentID: 1) - let vm = CommentComposerViewModel( + let vm = makeComposerVM( mode: .reply(parent: makeDetail(id: 1, status: .approved)), - coordinator: makeCoordinator(service: service, tracker: spy), - draftStore: store, + service: service, + store: store, tracker: spy ) vm.text = "hi" @@ -105,16 +123,15 @@ struct CommentComposerViewModelTests { #expect(outcome == .replied(notice: Strings.noticeReplySent)) #expect(store.deleted == [1]) + // repliedTo is tracked by the coordinator, not the composer; the + // composer only ever tracks editorOpened, and only in edit mode. + #expect(!spy.trackedEvents.contains(.editorOpened(commentID: 1, postID: 10))) } @Test func sendReplyPendingStatusWordsNoticeAccordingly() async { let service = FakeCommentsService() service.createReplyResult = .success(makeDetail(id: 99, status: .hold)) - let vm = CommentComposerViewModel( - mode: .reply(parent: makeDetail(id: 1, status: .approved)), - coordinator: makeCoordinator(service: service), - draftStore: FakeCommentDraftStore() - ) + let vm = makeComposerVM(mode: .reply(parent: makeDetail(id: 1, status: .approved)), service: service) vm.text = "hi" let outcome = await vm.send() @@ -127,10 +144,10 @@ struct CommentComposerViewModelTests { service.createReplyResult = .failure(WpApiError.stub(code: .CommentDuplicate)) let store = FakeCommentDraftStore() store.preloadDraft("draft", commentID: 1) - let vm = CommentComposerViewModel( + let vm = makeComposerVM( mode: .reply(parent: makeDetail(id: 1, status: .approved)), - coordinator: makeCoordinator(service: service), - draftStore: store + service: service, + store: store ) vm.text = "hi" @@ -145,10 +162,10 @@ struct CommentComposerViewModelTests { service.createReplyResult = .failure(FakeServiceError()) let store = FakeCommentDraftStore() store.preloadDraft("draft", commentID: 1) - let vm = CommentComposerViewModel( + let vm = makeComposerVM( mode: .reply(parent: makeDetail(id: 1, status: .approved)), - coordinator: makeCoordinator(service: service), - draftStore: store + service: service, + store: store ) vm.text = "hi" @@ -163,11 +180,7 @@ struct CommentComposerViewModelTests { @Test func sendReplyClosedErrorShowsClosedMessage() async { let service = FakeCommentsService() service.createReplyResult = .failure(WpApiError.stub(code: .CommentClosed)) - let vm = CommentComposerViewModel( - mode: .reply(parent: makeDetail(id: 1, status: .approved)), - coordinator: makeCoordinator(service: service), - draftStore: FakeCommentDraftStore() - ) + let vm = makeComposerVM(mode: .reply(parent: makeDetail(id: 1, status: .approved)), service: service) vm.text = "hi" let outcome = await vm.send() @@ -176,14 +189,37 @@ struct CommentComposerViewModelTests { #expect(vm.errorMessage == Strings.composerErrorClosed) } + // MARK: - Send: edit + + @Test func sendEditReturnsEditedOutcome() async { + let service = FakeCommentsService() + let comment = makeDetail(id: 1, editContext: true, content: "raw") + service.updateContentResult = .success(makeEditedDetail(id: 1, contentHTML: "new
", contentRaw: "new")) + let vm = makeComposerVM(mode: .edit(comment: comment), service: service) + vm.text = "new" + + let outcome = await vm.send() + + #expect(outcome == .edited) + } + + @Test func sendEditFailureShowsEditError() async { + let service = FakeCommentsService() + service.updateContentResult = .failure(FakeServiceError()) + let comment = makeDetail(id: 1, editContext: true, content: "raw") + let vm = makeComposerVM(mode: .edit(comment: comment), service: service) + vm.text = "new" + + let outcome = await vm.send() + + #expect(outcome == nil) + #expect(vm.errorMessage == Strings.composerErrorEditFailed) + } + // MARK: - Cancel flows @Test func replyIsDirtyOnlyWithNonBlankText() { - let vm = CommentComposerViewModel( - mode: .reply(parent: makeDetail(id: 1)), - coordinator: makeCoordinator(), - draftStore: FakeCommentDraftStore() - ) + let vm = makeComposerVM(mode: .reply(parent: makeDetail(id: 1))) #expect(!vm.isDirty) vm.text = " \n" @@ -193,13 +229,17 @@ struct CommentComposerViewModelTests { #expect(vm.isDirty) } + @Test func editIsDirtyOnlyWhenTextDiffersFromOriginal() { + let vm = makeComposerVM(mode: .edit(comment: makeDetail(id: 1, editContext: true, content: "raw"))) + #expect(!vm.isDirty) + + vm.text = "raw edited" + #expect(vm.isDirty) + } + @Test func saveDraftPersists() { let store = FakeCommentDraftStore() - let vm = CommentComposerViewModel( - mode: .reply(parent: makeDetail(id: 1)), - coordinator: makeCoordinator(), - draftStore: store - ) + let vm = makeComposerVM(mode: .reply(parent: makeDetail(id: 1)), store: store) vm.text = "draft text" vm.saveDraft() @@ -209,11 +249,7 @@ struct CommentComposerViewModelTests { @Test func deleteDraftDeletes() { let store = FakeCommentDraftStore() - let vm = CommentComposerViewModel( - mode: .reply(parent: makeDetail(id: 1)), - coordinator: makeCoordinator(), - draftStore: store - ) + let vm = makeComposerVM(mode: .reply(parent: makeDetail(id: 1)), store: store) vm.deleteDraft() @@ -223,11 +259,7 @@ struct CommentComposerViewModelTests { @Test func deleteDraftIfBlankDeletesOnlyWhenTextIsBlank() { let store = FakeCommentDraftStore() store.preloadDraft("draft", commentID: 1) - let vm = CommentComposerViewModel( - mode: .reply(parent: makeDetail(id: 1)), - coordinator: makeCoordinator(), - draftStore: store - ) + let vm = makeComposerVM(mode: .reply(parent: makeDetail(id: 1)), store: store) vm.deleteDraftIfBlank() #expect(store.deleted.isEmpty) @@ -236,4 +268,17 @@ struct CommentComposerViewModelTests { vm.deleteDraftIfBlank() #expect(store.deleted == [1]) } + + // MARK: - Analytics + + @Test func editModeTracksEditorOpenedOnce() { + let spy = SpyCommentsTracker() + + _ = makeComposerVM( + mode: .edit(comment: makeDetail(id: 1, post: 10, editContext: true, content: "raw")), + tracker: spy + ) + + #expect(spy.trackedEvents == [.editorOpened(commentID: 1, postID: 10)]) + } } diff --git a/Modules/Tests/WordPressCommentsTests/CommentDetailTests.swift b/Modules/Tests/WordPressCommentsTests/CommentDetailTests.swift index 2c77e3519586..4a9d8c0071d6 100644 --- a/Modules/Tests/WordPressCommentsTests/CommentDetailTests.swift +++ b/Modules/Tests/WordPressCommentsTests/CommentDetailTests.swift @@ -28,4 +28,14 @@ struct CommentDetailTests { let detail = CommentDetail(comment: .detailBuilder(authorName: "")) #expect(detail.authorName == Strings.anonymousAuthor) } + + @Test func contentRawIsNilFromViewContext() { + let detail = CommentDetail(comment: .detailBuilder(id: 1)) + #expect(detail.contentRaw == nil) + } + + @Test func contentRawIsMappedFromEditContext() { + let detail = CommentDetail(comment: .editDetailBuilder(id: 1, content: "Raw text")) + #expect(detail.contentRaw == "Raw text") + } } diff --git a/Modules/Tests/WordPressCommentsTests/CommentDetailViewModelTests.swift b/Modules/Tests/WordPressCommentsTests/CommentDetailViewModelTests.swift index aeabb9728fc6..9e9b277d323e 100644 --- a/Modules/Tests/WordPressCommentsTests/CommentDetailViewModelTests.swift +++ b/Modules/Tests/WordPressCommentsTests/CommentDetailViewModelTests.swift @@ -239,12 +239,9 @@ struct CommentDetailViewModelTests { } @Test func subscribedEventUpdatesLoadedStatus() async { - let service = FakeCommentsService() - service.fetchCommentResult = .success(makeDetail(id: 1, status: .approved, editContext: true)) let coordinator = CommentsModerationCoordinator(service: FakeCommentsService()) - let vm = makeVM(service: service, coordinator: coordinator) + let vm = await makeLoadedVM(status: .approved, coordinator: coordinator) - await vm.onAppear() #expect(vm.header?.status == .approved) // A late status change for this comment arrives while the screen is open. @@ -253,13 +250,28 @@ struct CommentDetailViewModelTests { #expect(vm.header?.status == .spam) } + @Test func contentChangedEventUpdatesLoadedDetail() async { + let coordinator = CommentsModerationCoordinator(service: FakeCommentsService()) + let vm = await makeLoadedVM(status: .approved, coordinator: coordinator) + + let headerBeforeEvent = vm.header + + coordinator.events.send(.contentChanged(id: vm.commentID, contentHTML: "edited
", contentRaw: "edited")) + + guard case .loaded(let detail) = vm.content else { + Issue.record("Expected loaded content") + return + } + #expect(detail.contentHTML == "edited
") + #expect(detail.contentRaw == "edited") + #expect(vm.header == headerBeforeEvent) + #expect(!vm.isDeleted) + } + @Test func replyCreatedEventLeavesDetailUntouched() async { - let service = FakeCommentsService() - service.fetchCommentResult = .success(makeDetail(id: 1, status: .approved, editContext: true)) let coordinator = CommentsModerationCoordinator(service: FakeCommentsService()) - let vm = makeVM(service: service, coordinator: coordinator) + let vm = await makeLoadedVM(status: .approved, coordinator: coordinator) - await vm.onAppear() let contentBeforeEvent = vm.content let headerBeforeEvent = vm.header @@ -371,13 +383,77 @@ struct CommentDetailViewModelTests { #expect(vm.parentPreview == nil) } - @Test func statusChangeWhileHiddenIsAppliedOnReturn() async { + @Test func parentContentEditRefreshesInReplyToStrip() async { let service = FakeCommentsService() - service.fetchCommentResult = .success(makeDetail(id: 1, status: .approved, editContext: true)) + service.numberOfRepliesResult = .success(0) + service.fetchCommentResultsByID = [ + 1: .success(makeDetail(id: 1, parent: 5)), + 5: .success(makeDetail(id: 5, status: .approved)) + ] + let coordinator = CommentsModerationCoordinator(service: FakeCommentsService()) + let vm = makeVM(service: service, coordinator: coordinator) + + await vm.onAppear() + #expect(vm.parentPreview?.id == 5) + + // The parent's own detail screen edits its content; this screen never + // subscribed to the child's id for that event, so it must also react + // to the parent's id to keep the "In reply to" strip fresh. + coordinator.events.send( + .contentChanged(id: 5, contentHTML: "edited parent
", contentRaw: "edited parent") + ) + + #expect(vm.parentPreview?.snippet == "edited parent") + } + + @Test func contentChangedForOwnIDStillUpdatesOwnContentWithParentSubscribed() async { + // Guards against the parent-content fix accidentally routing the + // child's own contentChanged event through the parent handler (or vice + // versa): both must keep working independently. + let service = FakeCommentsService() + service.numberOfRepliesResult = .success(0) + service.fetchCommentResultsByID = [ + 1: .success(makeDetail(id: 1, parent: 5, status: .approved, editContext: true)), + 5: .success(makeDetail(id: 5, status: .approved)) + ] let coordinator = CommentsModerationCoordinator(service: FakeCommentsService()) let vm = makeVM(service: service, coordinator: coordinator) await vm.onAppear() + let parentPreviewBeforeEvent = vm.parentPreview + + coordinator.events.send(.contentChanged(id: vm.commentID, contentHTML: "edited
", contentRaw: "edited")) + + guard case .loaded(let detail) = vm.content else { + Issue.record("Expected loaded content") + return + } + #expect(detail.contentHTML == "edited
") + #expect(vm.parentPreview == parentPreviewBeforeEvent) + } + + @Test func topLevelCommentHasNoParentSubscriptionAndDoesNotCrash() async { + // parent: 0 in makeDetail means no parentID (top-level comment). + let service = FakeCommentsService() + service.numberOfRepliesResult = .success(0) + service.fetchCommentResultsByID = [1: .success(makeDetail(id: 1, status: .approved))] + let coordinator = CommentsModerationCoordinator(service: FakeCommentsService()) + let vm = makeVM(service: service, coordinator: coordinator) + + await vm.onAppear() + #expect(vm.parentPreview == nil) + + // No parent subscription should exist; broadcasting a contentChanged + // for some other id must not affect (or crash) this screen. + coordinator.events.send(.contentChanged(id: 999, contentHTML: "irrelevant
", contentRaw: "irrelevant")) + + #expect(vm.parentPreview == nil) + } + + @Test func statusChangeWhileHiddenIsAppliedOnReturn() async { + let coordinator = CommentsModerationCoordinator(service: FakeCommentsService()) + let vm = await makeLoadedVM(status: .approved, coordinator: coordinator) + #expect(vm.header?.status == .approved) // The screen is hidden because a parent detail was pushed on top. The @@ -392,12 +468,9 @@ struct CommentDetailViewModelTests { // MARK: - Terminal (deleted) state @Test func deletedEventTerminatesAndLaterStatusChangeReenables() async { - let service = FakeCommentsService() - service.fetchCommentResult = .success(makeDetail(id: 1, status: .approved, editContext: true)) let coordinator = CommentsModerationCoordinator(service: FakeCommentsService()) - let vm = makeVM(service: service, coordinator: coordinator) + let vm = await makeLoadedVM(status: .approved, coordinator: coordinator) - await vm.onAppear() #expect(vm.showsToolbar == true) // A permanent delete succeeds and its `.deleted` event terminates the @@ -420,6 +493,7 @@ struct CommentDetailViewModelTests { let vm = makeVM(service: service, coordinator: coordinator) await vm.onAppear() + try? await coordinator.perform(.delete, on: makeDetail(id: 1, status: .approved)) #expect(vm.isDeleted) @@ -439,11 +513,7 @@ struct CommentDetailViewModelTests { let noticePresenter = FakeNoticePresenter() let coordinatorService = BlockingCommentsService() let coordinator = CommentsModerationCoordinator(service: coordinatorService) - var vm: CommentDetailViewModel? = await makeLoadedVM( - status: .hold, - coordinator: coordinator, - noticePresenter: noticePresenter - ) + var vm: CommentDetailViewModel? = await makeLoadedVM(status: .hold, coordinator: coordinator, noticePresenter: noticePresenter) vm!.perform(.approve) await waitUntil { !coordinatorService.setStatusInvocations.isEmpty } @@ -460,14 +530,6 @@ struct CommentDetailViewModelTests { // MARK: - Reply/edit composer gating and presentation @Test func canReplyRequiresModerationAndActiveStatus() async { - func makeLoadedVM(status: CommentStatus) async -> CommentDetailViewModel { - let service = FakeCommentsService() - service.fetchCommentResult = .success(makeDetail(id: 1, status: status, editContext: true)) - let vm = makeVM(service: service) - await vm.onAppear() - return vm - } - let approvedVM = await makeLoadedVM(status: .approved) #expect(approvedVM.canReply == true) @@ -499,6 +561,52 @@ struct CommentDetailViewModelTests { #expect(unfetchedVM.canReply == false) } + @Test func knownCapabilityGatesControlsBeforeAppear() async { + let vm = makeVM( + seed: makeItem(id: 1, status: .approved), + service: BlockingCommentsService(), + resolver: await makeResolvedCapabilities(canModerate: true) + ) + + // No await has run yet: the toolbar, Reply, and Edit already render + // (disabled), so they take part in the push transition. + #expect(vm.showsToolbar) + #expect(vm.toolbarModel == .approved) + #expect(vm.showsReply) + #expect(vm.showsEdit) + #expect(!vm.isToolbarEnabled) + #expect(!vm.canReply) + + let readOnlyVM = makeVM( + seed: makeItem(id: 1, status: .approved), + service: BlockingCommentsService(), + resolver: await makeResolvedCapabilities(canModerate: false) + ) + #expect(!readOnlyVM.showsToolbar) + #expect(!readOnlyVM.showsReply) + } + + @Test func knownCapabilitySkipsTheLookupAndFailedLookupDegradesToReadOnly() async { + let capabilities = FakeCommentsCapabilities() + let resolver = CommentsCapabilityResolver(capabilities: capabilities) + _ = await resolver.resolve() + let service = FakeCommentsService() + service.fetchCommentResult = .success(makeDetail(id: 1, editContext: true)) + let vm = makeVM(service: service, resolver: resolver) + await vm.onAppear() + #expect(capabilities.invocations == 1) // the resolver's, not a second one + #expect(vm.canModerate == true) + + let failing = FakeCommentsCapabilities() + failing.error = FakeServiceError() + let readOnlyService = FakeCommentsService() + readOnlyService.fetchCommentResult = .success(makeDetail(id: 1)) + let readOnlyVM = makeVM(service: readOnlyService, capabilities: failing) + await readOnlyVM.onAppear() + #expect(readOnlyVM.canModerate == false) + #expect(readOnlyService.fetchCommentInvocations.last?.allowsEditContext == false) + } + @Test func showsReplyRendersDisabledButtonBeforeFetchCompletes() async { let service = BlockingCommentsService() let vm = makeVM(seed: makeItem(id: 1, status: .approved), service: service) @@ -530,17 +638,88 @@ struct CommentDetailViewModelTests { } } + @Test func showsEditRendersDisabledItemBeforeFetchCompletes() async { + let service = BlockingCommentsService() + let vm = makeVM(seed: makeItem(id: 1, status: .trash), service: service) + #expect(!vm.showsEdit) + + async let appear: Void = vm.onAppear() + await waitUntil { !service.fetchCommentInvocations.isEmpty } + + // Spam/trash stay editable, so the item shows (disabled) from the + // seed; only a custom status hides it. + #expect(vm.showsEdit) + #expect(!vm.canEdit) + + service.resolveFetch(callIndex: 0, with: makeDetail(id: 1, status: .trash, editContext: true)) + await appear + + #expect(vm.showsEdit) + #expect(vm.canEdit) + + let customVM = makeVM(seed: makeItem(id: 1, status: .custom("draft")), service: FakeCommentsService()) + await customVM.onAppear() + #expect(!customVM.showsEdit) + } + + @Test func shareLinkComesFromSeedBeforeFetchAndOnlyForApproved() async { + let seed = makeItem(id: 1, status: .approved) + let service = BlockingCommentsService() + let vm = makeVM(seed: seed, service: service) + // Sharing needs no capability or fetch: the seed's link is enough. + #expect(vm.shareLink == seed.link) + + async let appear: Void = vm.onAppear() + await waitUntil { !service.fetchCommentInvocations.isEmpty } + let detail = makeDetail(id: 1, editContext: true) + service.resolveFetch(callIndex: 0, with: detail) + await appear + #expect(vm.shareLink == detail.link) + + for status in [CommentStatus.hold, .spam, .trash, .custom("draft")] { + let hiddenService = FakeCommentsService() + hiddenService.fetchCommentResult = .success(makeDetail(id: 1, status: status, editContext: true)) + let hiddenVM = makeVM(seed: makeItem(id: 1, status: status), service: hiddenService) + #expect(hiddenVM.shareLink == nil, "\(status) seed") + await hiddenVM.onAppear() + #expect(hiddenVM.shareLink == nil, "\(status) fetched") + } + } + + @Test func canEditRequiresEditContextAndModeledStatus() async { + let vm = await makeLoadedVM(status: .approved) + #expect(vm.canEdit == true) + + // A stale capability that fell back to view context (no edit context) + // already hides the toolbar; canEdit follows. + let noEditContextService = FakeCommentsService() + noEditContextService.fetchCommentResult = .success(makeDetail(id: 1, status: .approved, editContext: false)) + let noEditContextVM = makeVM(service: noEditContextService) + await noEditContextVM.onAppear() + #expect(noEditContextVM.canEdit == false) + + // A custom, non-modeled status is not editable. + let otherVM = await makeLoadedVM(status: .custom("draft")) + #expect(otherVM.canEdit == false) + + let cannotModerateCapabilities = FakeCommentsCapabilities() + cannotModerateCapabilities.canModerate = false + let cannotModerateService = FakeCommentsService() + cannotModerateService.fetchCommentResult = .success(makeDetail(id: 1, status: .approved, editContext: false)) + let cannotModerateVM = makeVM(service: cannotModerateService, capabilities: cannotModerateCapabilities) + await cannotModerateVM.onAppear() + #expect(cannotModerateVM.canEdit == false) + } + // MARK: - Status change refreshes the loaded detail (not just the header) @Test(arguments: [CommentListItem.Status.spam, .trash]) func statusChangeToSpamOrTrashRefreshesCanReplyGating(_ to: CommentListItem.Status) async { - let service = FakeCommentsService() - service.fetchCommentResult = .success(makeDetail(id: 1, status: .approved, editContext: true)) let coordinator = CommentsModerationCoordinator(service: FakeCommentsService()) - let vm = makeVM(service: service, coordinator: coordinator) + let vm = await makeLoadedVM(status: .approved, coordinator: coordinator) - await vm.onAppear() #expect(vm.canReply == true) + #expect(vm.canEdit == true) coordinator.noteExternalStatus(id: 1, to: to) @@ -552,15 +731,17 @@ struct CommentDetailViewModelTests { // pre-moderation status (Reply no longer shown for a spam/trash // comment). #expect(vm.canReply == false) + // canEdit's guard only excludes a custom, non-modeled status (`.other`); + // spam/trash remain intentionally editable, so canEdit is unaffected + // here. The loaded detail's status still refreshed correctly, which the + // next test demonstrates via the case canEdit's guard DOES react to. + #expect(vm.canEdit == true) } @Test func statusChangeToPendingKeepsCanReplyTrue() async { - let service = FakeCommentsService() - service.fetchCommentResult = .success(makeDetail(id: 1, status: .approved, editContext: true)) let coordinator = CommentsModerationCoordinator(service: FakeCommentsService()) - let vm = makeVM(service: service, coordinator: coordinator) + let vm = await makeLoadedVM(status: .approved, coordinator: coordinator) - await vm.onAppear() #expect(vm.canReply == true) coordinator.noteExternalStatus(id: 1, to: .pending) @@ -569,28 +750,47 @@ struct CommentDetailViewModelTests { #expect(vm.canReply == true) } + @Test func statusChangeAwayFromCustomStatusRefreshesCanEditGating() async { + // canEdit's guard only excludes a custom, non-modeled status (`.other`). + // Load with a custom status (canEdit false), then reconcile back to a + // modeled status: without the loadedDetail refresh, canEdit would stay + // stuck reading the stale `.other` status and never re-enable. + let coordinator = CommentsModerationCoordinator(service: FakeCommentsService()) + let vm = await makeLoadedVM(status: .custom("draft"), coordinator: coordinator) + + #expect(vm.canEdit == false) + + coordinator.noteExternalStatus(id: 1, to: .approved) + + #expect(vm.header?.status == .approved) + #expect(vm.canEdit == true) + } + @Test func replyTappedPresentsReplyComposer() async { - let service = FakeCommentsService() - let detail = makeDetail(id: 1, status: .approved, editContext: true) - service.fetchCommentResult = .success(detail) - let vm = makeVM(service: service) - await vm.onAppear() + let vm = await makeLoadedVM(status: .approved) #expect(vm.canReply == true) vm.replyTapped() #expect(vm.composer != nil) - #expect(vm.composer?.mode == .reply(parent: detail)) + #expect(vm.composer?.mode == .reply(parent: vm.loadedDetail!)) + } + + @Test func editTappedPresentsEditComposer() async { + let vm = await makeLoadedVM(status: .approved) + #expect(vm.canEdit == true) + + vm.editTapped() + + #expect(vm.composer != nil) + #expect(vm.composer?.mode == .edit(comment: vm.loadedDetail!)) } @Test func replyTappedIgnoredWhileMutating() async { let coordinatorService = BlockingCommentsService() let coordinator = CommentsModerationCoordinator(service: coordinatorService) - let service = FakeCommentsService() - service.fetchCommentResult = .success(makeDetail(id: 1, status: .approved, editContext: true)) - let vm = makeVM(service: service, coordinator: coordinator) + let vm = await makeLoadedVM(status: .approved, coordinator: coordinator) - await vm.onAppear() let spam = Task { try? await coordinator.perform(.spam, on: makeDetail(id: 1, status: .approved, editContext: true)) } @@ -605,18 +805,28 @@ struct CommentDetailViewModelTests { _ = await spam.value } - @Test func composerFinishedRepliedPresentsNoticeAndDismisses() async { + @Test func composerClosedRepliedPresentsNoticeAndDismisses() async { let noticePresenter = FakeNoticePresenter() - let service = FakeCommentsService() - service.fetchCommentResult = .success(makeDetail(id: 1, status: .approved, editContext: true)) - let vm = makeVM(service: service, noticePresenter: noticePresenter) - await vm.onAppear() + let vm = await makeLoadedVM(status: .approved, noticePresenter: noticePresenter) vm.replyTapped() #expect(vm.composer != nil) - vm.composerFinished(.replied(notice: "Reply sent.")) + vm.composerClosed(.replied(notice: "Reply sent.")) #expect(vm.composer == nil) #expect(noticePresenter.presented == ["Reply sent."]) } + + @Test(arguments: [CommentComposerViewModel.Outcome.edited, nil]) + func composerClosedEditedOrCancelledJustDismisses(_ outcome: CommentComposerViewModel.Outcome?) async { + let noticePresenter = FakeNoticePresenter() + let vm = await makeLoadedVM(status: .approved, noticePresenter: noticePresenter) + vm.editTapped() + #expect(vm.composer != nil) + + vm.composerClosed(outcome) + + #expect(vm.composer == nil) + #expect(noticePresenter.presented.isEmpty) + } } diff --git a/Modules/Tests/WordPressCommentsTests/CommentListItemTests.swift b/Modules/Tests/WordPressCommentsTests/CommentListItemTests.swift index 99628662deaf..9f2160ed2cbc 100644 --- a/Modules/Tests/WordPressCommentsTests/CommentListItemTests.swift +++ b/Modules/Tests/WordPressCommentsTests/CommentListItemTests.swift @@ -13,6 +13,11 @@ struct CommentListItemTests { #expect(item.avatarURL == URL(string: "https://example.com/avatar.png")) } + @Test func mapsLinkAndNormalizesEmptyToNil() { + #expect(CommentListItem(comment: makeComment(id: 7, post: 42)).link == URL(string: "https://example.com/?p=42#comment-7")) + #expect(CommentListItem(comment: makeComment(link: "")).link == nil) + } + @Test func stripsHTMLIntoSingleLineSnippet() { let item = CommentListItem( comment: makeComment(content: "Line one
\nLine & two
") diff --git a/Modules/Tests/WordPressCommentsTests/CommentsListViewModelEventTests.swift b/Modules/Tests/WordPressCommentsTests/CommentsListViewModelEventTests.swift index b42972607d2d..3f13c4e0405f 100644 --- a/Modules/Tests/WordPressCommentsTests/CommentsListViewModelEventTests.swift +++ b/Modules/Tests/WordPressCommentsTests/CommentsListViewModelEventTests.swift @@ -523,4 +523,102 @@ struct CommentsListViewModelEventTests { #expect(viewModel.state == .idle) } + + // MARK: - Content changed: update the row's snippet in place + + @Test func contentChangedUpdatesSnippetInPlace() async { + let service = FakeCommentsService() + let itemFour = makeItem(id: 4) + let itemFive = makeItem(id: 5, content: "old
") + let itemSix = makeItem(id: 6) + service.queuedResults = [.success(makePage(items: [itemFour, itemFive, itemSix], hasNext: false))] + let viewModel = CommentsListViewModel(filter: .all, service: service) + await viewModel.onAppear() + + viewModel.apply(.contentChanged(id: 5, contentHTML: "new\nline
", contentRaw: "new")) + + #expect(viewModel.items.map(\.id) == [4, 5, 6]) + #expect(viewModel.items[1].snippet == "new line") + #expect(viewModel.items[0] == itemFour) + #expect(viewModel.items[2] == itemSix) + #expect(viewModel.state == .loaded) + } + + @Test func inFlightPageOneDoesNotOverwriteInPlaceContentUpdate() async { + let service = BlockingCommentsService() + let viewModel = CommentsListViewModel(filter: .all, service: service) + + // First load (call 0): row 5 with the original snippet. + async let firstLoad: Void = viewModel.onAppear() + await waitUntil { service.callCount >= 1 } + service.resolve(callIndex: 0, with: makePage(items: [makeItem(id: 5, content: "old
")], hasNext: false)) + await firstLoad + #expect(viewModel.items.first?.snippet == "old") + + // Mark the tab stale (a pending comment that belongs here but is absent + // can't be placed in a paged list); the tab schedules its own page-one + // reload (call 1), which suspends. + viewModel.apply(.statusChanged(id: 99, to: .pending)) + await waitUntil { service.callCount >= 2 } + + // An edit corrects the row's snippet in place while the reload is in + // flight, invalidating the in-flight page. + viewModel.apply(.contentChanged(id: 5, contentHTML: "new
", contentRaw: "new")) + #expect(viewModel.items.first?.snippet == "new") + + // The invalidated page must NOT overwrite the edited snippet. Instead + // the reload refetches (exactly one more request) and converges on + // fresh authoritative data, without a manual refresh. + service.resolve(callIndex: 1, with: makePage(items: [makeItem(id: 5, content: "old
")], hasNext: false)) + await waitUntil { service.callCount >= 3 } + service.resolve(callIndex: 2, with: makePage(items: [makeItem(id: 5, content: "new
")], hasNext: false)) + await waitUntil { viewModel.state == .loaded } + #expect(viewModel.items.first?.snippet == "new") + #expect(viewModel.state == .loaded) + #expect(service.callCount == 3) + } + + @Test func contentChangedForAbsentIDIsNoOp() async { + let service = FakeCommentsService() + let item = makeItem(id: 1) + service.queuedResults = [.success(makePage(items: [item], hasNext: false))] + let viewModel = CommentsListViewModel(filter: .all, service: service) + await viewModel.onAppear() + + viewModel.apply(.contentChanged(id: 999, contentHTML: "new
", contentRaw: "new")) + + #expect(viewModel.items == [item]) + } + + @Test func inFlightLoadMoreDiscardsStaleContentForAbsentID() async { + let service = BlockingCommentsService() + let viewModel = CommentsListViewModel(filter: .all, service: service) + + // First load (call 0): row 1. + async let firstLoad: Void = viewModel.onAppear() + await waitUntil { service.callCount >= 1 } + service.resolve(callIndex: 0, with: makePage(items: [makeItem(id: 1)], hasNext: true)) + await firstLoad + #expect(viewModel.items.map(\.id) == [1]) + + // Start load-more (call 1); it suspends before appending. Row 5 is not + // yet in `items` for this tab. + async let more: Void = viewModel.loadMore() + await waitUntil { service.callCount >= 2 } + + // Row 5 is edited elsewhere while this fetch is in flight. There's no + // row to correct in place, but the fetch must still be invalidated: + // it was captured before the edit and could still deliver row 5 with + // its old snippet. + viewModel.apply(.contentChanged(id: 5, contentHTML: "new
", contentRaw: "new")) + + // The in-flight page carries row 5's OLD snippet (fetched before the + // edit). It must be discarded rather than appended with stale content. + service.resolve( + callIndex: 1, + with: makePage(items: [makeItem(id: 5, content: "old
")], hasNext: false) + ) + await more + #expect(viewModel.items.map(\.id) == [1]) + } } diff --git a/Modules/Tests/WordPressCommentsTests/CommentsModerationCoordinatorTests.swift b/Modules/Tests/WordPressCommentsTests/CommentsModerationCoordinatorTests.swift index 19caa967a7d4..70e6b1b1ba0c 100644 --- a/Modules/Tests/WordPressCommentsTests/CommentsModerationCoordinatorTests.swift +++ b/Modules/Tests/WordPressCommentsTests/CommentsModerationCoordinatorTests.swift @@ -414,6 +414,150 @@ struct CommentsModerationCoordinatorTests { #expect(service.createReplyInvocations.count == 1) } + // MARK: - Edit content + + @Test func editContentEmitsContentChangedAndReturnsDetail() async throws { + let service = FakeCommentsService() + service.updateContentResult = .success(makeEditedDetail(id: 1, contentHTML: "x
", contentRaw: "x")) + let spy = SpyCommentsTracker() + let coordinator = CommentsModerationCoordinator(service: service, tracker: spy) + let recorder = EventRecorder(coordinator) + let comment = makeDetail(id: 1, status: .approved) + + let detail = try await coordinator.editContent(on: comment, newContent: "x") + + #expect(detail.contentHTML == "x
") + #expect(detail.contentRaw == "x") + #expect(recorder.events == [.contentChanged(id: 1, contentHTML: "x
", contentRaw: "x")]) + #expect(spy.trackedEvents == [.edited(commentID: 1, postID: 10)]) + #expect(!coordinator.isMutating(id: 1)) + } + + @Test func editContentFailureThrowsWithoutEventOrReconcile() async { + let service = FakeCommentsService() + service.updateContentResult = .failure(FakeServiceError()) + let coordinator = CommentsModerationCoordinator(service: service) + let recorder = EventRecorder(coordinator) + let comment = makeDetail(id: 1, status: .approved) + + await #expect(throws: (any Error).self) { + try await coordinator.editContent(on: comment, newContent: "x") + } + + #expect(recorder.events.isEmpty) + // No reconcile: a failed edit is last-writer-wins, not refetched. + #expect(service.fetchStatusInvocations.isEmpty) + } + + @Test func editContentEmitsStatusCorrectionWhenServerStatusDiffers() async throws { + let service = FakeCommentsService() + // A concurrent moderator (or plugin) marked the comment as spam while + // the editor was open; the edit response carries that landed status. + var edited = makeEditedDetail(id: 1, contentHTML: "x
", contentRaw: "x") + edited.status = .spam + service.updateContentResult = .success(edited) + let coordinator = CommentsModerationCoordinator(service: service) + let recorder = EventRecorder(coordinator) + let comment = makeDetail(id: 1, status: .approved) + + let detail = try await coordinator.editContent(on: comment, newContent: "x") + + #expect(detail.status == .spam) + #expect( + recorder.events == [ + .contentChanged(id: 1, contentHTML: "x
", contentRaw: "x"), + .statusChanged(id: 1, to: .spam) + ] + ) + } + + @Test func editContentEmitsOnlyContentChangedWhenServerStatusMatches() async throws { + let service = FakeCommentsService() + service.updateContentResult = .success(makeEditedDetail(id: 1, contentHTML: "x
", contentRaw: "x")) + let coordinator = CommentsModerationCoordinator(service: service) + let recorder = EventRecorder(coordinator) + let comment = makeDetail(id: 1, status: .approved) + + _ = try await coordinator.editContent(on: comment, newContent: "x") + + #expect(recorder.events == [.contentChanged(id: 1, contentHTML: "x
", contentRaw: "x")]) + } + + @Test func editSerializesWithStatusMutations() async throws { + let service = BlockingCommentsService() + service.updateContentResult = .success(makeEditedDetail(id: 5, contentHTML: "y
", contentRaw: "y")) + let coordinator = CommentsModerationCoordinator(service: service) + let comment = makeDetail(id: 5, status: .approved) + + let spam = Task { try? await coordinator.perform(.spam, on: comment) } + await waitUntil { !service.setStatusInvocations.isEmpty } + + async let editOutcome = coordinator.editContent(on: comment, newContent: "y") + await Task.yield() + #expect(service.updateContentInvocations.isEmpty) + + service.resolveSetStatus(callIndex: 0, with: makeDetail(id: 5, status: .spam)) + let detail = try await editOutcome + _ = await spam.value + + #expect(detail.contentHTML == "y
") + #expect(service.updateContentInvocations.count == 1) + } + + // Regression test for the atomic-slot-claim fix: two callers awaiting the + // SAME in-flight mutation must not both resume and claim the slot. Three + // overlapping `editContent` calls on one comment expose this precisely, + // because with only two calls the single-waiter case behaves identically + // whether or not the claim is atomic. + @Test func secondAndThirdEditsWaitForSlotClaimAtomically() async throws { + let service = BlockingCommentsService() + let coordinator = CommentsModerationCoordinator(service: service) + let comment = makeDetail(id: 7, status: .approved) + + async let first = coordinator.editContent(on: comment, newContent: "one") + await waitUntil { !service.updateContentInvocations.isEmpty } + + async let second = coordinator.editContent(on: comment, newContent: "two") + async let third = coordinator.editContent(on: comment, newContent: "three") + // Let both late callers reach their re-checking wait loop. Neither can + // have claimed the slot yet, so the invocation count must still be 1. + for _ in 0..<5 { await Task.yield() } + #expect(service.updateContentInvocations.count == 1) + + service.resolveUpdateContent( + callIndex: 0, + with: makeEditedDetail(id: 7, contentHTML: "one
", contentRaw: "one") + ) + + // Exactly one of the two waiters claims the slot next. Under the bug, + // both would resume and claim, jumping straight to 3; the fix must + // land on 2 and hold there until that second mutation settles. + await waitUntil { service.updateContentInvocations.count >= 2 } + for _ in 0..<5 { await Task.yield() } + #expect(service.updateContentInvocations.count == 2) + + service.resolveUpdateContent( + callIndex: 1, + with: makeEditedDetail(id: 7, contentHTML: "two
", contentRaw: "two") + ) + + await waitUntil { service.updateContentInvocations.count >= 3 } + service.resolveUpdateContent( + callIndex: 2, + with: makeEditedDetail(id: 7, contentHTML: "three
", contentRaw: "three") + ) + + let firstResult = try await first + let secondResult = try await second + let thirdResult = try await third + + #expect( + Set([firstResult.contentHTML, secondResult.contentHTML, thirdResult.contentHTML]) == [ + "one
", "two
", "three
" + ] + ) + } + @Test func reentryFetchAwaitsReplyChain() async throws { let service = BlockingCommentsService() let coordinator = CommentsModerationCoordinator(service: service) diff --git a/Modules/Tests/WordPressCommentsTests/Support/BlockingCommentsService.swift b/Modules/Tests/WordPressCommentsTests/Support/BlockingCommentsService.swift index eff57a2de807..1a1e915e7a5a 100644 --- a/Modules/Tests/WordPressCommentsTests/Support/BlockingCommentsService.swift +++ b/Modules/Tests/WordPressCommentsTests/Support/BlockingCommentsService.swift @@ -25,6 +25,11 @@ final class BlockingCommentsService: CommentsServiceProtocol { /// tests that only need the call's timing relative to a blocked `setStatus`. var createReplyResult: ResultHello world
", post: Int64 = 10, status: CommentStatus = .approved, - date: Date = Date(timeIntervalSince1970: 1_700_000_000) + date: Date = Date(timeIntervalSince1970: 1_700_000_000), + link: String? = nil ) -> CommentWithViewContext { CommentWithViewContext( id: id, @@ -20,7 +21,7 @@ func makeComment( content: CommentContentWithViewContext(rendered: content), date: "2023-11-14T22:13:20", dateGmt: date, - link: "https://example.com/?p=\(post)#comment-\(id)", + link: link ?? "https://example.com/?p=\(post)#comment-\(id)", parent: 0, post: post, status: status, @@ -33,10 +34,13 @@ func makeComment( func makeItem( id: Int64 = 1, authorName: String = "Author", + content: String = "Hello world
", post: Int64 = 10, status: CommentStatus = .approved ) -> CommentListItem { - CommentListItem(comment: makeComment(id: id, authorName: authorName, post: post, status: status)) + CommentListItem( + comment: makeComment(id: id, authorName: authorName, content: content, post: post, status: status) + ) } extension CommentWithViewContext { diff --git a/Modules/Tests/WordPressCommentsTests/Support/CommentDetailTestHelpers.swift b/Modules/Tests/WordPressCommentsTests/Support/CommentDetailTestHelpers.swift index 7203ea18f4e1..9b94c0ce2e23 100644 --- a/Modules/Tests/WordPressCommentsTests/Support/CommentDetailTestHelpers.swift +++ b/Modules/Tests/WordPressCommentsTests/Support/CommentDetailTestHelpers.swift @@ -23,6 +23,15 @@ func makeDetail( return CommentDetail(comment: .detailBuilder(id: id, post: post, parent: parent, status: status)) } +/// A detail whose content differs from `makeDetail`'s defaults, standing in for +/// the server's response to an edit. +func makeEditedDetail(id: Int64 = 1, contentHTML: String, contentRaw: String?) -> CommentDetail { + var detail = makeDetail(id: id) + detail.contentHTML = contentHTML + detail.contentRaw = contentRaw + return detail +} + @MainActor func makeVM( commentID: Int64 = 1, @@ -64,7 +73,7 @@ func makeResolvedCapabilities(canModerate: Bool) async -> CommentsCapabilityReso @MainActor func makeLoadedVM( status: CommentStatus = .hold, - coordinator: CommentsModerationCoordinator, + coordinator: CommentsModerationCoordinator? = nil, noticePresenter: (any NoticePresenting)? = nil ) async -> CommentDetailViewModel { let service = FakeCommentsService() diff --git a/Modules/Tests/WordPressCommentsTests/Support/FakeCommentsService.swift b/Modules/Tests/WordPressCommentsTests/Support/FakeCommentsService.swift index 6b961db4687c..025c8a00867d 100644 --- a/Modules/Tests/WordPressCommentsTests/Support/FakeCommentsService.swift +++ b/Modules/Tests/WordPressCommentsTests/Support/FakeCommentsService.swift @@ -33,6 +33,9 @@ final class FakeCommentsService: CommentsServiceProtocol { var createReplyResult: Result