Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

[Chore] Split background tasks on develop branch #728

Merged
merged 9 commits into from
Jun 25, 2020

Conversation

i336616
Copy link
Contributor

@i336616 i336616 commented Jun 19, 2020

This PR changes the operation of the background tasks.

Previously, the background task only triggered once every 4 hours minimum. On triggering the task, the fetch-test-results API call is executed, and on completion, the exposure-detection API call is executed, effectively executing the two API asynchronous calls in series.

This change splits the execution of the API calls into two separate tasks. This allows the exposure detection call to maintain the higher priority, and adhere to the minimum timing specified by the ExposureNotification framework.

No changes have been made to the logic to determine whether to bypass the API calls, which are still driven by the RiskProvider class (for the exposure-detection API call) and the ENAExposureSubmissionService class (for the fetch-test-results API call)

- Added separate task for fetch-test-results back into app
- Use generic $(PRODUCT_BUNDLE_IDENTIFIER) taskID in Info.plist
- Use Bundle.main.bundleIdentifier as prefix in ENATaskScheduler
- Ensure execute functions are handled separately
…it-background-tasks-on-develop-branch

* fix/split-background-tasks-on-development:
  Use generic $(PRODUCT_BUNDLE_IDENTIFIER) taskID in Info.plist
  Split out execution of background tasks into separate tasks: - Added separate task for fetch-test-results back into app - Use generic $(PRODUCT_BUNDLE_IDENTIFIER) taskID in Info.plist - Use Bundle.main.bundleIdentifier as prefix in ENATaskScheduler - Ensure execute functions are handled separately

# Conflicts:
#	src/xcode/ENA/ENA/Source/Models/Task Scheduling/ENATaskScheduler.swift
@i336616 i336616 marked this pull request as ready for review June 19, 2020 15:18
@i336616 i336616 requested a review from a team June 19, 2020 15:18
@inf2381 inf2381 changed the title Fix/split background tasks on develop branch [Chore] Split background tasks on develop branch Jun 20, 2020
@inf2381 inf2381 added the chore Refactoring label Jun 22, 2020
* develop:
  fix wrong url
  Bring back non-breakable whitespaces
  Removed duplicate table entry
  further clarification
  Prominently add feature requests target
  Add visible not about closing PRs w/o issue assigned
  Add explicit link to cwa-wishlist repo
  Clarify contribution guidelines
  [INTERNAL] Translation delivery: commit by LX Lab
  [INTERNAL] Translation delivery: commit by LX Lab
  fixed risk view
  [INTERNAL] Translation delivery: commit by LX Lab
Copy link
Member

@johannesrohwer johannesrohwer left a comment

Choose a reason for hiding this comment

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

Splitting the background task into two separate ones makes the code a lot easier to understand. Thanks!

@@ -62,12 +66,18 @@ final class ENATaskScheduler {

func scheduleTasks() {
scheduleTask(for: .primaryBackgroundTask, cancelExisting: true)
scheduleTask(for: .secondaryBackgroundTask, cancelExisting: true)
}

func cancelTasks() {
BGTaskScheduler.shared.cancelAllTaskRequests()
Copy link
Member

@melloskitten melloskitten Jun 23, 2020

Choose a reason for hiding this comment

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

If you cancel all task requests, doesn't it mean that you would be canceling the secondaryBackgroundTask from the primaryBackgroundTask and vice versa? Do you really want this?
Nevermind, I misread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Member

@melloskitten melloskitten left a comment

Choose a reason for hiding this comment

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

Just a few minor questions, the rest looks fine to me :)

Renamed secondaryBackgroundTask to fetchTestResults
Fixed typo for taskHander to taskHandler
@melloskitten
Copy link
Member

Looks awesome! Thanks a lot! 🎈 🥳

Comment on lines +7 to +8
<string>$(PRODUCT_BUNDLE_IDENTIFIER).exposure-notification</string>
<string>$(PRODUCT_BUNDLE_IDENTIFIER).fetch-test-results</string>
Copy link
Member

Choose a reason for hiding this comment

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

why do you replace it with the environment var?

Copy link
Member

Choose a reason for hiding this comment

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

i guess its done to make sure there are no collisions with different app versions, e.g. one that was installed via test flight vs. a debug build? (e.g. the debug target uses de.rki.coronawarnapp-dev, production targets use de.rki.coronawarnapp)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It allows us to use the app bundle ID as the prefix for the identifier. In the code, we use Bundle.main.bundleIdentifier as the prefix. This means that we can deploy either app targets during dev (eg from 'de.rki.coronawarnapp' to 'de.rki.coronawarnapp-dev'), and ensure that background tasks from one instance will not collide with the other app's background task identifiers. It's also the method used by Apple's sample code for exposure notifications.

* develop: (39 commits)
  Fixed the height of Update button on Exposure Detection screen.
  Apply ENAFooterView to invite friends controller.
  Added full ENAButton dynamic type support.
  Removed ENACloneButton.
  Fixed lost navigation item observer when dismissing modal presentation.
  Minor fix.
  Disabled next button when sharing keys.
  Fixed ENATanInput bottom line visibility when entering text.
  Fixed fragile TAN input first responder handling for error state.
  Fixed spinner animation when injecting.
  Updated tests.
  Fixed IB warning.
  Improved share text.
  Add missing label
  Update pr guidelines
  Switch to grenrc.js
  Update gren config
  Bump version
  Integrate gren for release notes
  Removed deprecated file.
  ...
@i336616 i336616 merged commit 5fbef4e into develop Jun 25, 2020
@i336616 i336616 deleted the fix/split-background-tasks-on-develop-branch branch June 25, 2020 07:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
chore Refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants