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

rt4914 · 2019-08-27T18:25:53Z

Explanation

The PR contains data module, necessary libraries for API calls and Topic Management System models, retrofit instance and interceptor.

Also this PR was earlier on #61 and #75

Libraries Used

Retrofit: implementation 'com.squareup.retrofit2:retrofit:2.5.0'
This library is responsible for making API calls to Oppia backend
Retrofit Moshi Converter: implementation 'com.squareup.retrofit2:converter-moshi:2.5.0'
This library is the actual deserializer that we are using over GSON.
Moshi Kotlin Support: implementation"com.squareup.moshi:moshi-kotlin:1.8.0"
Moshi Kotlin support library for using Moshi in Kotlin.

Checklist

Work items:

Identify among all existing GAE endpoints (get/post), which we can use during the prototype
Loosely identify data missing from the above endpoints (this bucket is expected to be small/non-existent), and identify which endpoints have data which needs to be changed for the Android app
Specify the Retrofit interfaces to represent all of the identified endpoints (simple interfaces/no annotations or return types needed beyond pseudocode)
Identify** what data (via pseudocode data structures/YAML/JSON/etc) we will be passing along from the backend through these endpoints
Identify how error handling will work (e.g. what Retrofit does), whether RPC retry is supported & how it is/can be configured, etc.
Determine the testing strategy for the GAE endpoints for downstream app components

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.

rt4914 · 2019-08-27T18:27:10Z

@BenHenning PTAL. This PR should help @veena14cs in topic-index-handler as well as I will use this branch for my future PRs for other APIs.

BenHenning

Thanks @rt4914! I did a full review, though I'm expecting the tests to change some per my comment about testing via public APIs.

Generally, the PR looks really good! I think one more thorough pass will be sufficient to get this checked in since I'm especially interested in seeing tests that we can use as canonical examples for all future HTTP test code. Please let me know if you have any questions, concerns, or doubts about any of my comments or the PR.

BenHenning · 2019-08-28T06:26:23Z

data/src/main/java/org/oppia/data/Constants.kt

