Post-merge fixes.

Note that this + the previous merge likely introduced changes outside the scope of the now-slimmer introduce-asset-download-script branch, and such changes will eventually need to be split out to the correct precursor branches in future commits.
oppia · Jun 21, 2024 · 41a048e · 41a048e
1 parent 6e87842
commit 41a048e
Show file tree

Hide file tree

Showing 12 changed files with 74 additions and 60 deletions.
diff --git a/domain/BUILD.bazel b/domain/BUILD.bazel
@@ -37,6 +37,9 @@ MIGRATED_PROD_FILES = glob([
 
 DOMAIN_ASSETS = generate_assets_list_from_text_protos(
     name = "domain_assets",
+    classroom_list_file_names = [
+        "classrooms",
+    ],
     exploration_file_names = [
         "test_exp_id_2",
         "13",
@@ -96,9 +99,6 @@ DOMAIN_ASSETS = generate_assets_list_from_text_protos(
         "GJ2rLXRKD5hw",
         "omzF4oqgeTXd",
     ],
-    classroom_list_file_names = [
-        "classrooms",
-    ],
 )
 
 kt_android_library(

diff --git a/domain/src/main/java/org/oppia/android/domain/topic/TopicListController.kt b/domain/src/main/java/org/oppia/android/domain/topic/TopicListController.kt
@@ -21,7 +21,6 @@ import org.oppia.android.app.model.StoryRecord
 import org.oppia.android.app.model.StorySummary
 import org.oppia.android.app.model.SubtitledHtml
 import org.oppia.android.app.model.Topic
-import org.oppia.android.app.model.ClassroomList
 import org.oppia.android.app.model.TopicList
 import org.oppia.android.app.model.TopicPlayAvailability
 import org.oppia.android.app.model.TopicPlayAvailability.AvailabilityCase.AVAILABLE_TO_PLAY_IN_FUTURE

diff --git a/scripts/src/java/org/oppia/android/scripts/assets/DownloadLessonList.kt b/scripts/src/java/org/oppia/android/scripts/assets/DownloadLessonList.kt
@@ -83,7 +83,6 @@ class LessonListDownloader(
       apiDebugDir,
       forceCacheLoad = false,
       scriptBgDispatcher,
-      topicDependencies = topicDependenciesTable,
       imageDownloader,
       forcedVersions = null // Always load latest when creating the pin versions list.
     )

diff --git a/scripts/src/java/org/oppia/android/scripts/assets/DownloadLessons.kt b/scripts/src/java/org/oppia/android/scripts/assets/DownloadLessons.kt
@@ -22,7 +22,6 @@ import org.oppia.android.scripts.assets.DtoProtoToLegacyProtoConverter.convertTo
 import org.oppia.android.scripts.assets.DtoProtoToLegacyProtoConverter.convertToExploration
 import org.oppia.android.scripts.assets.DtoProtoToLegacyProtoConverter.convertToStoryRecord
 import org.oppia.android.scripts.assets.DtoProtoToLegacyProtoConverter.convertToSubtopicRecord
-import org.oppia.android.scripts.assets.DtoProtoToLegacyProtoConverter.convertToClassroomList
 import org.oppia.android.scripts.assets.DtoProtoToLegacyProtoConverter.convertToTopicRecord
 import org.oppia.android.scripts.gae.GaeAndroidEndpoint
 import org.oppia.android.scripts.gae.GaeAndroidEndpointJsonImpl
@@ -198,7 +197,7 @@ class LessonDownloader(
   apiSecret: String,
   private val cacheDir: File?,
   private val forceCacheLoad: Boolean,
-  downloadListVersions: DownloadListVersions
+  downloadListVersions: DownloadListVersions?
 ) {
   private val threadPool by lazy {
     Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors())
@@ -294,7 +293,8 @@ class LessonDownloader(
       val defaultLocalization = classroom.localizations.defaultMapping
       val nameContentId = classroom.name.contentId
       val textMapping = defaultLocalization.localizableTextContentMappingMap[nameContentId]
-      val classroomName = textMapping?.singleLocalizableText?.text?.takeIf { it.isNotEmpty() } ?: "<unknown>"
+      val classroomName =
+        textMapping?.singleLocalizableText?.text?.takeIf { it.isNotEmpty() } ?: "<unknown>"
       println("- ${classroom.id}: $classroomName (has ${classroom.topicIdsCount} topics)")
     }
 

diff --git a/scripts/src/java/org/oppia/android/scripts/assets/DtoProtoToLegacyProtoConverter.kt b/scripts/src/java/org/oppia/android/scripts/assets/DtoProtoToLegacyProtoConverter.kt
@@ -3,6 +3,7 @@ package org.oppia.android.scripts.assets
 import org.oppia.android.app.model.AnswerGroup
 import org.oppia.android.app.model.ChapterRecord
 import org.oppia.android.app.model.ClassroomList
+import org.oppia.android.app.model.ClassroomRecord
 import org.oppia.android.app.model.ConceptCard
 import org.oppia.android.app.model.ConceptCardList
 import org.oppia.android.app.model.CustomSchemaValue
@@ -31,8 +32,6 @@ import org.oppia.android.app.model.StoryRecord
 import org.oppia.android.app.model.SubtitledHtml
 import org.oppia.android.app.model.SubtitledUnicode
 import org.oppia.android.app.model.SubtopicRecord
-import org.oppia.android.app.model.ClassroomList
-import org.oppia.android.app.model.ClassroomRecord
 import org.oppia.android.app.model.TopicRecord
 import org.oppia.android.app.model.TranslatableHtmlContentId
 import org.oppia.android.app.model.TranslatableSetOfNormalizedString
@@ -42,13 +41,13 @@ import org.oppia.android.app.model.Voiceover
 import org.oppia.android.app.model.VoiceoverMapping
 import org.oppia.proto.v1.structure.AlgebraicExpressionInputInstanceDto
 import org.oppia.proto.v1.structure.ChapterSummaryDto
+import org.oppia.proto.v1.structure.ClassroomDto
 import org.oppia.proto.v1.structure.ConceptCardDto
 import org.oppia.proto.v1.structure.ConceptCardLanguagePackDto
 import org.oppia.proto.v1.structure.ContentLocalizationDto
 import org.oppia.proto.v1.structure.ContentLocalizationsDto
 import org.oppia.proto.v1.structure.ContinueInstanceDto
 import org.oppia.proto.v1.structure.DownloadableTopicSummaryDto
-import org.oppia.proto.v1.structure.ClassroomDto
 import org.oppia.proto.v1.structure.DragAndDropSortInputInstanceDto
 import org.oppia.proto.v1.structure.DragAndDropSortInputInstanceDto.RuleSpecDto.HasElementXBeforeElementYSpecDto
 import org.oppia.proto.v1.structure.DragAndDropSortInputInstanceDto.RuleSpecDto.IsEqualToOrderingWithOneItemAtIncorrectPositionSpecDto
@@ -259,11 +258,13 @@ object DtoProtoToLegacyProtoConverter {
         dto.localizations.extractDefaultThumbnail(imageReferenceReplacements)
       putAllWrittenTranslations(dto.localizations.toTranslationMappings(imageReferenceReplacements))
       this.translatableTitle = dto.localizations.extractDefaultSubtitledHtml(dto.name)
-      putAllTopicPrerequisites(dto.topicIdsList.associateWith { topicId ->
-        ClassroomRecord.TopicIdList.newBuilder().apply {
-          addAllTopicIds(topicSummaryMap.getValue(topicId).prerequisiteTopicIdsList)
-        }.build()
-      })
+      putAllTopicPrerequisites(
+        dto.topicIdsList.associateWith { topicId ->
+          ClassroomRecord.TopicIdList.newBuilder().apply {
+            addAllTopicIds(topicSummaryMap.getValue(topicId).prerequisiteTopicIdsList)
+          }.build()
+        }
+      )
     }.build()
   }
 

diff --git a/scripts/src/java/org/oppia/android/scripts/gae/GaeAndroidEndpointJsonImpl.kt b/scripts/src/java/org/oppia/android/scripts/gae/GaeAndroidEndpointJsonImpl.kt
@@ -17,8 +17,8 @@ import org.oppia.android.scripts.gae.compat.StructureCompatibilityChecker.Compat
 import org.oppia.android.scripts.gae.compat.TopicPackRepository
 import org.oppia.android.scripts.gae.compat.TopicPackRepository.MetricCallbacks.DataGroupType
 import org.oppia.android.scripts.gae.json.AndroidActivityHandlerService
-import org.oppia.android.scripts.gae.json.GaeSkill
 import org.oppia.android.scripts.gae.json.GaeClassroom
+import org.oppia.android.scripts.gae.json.GaeSkill
 import org.oppia.android.scripts.gae.json.GaeStory
 import org.oppia.android.scripts.gae.json.GaeSubtopic
 import org.oppia.android.scripts.gae.json.GaeSubtopicPage
@@ -241,21 +241,17 @@ class GaeAndroidEndpointJsonImpl(
           }.payload
         }
       }.awaitAll().map { it.filterTopics() }
-      // listOf(
-      //   "iX9kYCjnouWN", "sWBXKH4PZcK6", "C4fqwrvqWpRm", "qW12maD4hiA8", "0abdeaJhmfPm",
-      //   "5g0nxGUmx5J5"
-      // ).also {
-      //   tracker.countEstimator.setTopicCount(it.size)
-      //   tracker.reportDownloaded("math")
-      // }
     }
   }
 
   // TODO: Remover this filter once downloading & checking all the other topics works correctly.
   private fun GaeClassroom.filterTopics(): GaeClassroom {
     return copy(
       topicIdToPrereqTopicIds = topicIdToPrereqTopicIds.filterKeys {
-        it in listOf("iX9kYCjnouWN", "sWBXKH4PZcK6", "C4fqwrvqWpRm", "qW12maD4hiA8", "0abdeaJhmfPm", "5g0nxGUmx5J5")
+        it in listOf(
+          "iX9kYCjnouWN", "sWBXKH4PZcK6", "C4fqwrvqWpRm", "qW12maD4hiA8", "0abdeaJhmfPm",
+          "5g0nxGUmx5J5"
+        )
       }
     )
   }

