Fix #3786: Fix regex checks for translated strings

scripts/assets/file_content_validation_checks.textproto

@@ -100,5 +100,5 @@ file_content_checks {
   file_path_regex: ".+?.xml"
   prohibited_content_regex: "<string name=\"[^\"]+\">"
   failure_message: "All strings outside strings.xml must be marked as not translatable, or moved to strings.xml."
-  exempted_file_name: "app/src/main/res/values/strings.xml"
+  exempted_file_patterns: "app/src/main/res/values.*?/strings\\.xml"
 }

scripts/assets/filename_pattern_validation_checks.textproto

@@ -1,4 +1,8 @@
 filename_checks {
   prohibited_filename_regex: "^((?!(app|testing)).)+/src/main/.+?Activity.kt"
-  failure_message: "Activities cannot be placed outside the app or testing module"
+  failure_message: "Activities cannot be placed outside the app or testing module."
+}
+filename_checks {
+  prohibited_filename_regex: "^.+?/res/([^/]+/){2,}[^/]+$"
+  failure_message: "Only one level of subdirectories under res/ should be maintained (further subdirectories aren't supported by the project configuration)."
 }

scripts/src/java/org/oppia/android/scripts/proto/file_content_validation_checks.proto

@@ -24,4 +24,7 @@ message FileContentCheck {
 
   // Names of all the files which should be exempted for this check.
   repeated string exempted_file_name = 4;
+
+  // Regex patterns for all file/file paths that should be exempted for this check.
+  repeated string exempted_file_patterns = 5;
 }

scripts/src/java/org/oppia/android/scripts/regex/RegexPatternValidationCheck.kt