+/** Constant values for data module will be defined here */
+object Constants {
+
+  const val RESPONSE_SUCCESS = 200


Unless this is going to be processed outside of this file, we could just add it as a module-level constant to the interceptor.

Please also add a Javadoc to it, or name it something self documenting like HTTP_OK.

Unable to understand your first line. Do you mean I should declare this constant in gradle.properties ?

Also, I have renamed RESPONSE_SUCCESS oto HTTP_OK

BenHenning · 2019-08-28T06:26:59Z

data/src/main/java/org/oppia/data/backends/gae/NetworkInterceptor.kt

+   * @param rawJson: This is the string that we get in body of our response
+   * @return String: rawJson without XSSI_PREFIX
+   */
+  fun removeXSSIPrefixFromResponse(rawJson: String): String {


Since this is a helper method, let's make it private since it shouldn't be accessed outside of this class.

When testing the interceptor, we should only test it from its public API to have parity with how other classes will interact with it. In this case, we should use intercept() to test that the XSSI prefix is properly removed.

Is there a way to use Retrofit in tests? https://riggaroo.co.za/retrofit-2-mocking-http-responses/ suggests that this is possible.

This is actually a great opportunity to demonstrate how to mock HTTP responses in Retrofit, since that will be the foundation for our later downstream fakes that we use to support the HTTP side of the entire app infrastructure for UI tests.

BenHenning · 2019-08-28T06:30:01Z

data/src/main/java/org/oppia/data/backends/gae/NetworkInterceptor.kt

+
+/**
+ * Interceptor on top of Retrofit to modify requests and response
+ * The Interceptor intercepts requests and response and in every response


Please wrap Javadoc comments to the 120 character limit, or put new lines between Javadoc paragraphs.

@BenHenning PTAL.

BenHenning · 2019-08-28T06:32:14Z

data/src/main/java/org/oppia/data/backends/gae/OppiaGaeClient.kt

+        .client(client.build())
+        .build()
+    }
+    return@lazy retrofit


Out of curiosity, do we actually need to use return@lazy here? If so, why? I haven't needed that yet for lazy initialization. Do you know what it does?

Actually, in my case it was giving error if I use return retrofit and the solution for that was this.

BenHenning · 2019-08-28T06:32:54Z

data/src/main/java/org/oppia/data/backends/gae/OppiaGaeClient.kt

+  private var retrofit: Retrofit? = null
+
+  val retrofitInstance: Retrofit? by lazy {
+    if (retrofit == null) {


Is this additional logic needed? Can't we just initialize Retrofit lazily? I don't completely follow why we need this dual lazy logic.

Check the updated code for this.

BenHenning · 2019-08-28T06:39:29Z

data/src/main/java/org/oppia/data/backends/gae/model/StorySummary.kt

+
+/**
+ * Model for serialization/deserialization using Moshi with Retrofit
+ * @see <a href="https://github.com/oppia/oppia/blob/b33aa9cf1aa6372e12d0b35f95cceb44efe5320f/core/domain/story_domain.py#L1118">Oppia-Web StorySummary structure</a>


Two things regarding these links:

Since Kotlin uses markdown for Javadocs, you can make this a bit shorter by avoiding the href anchor tag

You can shorten these blob links by just using the first 6 hash characters, e.g. https://github.com/oppia/oppia/blob/b33aa9/core/domain/story_domain.py#L1118.

Ditto elsewhere in this PR.

I tried this @see [link](https://github.com/oppia/oppia/blob/b33aa9/core/domain/story_domain.py#L1118) , but it was not working. Can you guide me this?

Changed blob links to 6 hash characters only.

BenHenning · 2019-08-28T06:39:49Z

data/src/main/java/org/oppia/data/backends/gae/model/StorySummary.kt

+import com.squareup.moshi.JsonClass
+
+/**
+ * Model for serialization/deserialization using Moshi with Retrofit


Let's describe what this model actually is, rather than how it should be used. Ditto elsewhere in this PR.

Changed the description for all models

BenHenning · 2019-08-28T06:40:25Z

data/src/main/java/org/oppia/data/backends/gae/model/StorySummary.kt

+ * @see <a href="https://github.com/oppia/oppia/blob/b33aa9cf1aa6372e12d0b35f95cceb44efe5320f/core/domain/story_domain.py#L1118">Oppia-Web StorySummary structure</a>
+ */
+@JsonClass(generateAdapter = true)
+data class StorySummary(


I believe at some point we discussed suffixing all of these GAE models with 'Gae' to help avoid conflicts with future domain models that these will be converted through. Is that still the case? If so, let's go ahead and make that change to these model classes.

Added Gae prefix to all models

BenHenning · 2019-08-28T06:41:48Z

data/src/main/java/org/oppia/data/backends/gae/model/Topic.kt

+  @Json(name = "topic_name") val topicName: String?,
+  @Json(name = "canonical_story_dicts") val canonicalStoryDicts: List<StorySummary?>?,
+  @Json(name = "additional_story_dicts") val additionalStoryDicts: List<StorySummary?>?,
+  /** skill_descriptions map has skill id as key and skill name as value */


Consider phrasing this instead as something like:

/** A map of skill descriptions keyed by skill ID. */

That way the documentation is describing the property, and still includes the important bit about how the map is keyed & what it contains.

Ditto for degreesOfMastery below.

BenHenning · 2019-08-28T06:44:11Z

data/src/test/java/org/oppia/data/NetworkInterceptorTest.kt

+class NetworkInterceptorTest {
+
+  @Test
+  fun testNetworkInterceptor_removeXSSIPrefixFromResponse_withXSSIPREFIX() {


Nit on naming: when camel casing with fully capitalized names, you can lowercase all the letters after the first one (see https://google.github.io/styleguide/javaguide.html#s5.3-camel-case). In this case:

testNetworkInterceptor_removeXssiPrefixFromResponse_withXssiPrefix

Also, test names can be clearer when phrased as:

testAction_withOneCondition_andOtherCondition_hasExpectedOutcome

Your name is close to this, but consider the alternative:

testNetworkInterceptor_withXssiPrefix_removesXssiPrefix

If all tests are phrased this way, it will be really easy to see which behaviors are being tested which can help identify missing cases for more complex classes.

@BenHenning Thanks a lot. This makes sense.

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.

…e-abstraction-base

rt4914

@BenHenning I have replied to most of your comments and made changes accordingly. I will now work on Retrofit Test cases

rt4914 · 2019-08-28T07:14:50Z

data/src/main/java/org/oppia/data/Constants.kt

+/** Constant values for data module will be defined here */
+object Constants {
+
+  const val RESPONSE_SUCCESS = 200


Unable to understand your first line. Do you mean I should declare this constant in gradle.properties ?

Also, I have renamed RESPONSE_SUCCESS oto HTTP_OK

rt4914 · 2019-08-28T07:23:41Z

data/src/main/java/org/oppia/data/backends/gae/NetworkInterceptor.kt

+
+/**
+ * Interceptor on top of Retrofit to modify requests and response
+ * The Interceptor intercepts requests and response and in every response


@BenHenning PTAL.

rt4914 · 2019-08-28T07:31:01Z

data/src/main/java/org/oppia/data/backends/gae/NetworkSettings.kt

+object NetworkSettings {
+
+  // TODO(#74): Move this to DI graph
+  const val BASE_URL = "https://oppia.org"


I am multiple doubts in this.

DEVELOPER_URL similar to demo app? I haven't seen that anywhere.

For switching I am introduing a boolean value which will switch between developer and prod server.

rt4914 · 2019-08-28T07:39:48Z

data/src/main/java/org/oppia/data/backends/gae/NetworkSettings.kt

+object NetworkSettings {
+
+  // TODO(#74): Move this to DI graph
+  const val BASE_URL = "https://oppia.org"


I have removed BASE_URL and intoduced a fun getBaseUrl() and using boolean value this method switches between the server.

rt4914 · 2019-08-28T07:42:26Z

data/src/main/java/org/oppia/data/backends/gae/NetworkSettings.kt

+object NetworkSettings {
+
+  // TODO(#74): Move this to DI graph
+  const val BASE_URL = "https://oppia.org"


Also, not sure what should be the exact value of PROD and DEVELOPER URL for now.

rt4914 · 2019-08-28T09:11:01Z

data/src/main/java/org/oppia/data/backends/gae/model/StorySummary.kt

+
+/**
+ * Model for serialization/deserialization using Moshi with Retrofit
+ * @see <a href="https://github.com/oppia/oppia/blob/b33aa9cf1aa6372e12d0b35f95cceb44efe5320f/core/domain/story_domain.py#L1118">Oppia-Web StorySummary structure</a>


I tried this @see [link](https://github.com/oppia/oppia/blob/b33aa9/core/domain/story_domain.py#L1118) , but it was not working. Can you guide me this?

Changed blob links to 6 hash characters only.

rt4914 · 2019-08-28T09:12:17Z

data/src/main/java/org/oppia/data/backends/gae/model/StorySummary.kt

+ * @see <a href="https://github.com/oppia/oppia/blob/b33aa9cf1aa6372e12d0b35f95cceb44efe5320f/core/domain/story_domain.py#L1118">Oppia-Web StorySummary structure</a>
+ */
+@JsonClass(generateAdapter = true)
+data class StorySummary(


Added Gae prefix to all models

rt4914 · 2019-08-28T09:20:28Z

data/src/main/java/org/oppia/data/backends/gae/model/Topic.kt

+
+/**
+ * Model for serialization/deserialization using Moshi with Retrofit *
+ * @see <a href="https://github.com/oppia/oppia/blob/b33aa9cf1aa6372e12d0b35f95cceb44efe5320f/core/controllers/topic_viewer.py#L45">Oppia-Web Topic structure</a>


I think it is pointing correctly because the response is a mixture of multiple responses from different function calls.

rt4914 · 2019-08-28T09:21:34Z

data/src/main/java/org/oppia/data/backends/gae/model/Topic.kt

+  @Json(name = "topic_name") val topicName: String?,
+  @Json(name = "canonical_story_dicts") val canonicalStoryDicts: List<StorySummary?>?,
+  @Json(name = "additional_story_dicts") val additionalStoryDicts: List<StorySummary?>?,
+  /** skill_descriptions map has skill id as key and skill name as value */


rt4914 · 2019-08-28T10:19:52Z

data/src/test/java/org/oppia/data/NetworkInterceptorTest.kt

+class NetworkInterceptorTest {
+
+  @Test
+  fun testNetworkInterceptor_removeXSSIPrefixFromResponse_withXSSIPREFIX() {


@BenHenning Thanks a lot. This makes sense.

rt4914 · 2019-08-30T12:23:20Z

@BenHenning PTAL

BenHenning · 2019-09-01T21:22:35Z

NB: This is blocked on #85.

* 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

…-android into gae-abstraction-base

rt4914

@BenHenning PTAL

data/src/test/java/org/oppia/data/MockTopicService.kt

data/src/test/java/org/oppia/data/MockTopicTest.kt

rt4914 · 2019-09-05T07:38:55Z

data/src/test/java/org/oppia/data/MockTopicTest.kt

+  private var retrofit: Retrofit? = null
+
+  @Before
+  @Throws(Exception::class)


I did search this on internet but haven't found a proper answer for this. I have seen different resources which uses this and there are some resources which do not use this.

Even I am not sure, whether we actually need this or not.

rt4914 · 2019-09-05T11:49:43Z

data/src/main/java/org/oppia/data/Constants.kt

+
+  /** Constant which defines successful API call */
+  const val HTTP_OK = 200
+


@BenHenning Not sure how to dagger-ify this.

data/src/main/java/org/oppia/data/backends/gae/NetworkInterceptor.kt

data/src/main/java/org/oppia/data/backends/gae/NetworkModule.kt

app/build.gradle

data/build.gradle

data/src/test/java/org/oppia/data/backends/test/NetworkInterceptorTest.kt

BenHenning

Generally LGTM. Please re-assign to me if you'd like me to do another pass, but as long as you address my comments feel free to merge this.

app/build.gradle

data/build.gradle

data/src/main/java/org/oppia/data/backends/gae/NetworkInterceptor.kt

BenHenning · 2019-09-06T06:44:03Z

data/src/main/java/org/oppia/data/backends/gae/NetworkModule.kt

+import javax.inject.Singleton
+
+/**
+ * Sample resource https://github.com/gahfy/Feed-Me/tree/unitTests


Nit: let's just make this a separate paragraph in the below Javadoc. Currently, this is a floating Javadoc with no associated element. :)

Actually, I'm not sure where this Javadoc should go since this module isn't related to testing directly. Maybe nowhere for now?

I have shifted to this javadoc because this code has been referenced from that link and therefore it is worth mentioning it here for now. And later we can remove this.

data/src/main/java/org/oppia/data/backends/gae/NetworkModule.kt

data/src/main/java/org/oppia/data/backends/gae/model/GaeTopicSummarytDict.kt

data/src/test/java/org/oppia/data/backends/test/MockTopicTest.kt

BenHenning · 2019-09-06T06:52:38Z

data/src/test/java/org/oppia/data/backends/test/MockTopicTest.kt

+
+  @Test
+  @Throws(Exception::class)
+  fun testMockTopicService_withTopicName_success() {


Per the name, this doesn't seem to include the actual action that's being tested. Can you update it to include the operation itself?

Also, it's often more useful to be specific about what's successful (e.g. that the response has the expected topic name). That way we can easily check multiple tests with a similar naming style for thoroughness of behaviors being tested.

Changed this name to testTopicService_usingFakeJson_deserializationSuccessful but still not convinced. I am finding it difficult to name the methods correctly. Will need to see multiple code samples to get proper understanding.

This seems reasonable, though you could probably make it slightly clearer by including what is successful, e.g.:

testService_withFakeJson_deserializesCorrectTopicName

data/src/test/java/org/oppia/data/backends/test/NetworkInterceptorTest.kt

rt4914 · 2019-09-06T09:56:40Z

@BenHenning PTAL

app/build.gradle

data/src/main/java/org/oppia/data/backends/gae/NetworkModule.kt

data/src/test/java/org/oppia/data/backends/test/NetworkInterceptorTest.kt

BenHenning · 2019-09-10T06:39:40Z

data/src/test/java/org/oppia/data/backends/test/MockTopicTest.kt

+
+  @Test
+  @Throws(Exception::class)
+  fun testMockTopicService_withTopicName_success() {


This seems reasonable, though you could probably make it slightly clearer by including what is successful, e.g.:

testService_withFakeJson_deserializesCorrectTopicName

BenHenning

Thanks @rt4914. The recent changes LGTM; left a few comments to improve clarity on a few things.

…base

rt4914 · 2019-09-10T15:32:53Z

This PR is complete but it does need some more test cases to be introduced which will be finished with other PRs related to services.

…nt System [Blocked: #78] (#102) * Subtopic service, model and testcases * Reformatting code * Nit changes

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

Data module dependencies, retrofit and models

ac16d10

Removed kapt from build.gradle in data

bf96010

Javadoc comments

ef62740

rt4914 requested a review from BenHenning August 27, 2019 18:25

rt4914 assigned rt4914 and BenHenning and unassigned rt4914 Aug 27, 2019

rt4914 changed the base branch from develop to introduce-data-module August 27, 2019 18:30

This was referenced Aug 28, 2019

Fix part of #5: Abstraction on top of Oppia GAE for Topic Management System #75

Closed

Fix part of #5: Abstraction on top of Oppia GAE for Topic Management System #61

Closed

BenHenning requested changes Aug 28, 2019

View reviewed changes

BenHenning assigned rt4914 and unassigned BenHenning Aug 28, 2019

BenHenning and others added 6 commits August 28, 2019 00:05

Javadoc comments added

3679c4c

Reolved Javadoc mistakes

a4e0ad5

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

d187b56

Merge remote-tracking branch 'upstream/introduce-data-module' into ga…

d27d161

…e-abstraction-base

Renaming test cases

5d5da22

rt4914 commented Aug 28, 2019

View reviewed changes

Test cases for TopicService

07b51d5

rt4914 assigned BenHenning and unassigned rt4914 Aug 30, 2019

Rajat Talesra added 2 commits August 30, 2019 17:50

Made changes to OppiaGaeClient

a52bb57

Made changes to OppiaGaeClient

1d6c39b

BenHenning changed the title ~~Fix part of #5: GAE Abstraction part 1 - Topic Page handler~~ Fix part of #5: GAE Abstraction part 1 - Topic Page handler [Blocked: #85] Sep 1, 2019

veena14cs and others added 6 commits September 4, 2019 18:02

Resolve typo

ffde992

Merge branch 'gae-abstraction-base' of https://github.com/oppia/oppia…

f5ae749

…-android into gae-abstraction-base

Resolve merge conflicts with develop branch

0049548

NetworkModule introduced

cd94945

Daggerify NetworkInterceptor

e392406

rt4914 changed the base branch from introduce-data-module to develop September 5, 2019 11:48

Rajat Talesra added 2 commits September 5, 2019 17:31

Make retrofit dependency as api

5eccceb

Javadocs and linking of oppia-web to models

1751f36

rt4914 commented Sep 5, 2019

View reviewed changes

rt4914 assigned BenHenning and unassigned rt4914 Sep 5, 2019

BenHenning approved these changes Sep 6, 2019

View reviewed changes

BenHenning assigned rt4914 and unassigned BenHenning Sep 6, 2019

rt4914 mentioned this pull request Sep 6, 2019

Keep test dependencies separate from production dependencies [Blocked: #59] #97

Open

Changes in Javadoc, naming and test cases code

798cf41

rt4914 assigned BenHenning and unassigned rt4914 Sep 6, 2019

BenHenning reviewed Sep 10, 2019

View reviewed changes

BenHenning approved these changes Sep 10, 2019

View reviewed changes

BenHenning assigned rt4914 and unassigned BenHenning Sep 10, 2019

Rajat Talesra added 2 commits September 10, 2019 20:35

Merge remote-tracking branch 'upstream/develop' into gae-abstraction-…

80249a8

…base

Updated TODO comments and NetworkInterceptorTest

bb62037

rt4914 merged commit 689e970 into develop Sep 10, 2019

rt4914 deleted the gae-abstraction-base branch September 13, 2019 08:27

rt4914 added a commit that referenced this pull request Sep 17, 2019

Fix part of #5: Abstraction on top of Oppia GAE for Subtopic Manageme…

cbe9417

…nt System [Blocked: #78] (#102) * Subtopic service, model and testcases * Reformatting code * Nit changes


		/** Constant which defines successful API call */
		const val HTTP_OK = 200

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

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

Conversation

rt4914 commented Aug 27, 2019 • edited Loading

Explanation

Libraries Used

Checklist

rt4914 commented Aug 27, 2019

BenHenning 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

rt4914 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

rt4914 commented Aug 30, 2019

BenHenning commented Sep 1, 2019

rt4914 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHenning 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

rt4914 commented Sep 6, 2019

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

rt4914 commented Sep 10, 2019

rt4914 commented Aug 27, 2019 •

edited

Loading