[NT-280] Manage pledge screen from CTA container #831

Scollaco · 2019-09-09T19:39:02Z

📲 What

Presents the ManageViewPledge screen (blank for now) from the cta container.

🤔 Why

This was decided to make the access to the screen easier, compared to what was proposed previously.

🛠 How

Moved the code from RewardsCollectionViewController to ProjectPamphletViewController
Moved tests to the classes to be tested

👀 See

Managing 🐛	Viewing Pledge 🦋

✅ Acceptance criteria

With `featureNativeCheckoutPledgeView` enabled.

Select a non-backed project and tap the Back this project button. The Rewards Carousel should be presented.
Select a backed project and tap the Manage button. The ManageViewPledgeViewController (blank) should be presented.
Selected a backed and ended project and tap the View your pledge button. The ManageViewPledgeViewController (blank) should be presented.

Note:

By tapping to view your pledge, note that the title shown is Manage your pledge. The logic to change the title based of the pledge and project states will be implemented in a different PR.

With `featureNativeCheckoutPledgeView` disabled.

On the Rewards Carousel screen:

Select a non-backed project and tap the Back this project button. The Rewards Carousel should be presented.
Select a backed project and tap the Manage button. The DeprecatedRewardPledgeViewController should be presented.
Selected a backed and ended project and tap the View your pledge button. The BackingViewController should be presented.

ksr-ci-bot · 2019-09-09T19:50:08Z

	1 Warning
⚠️	Big PR

SwiftFormat found issues:

File	Rules
Library/ViewModels/ProjectPamphletViewModel.swift	spaceAroundOperators, wrapArguments

Generated by 🚫 Danger

dusi

Mostly questions

dusi · 2019-09-10T23:24:47Z

Kickstarter-iOS/Views/Controllers/ProjectPamphletViewController.swift

@@ -135,6 +135,26 @@ public final class ProjectPamphletViewController: UIViewController {
        self?.goToRewards(project: project, refTag: refTag)
      }

+    self.viewModel.outputs.goToManageViewPledge


I'm seeing a lot of inconsistencies between the naming of VMs output signal and the private action on the controller...do you think it would be worth it cleaning this up..

So for example:

self.viewModel.outputs.goToManageViewPledge would trigger self?.goToManageViewPledge, etc...

For the deprecated ones I'd suggest we simply add it as a suffix:

self.viewModel.outputs.goToViewBackingDeprecated links to self?.goToViewBacking...

It makes sense to change the private actions to match the name of the output. Will change that.

Regarding the deprecated naming, I'm following the naming convention that we adopted for deprecated classes (DeprecatedCheckoutViewModel, DeprecatedRewardPledgeViewController, DeprecatedRewardCell etc)

dusi · 2019-09-10T23:31:23Z

Kickstarter-iOS/Views/Controllers/ProjectPamphletViewController.swift

