Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

💲[Native Checkout] Reward Selection (no animation) #683

Merged
merged 4 commits into from May 24, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -13,7 +13,7 @@ final class RewardsCollectionViewController: UICollectionViewController {
// MARK: - Properties

private let dataSource = RewardsCollectionViewDataSource()
private let viewModel = RewardsCollectionViewModel()
fileprivate let viewModel = RewardsCollectionViewModel()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did the visibility change? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch! I'd forgotten that Swift 4 extended private access to include extensions in the same file 👍


private let hiddenPagingScrollView: UIScrollView = {
UIScrollView()
Expand Down Expand Up @@ -111,6 +111,12 @@ final class RewardsCollectionViewController: UICollectionViewController {
self?.dataSource.load(rewards: rewards)
self?.collectionView.reloadData()
}

self.viewModel.outputs.goToPledge
.observeForControllerAction()
.observeValues { [weak self] project, reward, refTag in
self?.goToPledge(project: project, reward: reward, refTag: refTag)
}
}

// MARK: - Private Helpers
Expand Down Expand Up @@ -190,13 +196,28 @@ final class RewardsCollectionViewController: UICollectionViewController {
return CGSize(width: itemWidth, height: itemHeight)
}

private func goToPledge(project: Project, reward: Reward, refTag: RefTag?) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why we are passing the refTag on this function? It's not being used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we're definitely going to need to pass it to the PledgeViewController, so I added it in anticipation for that.

let pledgeViewController = PledgeViewController.instantiate()
pledgeViewController.configureWith(project: project, reward: reward)

self.navigationController?.pushViewController(pledgeViewController, animated: true)
}

// MARK: - Public Functions

@objc func closeButtonTapped() {
self.navigationController?.dismiss(animated: true)
}
}

// MARK: - UICollectionViewDelegate

extension RewardsCollectionViewController {
override func collectionView(_ collectionView: UICollectionView, didSelectItemAt indexPath: IndexPath) {
self.viewModel.inputs.rewardSelected(at: indexPath.row)
}
}

// MARK: - UIScrollViewDelegate

extension RewardsCollectionViewController {
Expand Down
21 changes: 21 additions & 0 deletions Library/ViewModels/RewardsCollectionViewModel.swift
Expand Up @@ -4,12 +4,16 @@ import Result
import KsApi
import Prelude

public typealias GoToPledgeData = (project: Project, reward: Reward, refTag: RefTag?)

public protocol RewardsCollectionViewModelOutputs {
var goToPledge: Signal<GoToPledgeData, NoError> { get }
var reloadDataWithRewards: Signal<[Reward], NoError> { get }
}

public protocol RewardsCollectionViewModelInputs {
func configure(with project: Project, refTag: RefTag?)
func rewardSelected(at index: Int)
func viewDidLoad()
}

Expand All @@ -27,6 +31,17 @@ RewardsCollectionViewModelInputs, RewardsCollectionViewModelOutputs {
)
.map(first)
.map { $0.rewards }

let selectedReward = reloadDataWithRewards
.takePairWhen(self.rewardSelectedIndexProperty.signal.skipNil())
.map { rewards, index in rewards[index] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also include a test for covering func rewardSelected(at index: Int) with an invalid index...because this can potentially crash if the index is > rewards.count - 1

Copy link
Contributor Author

@ifbarrera ifbarrera May 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the dataSource is populated with rewards, I'd say it's highly unlikely that we would ever have an out of range exception. I'd err on the side of "let's keep an eye on it" and address the underlying issue if it comes up, since that would be a symptom of a more serious bug somewhere else in this view model if that were to happen.


self.goToPledge = Signal.combineLatest(self.configureWithProjectProperty.signal.skipNil(),
selectedReward,
self.configureWithRefTagProperty.signal)
.map { project, reward, refTag in
return GoToPledgeData(project: project, reward: reward, refTag: refTag)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A suggestion here: we usually use names starting with goTo to define an action that brings the user to a different screen in the app. Maybe PledgeData(project:reward:refTag) would be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep that's better, thanks for the suggestion!

}
}

private let configureWithProjectProperty = MutableProperty<Project?>(nil)
Expand All @@ -36,11 +51,17 @@ RewardsCollectionViewModelInputs, RewardsCollectionViewModelOutputs {
self.configureWithRefTagProperty.value = refTag
}

private let rewardSelectedIndexProperty = MutableProperty<Int?>(nil)
public func rewardSelected(at index: Int) {
self.rewardSelectedIndexProperty.value = index
}

private let viewDidLoadProperty = MutableProperty(())
public func viewDidLoad() {
self.viewDidLoadProperty.value = ()
}

public let goToPledge: Signal<GoToPledgeData, NoError>
public let reloadDataWithRewards: Signal<[Reward], NoError>

public var inputs: RewardsCollectionViewModelInputs { return self }
Expand Down
31 changes: 31 additions & 0 deletions Library/ViewModels/RewardsCollectionViewModelTests.swift
Expand Up @@ -9,11 +9,17 @@ import ReactiveExtensions_TestHelpers
final class RewardsCollectionViewModelTests: TestCase {
private let vm: RewardsCollectionViewModelType = RewardsCollectionViewModel()

private let goToPledgeProject = TestObserver<Project, NoError>()
private let goToPledgeReward = TestObserver<Reward, NoError>()
private let goToPledgeRefTag = TestObserver<RefTag?, NoError>()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed these are not in alphabetical order 😅 (Ref < Rew)

private let reloadDataWithRewards = TestObserver<[Reward], NoError>()

override func setUp() {
super.setUp()

self.vm.outputs.goToPledge.map { $0.project }.observe(self.goToPledgeProject.observer)
self.vm.outputs.goToPledge.map { $0.reward }.observe(self.goToPledgeReward.observer)
self.vm.outputs.goToPledge.map { $0.refTag }.observe(self.goToPledgeRefTag.observer)
self.vm.outputs.reloadDataWithRewards.observe(self.reloadDataWithRewards.observer)
}

Expand All @@ -33,4 +39,29 @@ final class RewardsCollectionViewModelTests: TestCase {

XCTAssertTrue(value?.count == rewardsCount)
}

func testGoToPledge() {
let project = Project.cosmicSurgery

self.vm.inputs.configure(with: project, refTag: .activity)
self.vm.inputs.viewDidLoad()

self.goToPledgeProject.assertDidNotEmitValue()
self.goToPledgeReward.assertDidNotEmitValue()
self.goToPledgeRefTag.assertDidNotEmitValue()

self.vm.inputs.rewardSelected(at: 0)

self.goToPledgeProject.assertValues([project])
//swiftlint:disable:next force_unwrapping
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//swiftlint:disable:next force_unwrapping
// swiftlint:disable:next force_unwrapping

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we can even get rid of the force unwrap by doing

self.goToPledgeReward.assertValues([project.rewards[0]])

and

self.goToPledgeReward.assertValues([project.rewards[0], project.rewards[project.rewards.count - 1]])

bellow

self.goToPledgeReward.assertValues([project.rewards.first!])
self.goToPledgeRefTag.assertValues([.activity])

self.vm.inputs.rewardSelected(at: project.rewards.endIndex - 1)

self.goToPledgeProject.assertValues([project, project])
//swiftlint:disable:next force_unwrapping
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//swiftlint:disable:next force_unwrapping
// swiftlint:disable:next force_unwrapping

self.goToPledgeReward.assertValues([project.rewards.first!, project.rewards.last!])
self.goToPledgeRefTag.assertValues([.activity, .activity])
}
}