Fix #4, #70, #71, #86, #87: Introduce data module #85

BenHenning · 2019-09-01T21:21:47Z

First, this introduces a fully functioning data module. It also introduces a lot of constituent pieces in order to make that data module work:

Dagger introduction
PersistentCacheStore
DataProvider framework

Functionally, this results in the Oppia app now properly showing "Welcome to Oppia!" on initial app open, and "Welcome back to Oppia!" upon returning to the app. Both of these states are also properly retained across configuration changes due to Dagger not leading to activities trying to recreate their dependent controllers.

From a testing side, this added new HomeActivity tests which rely on sleeping rather than idling resource. Long-term once #59 is resolved, we'll be able to consolidate the runBlockingTest from Robolectric and idling resource from Espresso into a consistent blocker that waits for real background-executed coroutines to fully resolve before proceeding to continue in tests. In the meantime, this PR updates MainActivity's Espresso behavior to use the AndroidX test orchestrator and properly reset app state across test runs. This is essential to ensure the on-disk cache is removed and reinitialized in each test run.

The Dagger introduction here is loosely based on a combination of https://proandroiddev.com/dagger-2-on-android-the-simple-way-f706a2c597e9 and a custom version of Dagger-Android (https://dagger.dev/android.html). We should eventually document and land on a final Dagger approach, but I like this implementation since it's simpler than Dagger-Android, and maximizes injectability. It introduces application, activity, and fragment scopes to provide more granular control over when dependencies should be recreated. It also introduces the concept of application, activity, and fragment controllers to perform actual logic, keeping activities and fragments essentially thin classes that delegate responsibility to their controllers. View models are also injectable, but I'm not sure whether we want to keep this long-term. In order to hook things up correctly, activities and fragments should extend special injectable base activity/fragment classes. Although this additional inheritance isn't great, it's reasonable if we don't add anything to these base classes. Long-term we might be able to replace this with code generation.

This PR introduces a fully-functioning DataProvider framework. This needs to be fully designed yet, as the current implementation will have value and error channel elision issues when converted to LiveData. However, this is a proposed start that lets us perform complex data pipeline transformations that are guaranteed to execute on background threads, and provides complete Kotlin coroutine interop. The conversion to LiveData also happens at the end, ensuring there's one LiveData per UI subscription (which I noticed was mentioned as a possible best practice somewhere, but I can't recall the source at the moment).

Finally, this PR also introduces an on-disk cache that's also a DataProvider. This cache stores protos and is expected to be a viable long-term replacement to SharedPreferences. I ran into a few issues with doing this in a clean way: in the test activity, we're setting that the user opened the app early on. However, we don't want this to change the on-disk file before it's read, and we don't want the change to notify subscribers. This resulted in added complexity:

An InMemoryCache was introduced to synchronize read/write operations
Write notifications can be silenced in the cache store
The in-memory cache of the store can be 'primed' to ensure that writes which occur before subscriptions trigger a read do not get picked up

This PR introduces reasonable tests for the components being covered here, but they are far from comprehensive. Observable behaviors in HomeActivity are at least tested, where all of the infrastructure introduced here is thoroughly integrated. Over time, these tests should be refined as the infrastructure itself matures. There's also some duplication between the tests that will be easier to clean up once #59 is resolved.

High-level notes: - Data sources are now being referred to as 'DataProviders' - This commit also fixes the build which was broken in a previous PR ('compile' needs to be used for the proto library) - The data source module is referred to as just 'data' - This introduces a few style changes to prohibit wildcard imports - This also normalizes the JDK versions needed across modules, potentially fixing some of the repeated reversions the IDE does for a Java SDK version setting Functional differences: the app now properly shows the correct string upon initial open ("Welcome to Oppia"), and reopening thereafter correctly shows the updated string ("Welcome back to Oppia"). Rotating the device is currently broken without Dagger integration. Overall, this moves all general data provider functionality into the new module, and introduces a PersistentCacheStore to store proto messages on-disk, or load them into memory if they are available. Overall, this commit is introducing a potentially reasonable medium-term solution for data providers and on-disk proto storage. A long-term design should still be determined, but this seems like a good place to start. Tests are currently broken or missing. New TODOs will also be resolved in a later commit.

This is loosely based on both https://github.com/tfcporciuncula/dagger-simple-way and https://dagger.dev/android.html, except this approach is meant to keep activities and fragments as thin as possible and push as much UI presentation and service business logic into Dagger-provided objects as possible. This maximizes Dagger use throughout the codebase, and opens the potential for future code generation for these thin adapter fragments & services. This fixes the rotation bug in the app: rotating upon the initial app open retains the 'Welcome to Oppia' label since the same user app history controller is used in both instances of HomeFragment due to it being singleton scoped and not needing to be recreated. No tests were updated with these changes.

modules & Dagger graph. Update HomeScreenActivityTest to include a new test for the second-app-launch test case. Note that a significant amount of the work going into this commit was around properly setting up tests to switch out their coroutine dispatchers with dispatchers that could be controlled in a test context. Although the affected test suites are passing in this commit, the work isn't quite done yet. Espresso and Robolectric require slightly different mechanisms to block on dispatchers, and with Gradle there isn't an easy way to set up the build graph such that Espresso can do this reliably. A temporary sleep-based workaround wad added for Espresso. Long-term, all tests should use Espresso's idling resource and facilitate proper background thread usage (which also simplifies the Robolectric side--no need to force sequential execution or switch the main thread dispatcher). Otherwise, this commit essentially "Dagger-ifies" the rest of the app. This will become more beneficial over time, especially after the codebase moves to Bazel since we'll be able to more easily swap out modules to change dependency implementations in different contexts.

…d into introduce-data-module

seanlip · 2019-09-02T10:15:51Z

utility/src/main/java/org/oppia/util/data/AsyncDataSubscriptionManager.kt

+
+internal typealias ObserveAsyncChange = suspend () -> Unit
+
+// TODO(BenHenning): Move this to the Dagger graph rather than making it a singleton.


Here and elsewhere (e.g. HomeActivityTest.kt): please link TODOs to issue numbers.

Your comment came before my commit to actually do this. :) Done.

seanlip · 2019-09-02T10:15:57Z

utility/src/main/java/org/oppia/util/data/AsyncDataSubscriptionManager.kt

+    subscriptionMap.enqueue(id, observeChange)
+  }
+
+  /** Ubsubscribes the specified callback function from the specified [DataProvider] ID. */


Nit: Unsubscribes