+      .configuredWith(
+        project: project, reward: reward
+      )
+    pledgeViewController.navigationItem.leftBarButtonItem = UIBarButtonItem(


Is there a reason we've decided to set this externally on DeprecatedRewardPledgeViewController?

If we do this inside of the controller and set self.navigationItem (like we do everywhere else we will be able to remove this code as well the identical code in ProjectPamphletContentViewController.

But maybe I'm missing the reason here? 🤔

On a rush to unblock the other tickets I had copied the code from somewhere else. Good catch.

dusi · 2019-09-10T23:31:46Z

Kickstarter-iOS/Views/Controllers/ProjectPamphletViewController.swift

+    if AppEnvironment.current.device.userInterfaceIdiom == .pad {
+      let nav = UINavigationController(rootViewController: backingViewController)
+        |> \.modalPresentationStyle .~ .formSheet
+      self.present(nav, animated: true, completion: nil)


Can we omit completion handler?

dusi · 2019-09-10T23:35:25Z

Kickstarter-iOS/Views/Controllers/ProjectPamphletViewController.swift

+        |> \.modalPresentationStyle .~ .formSheet
+      self.present(nav, animated: true, completion: nil)
+    } else {
+      self.navigationController?.pushViewController(backingViewController, animated: true)


So, I know we've previously discussed the fact that we're presenting manage screen over the previous context using .formSheet...but do we really want to push on non-iPad devices? Just trying to understand the decision behind this?

The from BackingViewController, we can access Messages (the vc's navigation controller pushes the MessagesViewController). If we present the BackingViewController modally instead, we are no longer able to see the messages, since it requires a navigation controller to work

dusi · 2019-09-10T23:38:27Z

Kickstarter-iOS/Views/PledgeCTAContainerView.swift

@@ -4,7 +4,7 @@ import Prelude
 import UIKit

 protocol PledgeCTAContainerViewDelegate: AnyObject {
-  func pledgeCTAButtonTapped()
+  func pledgeCTAButtonTapped(_ state: PledgeStateCTAType)


Maybe with state?

func pledgeCTAButtonTapped(with state: PledgeStateCTAType)

then we call it with more context like so

self?.delegate?.pledgeCTAButtonTapped(with: state)

Dunno which one is better ¯_(ツ)_/¯

Me gusta :)

dusi · 2019-09-10T23:40:20Z

Library/ViewModels/ProjectPamphletViewModel.swift

@@ -34,6 +34,12 @@ public protocol ProjectPamphletViewModelOutputs {
  /// Emits a (project, isLoading) tuple used to configure the pledge CTA view
  var configurePledgeCTAView: Signal<(Either<Project, ErrorEnvelope>, Bool), Never> { get }

+  var goToDeprecatedManagePledge: Signal<PledgeData, Never> { get }
+
+  var goToDeprecatedViewBacking: Signal<(Project, User?), Never> { get }


I know we're deprecating stuff but looking at these parameters I wonder if we can improve this by declaring BackingData typealias, etc. for (Project, User?) tuple

Just a suggestion

dusi · 2019-09-10T23:42:33Z

Library/ViewModels/ProjectPamphletViewModel.swift

+      .map(unpack)
+
+    let goToManagePledge = ctaButtonTapped
+      .filter(canShowManageViewPledgeScreen(_:_:state:))


Looks like canShowManageViewPledgeScreen does not need refTag? Could omit and have more readable call site canShowManageViewPledgeScreen(_:state:)

The problem with that is ctaButtonTapped emits (project, refTag, state). If we omit the refTag, the compiler will complain because the types emitted don't match the function arguments, thus the point free syntax will no longer be possible.

I see!

Thanks for clarifying this...I don't know whether a stylistic thing like being able to call something with point-free expression would overweight the code debt of passing argument that's not necessary.

In this case, I'd consider whether non-point-free expression would be more suitable (less code + less debt).

.filter { (project, _, state) in canShowManageViewPledgeScreen(project, state:state) }

or

.filter { canShowManageViewPledgeScreen($0.0, state:$0.2) }

dusi · 2019-09-10T23:43:28Z

Library/ViewModels/ProjectPamphletViewModel.swift

+    let goToManagePledge = ctaButtonTapped
+      .filter(canShowManageViewPledgeScreen(_:_:state:))
+      .map { (project, refTag, _) -> PledgeData in
+        let r = reward(from: project.personalization.backing, inProject: project)


Could we name this reward (it should fit on the screen and be more descriptive)?

That's what I did first, but kept getting the error Variable used within its own initial value . Any suggestions?

Right, good point!

How about something like this:

.map { (project, refTag, _) -> PledgeData in PledgeData( project: project, reward: reward(from: project.personalization.backing, inProject: project), refTag: refTag ) }

or

.map { PledgeData( project: $0.0, reward: reward(from: $0.0.personalization.backing, inProject: $0.0), refTag: $0.1 ) }

(but that might be more than confusing due to the repetition of those variables)

Since this is a language limitation I'd be OK with leaving it as is .. your decision! 🥂

dusi · 2019-09-10T23:44:20Z

Library/ViewModels/ProjectPamphletViewModel.swift

+      .filter { _ in featureNativeCheckoutPledgeViewIsEnabled() }
+
+    self.goToDeprecatedManagePledge = ctaButtonTapped
+      .filter(shouldGoToDeprecatedManagePledge(_:_:state:))


Same thing with refTag...is this because we'll need it in the future but don't need it just now?

Nah... we will only need the refTag for the manage pledge context. Here it's possibleto change because we don't need the refTag, so we can map the value creating a new tuple without it before filtering:

self.goToDeprecatedViewBacking = ctaButtonTapped .map { (project, _, state) in (project, state) } .filter(shouldGoToDeprecatedViewBacking(_:state:)) .map { project, _ in return BackingData(project, AppEnvironment.current.currentUser) }

Yeah, I'd probably try to carry over as little unused things as possible..this makes it easier to reason about code and will be less confusing in the future (read in a week when we forget WTF was this) :D

dusi · 2019-09-10T23:44:31Z

Library/ViewModels/ProjectPamphletViewModel.swift

+    self.goToDeprecatedManagePledge = ctaButtonTapped
+      .filter(shouldGoToDeprecatedManagePledge(_:_:state:))
+      .map { (project, refTag, _) -> PledgeData in
+        let r = reward(from: project.personalization.backing, inProject: project)


Same for the naming

dusi

Couple additional comments..starting to look good!!!

dusi · 2019-09-11T20:51:35Z

Kickstarter-iOS/Views/Controllers/ProjectPamphletViewController.swift

-  func pledgeCTAButtonTapped() {
-    self.viewModel.inputs.backThisProjectTapped()
+  func pledgeCTAButtonTapped(with state: PledgeStateCTAType) {
+    self.viewModel.inputs.pledgeCTAButtonTapped(state)


We're keeping the input this way? I thought we would also make it more descriptive (i.e. input.pledgeCTAButtonTapped(with: state)?

dusi · 2019-09-11T21:03:30Z

Library/ViewModels/ProjectPamphletViewModel.swift

+      .map(unpack)
+
+    let goToManagePledge = ctaButtonTapped
+      .filter(canShowManageViewPledgeScreen(_:_:state:))


I see!

Thanks for clarifying this...I don't know whether a stylistic thing like being able to call something with point-free expression would overweight the code debt of passing argument that's not necessary.

In this case, I'd consider whether non-point-free expression would be more suitable (less code + less debt).

.filter { (project, _, state) in canShowManageViewPledgeScreen(project, state:state) }

or

.filter { canShowManageViewPledgeScreen($0.0, state:$0.2) }

dusi · 2019-09-11T21:05:41Z

Library/ViewModels/ProjectPamphletViewModel.swift

+    let goToManagePledge = ctaButtonTapped
+      .filter(canShowManageViewPledgeScreen(_:_:state:))
+      .map { (project, refTag, _) -> PledgeData in
+        let r = reward(from: project.personalization.backing, inProject: project)


Right, good point!

How about something like this:

.map { (project, refTag, _) -> PledgeData in PledgeData( project: project, reward: reward(from: project.personalization.backing, inProject: project), refTag: refTag ) }

or

.map { PledgeData( project: $0.0, reward: reward(from: $0.0.personalization.backing, inProject: $0.0), refTag: $0.1 ) }

(but that might be more than confusing due to the repetition of those variables)

dusi · 2019-09-11T21:10:47Z

Library/ViewModels/ProjectPamphletViewModel.swift

+    let goToManagePledge = ctaButtonTapped
+      .filter(canShowManageViewPledgeScreen(_:_:state:))
+      .map { (project, refTag, _) -> PledgeData in
+        let r = reward(from: project.personalization.backing, inProject: project)


Since this is a language limitation I'd be OK with leaving it as is .. your decision! 🥂

dusi · 2019-09-11T21:12:53Z

Library/ViewModels/ProjectPamphletViewModel.swift

+      .filter { _ in featureNativeCheckoutPledgeViewIsEnabled() }
+
+    self.goToDeprecatedManagePledge = ctaButtonTapped
+      .filter(shouldGoToDeprecatedManagePledge(_:_:state:))


Yeah, I'd probably try to carry over as little unused things as possible..this makes it easier to reason about code and will be less confusing in the future (read in a week when we forget WTF was this) :D

dusi · 2019-09-11T21:15:15Z

Library/ViewModels/ProjectPamphletViewModel.swift

+  return userCanSeeNativeCheckout() && (state == .pledge || state == .viewRewards)
+}
+
+private func canShowManageViewPledgeScreen(


After we remove the unused params, these function names should fit on one line making it a bit more readable IMO.

private func canShowRewardsScreen(state: PledgeStateCTAType?) -> Bool { .. } private func canShowManageViewPledgeScreen(_ project: Project, state: PledgeStateCTAType?) -> Bool { .. } private func shouldGoToDeprecatedViewBacking(_ project: Project, state: PledgeStateCTAType?) -> Bool { .. } private func shouldGoToDeprecatedManagePledge(_ project: Project, state: PledgeStateCTAType?) -> Bool { .. }

dusi · 2019-09-11T21:16:46Z

Library/ViewModels/ProjectPamphletViewModelTests.swift

@@ -686,7 +862,7 @@ final class ProjectPamphletViewModelTests: TestCase {
      self.goToRewardsProject.assertDidNotEmitValue()
      self.goToRewardsRefTag.assertDidNotEmitValue()

-      self.vm.inputs.backThisProjectTapped()
+      self.vm.inputs.pledgeCTAButtonTapped(.pledge)


See, I think this is where a better naming convention would help us a little Tapped(with: .pledge)..What do you think?

Oh right. I changed in one place but missed that one. Will change it

dusi

Looks good nino!

I had couple suggestions but they shouldn't hold this review up...they're up to your judgment.

Cheers~

dusi · 2019-09-11T22:54:44Z

Library/ViewModels/ProjectPamphletViewModel.swift


+public typealias BackingData = (Project, User?)


Looks like

// BackingViewModel.swift fileprivate let projectAndBackerProperty = MutableProperty<(Project, User?)?>(nil)

could use this data type...should we update it?

// BackingViewModel.swift fileprivate let projectAndBackerProperty = MutableProperty<BackingData?>(nil)

dusi · 2019-09-11T23:36:13Z

Library/ViewModels/ProjectPamphletViewModel.swift

+      .map(unpack)
+
+    let goToManagePledge = ctaButtonTapped
+      .filter { canShowManageViewPledgeScreen($0.0, state:$0.2) }


Looking at this in action, it might actually be more readable to name those params. I'll leave this out to you, I'm OK with both - but wanted to raise it in case you were in favour of the other option.

.filter { project, _, state in canShowManageViewPledgeScreen(project, state: state) }

(this of course would apply to other places in this PR.

…hub.com/kickstarter/ios-oss into manage-pledge-screen-from-cta-container

Scollaco added 4 commits September 6, 2019 15:38

Added logic to present ManageViewPledge from Project Page

d5b28d8

Added logic to show deprecated manage/view backing screens

257fe50

Cleanup, Fixed transitions, Added tests

98f515d

Swiftformat

d12f942

Scollaco requested a review from dusi September 9, 2019 19:39

Scollaco assigned dusi Sep 9, 2019

Merge branch 'master' into manage-pledge-screen-from-cta-container

622d1e5

Merge branch 'master' into manage-pledge-screen-from-cta-container

0429d3e

dusi reviewed Sep 10, 2019

View reviewed changes

Scollaco added 2 commits September 11, 2019 11:47

Feedback from Pavel

d2e841e

Fixed navigation to View Rewards of finished projects

3f40c69

dusi reviewed Sep 11, 2019

View reviewed changes

Scollaco added 2 commits September 11, 2019 17:37

More feedback

1ac60bf

Putting functions in one line

cce443e

dusi approved these changes Sep 11, 2019

View reviewed changes

Scollaco added 3 commits September 12, 2019 10:41

Merge branch 'master' into manage-pledge-screen-from-cta-container

d0d9067

Replaced MutableProperty type for its typealias

101bc7a

Merge branch 'manage-pledge-screen-from-cta-container' of https://git…

f839d70

…hub.com/kickstarter/ios-oss into manage-pledge-screen-from-cta-container

Scollaco merged commit 627a60b into master Sep 12, 2019

Scollaco deleted the manage-pledge-screen-from-cta-container branch September 12, 2019 15:20

[NT-280] Manage pledge screen from CTA container #831

[NT-280] Manage pledge screen from CTA container #831

Conversation

Scollaco commented Sep 9, 2019 • edited by dusi Loading

📲 What

🤔 Why

🛠 How

👀 See

✅ Acceptance criteria

With featureNativeCheckoutPledgeView enabled.

Note:

With featureNativeCheckoutPledgeView disabled.

ksr-ci-bot commented Sep 9, 2019 • edited Loading

SwiftFormat found issues:

dusi 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

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

Scollaco Sep 11, 2019 • edited Loading

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

dusi 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dusi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scollaco commented Sep 9, 2019 •

edited by dusi

Loading

With `featureNativeCheckoutPledgeView` enabled.

With `featureNativeCheckoutPledgeView` disabled.

ksr-ci-bot commented Sep 9, 2019 •

edited

Loading

Scollaco Sep 11, 2019 •

edited

Loading