Exposure submission coordinator #910

johannesrohwer · 2020-07-15T14:35:46Z

This PR refactors the exposure submission flow to use a coordinator instead of individual segues. To test the changes, I recommend to mock the ExposureSubmissionService that is provided to the coordinator in the HomeInteractor which allows testing all screens even with limited access to the backend.

Checklist

This PR does not change text that hasn't been signed off with the RKI. Have a look at the pinned issue 440
Test your changes as thoroughly as possible before you commit them. Preferably, automate your test by unit/integration tests.
Set a speaking title. Format: {task_name} (closes #{issue_number}). For example: Use logger (closes # 41)
Make sure that your PR is not introducing unnecessary reformatting (e.g., introduced by on-save hooks in your IDE)

Description

…nator

melloskitten

Hi! Thanks for this major refactor of the navigation flow in the exposure submission part 👍 . Please take a look at some of my comments.

melloskitten · 2020-07-16T08:35:11Z

src/xcode/ENA/ENA/Source/Scenes/ExposureSubmission/ExposureSubmissionCoordinator.swift

+	// MARK: - Navigation.
+
+	/// Starts the coordinator and displays the initial root view controller.
+	func start(with result: TestResult?)


where's the "start with intro screen" part? in case it is handled by the same method, make sure to mention it here.

I agree, this is misleading! I will add a comment to clarify this behavior

melloskitten · 2020-07-16T08:37:27Z

src/xcode/ENA/ENA/Source/Scenes/ExposureSubmission/ExposureSubmissionCoordinator.swift

+}
+
+/// Concrete implementation of the ExposureSubmissionCoordinator protocol.
+class ESCoordinator: ExposureSubmissionCoordinator {


not a fan of the naming here, to be honest! Is there any better way you could name the protocol and the concrete implementation class?

maybe something like:

protocol: ExposureSubmissionCoordinating

implementation: ExposureSubmissionCoordinator

If you want to set ExposureSubmissionCoordinatorViewController as a Marker protocol only for view controller, you can use this syntax:

protocol ExposureSubmissionCoordinatorViewController: UIViewController { var coordinator: ExposureSubmissionCoordinator? { get set } }

This constraint makes sure, it can only be used by UIViewController.

Thanks for the proposal, I will remember your tip! However, I was able to completely get rid of the protocol by injecting the coordinator via the initializer 🎉 I guess that is simpler and cleaner than my initial approach

melloskitten · 2020-07-16T08:39:43Z

src/xcode/ENA/ENA/Source/Scenes/ExposureSubmission/ExposureSubmissionCoordinator.swift

+	// MARK: - Helpers.
+
+	private func push(_ vc: UIViewController) {
+		self.navigationController?.pushViewController(vc, animated: true)


which of our views are pushed and which are presented? why is the differentiation needed here?

the default behavior is push. however, the QR Scanner comes with its own navigation controller that needs to be presented. therefore i added the two separate methods.

src/xcode/ENA/ENA/Source/Scenes/ExposureSubmission/ExposureSubmissionCoordinator.swift

melloskitten · 2020-07-16T08:51:03Z

src/xcode/ENA/ENA/Source/Scenes/ExposureSubmission/ExposureSubmissionCoordinator.swift

+				exposureSubmissionService: self.exposureSubmissionService,
+				testResult: result


What is the reason for passing in the exposure submission service like this? I believe since we now have the coordinator, this is not necessary. please double check this.

the coordinator injects the service into the view controller. Am I missing something here?

melloskitten · 2020-07-16T09:06:55Z

...ce/Scenes/ExposureSubmission/View/Controller/ExposureSubmissionQRScannerViewController.swift


+	// MARK: - Attributes.


please leave break inbetween MARK and code. you did it for the rest of the change as well

melloskitten · 2020-07-16T09:07:01Z

...ce/Scenes/ExposureSubmission/View/Controller/ExposureSubmissionQRScannerViewController.swift

 	weak var scannerViewController: ExposureSubmissionQRScannerViewController? {
 		viewControllers.first as? ExposureSubmissionQRScannerViewController
 	}

+	// MARK: - Initializers.


same as above

...ce/Scenes/ExposureSubmission/View/Controller/ExposureSubmissionQRScannerViewController.swift

melloskitten · 2020-07-16T09:07:54Z

...rce/Scenes/ExposureSubmission/View/Controller/ExposureSubmissionTanInputViewController.swift

-					withIdentifier: Segue.labResultsSegue,
-					sender: self
-				)
+				// A TAN always indicates a positive test result.


thanks for the comment 👍👌

melloskitten · 2020-07-16T09:10:02Z

src/xcode/ENA/ENA/Source/Scenes/ExposureSubmission/ExposureSubmissionCoordinator.swift

+	func start(with result: TestResult?)
+	func dismiss()
+
+	func showOverviewScreen()


In MockExposureSubmissionCoordinator you had empty lines between each of those methods. please adjust in a unified way.

haosap

Nice refactoring👍 , please check my comments.

src/xcode/ENA/ENA/Source/Scenes/ExposureSubmission/ExposureSubmissionCoordinator.swift

haosap · 2020-07-16T09:40:39Z

src/xcode/ENA/ENA/Source/Scenes/ExposureSubmission/ExposureSubmissionCoordinator.swift

+}
+
+/// Concrete implementation of the ExposureSubmissionCoordinator protocol.
+class ESCoordinator: ExposureSubmissionCoordinator {


If you want to set ExposureSubmissionCoordinatorViewController as a Marker protocol only for view controller, you can use this syntax:

protocol ExposureSubmissionCoordinatorViewController: UIViewController { var coordinator: ExposureSubmissionCoordinator? { get set } }

This constraint makes sure, it can only be used by UIViewController.

...ce/Scenes/ExposureSubmission/View/Controller/ExposureSubmissionQRScannerViewController.swift

* develop: Improved readability. Changes in accordance with PR feedback. Adjust DynamicTableViewTextViewCell tint color Add more unit tests Add unit tests for DynamicTableViewTextViewCell Impressum + DynamicTableViewTextViewCell changes DynamicTableViewTextViewCell background color fix DynamicTableViewTextViewCell fixes Add interactivity to App Settings Imprint Screen Display underline in ENATanInput when character is invalid.

melloskitten · 2020-07-17T09:22:16Z

src/xcode/ENA/ENA/Source/Scenes/ExposureSubmission/ExposureSubmissionCoordinating.swift

 	weak var delegate: ExposureSubmissionCoordinatorDelegate?
 	weak var parentNavigationController: UINavigationController?
+
+	/// - Note: We keep a weak reference here to avoid a reference cycle.


just a teeny tiny comment: Up in line 62 you wrote NOTE capitalized. It's great that you have added so many comments though, love it! 😊

melloskitten

Hi! I have just found 2 minor things to look at. Would be cool if you ad changed them. Nonetheless, I will approve. Cheers! 👌

melloskitten · 2020-07-17T09:32:09Z

...e/Scenes/ExposureSubmission/View/Controller/ExposureSubmissionTestResultViewController.swift

-	var coordinator: ExposureSubmissionCoordinator?
+	private(set) weak var coordinator: ExposureSubmissionCoordinating?
+	private(set) weak var exposureSubmissionService: ExposureSubmissionService?
+


remove this line here

johannesrohwer added 7 commits July 14, 2020 15:14

chore: initial version of the exposure submission flow using a coordi…

ca0b71f

…nator

chore: remove ExposureSubmissionNavigationControllerDelegate protocol

e69f5af

chore: inject dependencies via init

ae69217

chore: update tests

6b3bbc6

chore: add tests

f7262eb

chore: change test result handling

b2cb849

chore: add comments

14f6390

johannesrohwer added the chore Refactoring label Jul 15, 2020

Merge branch 'develop' into chore/exposure-submission-coordinator

edb3811

johannesrohwer requested review from ChristianKienle, melloskitten and haosap July 15, 2020 14:38

melloskitten suggested changes Jul 16, 2020

View reviewed changes

haosap suggested changes Jul 16, 2020

View reviewed changes

johannesrohwer added 6 commits July 16, 2020 12:42

chore: rename protocol, add comments and rethink (weak) references

d6b1dd6

chore: update tests

f709ea7

chore: remove reference cycle

774ca4c

chore: add strong reference from navigation controller to coordinator

1f9de85

chore: add comment to delegate

595ddcc

johannesrohwer marked this pull request as ready for review July 17, 2020 08:21

johannesrohwer requested a review from a team July 17, 2020 08:21

haosap approved these changes Jul 17, 2020

View reviewed changes

melloskitten reviewed Jul 17, 2020

View reviewed changes

melloskitten approved these changes Jul 17, 2020

View reviewed changes

fix: formatting

32f8ff4

johannesrohwer merged commit 3825e89 into develop Jul 17, 2020

johannesrohwer deleted the chore/exposure-submission-coordinator branch July 17, 2020 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exposure submission coordinator #910

Exposure submission coordinator #910

johannesrohwer commented Jul 15, 2020

melloskitten left a comment

melloskitten Jul 16, 2020

johannesrohwer Jul 16, 2020

melloskitten Jul 16, 2020

johannesrohwer Jul 16, 2020

haosap Jul 16, 2020

johannesrohwer Jul 16, 2020

melloskitten Jul 16, 2020

johannesrohwer Jul 16, 2020

melloskitten Jul 16, 2020

johannesrohwer Jul 16, 2020

melloskitten Jul 16, 2020

melloskitten Jul 16, 2020

melloskitten Jul 16, 2020

melloskitten Jul 16, 2020

haosap left a comment

haosap Jul 16, 2020

melloskitten Jul 17, 2020

melloskitten left a comment

melloskitten Jul 17, 2020

		exposureSubmissionService: self.exposureSubmissionService,
		testResult: result

Exposure submission coordinator #910

Exposure submission coordinator #910

Conversation

johannesrohwer commented Jul 15, 2020

Checklist

Description

melloskitten 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

haosap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melloskitten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment