[wip] Remove Project.LiveStream #63

justinswart · 2017-01-26T15:17:27Z

When we initially began integrating the LiveStream framework we were receiving some information about live streams along with the Project fetch JSON payload. All that this contained was the live stream eventId, a title, startDate, etc. We then, upon launching the LiveStreamContainerViewController or LiveStreamCountdownViewController, would perform an additional fetch for the LiveStreamEvent which would tell us everything else about the event.

This quickly turned out to be a bad idea and there was just too much information being duplicated between Project.LiveStream and LiveStreamEvent so the decision was taken to remove Project.LiveStream in its entirety and to fetch a list of LiveStreamEvents from a new endpoint based on the Project's ID. We perform this fetch when we land on a project page and once the list is returned we reload the project's subpage cells with any live streams that may have been returned.

The amount of files that this touched is a lot more than I'd anticipated! But it's much simpler this was and it should also prepare us for a lot the work that we would have needed to do for discovery.

Things I've done here:

Removed Project.LiveStream from models and tests that referenced it.
Removed fetching of LiveStreamEvent from LiveStreamEventDetailsViewModel.
Changed all initialisers that previously allowed an optional LiveStreamEvent to now expect this as non-optional.
Updated Koala tracking tests. @mbrandonw you'll want to confirm that I correctly understood how your tests were working around the timeout of fetching live streams and tracking that, etc. I believe it's all still working correctly.
Updated all view model tests that had been referencing Project.LiveStream.
Copied across the changes @mbrandonw had made to LiveStreamEvent for the live stream discovery work. This was necessary to parse the new endpoint @Jwomers had set up for us.

Things to review:

Are the tests still sound?
Should I add some more tests?
Is there a clever way to reduce the amount of emissions when the project page loads now that it's a combineLatest of project, freshProject and liveStreams?

Things still needed:

In testing I wasn't seeing any user subscription info, Firebase paths, OpenTok info, replay URLs, etc. from this endpoint. Will you still be adding those @Jwomers?
May still want to write more tests for ProjectPamphletViewModelTests and confirm that we're still satisfied withProjectPamphletContentDataSourceTests (these should be fine).
Kind've regret not changing LiveStreamEvent.startDate to a TimeInterval as I went through this 😐

…sts that will be fixed soon

justinswart · 2017-01-26T17:35:11Z

Library/ViewModels/ProjectPamphletViewModel.swift

+    )
+
+    self.configureChildViewControllersWithProjectAndLiveStreams =
+      Signal.combineLatest(project, liveStreamEvents, refTag)


This is the combineLatest that I mentioned in the PR. This emits quite a few times and each will cause a reload of the datasource and table view, perhaps we can buffer/throttle it in some way?

i think in this case we actually want all of those emissions! for once!

basically we want the most recent values of project and live streams for any emission and update the data source accordingly.

or, we could model it to emit only twice, once for the initial data and once we have both the newest project and newest live streams. however, for that we'll need to add a timeout to the live streams so that it doesnt prevent the project from showing if it fails. wanna discuss over slack how to do that?

mbrandonw

did a first pass review of this. most of it looks great and is so much simpler. let's attack my comments and then i'll do one more pass and we'll get it merged!

mbrandonw · 2017-01-26T16:12:15Z

Kickstarter-iOS/Views/Controllers/LiveStreamContainerViewController.swift

@@ -126,7 +124,7 @@ public final class LiveStreamContainerViewController: UIViewController {

    _  = self.loaderActivityIndicatorView
      |> UIActivityIndicatorView.lens.activityIndicatorViewStyle .~ .white
-      |> UIActivityIndicatorView.lens.animating .~ true
+      |> UIActivityIndicatorView.lens.hidesWhenStopped .~ true


👍 i remember us talking about this. important to do only stylistic stuff in bindStyles since it can be called multiple times (like on rotation)

mbrandonw · 2017-01-26T16:13:55Z

Library/Koala/Koala.swift

    return .replay
  }

  return .countdown
 }

-private func stateContext(forLiveStream liveStream: Project.LiveStream) -> LiveStreamStateContext {


boy does it warm my heart to see this code go away!

mbrandonw · 2017-01-26T16:18:14Z

Library/Tests/ViewModels/ProjectPamphletViewModelTests.swift

@@ -105,16 +114,17 @@ final class ProjectPamphletViewModelTests: TestCase {
    self.setNavigationBarAnimated.assertValues([false, true, false])
  }

-  // Tests that ref tags and referral credit cookies are tracked in koala and saved like we expect.
+   //Tests that ref tags and referral credit cookies are tracked in koala and saved like we expect.


🤔 whatchu doin

mbrandonw · 2017-01-26T17:39:31Z

Library/ViewModels/LiveStreamCountdownViewModel.swift

@@ -213,22 +203,8 @@ LiveStreamCountdownViewModelInputs, LiveStreamCountdownViewModelOutputs {
  public var outputs: LiveStreamCountdownViewModelOutputs { return self }
 }

-private func flipProjectLiveStreamToLive(project: Project, currentLiveStream: Project.LiveStream) ->


mbrandonw · 2017-01-26T17:40:06Z

Library/ViewModels/LiveStreamEventDetailsViewModel.swift

@@ -285,16 +238,3 @@ public final class LiveStreamEventDetailsViewModel: LiveStreamEventDetailsViewMo
  public var inputs: LiveStreamEventDetailsViewModelInputs { return self }
  public var outputs: LiveStreamEventDetailsViewModelOutputs { return self }
 }
-
-private func fetchEvent(forProject project: Project, liveStream: Project.LiveStream, event: LiveStreamEvent?)


mbrandonw · 2017-01-26T17:48:20Z

Library/ViewModels/ProjectPamphletViewModel.swift

-      .map { $0.liveStreams }
-      .skipNil()
+    let trackLiveStreamEvents = liveStreamEvents
+      .skip(first: 1) // Skip first as we are prefixing with an empty array


nice nice, im guess a test caught this right?

mbrandonw · 2017-01-26T17:50:49Z

LiveStream/Models/LiveStreamEventsEnvelope.swift

