[NT-425] Manage Pledge Refresh #905

ifbarrera · 2019-10-22T15:11:04Z

📲 What

Refreshes the manage pledge screen when a user:

updates their pledge amount
changes their payment method
selects a new reward

🤔 Why

So that the updated pledge is reflected in the manage pledge screen when a change has been made.

🛠 How

Initially I was thinking the best approach would be to just refresh the backing when any change is updated. But after discussing with the Android team, it was brought up that it's nice to refresh the whole project in case rewards have changed. I decided to go with that approach based on that point.

👀 See

♿️ Accessibility

N/A

🏎 Performance

N/A

✅ Acceptance criteria

Test update pledge: navigate to a backing that was created in the same environment that you are testing on. Then, navigate to "Update pledge" from the Manage Pledge screen. Change your pledge amount and tap "Confirm". You should be taken back to the Manage Pledge Screen and the amount should be updated.
Test update payment method: navigate to a backing that was created in the same environment that you are testing on. Then, navigate to "Change Payment Method" from the Manage Pledge screen. Change your payment method and tap "Confirm". You should be taken back to the Manage Pledge Screen and the payment source should be updated.
Test choose a new reward: navigate to a backing that was created in the same environment that you are testing on. Then, navigate to "Choose a new reward" from the Manage Pledge screen. Change your selected reward tap "Confirm". You should be taken back to the Manage Pledge Screen and the reward should be updated.

…manage-pledge-refresh

ifbarrera · 2019-10-22T15:11:28Z

Kickstarter-iOS/Views/Controllers/ManagePledgeViewController.swift

@@ -93,13 +93,6 @@ final class ManagePledgeViewController: UIViewController, MessageBannerViewContr
    [self.pledgeSummarySectionSeparator, self.paymentMethodSectionSeparator, self.rewardSectionSeparator]
  }()