PersistentCacheStore. This fixes a few issues in PersistentCacheStore and the DataProvider->LiveData conversion (mostly to ensure that duplicate data notifications don't actually result in propagations to the UI). This also includes correct equals, hash code, and toString behavior for AsyncResult. The PersistentCacheStore tests aren't perfect, but they do expose some issues with the current implementation and seem to provide a reasonable breadth in core coverage. It may be worthwhile to eventually rewrite this class into something both cleaner and easier to maintain.

BenHenning · 2019-09-03T07:20:41Z

This is now ready to review. PTAL!

rt4914

Update comments as per JavaDocs.
Other than comments everything is good.
@BenHenning Maybe we can merge this for now and updates comments later on?

utility/src/main/java/org/oppia/util/threading/ConcurrentCollections.kt

rt4914

LGTM

This reverts commit bda2f7c.

BenHenning · 2019-09-03T17:55:50Z

Thanks @rt4914. I'll follow up on your comment separately if needed.

* Initial introduction of the data source module. High-level notes: - Data sources are now being referred to as 'DataProviders' - This commit also fixes the build which was broken in a previous PR ('compile' needs to be used for the proto library) - The data source module is referred to as just 'data' - This introduces a few style changes to prohibit wildcard imports - This also normalizes the JDK versions needed across modules, potentially fixing some of the repeated reversions the IDE does for a Java SDK version setting Functional differences: the app now properly shows the correct string upon initial open ("Welcome to Oppia"), and reopening thereafter correctly shows the updated string ("Welcome back to Oppia"). Rotating the device is currently broken without Dagger integration. Overall, this moves all general data provider functionality into the new module, and introduces a PersistentCacheStore to store proto messages on-disk, or load them into memory if they are available. Overall, this commit is introducing a potentially reasonable medium-term solution for data providers and on-disk proto storage. A long-term design should still be determined, but this seems like a good place to start. Tests are currently broken or missing. New TODOs will also be resolved in a later commit. * Data module dependencies, retrofit and models * Removed kapt from build.gradle in data * Javadoc comments * Introduce one possible integration with Dagger 2. This is loosely based on both https://github.com/tfcporciuncula/dagger-simple-way and https://dagger.dev/android.html, except this approach is meant to keep activities and fragments as thin as possible and push as much UI presentation and service business logic into Dagger-provided objects as possible. This maximizes Dagger use throughout the codebase, and opens the potential for future code generation for these thin adapter fragments & services. This fixes the rotation bug in the app: rotating upon the initial app open retains the 'Welcome to Oppia' label since the same user app history controller is used in both instances of HomeFragment due to it being singleton scoped and not needing to be recreated. No tests were updated with these changes. * Javadoc comments added * Reolved Javadoc mistakes * Renaming test cases * Test cases for TopicService * Made changes to OppiaGaeClient * Made changes to OppiaGaeClient * TopicService test code using Json response * Revert changes in codeStyles/Project.xml * Revert changes in codeStyles/Project.xml * Optimise imports * Check NetworkInterceptor part * Fix part of #9: Introduce Retrofit classroom data handler service (#83) * introduced topic index handler * Call retrofit data handlers from data module * Update topic_index_items.xml * removed extra space * fixed issues * Update AndroidManifest.xml * Removed Ui part. http handlers are included in data module * done changes in handler and data as per the specs * Update TopicSummarytDicts.kt * Update NetworkSettings.kt * added classroom datahandler * renamed corresponding classes to classroom * fixed issues * Update GaeClassroom.kt * removed spaces * Update AndroidManifest.xml * removed unwanted spaces * added space * Resolve typo * NetworkModule introduced * Daggerify NetworkInterceptor * Make retrofit dependency as api * Javadocs and linking of oppia-web to models * Changes in Javadoc, naming and test cases code * Updated TODO comments and NetworkInterceptorTest

BenHenning and others added 5 commits August 26, 2019 20:40

Update build.gradle proto directive to use 'compile' instead of (#81)

d187b56

Merge branch 'introduce-data-module' of github.com:oppia/oppia-androi…

ff819c2

…d into introduce-data-module

BenHenning self-assigned this Sep 1, 2019

BenHenning mentioned this pull request Sep 1, 2019

Fix #5: GAE Abstraction part 1 - Topic Page handler [Blocked: #85] #78

Merged

6 tasks

BenHenning changed the title ~~Fix #4: Introduce data module~~ Fix #4, #70, #71, #86, #87: Introduce data module Sep 1, 2019

seanlip reviewed Sep 2, 2019

View reviewed changes

BenHenning added 3 commits September 2, 2019 13:04

Switch TODOs with references to issues.

09706fc

Fix typo.

bdeb6d4

BenHenning requested review from seanlip, rt4914, droidizer, veena14cs and vinitamurthi September 3, 2019 07:20

seanlip assigned droidizer, rt4914, seanlip, vinitamurthi and veena14cs and unassigned BenHenning Sep 3, 2019

rt4914 reviewed Sep 3, 2019

View reviewed changes

utility/src/main/java/org/oppia/util/threading/ConcurrentCollections.kt Show resolved Hide resolved

rt4914 approved these changes Sep 3, 2019

View reviewed changes

rt4914 merged commit bda2f7c into develop Sep 3, 2019

BenHenning added a commit that referenced this pull request Sep 3, 2019

Revert "Fix #4, #70, #71, #86, #87: Introduce data module (#85)"

5f0d786

This reverts commit bda2f7c.

BenHenning mentioned this pull request Sep 5, 2019

Threading library #13

Closed

BenHenning deleted the introduce-data-module branch June 10, 2020 22:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4, #70, #71, #86, #87: Introduce data module #85

Fix #4, #70, #71, #86, #87: Introduce data module #85

BenHenning commented Sep 1, 2019 •

edited

Loading

seanlip Sep 2, 2019

BenHenning Sep 2, 2019

seanlip Sep 2, 2019

BenHenning Sep 2, 2019

BenHenning commented Sep 3, 2019

rt4914 left a comment

rt4914 left a comment

BenHenning commented Sep 3, 2019


		internal typealias ObserveAsyncChange = suspend () -> Unit

		// TODO(BenHenning): Move this to the Dagger graph rather than making it a singleton.

Fix #4, #70, #71, #86, #87: Introduce data module #85

Fix #4, #70, #71, #86, #87: Introduce data module #85

Conversation

BenHenning commented Sep 1, 2019 • edited Loading

seanlip Sep 2, 2019

Choose a reason for hiding this comment

BenHenning Sep 2, 2019

Choose a reason for hiding this comment

seanlip Sep 2, 2019

Choose a reason for hiding this comment

BenHenning Sep 2, 2019

Choose a reason for hiding this comment

BenHenning commented Sep 3, 2019

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 left a comment

Choose a reason for hiding this comment

BenHenning commented Sep 3, 2019

BenHenning commented Sep 1, 2019 •

edited

Loading