+  }
+}
+
+public func == (lhs: LiveStreamEventsEnvelope, rhs: LiveStreamEventsEnvelope) -> Bool {


curious if where this is needed...

yeah probably not necessary, remove it?

yeah if it's not used, prob for the best

mbrandonw · 2017-01-26T17:57:47Z

LiveStream/Service/LiveStreamService.swift

+
+      let urlString = "\(Secrets.LiveStreams.Api.base)/projects/\(projectId)\(uidString)"
+      guard let url = URL(string: urlString) else {
+        observer.send(error: .invalidEventId)


oof, so "invalidEventId" isn't quite the right error here

mbrandonw · 2017-01-26T18:00:33Z

LiveStream/Service/LiveStreamService.swift

+        .flatMap { "?uid=\($0)" }
+        .coalesceWith("")
+
+      let urlString = "\(Secrets.LiveStreams.Api.base)/projects/\(projectId)\(uidString)"


this we cant really test this function i'd like to do something a little safer around url construction. we can use URLComponents for the query params

let url = URL(string: Secrets.LiveStreams.Api.base)?.appendingPathComponent("projects/\(projectId)") var components = url.flatMap { URLComponents(url: $0, resolvingAgainstBaseURL: false) } components?.queryItems = uid.map { [URLQueryItem(name: "uid", value: "\($0)")] } guard let url = components?.url ...

did that without the type checker, but something along those lines...

Ok cool, looks good!

mbrandonw · 2017-01-26T18:02:09Z

LiveStream/ViewModels/LiveStreamViewModel.swift

@@ -132,7 +132,7 @@ internal final class LiveStreamViewModel: LiveStreamViewModelType, LiveStreamVie
      isMaxOpenTokViewersReached,

      liveStreamEvent
-        .map { event in event.stream.isRtmp || didEndNormally(event: event) }
+        .map { event in event.isRtmp ?? false || didEndNormally(event: event) }


i'm a big fan of doing explicit event.isRtmp == .some(true) over coalescing for booleans

# Conflicts: # Screenshots/_64/Kickstarter_Framework_iOSTests.ProjectPamphletContentViewControllerTests/testNonBacker_LiveProject_WithLiveStreams_lang_es_device_pad@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.ProjectPamphletContentViewControllerTests/testNonBacker_LiveProject_WithLiveStreams_lang_es_device_phone4_7inch@2x.png

justinswart · 2017-01-27T06:34:24Z

Library/ViewModels/LiveStreamContainerViewModel.swift

@@ -214,14 +214,12 @@ LiveStreamContainerViewModelInputs, LiveStreamContainerViewModelOutputs {

    let hideWhenReplay = Signal.merge(
      project.mapConst(true),
-      event.map { !$0.liveNow },
-      self.showErrorAlert.mapConst(true)


The screenshot tests that were failing were because of this line - previously we would fetch the LiveStreamEvent asynchronously and if it failed we would keep some views hidden. Because this is now available immediately at initialisation this sort of thing is no longer necessary as errors are now only shown for live stream playback issues.

To be honest we probably don't even need the project.mapConst(true) anymore either as we know immediately if it's live or a replay. I'll look at maybe removing that for simplification.

…arter/ios-oss into remove_project_livestream

…nt on configure of view model, updated tests

justinswart · 2017-01-29T17:15:52Z

Library/ViewModels/LiveStreamContainerViewModel.swift

    )

-    self.createAndConfigureLiveStreamViewController = Signal.combineLatest(project, event)
+    self.configureLiveStreamViewController = Signal.combineLatest(project, event.skip(first: 1))


What's the feeling on using .skip(first: 1) here? We know that we only want to configure the live stream view controller once the fresh event has been fetched and our tests confirm this so should be cool?

# Conflicts: # Library/ViewModels/ProjectPamphletSubpageCellViewModel.swift # Screenshots/_64/Kickstarter_Framework_iOSTests.ProjectPamphletContentViewControllerTests/testNonBacker_LiveProject_WithLiveStreams_lang_de_device_pad@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.ProjectPamphletContentViewControllerTests/testNonBacker_LiveProject_WithLiveStreams_lang_de_device_phone4_7inch@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.ProjectPamphletContentViewControllerTests/testNonBacker_LiveProject_WithLiveStreams_lang_en_device_pad@2x.png # Screenshots/_64/Kickstarter_Framework_iOSTests.ProjectPamphletContentViewControllerTests/testNonBacker_LiveProject_WithLiveStreams_lang_en_device_phone4_7inch@2x.png

justinswart · 2017-01-31T11:32:57Z

These tests will pass once #67 is merged.

# Conflicts: # Frameworks/ios-ksapi

justinswart · 2017-01-31T19:17:27Z

Library/ViewModels/LiveStreamContainerViewModel.swift

-      .map { URL(string: $0.photo.full) }
+    self.projectImageUrl = Signal.merge(
+      configData.mapConst(nil),
+      Signal.combineLatest(


@mbrandonw this was a weird one, even though configData is definitely emitting before project (because project is configData.map(first), I couldn't seem to get nil to emit first without merging in that combineLatest?

mbrandonw · 2017-01-31T19:21:14Z

Library/ViewModels/LiveStreamContainerViewModel.swift

+        project.map { URL(string: $0.photo.full) },
+        configData.ignoreValues()
+      ).map(first)
+    )


i think we can just re-express as:

self.projectImageUrl = Signal.merge( self.viewDidLoadProperty.signal.mapConst(nil), project.map { URL(string: $0.photo.full) } )

?

also down to just remove the placeholder image in the storyboard.

So I initially had it that way (with viewDidLoad), and just tested it again, bizarrely:

self.projectImageUrl = Signal.merge( self.viewDidLoadProperty.signal.mapConst(nil), project.map { URL(string: $0.photo.full) } )

I'll debug a little more but will probably just remove it from the story board 😄

justinswart · 2017-02-01T09:31:45Z

Library/ViewModels/LiveStreamContainerViewModel.swift

-    self.projectImageUrl = project
-      .map { URL(string: $0.photo.full) }
+    self.projectImageUrl = project.flatMap { project in
+      SignalProducer(value: URL(string: project.photo.full))


@mbrandonw ok so this fixes it and the tests pass. I know using a SignalProducer might not be the desired approach but it's handy to be able to prefix it with nil and makes what we're wanting to do pretty clear?

I could not get nil to emit first using viewDidLoad without a nested combineLatest which I also didn't like.

And I think having this testable is safer than only removing the image from the story board in case it was ever added back?

justinswart added 16 commits January 24, 2017 10:01

Update KsApi

451d609

Update KsApi

00f05cd

Update KsApi

b7c59f3

Started down the path of removing Project.LiveStream

fba549b

Got Library building, fixed majority of tests and added fixe’s for te…

21ca200

…sts that will be fixed soon

Project building, fixme’s added

0ceb1d9

Refactor continues, fixed some tests, added more fixme’s,

3b35ac4

Copied LiveStreamEvent changes from discovery branch

d985b53

Improved how live streams are fetched on project view load

d7ef2c6

Fix ProjectPamphletViewModelTests

c4eeb26

Fix KoalaTests

363d2a1

Fixed tests, removed redundancies

f696136

Refactor to use LiveStreamEventsEnvelope

a212835

swiftlint

2c4a277

Merge branch 'master' into remove_project_livestream

3820831

Update newer tests that were merged

e71ce75

justinswart assigned mbrandonw Jan 26, 2017

justinswart mentioned this pull request Jan 26, 2017

Remove Project.LiveStream kickstarter/ios-ksapi#9

Merged

Fix secrets example file

588c989

justinswart commented Jan 26, 2017

View reviewed changes

mbrandonw suggested changes Jan 26, 2017

View reviewed changes

justinswart added 4 commits January 26, 2017 21:21

Fix tests, fix some screenshot tests

1f31cc0

Update KsApi

ba9dd21

Fixed screenshot tests

28a713e

justinswart commented Jan 27, 2017

View reviewed changes

mbrandonw added 3 commits January 27, 2017 08:44

Update ksapi and alamofireimage

9a71c1d

swiftlint

f682197

Merge branch 'remove_project_livestream' of https://github.com/kickst…

c63f2d0

…arter/ios-oss into remove_project_livestream

justinswart changed the title ~~Remove Project.LiveStream~~ [wip ]Remove Project.LiveStream Jan 29, 2017

justinswart changed the title ~~[wip ]Remove Project.LiveStream~~ [wip] Remove Project.LiveStream Jan 29, 2017

justinswart added 5 commits January 29, 2017 17:14

Improve URL construction

0c18991

Only configuring live stream view controller after fetching fresh eve…

bcfd4d6

…nt on configure of view model, updated tests

swiftlint

9234854

Merge branch 'master' into remove_project_livestream

41f6aee

Set to the correct ksapi for this branch

3ca407a

justinswart commented Jan 29, 2017

View reviewed changes

justinswart added 4 commits January 30, 2017 11:01

Added screenshot test for when a fresh event fetch fails

a0f1ff6

Remove unnecessary Equatable conformance

2ae7bc2

Fix screenshots (scrollview indicators again)

8161b1b

justinswart and others added 3 commits January 31, 2017 14:53

Ensure that projectImageUrl emits nil first

484015c

Merge branch 'master' into remove_project_livestream

6fb5df1

# Conflicts: # Frameworks/ios-ksapi

paired with justin on some pinging stuff

59d82da

justinswart commented Jan 31, 2017

View reviewed changes

mbrandonw reviewed Jan 31, 2017

View reviewed changes

justinswart added 2 commits February 1, 2017 11:26

Prefer a prefixed SignalProducer approach over the nested combineLatest

4bad2bf

Merge branch 'master' into remove_project_livestream

bddc077

justinswart commented Feb 1, 2017

View reviewed changes

Update KsApi

c3a8717

mbrandonw approved these changes Feb 1, 2017

View reviewed changes

justinswart merged commit eb74738 into master Feb 1, 2017

justinswart deleted the remove_project_livestream branch February 1, 2017 15:51

mbrandonw mentioned this pull request Feb 1, 2017

[wip stretch goal] Live stream discovery #53

Merged

justinswart mentioned this pull request Feb 7, 2017

Live stream subscription state refresh #77

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] Remove Project.LiveStream #63

[wip] Remove Project.LiveStream #63

justinswart commented Jan 26, 2017

justinswart Jan 26, 2017

mbrandonw Jan 26, 2017

justinswart Jan 26, 2017

mbrandonw left a comment

mbrandonw Jan 26, 2017

justinswart Jan 26, 2017

mbrandonw Jan 26, 2017

mbrandonw Jan 26, 2017

justinswart Jan 26, 2017

mbrandonw Jan 26, 2017

mbrandonw Jan 26, 2017

mbrandonw Jan 26, 2017

justinswart Jan 26, 2017

mbrandonw Jan 26, 2017

justinswart Jan 26, 2017

mbrandonw Jan 27, 2017

mbrandonw Jan 26, 2017

mbrandonw Jan 26, 2017

justinswart Jan 26, 2017

mbrandonw Jan 26, 2017

justinswart Jan 27, 2017

justinswart Jan 27, 2017

justinswart Jan 29, 2017

justinswart commented Jan 31, 2017

justinswart Jan 31, 2017

mbrandonw Jan 31, 2017

justinswart Jan 31, 2017

justinswart Feb 1, 2017

[wip] Remove Project.LiveStream #63

[wip] Remove Project.LiveStream #63

Conversation

justinswart commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrandonw 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

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

justinswart commented Jan 31, 2017

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