-  static func instantiate(with project: Project, reward: Reward) -> ManagePledgeViewController {


This instantiate function was not being used.

ifbarrera · 2019-10-22T15:11:54Z

Kickstarter-iOS/Views/Controllers/ManagePledgeViewController.swift

@@ -245,8 +238,8 @@ final class ManagePledgeViewController: UIViewController, MessageBannerViewContr

  // MARK: - Configuration

-  func configureWith(project: Project, reward: Reward) {
-    self.viewModel.inputs.configureWith(project, reward: reward)
+  func configureWith(project: Project) {


We can actually grab the reward directly from the backing and the project.

This centralizes how we retrieve the reward when the project refreshes (the reward is now always taken from the backing)

justinswart

Looking good so far, will deep-dive tomorrow!

justinswart · 2019-10-23T03:18:37Z

Library/SharedFunctions.swift

+ - returns: A reward
+ */
+
+internal func reward(from backing: Backing?, inProject project: Project) -> Reward {


Ah nice 👍

I suppose this could also be a computed var on Project like backedReward: Reward?.

That said - if nothing is backed, it'll return Reward.noReward which doesn't seem correct, so I think the return type should be optional for this?

I copied and pasted this straight out of ProjectPamphletViewModel, but can update. I think ideally it would take a non-optional Backing and return a non-optional Reward.

Also just realized I forgot to add tests for this 😅

justinswart · 2019-10-23T03:25:07Z

Library/ViewModels/ProjectPamphletViewModel.swift

@@ -99,13 +99,7 @@ public final class ProjectPamphletViewModel: ProjectPamphletViewModelType, Proje

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


I think this (and other) signals here would read better with something like:

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

I refactored all this 😅

ksr-ci-bot · 2019-10-23T20:54:28Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

justinswart

Nice! This appears to work great. Approved with some minor suggestions.

Also, I was unable to test the third AC because that is something I'm currently fixing separately and that depends on work being done in #899.

Oh, also - of course this doesn't handle errors from the refreshing. I think that's fine for this? One would have to lose connectivity immediately after saving changes which is kind've an edge case. However if something did go wrong with the refreshing endpoint we wouldn't know about it.

Let me know if you think we should add error messaging for the refresh and I can re-review.

justinswart · 2019-10-23T22:35:42Z

Library/ViewModels/ProjectPamphletViewModel.swift

@@ -46,7 +46,7 @@ public protocol ProjectPamphletViewModelOutputs {

  var goToDeprecatedViewBacking: Signal<BackingData, Never> { get }

-  var goToManageViewPledge: Signal<PledgeData, Never> { get }
+  var goToManageViewPledge: Signal<Project, Never> { get }


This output and related TestObservers really should just be goToManagePledge, I think we fixed this naming elsewhere but forgot to update it here 😬

I can update it 👍

justinswart · 2019-10-23T22:45:48Z

Library/ViewModels/ManagePledgeViewModelTests.swift

+      |> Backing.lens.reward .~ Reward.noReward
+
+    let project = Project.cosmicSurgery
+      |> Project.lens.rewards %~ { rewards in


If we just want to switch out the reward at index 0 we could write this here and in other places as:

|> Project.lens.rewards .~ ([Reward.noReward] + Project.cosmicSurgery.rewards.suffix(from: 1))

ifbarrera · 2019-10-24T19:39:52Z

Nice! This appears to work great. Approved with some minor suggestions.

Also, I was unable to test the third AC because that is something I'm currently fixing separately and that depends on work being done in #899.

Oh, also - of course this doesn't handle errors from the refreshing. I think that's fine for this? One would have to lose connectivity immediately after saving changes which is kind've an edge case. However if something did go wrong with the refreshing endpoint we wouldn't know about it.

Let me know if you think we should add error messaging for the refresh and I can re-review.

Yeah this was a conscious decision – I figured that since we already show the "Got, it your pledge has been updated" message when the update request succeeds, it would be really confusing as a user to then see something like "Failed to get project" or "Something went wrong" because there's no context for the user around what actually failed. I felt it was best to fail silently, but open to discussing it. Fwiw Android also fails silently on refreshing the project.

ifbarrera added 4 commits October 21, 2019 12:28

Refresh project after update

995f50e

Tests

77b67be

Merge branch 'master' of https://github.com/kickstarter/ios-oss into …

5ab2f9e

…manage-pledge-refresh

Linting

ea9400b

ifbarrera commented Oct 22, 2019

View reviewed changes

ifbarrera assigned justinswart Oct 22, 2019

justinswart reviewed Oct 23, 2019

View reviewed changes

ifbarrera added 2 commits October 23, 2019 16:42

Refactoring, adding tests

00931c2

Formatting

bc76a16

justinswart approved these changes Oct 23, 2019

View reviewed changes

justinswart mentioned this pull request Oct 23, 2019

[NT-361] Select new reward polish #899

Merged

2 tasks

ifbarrera added 2 commits October 24, 2019 15:58

Renaming output and improving test creation

97e4865

Merge branch 'master' into manage-pledge-refresh

44c48c3

ifbarrera merged commit 1e3f58f into master Oct 24, 2019

ifbarrera deleted the manage-pledge-refresh branch October 24, 2019 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NT-425] Manage Pledge Refresh #905

[NT-425] Manage Pledge Refresh #905

ifbarrera commented Oct 22, 2019 •

edited by justinswart

Loading

ifbarrera Oct 22, 2019

ifbarrera Oct 22, 2019

ifbarrera Oct 22, 2019

justinswart left a comment

justinswart Oct 23, 2019

ifbarrera Oct 23, 2019

ifbarrera Oct 23, 2019

justinswart Oct 23, 2019

ifbarrera Oct 24, 2019

ksr-ci-bot commented Oct 23, 2019

justinswart left a comment •

edited

Loading

justinswart Oct 23, 2019

ifbarrera Oct 24, 2019

justinswart Oct 23, 2019

ifbarrera commented Oct 24, 2019

		@@ -99,13 +99,7 @@ public final class ProjectPamphletViewModel: ProjectPamphletViewModelType, Proje

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

[NT-425] Manage Pledge Refresh #905

[NT-425] Manage Pledge Refresh #905

Conversation

ifbarrera commented Oct 22, 2019 • edited by justinswart Loading

📲 What

🤔 Why

🛠 How

👀 See

♿️ Accessibility

🏎 Performance

✅ Acceptance criteria

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinswart 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

ksr-ci-bot commented Oct 23, 2019

justinswart left a comment • 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

ifbarrera commented Oct 24, 2019

ifbarrera commented Oct 22, 2019 •

edited by justinswart

Loading

justinswart left a comment •

edited

Loading