@@ -32,24 +32,25 @@ fun main(vararg args: String) {
 
   // Check if the repo has any filename failure.
   val hasFilenameCheckFailure = retrieveFilenameChecks()
-    .fold(initial = false) { isFailing, filenameCheck ->
-      val checkFailed = checkProhibitedFileNamePattern(
+    .fold(initial = false) { hasFailingFile, filenameCheck ->
+      val fileFails = checkProhibitedFileNamePattern(
         repoRoot,
         searchFiles,
         filenameCheck,
       )
-      isFailing || checkFailed
+      return@fold hasFailingFile || fileFails
     }
 
   // Check if the repo has any file content failure.
-  val hasFileContentCheckFailure = retrieveFileContentChecks()
-    .fold(initial = false) { isFailing, fileContentCheck ->
-      val checkFailed = checkProhibitedContent(
+  val contentChecks = retrieveFileContentChecks().map { MatchableFileContentCheck.createFrom(it) }
+  val hasFileContentCheckFailure =
+    searchFiles.fold(initial = false) { hasFailingFile, searchFile ->
+      val fileFails = checkProhibitedContent(
         repoRoot,
-        searchFiles,
-        fileContentCheck
+        searchFile,
+        contentChecks
       )
-      isFailing || checkFailed
+      return@fold hasFailingFile || fileFails
     }
 
   if (hasFilenameCheckFailure || hasFileContentCheckFailure) {
@@ -138,38 +139,33 @@ private fun checkProhibitedFileNamePattern(
  * Checks for a prohibited file content.
  *
  * @param repoRoot the root directory of the repo
- * @param searchFiles a list of all the files which needs to be checked
- * @param fileContentCheck proto object of FileContentCheck
+ * @param searchFile the file to check for prohibited content
+ * @param fileContentChecks contents to check for validity
  * @return whether the file content pattern is correct or not
  */
 private fun checkProhibitedContent(
   repoRoot: File,
-  searchFiles: List<File>,
-  fileContentCheck: FileContentCheck
+  searchFile: File,
+  fileContentChecks: Iterable<MatchableFileContentCheck>
 ): Boolean {
-  val filePathRegex = fileContentCheck.filePathRegex.toRegex()
-  val prohibitedContentRegex = fileContentCheck.prohibitedContentRegex.toRegex()
-
-  val matchedFiles = searchFiles.filter { file ->
-    val fileRelativePath = file.toRelativeString(repoRoot)
-    val isExempted = fileRelativePath in fileContentCheck.exemptedFileNameList
-    return@filter if (!isExempted && filePathRegex.matches(fileRelativePath)) {
-      file.useLines { lines ->
-        lines.foldIndexed(initial = false) { lineIndex, isFailing, lineContent ->
-          val matches = prohibitedContentRegex.containsMatchIn(lineContent)
-          if (matches) {
-            logProhibitedContentFailure(
-              lineIndex + 1, // Increment by 1 since line numbers begin at 1 rather than 0.
-              fileContentCheck.failureMessage,
-              fileRelativePath
-            )
-          }
-          isFailing || matches
+  val lines = searchFile.readLines()
+  return fileContentChecks.fold(initial = false) { hasFailingFile, fileContentCheck ->
+    val fileRelativePath = searchFile.toRelativeString(repoRoot)
+    val fileFails = if (fileContentCheck.isFileAffectedByCheck(fileRelativePath)) {
+      val affectedLines = fileContentCheck.computeAffectedLines(lines)
+      if (affectedLines.isNotEmpty()) {
+        affectedLines.forEach { lineIndex ->
+          logProhibitedContentFailure(
+            lineIndex + 1, // Increment by 1 since line numbers begin at 1 rather than 0.
+            fileContentCheck.failureMessage,
+            fileRelativePath
+          )
         }
       }
+      affectedLines.isNotEmpty()
     } else false
+    return@fold hasFailingFile || fileFails
   }
-  return matchedFiles.isNotEmpty()
 }
 
 /**
@@ -208,3 +204,46 @@ private fun logProhibitedContentFailure(
   val failureMessage = "$filePath:$lineNumber: $errorToShow"
   println(failureMessage)
 }
+
+/** A matchable version of [FileContentCheck]. */
+private data class MatchableFileContentCheck(
+  val filePathRegex: Regex,
+  val prohibitedContentRegex: Regex,
+  val failureMessage: String,
+  val exemptedFileNames: List<String>,
+  val exemptedFilePatterns: List<Regex>
+) {
+  /**
+   * Returns whether the relative file given by the specified path should be affected by this check
+   * (i.e. that it matches the inclusion pattern and is not explicitly or implicitly excluded).
+   */
+  fun isFileAffectedByCheck(relativePath: String): Boolean =
+    filePathRegex.matches(relativePath) && !isFileExempted(relativePath)
+
+  /**
+   * Returns the list of line indexes which contain prohibited content per this check (given an
+   * iterable of lines). Note that the returned indexes are based on the iteration order of the
+   * provided iterable.
+   */
+  fun computeAffectedLines(lines: Iterable<String>): List<Int> {
+    return lines.withIndex().filter { (_, line) ->
+      prohibitedContentRegex.containsMatchIn(line)
+    }.map { (index, _) -> index }
+  }
+
+  private fun isFileExempted(relativePath: String): Boolean =
+    relativePath in exemptedFileNames || exemptedFilePatterns.any { it.matches(relativePath) }
+
+  companion object {
+    /** Returns a new [MatchableFileContentCheck] based on the specified [FileContentCheck]. */
+    fun createFrom(fileContentCheck: FileContentCheck): MatchableFileContentCheck {
+      return MatchableFileContentCheck(
+        filePathRegex = fileContentCheck.filePathRegex.toRegex(),
+        prohibitedContentRegex = fileContentCheck.prohibitedContentRegex.toRegex(),
+        failureMessage = fileContentCheck.failureMessage,
+        exemptedFileNames = fileContentCheck.exemptedFileNameList,
+        exemptedFilePatterns = fileContentCheck.exemptedFilePatternsList.map { it.toRegex() }
+      )
+    }
+  }
+}

scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt

@@ -17,13 +17,18 @@ class RegexPatternValidationCheckTest {
   private val originalOut: PrintStream = System.out
   private val REGEX_CHECK_PASSED_OUTPUT_INDICATOR: String = "REGEX PATTERN CHECKS PASSED"
   private val REGEX_CHECK_FAILED_OUTPUT_INDICATOR: String = "REGEX PATTERN CHECKS FAILED"
+  private val activitiesPlacementErrorMessage =
+    "Activities cannot be placed outside the app or testing module."
+  private val nestedResourceSubdirectoryErrorMessage =
+    "Only one level of subdirectories under res/ should be maintained (further subdirectories " +
+      "aren't supported by the project configuration)."
   private val supportLibraryUsageErrorMessage =
     "AndroidX should be used instead of the support library"
   private val coroutineWorkerUsageErrorMessage =
     "For stable tests, prefer using ListenableWorker with an Oppia-managed dispatcher."
   private val settableFutureUsageErrorMessage =
-    "SettableFuture should only be used in pre-approved locations since it's easy to potentially" +
-      " mess up & lead to a hanging ListenableFuture."
+    "SettableFuture should only be used in pre-approved locations since it's easy to potentially " +
+      "mess up & lead to a hanging ListenableFuture."
   private val androidGravityLeftErrorMessage =
     "Use android:gravity=\"start\", instead, for proper RTL support"
   private val androidGravityRightErrorMessage =
@@ -47,8 +52,8 @@ class RegexPatternValidationCheckTest {
   private val androidTouchAnchorSideRightErrorMessage =
     "Use motion:touchAnchorSide=\"end\", instead, for proper RTL support"
   private val oppiaCantBeTranslatedErrorMessage =
-    "Oppia should never used directly in a string (since it shouldn't be translated). Instead," +
-      " use a parameter & insert the string retrieved from app_name."
+    "Oppia should never used directly in a string (since it shouldn't be translated). Instead, " +
+      "use a parameter & insert the string retrieved from app_name."
   private val untranslatableStringsGoInSpecificFileErrorMessage =
     "Untranslatable strings should go in untranslated_strings.xml, instead."
   private val translatableStringsGoInMainFileErrorMessage =
@@ -104,8 +109,58 @@ class RegexPatternValidationCheckTest {
     assertThat(exception).hasMessageThat().contains(REGEX_CHECK_FAILED_OUTPUT_INDICATOR)
     assertThat(outContent.toString().trim()).isEqualTo(
       """
-      File name/path violation: Activities cannot be placed outside the app or testing module
+      File name/path violation: $activitiesPlacementErrorMessage
       - data/src/main/TestActivity.kt
+
+      $wikiReferenceNote
+      """.trimIndent()
+    )
+  }
+
+  @Test
+  fun testFileNamePattern_appResources_stringsFile_fileNamePatternIsCorrect() {
+    tempFolder.newFolder("testfiles", "app", "src", "main", "res", "values")
+    tempFolder.newFile("testfiles/app/src/main/res/values/strings.xml")
+
+    runScript()
+
+    assertThat(outContent.toString().trim()).isEqualTo(REGEX_CHECK_PASSED_OUTPUT_INDICATOR)
+  }
+
+  @Test
+  fun testFileNamePattern_appResources_subValuesDir_stringsFile_fileNamePatternIsNotCorrect() {
+    tempFolder.newFolder("testfiles", "app", "src", "main", "res", "values", "subdir")
+    tempFolder.newFile("testfiles/app/src/main/res/values/subdir/strings.xml")
+
+    val exception = assertThrows(Exception::class) {
+      runScript()
+    }
+
+    assertThat(exception).hasMessageThat().contains(REGEX_CHECK_FAILED_OUTPUT_INDICATOR)
+    assertThat(outContent.toString().trim()).isEqualTo(
+      """
+      File name/path violation: $nestedResourceSubdirectoryErrorMessage
+      - app/src/main/res/values/subdir/strings.xml
+
+      $wikiReferenceNote
+      """.trimIndent()
+    )
+  }
+
+  @Test
+  fun testFileNamePattern_domainResources_subValuesDir_stringsFile_fileNamePatternIsNotCorrect() {
+    tempFolder.newFolder("testfiles", "domain", "src", "main", "res", "drawable", "subdir")
+    tempFolder.newFile("testfiles/domain/src/main/res/drawable/subdir/example.png")
+
+    val exception = assertThrows(Exception::class) {
+      runScript()
+    }
+
+    assertThat(exception).hasMessageThat().contains(REGEX_CHECK_FAILED_OUTPUT_INDICATOR)
+    assertThat(outContent.toString().trim()).isEqualTo(
+      """
+      File name/path violation: $nestedResourceSubdirectoryErrorMessage
+      - domain/src/main/res/drawable/subdir/example.png
 
       $wikiReferenceNote
       """.trimIndent()
@@ -828,6 +883,30 @@ class RegexPatternValidationCheckTest {
     assertThat(outContent.toString().trim()).isEqualTo(REGEX_CHECK_PASSED_OUTPUT_INDICATOR)
   }
 
+  @Test
+  fun testFileContent_translatableString_inPrimaryStringsFile_fileContentIsCorrect() {
+    val prohibitedContent = "<string name=\"test\">Translatable</string>"
+    tempFolder.newFolder("testfiles", "app", "src", "main", "res", "values")
+    val stringFilePath = "app/src/main/res/values/strings.xml"
+    tempFolder.newFile("testfiles/$stringFilePath").writeText(prohibitedContent)
+
+    runScript()
+
+    assertThat(outContent.toString().trim()).isEqualTo(REGEX_CHECK_PASSED_OUTPUT_INDICATOR)
+  }
+
+  @Test
+  fun testFileContent_translatableString_inTranslatedPrimaryStringsFile_fileContentIsCorrect() {
+    val prohibitedContent = "<string name=\"test\">Translatable</string>"
+    tempFolder.newFolder("testfiles", "app", "src", "main", "res", "values-ar")
+    val stringFilePath = "app/src/main/res/values-ar/strings.xml"
+    tempFolder.newFile("testfiles/$stringFilePath").writeText(prohibitedContent)
+
+    runScript()
+
+    assertThat(outContent.toString().trim()).isEqualTo(REGEX_CHECK_PASSED_OUTPUT_INDICATOR)
+  }
+
   @Test
   fun testFilenameAndContent_useProhibitedFileName_useProhibitedFileContent_multipleFailures() {
     tempFolder.newFolder("testfiles", "data", "src", "main")
@@ -842,10 +921,10 @@ class RegexPatternValidationCheckTest {
     assertThat(exception).hasMessageThat().contains(REGEX_CHECK_FAILED_OUTPUT_INDICATOR)
     assertThat(outContent.toString().trim()).isEqualTo(
       """
-      File name/path violation: Activities cannot be placed outside the app or testing module
+      File name/path violation: $activitiesPlacementErrorMessage
       - data/src/main/TestActivity.kt
 
-      data/src/main/TestActivity.kt:1: AndroidX should be used instead of the support library
+      data/src/main/TestActivity.kt:1: $supportLibraryUsageErrorMessage
       $wikiReferenceNote
       """.trimIndent()
     )