MBL-1014: Replace ReactiveSwift in ReportProjectFormViewModel with Combine #1873

amy-at-kickstarter · 2023-10-26T15:33:25Z

📲 What

This removes ReactiveSwift from ReportProjectFormViewModel. As part of that change, I also did some refactoring, to take advantage of as much brevity as I could squeeze out of that change.

🤔 Why

This is one of our experiments in moving the app off of ReactiveSwift. The goal here is twofold:

Learn more about what that might look like, and how we'd have to architect things
Make sure that we can actually ship a Combine + SwiftUI feature without something going terribly wrong

👀 See

Because I had to make a lot of architectural choices here, I'm leaving a lot of comments in the code. Take a look and let me know if you agree or disagree! This is all part of figuring out how we want to use Combine in the future.

✅ Acceptance criteria

You'll need to turn on the Report Projects feature flag (either in the code directly, or in the app debug interface.) This should have no changes from previous Report Projects behavior, except for a little extra UI when the e-mail is loading.

amy-at-kickstarter · 2023-10-26T21:21:49Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

@@ -8,45 +8,54 @@ enum ReportFormFocusField {

 @available(iOS 15.0, *)
 struct ReportProjectFormView: View {
-  let projectID: String


I moved these properties into the view model for two reasons:

The view model needs projectId and projectFlaggingKind to submit the report project mutation

Just in general, it seems more MVVM-y to me to have the view model own this state, instead of the view

Agreed. I mentioned in a comment below that we could consider injecting the VM with these properties from the outset instead of having the view be something of a middleman here.

amy-at-kickstarter · 2023-10-26T21:23:00Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

+    self.viewModel = ReportProjectFormViewModel(
+      projectID: projectID,
+      projectURL: projectURL,
+      projectFlaggingKind: projectFlaggingKind


Passing these in on init so that viewModel has direct access to them.

One interesting note here - to create a custom init, we needed to include the Binding<Bool> popToRoot. The syntax for assigning that binding is weird, hence self._popToRoot above.

I wonder if it would be cleaner to inject the VM into this view with some of these properties when it gets instantiated in ReportProjectInfoView. Something like

ReportProjectFormView( ReportProjectFormViewModel( projectID: self.projectID, projectURL: self.projectUrl, projectFlaggingKind: item.flaggingKind ?? GraphAPI.FlaggingKind.guidelinesViolation, ) )

Since the view is essentially just acting as the middleman to get these properties to the VM anyway. Thought I could see an argument for keeping projectURL in the view See my comment above about views owning UI-specific state. That might also allow us to avoid a custom init

I like that pattern - avoiding the slightly odd custom init seems like it would save us some effort.

+1. Creating the view model first and then passing it to the view is also nice for testability. I'm not sure if it matters as much in our codebase, since we typically use mocks for testing, but if we ever want to create a fake of an entire view model, we can use that to make simple snapshot tests.

amy-at-kickstarter · 2023-10-26T21:23:45Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

  @State private var showLoading: Bool = false
-  @State private var showBannerMessage = false
-  @State private var submitSuccess = false
-  @State private var bannerMessage: MessageBannerViewViewModel?


Now that the view model is using Combine, we can bind the state of the view model directly to the SwiftUI view. That means we don't need these extra state variables in the view, or any of the onReceive code to keep them up-to-date.

I see your point. This pattern makes me wonder what the benefit of @State is. In my experience, SwiftUI views typically handle UI specific state, for the most part, and then only pass what the VM needs when it's needed. Once save is pressed for example. I like the idea of having the VM manage non UI specific state like saveEnabled, bannerMessage, possibly showLoading, etc., but does it make sense for it to own the more UI specific state like retrievedEmail and details?

Yeah, I think that's a really interesting question, and I'm not sure I have a great answer for it, either. The way we've structured things here, the viewmodel actually has no knowledge of the view directly, so at there would be no way for it to even ask the view for its detailsText.

I think @Binding kind of raises some interesting questions about ownership in general, since it's really a two-way relationship between the model and the UI. It would be really interesting to try and find some other examples of MVVM in SwiftUI and see what other patterns folks have cooked up.

At present, my understanding of @State is that it's for a variable that is owned by a view, and/or used by its subviews. At first glance to me, it seems as if we don't have much in the way of state that is ever truly owned by a view - unless it was something like, say, a tab collapsing/expanding.

I agree with this - the default should be for the viewmodel to own things (unless it's literally something that only affects the UI). Any time logic can live in the viewmodel instead of the view, it should.

amy-at-kickstarter · 2023-10-26T21:26:00Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

            Text(retrievedEmail)
              .font(Font(UIFont.ksr_body()))
              .foregroundColor(Color(.ksr_support_400))
              .disabled(true)
+          } else {
+            Text(Strings.Loading())


Added this because it makes the UI feel a little smoother. When the e-mail loads, the layout of the UI won't flicker.

amy-at-kickstarter · 2023-10-26T21:27:11Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

          }
        }

        SwiftUI.Section(Strings.Project_url()) {
-          Text(projectURL)
+          Text(viewModel.projectURL)


n.B. that now these are all coming directly from the view model

amy-at-kickstarter · 2023-10-26T21:27:43Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

            .font(Font(UIFont.ksr_body()))
            .foregroundColor(Color(.ksr_support_400))
            .disabled(true)
        }

        SwiftUI.Section {
-          TextEditor(text: $details)
+          TextEditor(text: $viewModel.detailsText)


Here's an example of a two-way binding - TextEditor can mutate detailsText, which will update that property in the viewmodel

My only thought here is that the view could manage state like this until it is ready to be passed to the view model in its final form

I could see us passing state back to the viewmodel with an input method like didSubmitReport(withDetailsText detailsText: String). But this also raises another question, which is whether or not it would work with combine and buttonEnabled.

Based on what I understand right now - a @State variable on a View is not a publisher. Right now we're doing this in the viewmodel:

self.$detailsText .map { !$0.isEmpty } .assign(to: &$saveButtonEnabled)

but if detailsText is a @State and not @Published, this pattern doesn't work.

Playing with how this is organized seems like a really good thing to mess around with during a pairing session, it'd be interesting to see what you mean and figure out how else it might work.

amy-at-kickstarter · 2023-10-26T21:31:02Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

-            saveEnabled: $saveEnabled,
-            saveTriggered: $saveTriggered,
+            saveEnabled: $viewModel.saveButtonEnabled,
+            saveTriggered: $viewModel.saveTriggered,


So this is an interesting one - I'm not actually sure that it makes sense for a triggered action to be a binding like this. Right now, we're modeling saveTriggered as an event stream of Bools. For example, if you press the button, you'll get the following stream:

false - initial state true - the button was pressed false - we set it back to false once the loading is done

However, it's not really a state, it's more something that happens. It would make more sense to me to model it internally as a stream of voids, where only one event occurs when the button is pressed.

I actually like Apple's pattern here with the underlying View class Button, which takes a closure that is executed when the button is pressed. This would let us follow more of our old input/output pattern that we used with UIKit:

LoadingBarButtonItem( saveEnabled: $saveEnabled, showLoading: $showLoading, titleText: Strings.Send() ) { viewModel.didPressSaveButton() }

Good callout. This makes more sense to me too!

I like the closure approach better, too!

Cool, I'll throw myself a quick ticket to refactor this. Just to keep this PR a bit more svelte.

amy-at-kickstarter · 2023-10-26T21:33:00Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

      }
-      .onChange(of: bannerMessage) { newValue in
+      .onReceive(viewModel.$bannerMessage) { newValue in
+        showLoading = false


It might be cleaner to move the banner logic into the ViewModel, so that the submit, banner display, timeout, and dismiss all happen in the same place. However, that was going to be a bigger piece of refactoring, so I left this as-is.

Yea, that could be cleaner. What if we moved showLoading along with it?

Yeah I think that'd be good too

amy-at-kickstarter · 2023-10-26T21:34:59Z

KsApi/Service.swift

+          return error
+        }
+
+        return ErrorEnvelope.couldNotParseJSON


Not clear if there would be a better error to use here. This would be the case when something in userEnvelope(from: data) threw an exception.

Good question. We could probably use a more verbose error. It looks like the rest of the app currently returns this couldNotParseJSON enveloper

amy-at-kickstarter · 2023-10-26T21:35:25Z

KsApi/combine/CombineTestObserver.swift

+import Combine
+import Foundation
+
+public final class CombineTestObserver<Value, Error: Swift.Error> {


There's another ticket for fleshing this out. This is just the barest-bones stub to get the tests working.

amy-at-kickstarter · 2023-10-26T21:35:41Z

KsApi/combine/Signal+Combine.swift

+import Foundation
+import ReactiveSwift
+
+extension Signal where Error == Never {


This was an experiment in bridging code that I'm not using any more. Kind of nifty though.

amy-at-kickstarter · 2023-10-26T21:36:16Z

KsApi/extensions/ApolloClient+RAC.swift

+
+   - returns: A `AnyPublisher` generic over `Query.Data` and `ErrorEnvelope`.
+   */
+  public func fetch<Query: GraphQLQuery>(query: Query) -> AnyPublisher<Query.Data, ErrorEnvelope> {


I pulled these changes from Mubarak's PR: #1854

amy-at-kickstarter · 2023-10-26T21:37:29Z

KsApi/extensions/ApolloClient+RAC.swift

+  public func perform<Mutation: GraphQLMutation>(
+    mutation: Mutation
+  ) -> AnyPublisher<Mutation.Data, ErrorEnvelope> {
+    let fetchSubject: PassthroughSubject<Mutation.Data, ErrorEnvelope> = .init()


One interesting note - PassthroughSubject just drops events if nobody is subscribed, and this fires off the networking request immediately. I don't think that will be a problem for these network requests, since presumably they'll be created + subscribed at the same time. But could be an edge case to watch out for.

amy-at-kickstarter · 2023-10-26T21:37:53Z

KsApi/extensions/MockGraphQLClient.swift

@@ -73,6 +88,18 @@ private func producer<T, E>(for property: Result<T, E>?) -> SignalProducer<T, E>
  }
 }

+private func producer<T, E>(for property: Result<T, E>?) -> AnyPublisher<T, E> {


This is just copying the old pattern for ReactiveSwift in MockGraphQLClient. Keeping it as close as possible.

amy-at-kickstarter · 2023-10-26T21:38:10Z

KsApi/extensions/MockGraphQLClient.swift

+  case let .success(data):
+    return CurrentValueSubject(data).eraseToAnyPublisher()
+  case .failure:
+    assertionFailure("Need to implement this behavior. I think the Fail() subject is what we want, possibly with a deferred?")


We'll have to implement this when we want to actually test failing a network request.

Nit: maybe add a TODO here, so we don't lose track of it? I expect we'll want to to be able to test failing network requests, especially if we want to improve how we handle them, so doing that as part of a proof-of-concept sounds like a good idea.

amy-at-kickstarter · 2023-10-26T21:42:45Z

Library/ViewModels/ReportProjectFormViewModel.swift

-  /// Emits the currently logged in user's email
-  var userEmail: Signal<String, Never> { get }
-}
+public protocol ReportProjectFormViewModelOutputs {}


So here is an interesting problem: the outputs of our view model are @Published properties, and Swift won't let you define a wrapped property like @Published in a protocol. That means we can't actually put userEmail up here in the outputs, nor any of our other computed/bindable properties.

We could potentially define it like this:

public protocol ReportProjectFormViewModelOutputs { public var retrievedEmailPublisher: Publisher<String?> { get } public var retrievedEmail: String? }

but that seems a bit messy. An alternative could also be just making the inputs and outputs a concrete object, instead of a protocol. Or getting rid of them entirely! Thoughts?

I could be wrong, but I don't think this input/output protocol pattern is common when using Combine. It seems like more of an RxSwift pattern. I'm happy to explore removing it entirely. I'm also happy to explore keeping our @Published properties more organized in a separate concrete object as you mentioned.

Ah, interesting, so maybe there's no good reason to keep it. I'm open to either. It feels a little verbose to me, but it is nice in terms of auto-complete.

I vote we get rid of the inputs and outputs. I think with swiftui/combine bindings (where UI and view model stay in sync almost for us) it's not that useful to distinguish between the inputs and the outputs anymore.

Fair point!

amy-at-kickstarter · 2023-10-26T21:43:42Z

Library/ViewModels/ReportProjectFormViewModel.swift

+    /// Only enable the save button if the user has entered detail text
+    self.$detailsText
+      .map { !$0.isEmpty }
+      .assign(to: &$saveButtonEnabled)


This is an example of a nice, very brief binding between Combine + SwiftUI. Elegant!

Oh this is pretty cool how .assign(to manages the life cycle of the subscription automatically so we don't have to keep track of an AnyCancellable

Yeah, I really dig this pattern

amy-at-kickstarter · 2023-10-26T21:44:32Z

Library/ViewModels/ReportProjectFormViewModel.swift

-      .observeForUI()
-      .observeValues { [weak self] _ in
-        let messageBannerViewViewModel = MessageBannerViewViewModel((
+        self?.saveTriggered = false


Some of this logic was in the view; I moved into the view model. These are hard to read in the git diff view so I recommend checking them out in the actual full file view.

amy-at-kickstarter · 2023-10-26T21:45:32Z

Library/ViewModels/ReportProjectFormViewModelTests.swift

+      let userEmail = CombineTestObserver<String?, Never>()
+      userEmail.observe(vm.$retrievedEmail)
+
+      XCTAssertEqual(userEmail.events.count, 1)


Interesting implementation detail: @Published properties emit their initial value when a subscriber attaches. So retrieving the e-mail triggers two events - the first containing nil, the second containing the actual e-mail.

codecov · 2023-10-30T15:03:58Z

Codecov Report

Merging #1873 (fc4e5e7) into main (b74f1cb) will decrease coverage by 0.07%.
The diff coverage is 23.03%.

@@            Coverage Diff             @@
##             main    #1873      +/-   ##
==========================================
- Coverage   83.90%   83.83%   -0.07%     
==========================================
  Files        1222     1224       +2     
  Lines      111255   111347      +92     
  Branches    29559    29599      +40     
==========================================
  Hits        93350    93350              
- Misses      16891    16984      +93     
+ Partials     1014     1013       -1

Files	Coverage Δ
...starter-iOS/SharedViews/LoadingBarButtonItem.swift	`0.00% <ø> (ø)`
KsApi/ServiceType.swift	`75.65% <ø> (ø)`
...y/ViewModels/ReportProjectFormViewModelTests.swift	`94.44% <96.29%> (+4.97%)`	⬆️
...Features/ReportProject/ReportProjectInfoView.swift	`0.00% <0.00%> (ø)`
KsApi/combine/Signal+Combine.swift	`0.00% <0.00%> (ø)`
KsApi/combine/CombineTestObserver.swift	`0.00% <0.00%> (ø)`
KsApi/MockService.swift	`13.14% <0.00%> (-0.16%)`	⬇️
KsApi/extensions/MockGraphQLClient.swift	`25.86% <0.00%> (-8.23%)`	⬇️
...Features/ReportProject/ReportProjectFormView.swift	`0.00% <0.00%> (ø)`
KsApi/Service.swift	`8.17% <0.00%> (-0.41%)`	⬇️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

scottkicks · 2023-10-31T19:29:20Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

  @State private var showLoading: Bool = false
-  @State private var showBannerMessage = false
-  @State private var submitSuccess = false
-  @State private var bannerMessage: MessageBannerViewViewModel?


I see your point. This pattern makes me wonder what the benefit of @State is. In my experience, SwiftUI views typically handle UI specific state, for the most part, and then only pass what the VM needs when it's needed. Once save is pressed for example. I like the idea of having the VM manage non UI specific state like saveEnabled, bannerMessage, possibly showLoading, etc., but does it make sense for it to own the more UI specific state like retrievedEmail and details?

scottkicks · 2023-11-01T13:44:42Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

+    self.viewModel = ReportProjectFormViewModel(
+      projectID: projectID,
+      projectURL: projectURL,
+      projectFlaggingKind: projectFlaggingKind


I wonder if it would be cleaner to inject the VM into this view with some of these properties when it gets instantiated in ReportProjectInfoView. Something like

ReportProjectFormView( ReportProjectFormViewModel( projectID: self.projectID, projectURL: self.projectUrl, projectFlaggingKind: item.flaggingKind ?? GraphAPI.FlaggingKind.guidelinesViolation, ) )

Since the view is essentially just acting as the middleman to get these properties to the VM anyway. Thought I could see an argument for keeping projectURL in the view See my comment above about views owning UI-specific state. That might also allow us to avoid a custom init

scottkicks · 2023-11-01T13:46:57Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

            .font(Font(UIFont.ksr_body()))
            .foregroundColor(Color(.ksr_support_400))
            .disabled(true)
        }

        SwiftUI.Section {
-          TextEditor(text: $details)
+          TextEditor(text: $viewModel.detailsText)


My only thought here is that the view could manage state like this until it is ready to be passed to the view model in its final form

scottkicks · 2023-11-01T13:48:18Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

-            saveEnabled: $saveEnabled,
-            saveTriggered: $saveTriggered,
+            saveEnabled: $viewModel.saveButtonEnabled,
+            saveTriggered: $viewModel.saveTriggered,


Good callout. This makes more sense to me too!

scottkicks · 2023-11-01T13:49:19Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

      }
-      .onChange(of: bannerMessage) { newValue in
+      .onReceive(viewModel.$bannerMessage) { newValue in
+        showLoading = false


Yea, that could be cleaner. What if we moved showLoading along with it?

scottkicks · 2023-11-01T14:05:48Z

KsApi/Service.swift

+          return error
+        }
+
+        return ErrorEnvelope.couldNotParseJSON


Good question. We could probably use a more verbose error. It looks like the rest of the app currently returns this couldNotParseJSON enveloper

scottkicks · 2023-11-01T14:12:26Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

@@ -8,45 +8,54 @@ enum ReportFormFocusField {

 @available(iOS 15.0, *)
 struct ReportProjectFormView: View {
-  let projectID: String


Agreed. I mentioned in a comment below that we could consider injecting the VM with these properties from the outset instead of having the view be something of a middleman here.

scottkicks · 2023-11-01T14:16:22Z

Library/ViewModels/ReportProjectFormViewModel.swift

-  /// Emits the currently logged in user's email
-  var userEmail: Signal<String, Never> { get }
-}
+public protocol ReportProjectFormViewModelOutputs {}


I could be wrong, but I don't think this input/output protocol pattern is common when using Combine. It seems like more of an RxSwift pattern. I'm happy to explore removing it entirely. I'm also happy to explore keeping our @Published properties more organized in a separate concrete object as you mentioned.

scottkicks · 2023-11-01T14:19:08Z

Library/ViewModels/ReportProjectFormViewModel.swift

+    /// Only enable the save button if the user has entered detail text
+    self.$detailsText
+      .map { !$0.isEmpty }
+      .assign(to: &$saveButtonEnabled)


Oh this is pretty cool how .assign(to manages the life cycle of the subscription automatically so we don't have to keep track of an AnyCancellable

scottkicks · 2023-11-01T14:28:29Z

Library/ViewModels/ReportProjectFormViewModelTests.swift

+    let saveButtonEnabled = CombineTestObserver<Bool, Never>()
+    saveButtonEnabled.observe(vm.$saveButtonEnabled)
+
+    XCTAssertEqual(saveButtonEnabled.events.count, 1)
+    XCTAssertEqual(saveButtonEnabled.events.last, false)
+
+    vm.detailsText = "This is my report. I don't like this project very much."
+    XCTAssertEqual(saveButtonEnabled.events.count, 2)
+    XCTAssertEqual(saveButtonEnabled.events.last, true)
+
+    vm.detailsText = ""
+    XCTAssertEqual(saveButtonEnabled.events.count, 3)
+    XCTAssertEqual(saveButtonEnabled.events.last, false)


Since saveButtonEnabled.events is an array, we could write these assertions in one line. Def not necessary

Suggested change

let saveButtonEnabled = CombineTestObserver<Bool, Never>()

saveButtonEnabled.observe(vm.$saveButtonEnabled)

XCTAssertEqual(saveButtonEnabled.events.count, 1)

XCTAssertEqual(saveButtonEnabled.events.last, false)

vm.detailsText = "This is my report. I don't like this project very much."

XCTAssertEqual(saveButtonEnabled.events.count, 2)

XCTAssertEqual(saveButtonEnabled.events.last, true)

vm.detailsText = ""

XCTAssertEqual(saveButtonEnabled.events.count, 3)

XCTAssertEqual(saveButtonEnabled.events.last, false)

let saveButtonEnabled = CombineTestObserver<Bool, Never>()

saveButtonEnabled.observe(vm.$saveButtonEnabled)

XCTAssertEqual(saveButtonEnabled.events, [false])

vm.detailsText = "This is my report. I don't like this project very much."

XCTAssertEqual(saveButtonEnabled.events, [false, true])

vm.detailsText = ""

XCTAssertEqual(saveButtonEnabled.events, [false, true, false])

amy-at-kickstarter · 2023-11-01T20:13:19Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift


  @SwiftUI.Environment(\.dismiss) private var dismiss
-  @ObservedObject private var viewModel = ReportProjectFormViewModel()
+  @StateObject private var viewModel = ReportProjectFormViewModel()


@scottkicks OK, just learned something interesting. In a truly react-like way, ReportProjectFormView may be initialized many times - any time something calling it in a closure re-renders, for example. That means that ReportProjectFormViewModel was being initialized multiple times, which is a performance drag (and in the case of some re-renders, could also potentially mean we don't have the correct instance of the viewmodel). Apparently the solution is that the View should own the ViewModel using the @StateObject wrapper, which ensures that the view model is only instantiated once for the entire lifecycle of our view.

This also means we can't pass in state directly on init of the viewmodel, or pass the viewmodel directly into the view...so I set some properties on the viewmodel in onAppear.

ifosli

Nice job! I like this a lot! One thing that's not really about this PR at all: it would be nice to link JIRA tickets/add jira ticket numbers to the TODOs. I'm not sure if that's something we actually currently do as a team, but it's a nice way to actually keep track of TODOs and their context (and it's nice to be able to search for the ticket number to find the relevant code if you pick up a backlog bug).

Where the view model is initialized would also be nice to update in this PR, but I like it otherwise (with some expected followup work that you already called out).

ifosli · 2023-11-01T20:25:27Z

Library/ViewModels/ReportProjectFormViewModel.swift

-  /// Emits the currently logged in user's email
-  var userEmail: Signal<String, Never> { get }
-}
+public protocol ReportProjectFormViewModelOutputs {}


I vote we get rid of the inputs and outputs. I think with swiftui/combine bindings (where UI and view model stay in sync almost for us) it's not that useful to distinguish between the inputs and the outputs anymore.

ifosli · 2023-11-01T20:37:04Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

-            saveEnabled: $saveEnabled,
-            saveTriggered: $saveTriggered,
+            saveEnabled: $viewModel.saveButtonEnabled,
+            saveTriggered: $viewModel.saveTriggered,


I like the closure approach better, too!

ifosli · 2023-11-01T20:37:38Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

+    self.viewModel = ReportProjectFormViewModel(
+      projectID: projectID,
+      projectURL: projectURL,
+      projectFlaggingKind: projectFlaggingKind


+1. Creating the view model first and then passing it to the view is also nice for testability. I'm not sure if it matters as much in our codebase, since we typically use mocks for testing, but if we ever want to create a fake of an entire view model, we can use that to make simple snapshot tests.

ifosli · 2023-11-01T20:37:58Z

Kickstarter-iOS/Features/ReportProject/ReportProjectFormView.swift

  @State private var showLoading: Bool = false
-  @State private var showBannerMessage = false
-  @State private var submitSuccess = false
-  @State private var bannerMessage: MessageBannerViewViewModel?


I agree with this - the default should be for the viewmodel to own things (unless it's literally something that only affects the UI). Any time logic can live in the viewmodel instead of the view, it should.

ifosli · 2023-11-01T20:42:14Z

KsApi/extensions/MockGraphQLClient.swift

+  case let .success(data):
+    return CurrentValueSubject(data).eraseToAnyPublisher()
+  case .failure:
+    assertionFailure("Need to implement this behavior. I think the Fail() subject is what we want, possibly with a deferred?")


Nit: maybe add a TODO here, so we don't lose track of it? I expect we'll want to to be able to test failing network requests, especially if we want to improve how we handle them, so doing that as part of a proof-of-concept sounds like a good idea.

amy-at-kickstarter · 2023-11-02T14:45:55Z

Nice job! I like this a lot! One thing that's not really about this PR at all: it would be nice to link JIRA tickets/add jira ticket numbers to the TODOs. I'm not sure if that's something we actually currently do as a team, but it's a nice way to actually keep track of TODOs and their context (and it's nice to be able to search for the ticket number to find the relevant code if you pick up a backlog bug).

Where the view model is initialized would also be nice to update in this PR, but I like it otherwise (with some expected followup work that you already called out).

Good point about the //TODOs, I'll see about setting up some tickets for those.

…mbine

amy-at-kickstarter force-pushed the feat/adyer/remove-reactiveswift-from-report-project branch from bdfb368 to 8b83227 Compare October 26, 2023 21:11

amy-at-kickstarter changed the title ~~Feat/adyer/remove reactiveswift from report project~~ MBL-1014: Replace ReactiveSwift in ReportProjectFormViewModel with Combine Oct 26, 2023

amy-at-kickstarter commented Oct 26, 2023

View reviewed changes

amy-at-kickstarter requested review from scottkicks and ifosli and removed request for scottkicks October 26, 2023 21:45

amy-at-kickstarter self-assigned this Oct 26, 2023

amy-at-kickstarter added the enhancement label Oct 26, 2023

amy-at-kickstarter marked this pull request as ready for review October 26, 2023 21:46

scottkicks reviewed Nov 1, 2023

View reviewed changes

amy-at-kickstarter commented Nov 1, 2023

View reviewed changes

ifosli approved these changes Nov 1, 2023

View reviewed changes

amy-at-kickstarter force-pushed the feat/adyer/remove-reactiveswift-from-report-project branch from bd3e66b to 459edfa Compare November 2, 2023 19:06

amy-at-kickstarter added 6 commits November 2, 2023 15:47

[msadoon] Add Combine support to Apollo client

3866f27

Add createFlaggingInputCombine to Service

5ff0407

Add fetchGraphUserEmailCombine to Service

1181a32

MBL-1014: Replace ReactiveSwift in ReportProjectFormViewModel with Co…

2d5dfa3

…mbine

Use @StateObject for ReportProjectFormViewModel

2fd59e0

Add //TODOs with ticket numbers for leftover tasks

fc4e5e7

amy-at-kickstarter force-pushed the feat/adyer/remove-reactiveswift-from-report-project branch from 8420d34 to fc4e5e7 Compare November 2, 2023 19:47

amy-at-kickstarter merged commit 36ea6fe into main Nov 2, 2023
7 checks passed

amy-at-kickstarter deleted the feat/adyer/remove-reactiveswift-from-report-project branch November 2, 2023 20:14

MBL-1014: Replace ReactiveSwift in ReportProjectFormViewModel with Combine #1873

MBL-1014: Replace ReactiveSwift in ReportProjectFormViewModel with Combine #1873

Conversation

amy-at-kickstarter commented Oct 26, 2023 • edited

📲 What

🤔 Why

👀 See

✅ Acceptance criteria

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter Nov 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter Oct 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter Oct 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter Oct 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 30, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter Nov 1, 2023 • edited

Choose a reason for hiding this comment

ifosli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter commented Nov 2, 2023

amy-at-kickstarter commented Oct 26, 2023 •

edited

amy-at-kickstarter Nov 1, 2023 •

edited

amy-at-kickstarter Oct 26, 2023 •

edited

amy-at-kickstarter Oct 26, 2023 •

edited

amy-at-kickstarter Oct 26, 2023 •

edited

codecov bot commented Oct 30, 2023 •

edited

amy-at-kickstarter Nov 1, 2023 •

edited