diff --git a/scripts/src/java/org/oppia/android/scripts/gae/compat/TopicPackRepository.kt b/scripts/src/java/org/oppia/android/scripts/gae/compat/TopicPackRepository.kt
@@ -350,14 +350,18 @@ class TopicPackRepository(
   ): GenericLoadResult {
     return when (val result = versionStructureMapManager.lookUp(structureId, reference)) {
       is LoadResult.Pending -> {
-        val nextVersions = reference.computeNextBatchOfVersionsToCheck(versionStructureMap)
-        check(nextVersions.isNotEmpty()) { "At least one reference should be pending for: $reference." }
-        reference.loadVersioned(
-          androidService, compatibilityChecker, nextVersions
-        ).forEach(versionStructureMap::put)
+        val nextVersions = versionStructureMapManager.computeNextBatchOfVersionsToCheck(structureId)
+        check(nextVersions.isNotEmpty()) {
+          "At least one reference should be pending for: $reference."
+        }
+        versionStructureMapManager.update(
+          structureId, reference.loadVersioned(androidService, compatibilityChecker, nextVersions)
+        )
         // This should be present now.
-        versionStructureMap.getValue(reference).also {
-          check(it !is LoadResult.Pending) { "Expected reference to be loaded: $reference (found: $it)." }
+        versionStructureMapManager.lookUp(structureId, reference).also {
+          check(it !is LoadResult.Pending) {
+            "Expected reference to be loaded: $reference (found: $it)."
+          }
         }
       }
       is LoadResult.Success, is LoadResult.Failure -> result
@@ -553,8 +557,6 @@ private interface VersionedStructureFetcher<I : StructureId, S> {
 }
 
 private sealed class VersionedStructureReference<I : StructureId, S> {
-  // TODO: Try 50 or a higher number once multi-version fetching works on Oppia web (see https://github.com/oppia/oppia/issues/18241).
-  private val defaultVersionFetchCount: Int = 1
   abstract val structureId: I
   abstract val version: Int
   abstract val fetcher: VersionedStructureFetcher<I, S>
@@ -577,13 +579,6 @@ private sealed class VersionedStructureReference<I : StructureId, S> {
     return result.await().payload to result.toLoadResult(checker)
   }
 
-  fun computeNextBatchOfVersionsToCheck(structureMap: VersionStructureMap): List<Int> {
-    val pendingVersions = structureMap.filter { (_, result) ->
-      result is LoadResult.Pending
-    }.map { (reference, _) -> reference.version }
-    return pendingVersions.toList().sortedDescending().take(defaultVersionFetchCount)
-  }
-
   suspend fun loadVersioned(
     service: AndroidActivityHandlerService,
     checker: StructureCompatibilityChecker,
@@ -611,7 +606,7 @@ private sealed class VersionedStructureReference<I : StructureId, S> {
   ): LoadResult<S> {
     return when (val compatibilityResult = checkCompatibility(checker, payload)) {
       Compatible -> LoadResult.Success(payload)
-      is Incompatible -> LoadResult.Failure<S>(compatibilityResult.failures)//.also {
+      is Incompatible -> LoadResult.Failure<S>(compatibilityResult.failures) // .also {
       //   // TODO: Remove.
       //   error("Failed to load: $it.")
       // }
@@ -687,6 +682,9 @@ private sealed class VersionedStructureReference<I : StructureId, S> {
 
   companion object {
     const val INVALID_VERSION = 0
+
+    // TODO: Try 50 or a higher number once multi-version fetching works on Oppia web (see https://github.com/oppia/oppia/issues/18241).
+    val defaultVersionFetchCount: Int = 1
   }
 }
 
@@ -830,6 +828,8 @@ private interface VersionStructureMapManager {
 
   fun findMostRecent(structureId: StructureId): GenericStructureReference
 
+  fun computeNextBatchOfVersionsToCheck(structureId: StructureId): List<Int>
+
   fun invalidateVersion(structureId: StructureId, reference: GenericStructureReference)
 
   fun update(
@@ -886,6 +886,14 @@ private class VersionStructureMapManagerTakeLatestImpl(
     }
   }
 
+  override fun computeNextBatchOfVersionsToCheck(structureId: StructureId): List<Int> {
+    return lock.withLock {
+      cachedStructures.getValue(structureId).filter { (_, result) ->
+        result is LoadResult.Pending
+      }.map { (reference, _) -> reference.version }
+    }.sortedDescending().take(VersionedStructureReference.defaultVersionFetchCount)
+  }
+
   override fun invalidateVersion(structureId: StructureId, reference: GenericStructureReference) {
     lock.withLock {
       val structureMap = cachedStructures.getValue(structureId)
@@ -955,6 +963,14 @@ private class VersionStructureMapManagerFixVersionsImpl(
   // There's only at most one version per ID, so that's always the 'latest'.
   override fun findMostRecent(structureId: StructureId) = lookUp(structureId).first
 
+  override fun computeNextBatchOfVersionsToCheck(structureId: StructureId): List<Int> {
+    return lock.withLock {
+      // There's only at most one version per ID, and it's either loaded or isn't.
+      val (reference, result) = lookUp(structureId)
+      return@withLock if (result is LoadResult.Pending) listOf(reference.version) else listOf()
+    }
+  }
+
   override fun invalidateVersion(structureId: StructureId, reference: GenericStructureReference) {
     error("Cannot invalidate versions when versions are fixed, for reference:\n$reference")
   }

diff --git a/scripts/src/java/org/oppia/android/scripts/gae/proto/JsonToProtoConverter.kt b/scripts/src/java/org/oppia/android/scripts/gae/proto/JsonToProtoConverter.kt
@@ -1,11 +1,7 @@
 package org.oppia.android.scripts.gae.proto
 
-<<<<<<< HEAD
-=======
-import org.oppia.android.scripts.gae.compat.CompleteExploration
-import org.oppia.android.scripts.gae.json.GaeClassroom
->>>>>>> integrate-multiple-classrooms-support
 import org.oppia.android.scripts.gae.json.GaeAnswerGroup
+import org.oppia.android.scripts.gae.json.GaeClassroom
 import org.oppia.android.scripts.gae.json.GaeCustomizationArgValue
 import org.oppia.android.scripts.gae.json.GaeCustomizationArgValue.GaeImageWithRegions
 import org.oppia.android.scripts.gae.json.GaeCustomizationArgValue.GaeImageWithRegions.GaeLabeledRegion
@@ -52,8 +48,8 @@ import org.oppia.android.scripts.gae.proto.SolutionAnswer.AnswerTypeCase.REAL
 import org.oppia.proto.v1.structure.AlgebraicExpressionInputInstanceDto
 import org.oppia.proto.v1.structure.BaseAnswerGroupDto
 import org.oppia.proto.v1.structure.BaseSolutionDto
-import org.oppia.proto.v1.structure.ClassroomDto
 import org.oppia.proto.v1.structure.ChapterSummaryDto
+import org.oppia.proto.v1.structure.ClassroomDto
 import org.oppia.proto.v1.structure.ConceptCardDto
 import org.oppia.proto.v1.structure.ConceptCardDto.WorkedExampleDto
 import org.oppia.proto.v1.structure.ConceptCardLanguagePackDto
@@ -335,7 +331,10 @@ class JsonToProtoConverter(
     }
   }
 
-  suspend fun convertToClassroom(gaeClassroom: GaeClassroom, defaultLanguage: LanguageType): ClassroomDto {
+  suspend fun convertToClassroom(
+    gaeClassroom: GaeClassroom,
+    defaultLanguage: LanguageType
+  ): ClassroomDto {
     val containerId = LocalizationTracker.ContainerId.createFrom(gaeClassroom)
     return ClassroomDto.newBuilder().apply {
       this.protoVersion = ProtoVersionProvider.createLatestClassroomProtoVersion()

diff --git a/scripts/src/java/org/oppia/android/scripts/gae/proto/LocalizationTracker.kt b/scripts/src/java/org/oppia/android/scripts/gae/proto/LocalizationTracker.kt
@@ -6,11 +6,8 @@ import com.squareup.moshi.Moshi
 import com.squareup.moshi.kotlin.reflect.KotlinJsonAdapterFactory
 import kotlinx.coroutines.awaitAll
 import org.oppia.android.scripts.gae.gcs.GcsService
-<<<<<<< HEAD
-import org.oppia.android.scripts.gae.json.GaeEntityTranslations
-=======
 import org.oppia.android.scripts.gae.json.GaeClassroom
->>>>>>> integrate-multiple-classrooms-support
+import org.oppia.android.scripts.gae.json.GaeEntityTranslations
 import org.oppia.android.scripts.gae.json.GaeExploration
 import org.oppia.android.scripts.gae.json.GaeRecordedVoiceovers
 import org.oppia.android.scripts.gae.json.GaeSkill
@@ -187,7 +184,11 @@ class LocalizationTracker private constructor(
   suspend fun computeSpecificContentLocalization(
     id: ContainerId,
     language: LanguageType
-  ): ContentLocalizationDto = getExpectedContainer(id).also { checkForNoErrors() }.computeSpecificContentLocalization(language)
+  ): ContentLocalizationDto {
+    return getExpectedContainer(id).also {
+      checkForNoErrors()
+    }.computeSpecificContentLocalization(language)
+  }
 
   // TODO: Document that 'defaultLanguage' can redefine the default language of the container based
   //  on available languages.
@@ -261,7 +262,7 @@ class LocalizationTracker private constructor(
       override val gcsEntityId: String = subtopicPageIdDto.topicId
     }
 
-    data class Classroom(val id: String): ContainerId() {
+    data class Classroom(val id: String) : ContainerId() {
       override val webTranslatableActivityId by lazy { TranslatableActivityId.Classroom(id) }
       override val gcsImageContainerType = GcsService.ImageContainerType.CLASSROOM
       override val gcsEntityId = id
@@ -483,7 +484,7 @@ class LocalizationTracker private constructor(
       check(expectedExemptionCase == 0) { "Exemption $expectedExemptionCase should be removed." }
       if (contentId !in defaultContentIds) {
         errors +=
-        "Attempting to add an asset for a content ID that hasn't been defaulted in container:" +
+          "Attempting to add an asset for a content ID that hasn't been defaulted in container:" +
           " $id, content ID: $contentId."
       }
       // check(contentId in defaultContentIds) {
@@ -603,7 +604,8 @@ class LocalizationTracker private constructor(
       }
       if (contentId in textTranslations && expectedExemption) return
       if (contentId in textTranslations) {
-        errors+="Translation already recorded for content ID: $contentId, for language: $language, in" +
+        errors +=
+          "Translation already recorded for content ID: $contentId, for language: $language, in" +
           " container: $id."
         return
       }

diff --git a/scripts/src/java/org/oppia/android/scripts/gae/proto/OppiaWebTranslationExtractor.kt b/scripts/src/java/org/oppia/android/scripts/gae/proto/OppiaWebTranslationExtractor.kt
@@ -31,7 +31,9 @@ class OppiaWebTranslationExtractor private constructor(
       "I18N_${upperCasedActivityType}_${activityId}_${contentId.uppercase()}"
 
     // TODO: Figure out if this should be ID or URL fragment (or if there's even web translations for these).
-    data class Classroom(val classroomId: String)  : TranslatableActivityId(activityType = "classroom") {
+    data class Classroom(
+      val classroomId: String
+    ) : TranslatableActivityId(activityType = "classroom") {
       override val activityId: String = classroomId
     }
 

diff --git a/scripts/src/java/org/oppia/android/scripts/gae/proto/ProtoVersionProvider.kt b/scripts/src/java/org/oppia/android/scripts/gae/proto/ProtoVersionProvider.kt
@@ -4,10 +4,10 @@ import com.google.protobuf.Descriptors.Descriptor
 import com.google.protobuf.Message
 import org.oppia.proto.v1.api.ClientCompatibilityContextDto
 import org.oppia.proto.v1.versions.ApiVersions
+import org.oppia.proto.v1.versions.ClassroomProtoVersion
 import org.oppia.proto.v1.versions.ConceptCardProtoVersion
 import org.oppia.proto.v1.versions.ExplorationProtoVersion
 import org.oppia.proto.v1.versions.ImageProtoVersion
-import org.oppia.proto.v1.versions.ClassroomProtoVersion
 import org.oppia.proto.v1.versions.LanguageProtosVersion
 import org.oppia.proto.v1.versions.QuestionProtoVersion
 import org.oppia.proto.v1.versions.RevisionCardProtoVersion

diff --git a/third_party/versions.bzl b/third_party/versions.bzl
@@ -102,11 +102,11 @@ MAVEN_TEST_DEPENDENCY_VERSIONS = {
     "androidx.work:work-testing": "2.4.0",
     "com.android.tools.apkparser:apkanalyzer": "30.0.4",
     "com.github.bumptech.glide:mocks": "4.11.0",
+    "com.github.weisj:jsvg": "1.0.0",
     "com.google.protobuf:protobuf-java": "3.17.3",
     "com.google.protobuf:protobuf-java-util": "3.17.3",
     "com.google.truth.extensions:truth-liteproto-extension": "1.1.3",
     "com.google.truth:truth": "0.43",
-    "com.github.weisj:jsvg": "1.0.0",
     "com.squareup.okhttp3:mockwebserver": "4.7.2",
     "com.squareup.retrofit2:retrofit-mock": "2.5.0",
     "io.xlate:yaml-json": "0.1.0",