Add event logging upload #127

jkmassel · 2020-03-23T23:27:04Z

Adds support for uploading encrypted logs.

Assigned to both @oguzkocer and @shiki, though either (or both) can approve and provide comments as desired.

In the scope of review, I'd love if you're able to comment specifically on whether:

All public method documentation is easy to read and understand.
The use of preconditions + Implicitly Unwrapped Optionals in EventLoggingUploadManager seems reasonable. Not providing a delegate and a dataSource is a programmer error, so I used these to avoid a bunch of runtime error handling code that should never be called anyway.

To Test:

Ensure CI passes
Ensure tests pass locally

Improve documentation and error handling . Remove extras Revert CrashLogging changes Don’t make the internal explicit Tiny fixes Fix docs

oguzkocer

@jkmassel My iOS chops are not up to par, so I have a bunch of questions with some of them being possibly stupid. I wanted to get @shiki to review my comments, but he said he was swamped this week. So if anything doesn't make sense to either one of us, we can do a rain dance and conjure him for help.

I skipped reviewing the tests now so I can do them separately once the implementation is all figured out. I hope that's OK with you.

Please let me know if you have any questions.

Automattic-Tracks-iOS/Services/EventLoggingNetworkService.swift

Automattic-Tracks-iOS/Model/UploadError.swift

Automattic-Tracks-iOS/Services/EventLoggingUploadQueue.swift

Automattic-Tracks-iOS/Event Logging/EventLoggingUploadManager.swift

Automattic-Tracks-iOS/Event Logging/EventLoggingDelegate.swift

This makes it more consistent

We can’t guarantee it _won’t_ need to be translated.

Reworks EventLoggingUploadQueue to allow additional customization without subclassing.

Simplifies setup for this class by providing a dataSource and delegate at initialization

Reworks the EventLoggingUploadQueue to avoid exposing file parsing bits. Adds a test to cover the changes

shiki

Hey! Sorry for the delay on this. I was able to do a quick look. @oguzkocer had some really great feedback that helped improve the code. So, I just have some tiny suggestions and comments.

It'd also be great if we can add comments to the public symbols like EventLoggingDataSource and EventLoggingDelegate. Comments like how they all connect together or how they should be used would be great. I suppose this could be in future iterations.

Automattic-Tracks-iOS/Event Logging/EventLoggingUploadManager.swift

Automattic-Tracks-iOS/Event Logging/EventLoggingDelegate.swift

Automattic-Tracks-iOS/Event Logging/EventLoggingUploadManager.swift

Automattic-Tracks-iOS/Services/EventLoggingNetworkService.swift

Automattic-Tracks-iOSTests/Test Helpers/Extensions.swift

Automattic-Tracks-iOSTests/Tests/Event Logging/EventLoggingNetworkServiceTests.swift

The RW style guide suggests it this way: https://github.com/raywenderlich/swift-style-guide/#constants

jkmassel · 2020-03-26T22:42:20Z

It'd also be great if we can add comments to the public symbols like EventLoggingDataSource and EventLoggingDelegate. Comments like how they all connect together or how they should be used would be great. I suppose this could be in future iterations.

I agree – I'll do this as part of the next PR – the instructions on how to use them make more sense in the context of the wrapper class for this functionality, so I think it'd be hard to review here.

shiki

Looking good. I'm letting @oguzkocer have the final say since he's the lead reviewer. 🙂

Automattic-Tracks-iOS/Services/EventLoggingUploadQueue.swift

Automattic-Tracks-iOS/Event Logging/EventLoggingDelegate.swift

oguzkocer

@jkmassel The implementation part of this PR turned out really well, thanks for taking the time to polish things up!

I left some non-blocking comments for the tests with some examples of how I like to write these type of tests hoping that you find it useful. Feel free to cherry pick them, implement them yourself, merge add/event-logging-upload-pr-suggestions branch which contains all 3 commits or completely ignore them and merge this PR as is. :)

oguzkocer · 2020-03-27T19:27:19Z

Automattic-Tracks-iOSTests/Test Helpers/Mocks.swift

