Fix oppia#5344: Remove temporary functions from TopicListController (o…

…ppia#5528)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation
<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->
  
 Fixes oppia#5344

This PR surfaces the `getClassrooms` and `getClassroomById` functions
from the `ClassroomController`, adds tests to ensure their correctness,
and refactors the `TopicListController` to replace temporary functions
with these new implementations.

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
<!-- Delete these section if this PR does not include UI-related
changes. -->
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

domain/src/main/java/org/oppia/android/domain/classroom/ClassroomController.kt

@@ -72,6 +72,32 @@ class ClassroomController @Inject constructor(
     )
   }
 
+  /**
+   * Returns the list of [ClassroomRecord]s currently available in the app.
+   */
+  fun getClassrooms(): List<ClassroomRecord> {
+    return if (loadLessonProtosFromAssets) {
+      assetRepository.loadProtoFromLocalAssets(
+        assetName = "classrooms",
+        baseMessage = ClassroomIdList.getDefaultInstance()
+      ).classroomIdsList.map { classroomId ->
+        getClassroomById(classroomId)
+      }
+    } else loadClassroomsFromJson()
+  }
+
+  /**
+   * Returns the [ClassroomRecord] associated with the given [classroomId].
+   */
+  fun getClassroomById(classroomId: String): ClassroomRecord {
+    return if (loadLessonProtosFromAssets) {
+      assetRepository.tryLoadProtoFromLocalAssets(
+        assetName = classroomId,
+        defaultMessage = ClassroomRecord.getDefaultInstance()
+      ) ?: ClassroomRecord.getDefaultInstance()
+    } else loadClassroomByIdFromJson(classroomId)
+  }
+
   /**
    * Returns the list of [TopicSummary]s currently tracked by the app, possibly up to
    * [EVICTION_TIME_MILLIS] old.
@@ -90,7 +116,7 @@ class ClassroomController @Inject constructor(
    */
   fun getClassroomIdByTopicId(topicId: String): String {
     var classroomId = ""
-    loadClassrooms().forEach {
+    getClassrooms().forEach {
       if (it.topicPrerequisitesMap.keys.contains(topicId)) {
         classroomId = it.id
       }
@@ -333,17 +359,6 @@ class ClassroomController @Inject constructor(
       .build()
   }
 
-  private fun loadClassrooms(): List<ClassroomRecord> {
-    return if (loadLessonProtosFromAssets) {
-      assetRepository.loadProtoFromLocalAssets(
-        assetName = "classrooms",
-        baseMessage = ClassroomIdList.getDefaultInstance()
-      ).classroomIdsList.map { classroomId ->
-        loadClassroomById(classroomId)
-      }
-    } else loadClassroomsFromJson()
-  }
-
   private fun loadClassroomsFromJson(): List<ClassroomRecord> {
     // Load the classrooms.json file.
     val classroomIdsObj = jsonAssetRetriever.loadJsonFromAsset("classrooms.json")
@@ -359,27 +374,20 @@ class ClassroomController @Inject constructor(
       val classroomId = checkNotNull(classroomIds.optString(i)) {
         "Expected non-null classroom ID at index $i."
       }
-      val classroomRecord = loadClassroomById(classroomId)
+      val classroomRecord = getClassroomById(classroomId)
       classroomRecords.add(classroomRecord)
     }
 
     return classroomRecords
   }
 
-  private fun loadClassroomById(classroomId: String): ClassroomRecord {
-    return if (loadLessonProtosFromAssets) {
-      assetRepository.tryLoadProtoFromLocalAssets(
-        assetName = classroomId,
-        defaultMessage = ClassroomRecord.getDefaultInstance()
-      ) ?: ClassroomRecord.getDefaultInstance()
-    } else loadClassroomByIdFromJson(classroomId)
-  }
-
   private fun loadClassroomByIdFromJson(classroomId: String): ClassroomRecord {
     // Load the classroom obj.
     val classroomObj = jsonAssetRetriever.loadJsonFromAsset("$classroomId.json")
     checkNotNull(classroomObj) { "Failed to load $classroomId.json." }
 
+    val classroomTitle = classroomObj.getJSONObject("classroom_title")
+
     // Load the topic prerequisite map.
     val topicPrereqsObj = checkNotNull(classroomObj.optJSONObject("topic_prerequisites")) {
       "Expected classroom to have non-null topic_prerequisites."
@@ -398,6 +406,10 @@ class ClassroomController @Inject constructor(
       id = checkNotNull(classroomObj.optString("classroom_id")) {
         "Expected classroom to have ID."
       }
+      translatableTitle = SubtitledHtml.newBuilder().apply {
+        contentId = classroomTitle.getStringFromObject("content_id")
+        html = classroomTitle.getStringFromObject("html")
+      }.build()
       putAllTopicPrerequisites(
         topicPrereqs.mapValues { (_, topicIds) ->
           ClassroomRecord.TopicIdList.newBuilder().apply {

domain/src/main/java/org/oppia/android/domain/topic/TopicListController.kt

@@ -5,9 +5,7 @@ import org.json.JSONObject
 import org.oppia.android.app.model.ChapterPlayState
 import org.oppia.android.app.model.ChapterProgress
 import org.oppia.android.app.model.ChapterSummary
-import org.oppia.android.app.model.ClassroomIdList
 import org.oppia.android.app.model.ClassroomRecord
-import org.oppia.android.app.model.ClassroomRecord.TopicIdList
 import org.oppia.android.app.model.ComingSoonTopicList
 import org.oppia.android.app.model.EphemeralTopicSummary
 import org.oppia.android.app.model.LessonThumbnail
@@ -29,7 +27,7 @@ import org.oppia.android.app.model.TopicProgress
 import org.oppia.android.app.model.TopicRecord
 import org.oppia.android.app.model.TopicSummary
 import org.oppia.android.app.model.UpcomingTopic
-import org.oppia.android.domain.classroom.TEST_CLASSROOM_ID_0
+import org.oppia.android.domain.classroom.ClassroomController
 import org.oppia.android.domain.translation.TranslationController
 import org.oppia.android.domain.util.JsonAssetRetriever
 import org.oppia.android.domain.util.getStringFromObject
@@ -101,6 +99,7 @@ class TopicListController @Inject constructor(
   private val oppiaClock: OppiaClock,
   private val assetRepository: AssetRepository,
   private val translationController: TranslationController,
+  private val classroomController: ClassroomController,
   @LoadLessonProtosFromAssets private val loadLessonProtosFromAssets: Boolean
 ) {
 
@@ -137,7 +136,7 @@ class TopicListController @Inject constructor(
 
   private fun createTopicList(contentLocale: OppiaLocale.ContentLocale): TopicList {
     return if (loadLessonProtosFromAssets) {
-      val topicIdList = loadCombinedClassroomsTopicIdList()
+      val topicIdList = loadCombinedTopicIdList()
       return TopicList.newBuilder().apply {
         // Only include topics currently playable in the topic list.
         addAllTopicSummary(
@@ -152,7 +151,7 @@ class TopicListController @Inject constructor(
   }
 
   private fun loadTopicListFromJson(contentLocale: OppiaLocale.ContentLocale): TopicList {
-    val topicIdList = loadCombinedClassroomsTopicIdList()
+    val topicIdList = loadCombinedTopicIdList()
     val topicListBuilder = TopicList.newBuilder()
     for (topicId in topicIdList) {
       val ephemeralSummary = createEphemeralTopicSummary(topicId, contentLocale)
@@ -166,7 +165,7 @@ class TopicListController @Inject constructor(
   }
 
   private fun computeComingSoonTopicList(): ComingSoonTopicList {
-    val topicIdList = loadCombinedClassroomsTopicIdList()
+    val topicIdList = loadCombinedTopicIdList()
     val comingSoonTopicListBuilder = ComingSoonTopicList.newBuilder()
     for (topicId in topicIdList) {
       val upcomingTopicSummary = createUpcomingTopicSummary(topicId)
@@ -185,7 +184,7 @@ class TopicListController @Inject constructor(
     contentLocale: OppiaLocale.ContentLocale
   ): EphemeralTopicSummary {
     val topicSummary = createTopicSummary(topicId)
-    val classroomRecord = loadClassroomById(topicSummary.classroomId)
+    val classroomRecord = classroomController.getClassroomById(topicSummary.classroomId)
     return EphemeralTopicSummary.newBuilder().apply {
       this.topicSummary = topicSummary
       writtenTranslationContext =
@@ -217,7 +216,7 @@ class TopicListController @Inject constructor(
         this.topicId = topicId
         putAllWrittenTranslations(topicRecord.writtenTranslationsMap)
         title = topicRecord.translatableTitle
-        classroomId = getClassroomIdByTopicId(topicId)
+        classroomId = classroomController.getClassroomIdByTopicId(topicId)
         totalChapterCount = storyRecords.map { it.chaptersList.size }.sum()
         topicThumbnail = topicRecord.topicThumbnail
         topicPlayAvailability = if (topicRecord.isPublished) {
@@ -259,7 +258,7 @@ class TopicListController @Inject constructor(
       contentId = "title"
       html = jsonObject.getStringFromObject("topic_name")
     }.build()
-    val classroomId = getClassroomIdByTopicId(topicId)
+    val classroomId = classroomController.getClassroomIdByTopicId(topicId)
     // No written translations are included since none are retrieved from JSON.
     return TopicSummary.newBuilder()
       .setTopicId(topicId)
@@ -296,7 +295,7 @@ class TopicListController @Inject constructor(
       html = jsonObject.getStringFromObject("topic_name")
     }.build()
 
-    val classroomId = getClassroomIdByTopicId(topicId)
+    val classroomId = classroomController.getClassroomIdByTopicId(topicId)
 
     val classroomJsonObject = jsonAssetRetriever.loadJsonFromAsset("$classroomId.json")!!
     val classroomTitle = classroomJsonObject.getJSONObject("classroom_title").let {
@@ -369,8 +368,8 @@ class TopicListController @Inject constructor(
     sortedTopicProgressList.forEach { topicProgress ->
       val topic = topicController.retrieveTopic(topicProgress.topicId)
       val classroom = topic?.topicId?.let { topicId ->
-        val classroomId = getClassroomIdByTopicId(topicId)
-        loadClassroomById(classroomId)
+        val classroomId = classroomController.getClassroomIdByTopicId(topicId)
+        classroomController.getClassroomById(classroomId)
       } ?: ClassroomRecord.getDefaultInstance()
       // Ignore topics that are no longer on the device, or that have been unpublished.
       if (topic?.topicPlayAvailability?.availabilityCase == AVAILABLE_TO_PLAY_NOW) {
@@ -556,7 +555,7 @@ class TopicListController @Inject constructor(
    * being suggested.
    */
   private fun retrieveTopicDependencies(topicId: String): List<String> {
-    val classrooms = loadClassrooms()
+    val classrooms = classroomController.getClassrooms()
     for (classroom in classrooms) {
       if (classroom.topicPrerequisitesMap.containsKey(topicId)) {
         return classroom.topicPrerequisitesMap.getValue(topicId).topicIdsList
@@ -589,7 +588,7 @@ class TopicListController @Inject constructor(
     contentLocale: OppiaLocale.ContentLocale
   ): List<PromotedStory> {
     return if (loadLessonProtosFromAssets) {
-      val topicIdList = loadCombinedClassroomsTopicIdList()
+      val topicIdList = loadCombinedTopicIdList()
       return computeSuggestedStoriesForTopicIds(topicProgressList, topicIdList, contentLocale)
     } else computeSuggestedStoriesFromJson(topicProgressList, contentLocale)
   }
@@ -599,7 +598,7 @@ class TopicListController @Inject constructor(
     contentLocale: OppiaLocale.ContentLocale
   ): List<PromotedStory> {
     // All topics that could potentially be recommended.
-    val topicIdList = loadCombinedClassroomsTopicIdList()
+    val topicIdList = loadCombinedTopicIdList()
     return computeSuggestedStoriesForTopicIds(topicProgressList, topicIdList, contentLocale)
   }
 
@@ -713,7 +712,7 @@ class TopicListController @Inject constructor(
         )
       val classroomRecord =
         assetRepository.loadProtoFromLocalAssets(
-          assetName = getClassroomIdByTopicId(topicId),
+          assetName = classroomController.getClassroomIdByTopicId(topicId),
           baseMessage = ClassroomRecord.getDefaultInstance()
         )
       return PromotedStory.newBuilder().apply {
@@ -783,7 +782,7 @@ class TopicListController @Inject constructor(
         }.build()
       } ?: SubtitledHtml.getDefaultInstance()
 
-      val classroomId = getClassroomIdByTopicId(topicId)
+      val classroomId = classroomController.getClassroomIdByTopicId(topicId)
 
       val classroomJson = jsonAssetRetriever.loadJsonFromAsset("$classroomId.json")
       if (classroomJson!!.optString("classroom_title").isNullOrEmpty()) return null
@@ -867,104 +866,8 @@ class TopicListController @Inject constructor(
       .build()
   }
 
-  private fun getClassroomIdByTopicId(topicId: String): String {
-    var classroomId = TEST_CLASSROOM_ID_0
-    loadClassrooms().forEach {
-      if (it.topicPrerequisitesMap.keys.contains(topicId)) {
-        classroomId = it.id
-      }
-    }
-    return classroomId
-  }
-
-  // TODO(#5344): Remove this in favor of per-classroom data handling.
-  private fun loadClassrooms(): List<ClassroomRecord> {
-    return if (loadLessonProtosFromAssets) {
-      assetRepository.loadProtoFromLocalAssets(
-        assetName = "classrooms",
-        baseMessage = ClassroomIdList.getDefaultInstance()
-      ).classroomIdsList.map { classroomId ->
-        loadClassroomById(classroomId)
-      }
-    } else loadClassroomsFromJson()
-  }
-
-  // TODO(#5344): Remove this in favor of per-classroom data handling.
-  private fun loadClassroomsFromJson(): List<ClassroomRecord> {
-    // Load the classrooms.json file.
-    val classroomIdsObj = jsonAssetRetriever.loadJsonFromAsset("classrooms.json")
-    checkNotNull(classroomIdsObj) { "Failed to load classrooms.json." }
-    val classroomIds = classroomIdsObj.optJSONArray("classroom_id_list")
-    checkNotNull(classroomIds) { "classrooms.json is missing classroom IDs." }
-
-    // Initialize a list to store the [ClassroomRecord]s.
-    val classroomRecords = mutableListOf<ClassroomRecord>()
-
-    // Iterate over all classroomIds and load each classroom's JSON.
-    for (i in 0 until classroomIds.length()) {
-      val classroomId = checkNotNull(classroomIds.optString(i)) {
-        "Expected non-null classroom ID at index $i."
-      }
-      val classroomRecord = loadClassroomById(classroomId)
-      classroomRecords.add(classroomRecord)
-    }
-
-    return classroomRecords
-  }
-
-  // TODO(#5344): Move this to classroom controller.
-  private fun loadClassroomById(classroomId: String): ClassroomRecord {
-    return if (loadLessonProtosFromAssets) {
-      assetRepository.tryLoadProtoFromLocalAssets(
-        assetName = classroomId,
-        defaultMessage = ClassroomRecord.getDefaultInstance()
-      ) ?: ClassroomRecord.getDefaultInstance()
-    } else loadClassroomByIdFromJson(classroomId)
-  }
-
-  // TODO(#5344): Remove this in favor of per-classroom data handling.
-  private fun loadClassroomByIdFromJson(classroomId: String): ClassroomRecord {
-    // Load the classroom obj.
-    val classroomObj = jsonAssetRetriever.loadJsonFromAsset("$classroomId.json")
-    checkNotNull(classroomObj) { "Failed to load $classroomId.json." }
-
-    val classroomTitle = classroomObj.getJSONObject("classroom_title")
-
-    // Load the topic prerequisite map.
-    val topicPrereqsObj = checkNotNull(classroomObj.optJSONObject("topic_prerequisites")) {
-      "Expected classroom to have non-null topic_prerequisites."
-    }
-    val topicPrereqs = topicPrereqsObj.keys().asSequence().associateWith { topicId ->
-      val topicIdArray = checkNotNull(topicPrereqsObj.optJSONArray(topicId)) {
-        "Expected topic $topicId to have a non-null string list."
-      }
-      return@associateWith List(topicIdArray.length()) { index ->
-        checkNotNull(topicIdArray.optString(index)) {
-          "Expected topic $topicId to have non-null string at index $index."
-        }
-      }
-    }
-    return ClassroomRecord.newBuilder().apply {
-      id = checkNotNull(classroomObj.optString("classroom_id")) {
-        "Expected classroom to have ID."
-      }
-      translatableTitle = SubtitledHtml.newBuilder().apply {
-        contentId = classroomTitle.getStringFromObject("content_id")
-        html = classroomTitle.getStringFromObject("html")
-      }.build()
-      putAllTopicPrerequisites(
-        topicPrereqs.mapValues { (_, topicIds) ->
-          TopicIdList.newBuilder().apply {
-            addAllTopicIds(topicIds)
-          }.build()
-        }
-      )
-    }.build()
-  }
-
-  // TODO(#5344): Remove this in favor of per-classroom data handling.
-  private fun loadCombinedClassroomsTopicIdList(): List<String> =
-    loadClassrooms().flatMap { it.topicPrerequisitesMap.keys.toList() }
+  private fun loadCombinedTopicIdList(): List<String> =
+    classroomController.getClassrooms().flatMap { it.topicPrerequisitesMap.keys.toList() }
 }
 
 internal fun createTopicThumbnailFromJson(topicJsonObject: JSONObject): LessonThumbnail {

domain/src/test/java/org/oppia/android/domain/classroom/ClassroomControllerTest.kt

@@ -131,6 +131,23 @@ class ClassroomControllerTest {
     assertThat(classroomList.classroomSummaryList.size).isEqualTo(2)
   }
 
+  @Test
+  fun testGetClassrooms_returnsAllClassrooms() {
+    val classrooms = classroomController.getClassrooms()
+
+    assertThat(classrooms[0].id).isEqualTo(TEST_CLASSROOM_ID_0)
+    assertThat(classrooms[1].id).isEqualTo(TEST_CLASSROOM_ID_1)
+    assertThat(classrooms[2].id).isEqualTo(TEST_CLASSROOM_ID_2)
+  }
+
+  @Test
+  fun testGetClassroomById_hasCorrectClassroomInfo() {
+    val classroom = classroomController.getClassroomById(TEST_CLASSROOM_ID_0)
+
+    assertThat(classroom.id).isEqualTo(TEST_CLASSROOM_ID_0)
+    assertThat(classroom.translatableTitle.html).isEqualTo("Science")
+  }
+
   @Test
   fun testRetrieveTopicList_isSuccessful() {
     val topicListProvider = classroomController.getTopicList(profileId0, TEST_CLASSROOM_ID_0)