+typealias LogFileCallback = (LogFile) -> ()
+typealias ErrorWithLogFileCallback = (Error, LogFile) -> ()
+
+enum MockError: Error {


Should we use a more specific name for the file? Since I can't comment on the file itself, I am adding it here.

oguzkocer · 2020-03-27T20:39:18Z

Automattic-Tracks-iOSTests/Test Helpers/Mocks.swift

+    case generic
+}
+
+class MockEventLoggingDataSource: EventLoggingDataSource {


I think the way this class is written is totally fine for tests, but here is how I would have written it myself: 8723261.

The idea is to promote immutability and control how this class can be used. It has 0 value in this PR in practice, but I thought I'd take the time to mention this in a couple places in this PR for future reference, only if you find it useful of course.

oguzkocer · 2020-03-27T20:40:09Z

Automattic-Tracks-iOSTests/Test Helpers/Mocks.swift

+    }
+}
+
+class MockEventLoggingDelegate: EventLoggingDelegate {


Very similar to my last comment, here is how I'd have written this class: c3debf1

oguzkocer · 2020-03-27T20:43:32Z

Automattic-Tracks-iOSTests/Tests/Event Logging/EventLoggingUploadManagerTests.swift

+import XCTest
+@testable import AutomatticTracks
+
+class EventLoggingUploadManagerTests: XCTestCase {


In my previous PR review I tried to explain my thought process for property usage, but it was hard to do so without examples, so I took the time to create one for this PR: f7d87b2.

Similar to my previous PRs it doesn't have much value in tests, but I hope that it'll at least present a good example of how to work with immutability in tests.

There are a couple other files with very minor possible improvements similar to these, but I am leaving these because it's much of the same thing and mentioning these from here after won't have any example value either.

jkmassel · 2020-03-30T19:48:41Z

@oguzkocer – I like the immutability suggestions for the tests – want to open another PR for those changes, and I can approve that one?

oguzkocer · 2020-04-01T23:05:17Z

@jkmassel Looks like develop and that suggestion branch diverged quite a bit, so it creates a big merge conflict. Since the goal of that branch was mostly showcase a different way to go about things, and considering you're leaving for parental leave today :), I think it's fine to just let it die.

jkmassel added the enhancement label Mar 23, 2020

jkmassel requested a review from shiki March 23, 2020 23:27

jkmassel self-assigned this Mar 23, 2020

jkmassel and others added 2 commits March 23, 2020 17:32

Implement Event Logging

d1cd9c9

Improve documentation and error handling . Remove extras Revert CrashLogging changes Don’t make the internal explicit Tiny fixes Fix docs

Bump the Podspec to Swift 5

7dafc39

jkmassel force-pushed the add/event-logging-upload branch from 155f5b1 to 7dafc39 Compare March 23, 2020 23:33

jkmassel marked this pull request as ready for review March 23, 2020 23:36

jkmassel requested a review from oguzkocer March 23, 2020 23:37

oguzkocer reviewed Mar 25, 2020

View reviewed changes

jkmassel added 3 commits March 25, 2020 16:22

Crash if event network response is invalid type

4f20dc6

Inject URLSession in EventLoggingNetworkService

fac0f65

Move NetworkResultCallback inside the class

b3ce8fe

This makes it more consistent

jkmassel force-pushed the add/event-logging-upload branch from 09af253 to 4435485 Compare March 25, 2020 22:29

Rename UploadError to EventLoggingFileUploadError

6b135c5

jkmassel force-pushed the add/event-logging-upload branch from 4435485 to 6b135c5 Compare March 25, 2020 22:30

jkmassel added 4 commits March 25, 2020 16:32

Fix code formatting

01408c7

Add localization for missing files error message

ac75328

We can’t guarantee it _won’t_ need to be translated.

Use current instance of FileManager in extensions

de728bd

Allow injecting storageDirectory and fileManager

810b5c9

Reworks EventLoggingUploadQueue to allow additional customization without subclassing.

jkmassel force-pushed the add/event-logging-upload branch from 1c3b452 to 810b5c9 Compare March 25, 2020 23:00

jkmassel added 4 commits March 25, 2020 17:08

Fix some style issues

92afe2d

Rework EventLoggingUploadManager initialization

4ea3255

Simplifies setup for this class by providing a dataSource and delegate at initialization

Don’t leak LogFile abstractions when reading

0f92472

Reworks the EventLoggingUploadQueue to avoid exposing file parsing bits. Adds a test to cover the changes

Document delegate ordering

5e34b4e

jkmassel requested a review from oguzkocer March 26, 2020 15:19

jkmassel added 2 commits March 26, 2020 09:45

Add direct tests for EventLoggingNetworkService

d0b44f9

Test style fixes

4f2808a

shiki reviewed Mar 26, 2020

View reviewed changes

jkmassel added 2 commits March 26, 2020 11:42

Use an enum instead of a struct

0c5b5bf

The RW style guide suggests it this way: https://github.com/raywenderlich/swift-style-guide/#constants

Avoid mutability in EventLoggingUploadManager

a970df2

jkmassel added 6 commits March 26, 2020 12:05

Rename async_test to waitForExpectation

5da3755

Merge branch 'develop' into add/event-logging-upload

7458a4b

Fix Swiftlint nits

ffd68af

Remove open modifier

a8fb2c0

Improve inline documentation for force-unwrap

1d9e9f8

One last Swiftlint nit

8cc85d7

shiki reviewed Mar 27, 2020

View reviewed changes

Automattic-Tracks-iOS/Services/EventLoggingUploadQueue.swift Outdated Show resolved Hide resolved

Automattic-Tracks-iOS/Event Logging/EventLoggingDelegate.swift Show resolved Hide resolved

oguzkocer approved these changes Mar 27, 2020

View reviewed changes

jkmassel merged commit c7bfcc6 into develop Mar 30, 2020

jkmassel deleted the add/event-logging-upload branch March 30, 2020 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add event logging upload #127

Add event logging upload #127

jkmassel commented Mar 23, 2020 •

edited

Loading

oguzkocer left a comment

shiki left a comment

jkmassel commented Mar 26, 2020

shiki left a comment

oguzkocer left a comment

oguzkocer Mar 27, 2020

oguzkocer Mar 27, 2020

oguzkocer Mar 27, 2020

oguzkocer Mar 27, 2020

jkmassel commented Mar 30, 2020

oguzkocer commented Apr 1, 2020

Add event logging upload #127

Add event logging upload #127

Conversation

jkmassel commented Mar 23, 2020 • edited Loading

oguzkocer left a comment

Choose a reason for hiding this comment

shiki left a comment

Choose a reason for hiding this comment

jkmassel commented Mar 26, 2020

shiki left a comment

Choose a reason for hiding this comment

oguzkocer left a comment

Choose a reason for hiding this comment

oguzkocer Mar 27, 2020

Choose a reason for hiding this comment

oguzkocer Mar 27, 2020

Choose a reason for hiding this comment

oguzkocer Mar 27, 2020

Choose a reason for hiding this comment

oguzkocer Mar 27, 2020

Choose a reason for hiding this comment

jkmassel commented Mar 30, 2020

oguzkocer commented Apr 1, 2020

jkmassel commented Mar 23, 2020 •

edited

Loading