From 4730edb0263135e88dab08ab0f6dc724a1585a67 Mon Sep 17 00:00:00 2001 From: RD Rama Devi <122200035+Rd4dev@users.noreply.github.com> Date: Thu, 5 Sep 2024 22:52:56 +0530 Subject: [PATCH 1/5] Fix #1730 : Prevent binary files from being checked in using pre-commit hook (#5525) ## Explanation Fixes #1730 ### The PR includes - A pre-commit hook that identifies binary files in both - staged changes (for local checks before committing) - committed files (for CI checks after pushing to a PR) - If binary files are found, the commit/check is blocked, and the offending files are listed for removal. - The pre-commit hook is integrated via a setup script. - The same script is utilized for the CI pipeline. - The 'Pass' statement is only included in the CI checks to keep the local commit process cleaner. ### Local block as pre-commit hook when a binary file is detected ![image](https://github.com/user-attachments/assets/b953b2d7-55ec-46e6-a0b9-00967200509c) ### CI re-check if the PR includes a binary - [ Fail - [stack trace](https://github.com/Rd4dev/oppia-android/actions/runs/10715972847/job/29712474511#step:7:15) ] [ Pass - [stack trace](https://github.com/Rd4dev/oppia-android/actions/runs/10716040065/job/29712695824?pr=11#step:7:10) ] ![image](https://github.com/user-attachments/assets/64352473-86cb-49e0-b888-d3247286e9e5) ## Essential Checklist - [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)). --- .github/workflows/static_checks.yml | 11 +++++++++ scripts/pre-commit.sh | 35 +++++++++++++++++++++++++++++ scripts/setup.sh | 3 +++ 3 files changed, 49 insertions(+) create mode 100644 scripts/pre-commit.sh diff --git a/.github/workflows/static_checks.yml b/.github/workflows/static_checks.yml index 15dd3e28e76..ae04da9c0f4 100644 --- a/.github/workflows/static_checks.yml +++ b/.github/workflows/static_checks.yml @@ -107,6 +107,8 @@ jobs: CACHE_DIRECTORY: ~/.bazel_cache steps: - uses: actions/checkout@v2 + with: + fetch-depth: 0 - name: Set up Bazel uses: abhinavsingh/setup-bazel@v3 @@ -205,6 +207,15 @@ jobs: run: | bazel run //scripts:string_resource_validation_check -- $(pwd) + - name: Binary files check + # The expression if: ${{ !cancelled() }} runs a job or step regardless of its success or failure while responding to cancellations, + # serving as a cancellation-compliant alternative to if: ${{ always() }} in concurrent workflows. + if: ${{ !cancelled() }} + run: | + bash /home/runner/work/oppia-android/oppia-android/scripts/pre-commit.sh + echo "No binary files found in commit" + echo "BINARY FILES CHECK PASSED" + # Note that caching is intentionally not enabled for this check since licenses should always be # verified without any potential influence from earlier builds (i.e. always from a clean build to # ensure the results exactly match the current state of the repository). diff --git a/scripts/pre-commit.sh b/scripts/pre-commit.sh new file mode 100644 index 00000000000..26061ef5c09 --- /dev/null +++ b/scripts/pre-commit.sh @@ -0,0 +1,35 @@ +#!/bin/bash + +# Pre-commit hook to check for binary files. + +# Find the common ancestor between develop and the current branch +base_commit=$(git merge-base 'origin/develop' HEAD) + +# Get the list of staged changes (files ready to be committed) +staged_files=$(git diff --cached --name-only) + +# Get the list of changed files compared to the base commit +changed_files=$(git diff --name-only "$base_commit" HEAD) + +# Combine both lists of files, ensuring no duplicates +all_files=$(echo -e "$staged_files\n$changed_files" | sort -u) + +function checkForBinaries() { + binaryFilesCount=0 + + # Iterate over all files (both staged and changed) + for file in $all_files; do + if [ -f "$file" ] && file --mime "$file" | grep -q 'binary'; then + ((binaryFilesCount++)) + printf "\n\033[33m%s\033[0m" "$file" + fi + done + + if [[ "${binaryFilesCount}" -gt 0 ]]; then + printf "\n\nPlease remove the %d detected binary file(s)." "$binaryFilesCount" + printf "\nBINARY FILES CHECK FAILED" + exit 1 + fi +} + +checkForBinaries diff --git a/scripts/setup.sh b/scripts/setup.sh index 8c3ef595e1d..9f822095f32 100644 --- a/scripts/setup.sh +++ b/scripts/setup.sh @@ -13,6 +13,9 @@ # Move file from script folder to .git/hooks folder cp scripts/pre-push.sh .git/hooks/pre-push +# Copy the pre-commit hook from script to .git/hooks folder +cp scripts/pre-commit.sh .git/hooks/pre-commit + # Create a folder where all the set up files will be downloaded mkdir -p ../oppia-android-tools cd ../oppia-android-tools From 4f4831ac9d251930568369471efabea3175e9001 Mon Sep 17 00:00:00 2001 From: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> Date: Wed, 11 Sep 2024 02:17:46 +0300 Subject: [PATCH 2/5] Fix #5357: Remove CDATA from translatable strings (#5524) ## Explanation Fixes https://github.com/oppia/oppia-android/issues/5357 This PR includes the changes previously included in #5361, with an additional fix for missing closing html tags described in #5403: ``` faq_answer_app_language: Missing tag after the email address. faq_answer_bug_reporting: Missing closing tag. faq_answer_exploration_player: Missing closing tag. faq_answer_update_app: Missing closing tag and closing

tag. faq_answer_update_os: Missing closing tag and closing

tag. ``` This PR removes all CDATA declarations in the translatable strings.xml file and instead escapes all necessary characters: < and > (& didn't need to be escaped since no strings use that character at the moment). This is needed because Translatewiki doesn't seem to extract the HTML within the CDATA declaration correctly, so it may not be translated (some existing strings were never translated, and per https://github.com/oppia/oppia-android/pull/5274 the latest FAQ changes aren't being processed correctly (leading to empty translation strings being submitted). This PR also introduces a regex check + test to ensure that CDATA isn't used anymore in strings. ## Essential Checklist - [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 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 --- app/src/main/res/values-sw/strings.xml | 2 +- app/src/main/res/values/strings.xml | 44 +++++++++---------- .../file_content_validation_checks.textproto | 5 +++ .../regex/RegexPatternValidationCheckTest.kt | 24 ++++++++++ 4 files changed, 52 insertions(+), 23 deletions(-) diff --git a/app/src/main/res/values-sw/strings.xml b/app/src/main/res/values-sw/strings.xml index 049ebac057a..17d2fd54cec 100644 --- a/app/src/main/res/values-sw/strings.xml +++ b/app/src/main/res/values-sw/strings.xml @@ -152,7 +152,7 @@ Tafadhali anza jibu lako kwa nambari (k.m.,”0” katika 0.5). Tafadhali weka nambari halali. Jibu linaweza kuwa na tarakimu zisizozidi 15 (0–9) au alama (. au -). - Tafadhali andika uwiano unaojumuisha tarakimu zilizotenganishwa na koloni(k.m. 1:2 au 1:2:3). + Tafadhali andika uwiano unaojumuisha tarakimu zilizotenganishwa na koloni (k.m. 1:2 au 1:2:3). Tafadhali weka uwiano sahihi (k.m. 1:2 au 1:2:3). Jibu lako lina koloni mbili (:) karibu na kila moja. Idadi ya masharti si sawa na masharti yanayohitajika. diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 1395b1d24a3..319d70ff93a 100755 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -175,12 +175,12 @@ Please enter a valid number. The answer can contain at most 15 digits (0–9) or symbols (. or -). Enter a number to continue. - Please write a ratio that consists of digits separated by colons (e.g. 1:2 or 1:2:3). - Please enter a valid ratio (e.g. 1:2 or 1:2:3). - Your answer has two colons (:) next to each other. - Number of terms is not equal to the required terms. - Ratios cannot have 0 as an element. - Enter a ratio to continue. + Please write a ratio that consists of digits separated by colons (e.g. 1:2 or 1:2:3). + Please enter a valid ratio (e.g. 1:2 or 1:2:3). + Your answer has two colons (:) next to each other. + Number of terms is not equal to the required terms. + Ratios cannot have 0 as an element. + Enter a ratio to continue. Enter text to continue. Choose an answer to continue. Unknown size @@ -549,11 +549,11 @@ Policy Page Privacy Policy - this page for the latest version of this privacy policy.]]> + Please visit <a href="https://www.oppia.org/privacy-policy">this page</a> for the latest version of this privacy policy. Terms of Service - Terms of Service and Privacy Policy.]]> - this page for the latest version of these terms.]]> + By using %s, you agree to our <br> <oppia-noninteractive-policy link="tos">Terms of Service</oppia-noninteractive-policy> and <oppia-noninteractive-policy link="privacy">Privacy Policy</oppia-noninteractive-policy>. + Please visit <a href="https://www.oppia.org/terms">this page</a> for the latest version of these terms. What is %s? Who is an Administrator? @@ -569,19 +569,19 @@ How do I update my Android OS? I can\'t find my question here. What now? - %1$s "O-pee-yah" (Finnish) - "to learn"


%1$s\'s mission is to help anyone learn anything they want in an effective and enjoyable way.


By creating a set of free, high-quality, demonstrably effective lessons with the help of educators from around the world, %1$s aims to provide students with quality education — regardless of where they are or what traditional resources they have access to.


As a student, you can begin your learning adventure by browsing the topics listed on the Home Page!

]]> - An Administrator is the main user that manages profiles and settings for every profile on their account. They are most likely your parent, teacher, or guardian that created this profile for you.


Administrators have the ability to manage profiles, assign PINs, and change other settings under their account. Depending on your profile, Administrator permissions may be required for certain features such as changing your PIN, and more.


To see who your Administrator is, go to the Profile Chooser. The first profile listed and has "Administrator" written under their name is the Administrator.

]]> - If it is your first time creating a profile and you do not have a PIN:
  1. From the Profile Chooser, tap on Set up Multiple Profiles.
  2. Create a PIN and Save.
  3. Fill in all fields for the profile.
    1. (Optional) Upload a photo.
    2. Enter a name.
    3. (Optional) Assign a 3-digit PIN.
  4. Tap Create. This profile is added to your Profile Chooser!

If you have created a profile before and have a PIN:

  1. From the Profile Chooser, tap on Add Profile.
  2. Enter your PIN and tap Submit.
  3. Fill in all fields for the profile.
    1. (Optional) Upload a photo.
    2. Enter a name.
    3. (Optional) Assign a 3-digit PIN.
  4. Tap Create. This profile is added to your Profile Chooser!


Note: Only the Administrator is able to manage profiles.

]]> - The %s app currently supports English, Brazilian Portuguese, Arabic, Swahili and Nigerian Pidgin. Choose one of these languages in the menu, under Options. To request the app in your language, please contact us at admin@oppia.org.

]]> -
  1. From your %s app home screen, tap the menu in the top left corner.
  2. Tap Share feedback.
  3. Follow the instructions to report the bug or share feedback.

    4. ]]> - %1$s’s mission is to help learners gain necessary life skills. Math is an essential skill in everyday life. %1$s will be offering new lessons on science and other subjects soon!

    ]]>     - Yes, %s will be offering new lessons on science and other subjects soon. Please check back for updates!

    ]]>     - If the Exploration Player is not loading


    Check to see if the app is up to date:

    • Go to the Play Store and make sure the app is updated to its latest version


    Check your internet connection:

    • If your internet connection is slow, try re-connecting to your Wi-Fi network or connecting to a different network.

    Ask the Administrator to check their device and internet connection:

    • Get the Administrator to troubleshoot using the steps above

    Let us know if you still have issues with loading:

    • Report a problem by contacting us at admin@oppia.org.
    ]]>     - If your audio is not playing


    Check to see if the app is up to date:

    • Go to the Play Store and make sure the app is updated to its latest version


    Check your internet connection:

    • If your internet connection is slow, try re-connecting to your Wi-Fi network or connecting to a different network. Slow internet may cause the audio to load irregularly, making it difficult to play.


    Ask the Administrator to check their device and internet connection:

    • Get the Administrator to troubleshoot using the steps above


    Let us know if you still have issues with loading:

    • Report a problem by contacting us at admin@oppia.org.
    ]]>     - Once a profile is deleted:

    1. The profile cannot be recovered.
    2. Profile information such as name, photos, and progress will be permanently deleted.

    To delete a profile (excluding the Administrator\'s):

    1. From the Administrator\'s Home Page, tap on the menu button on the top left.
    2. Tap on Administrator Controls.
    3. Tap on Edit Profiles.
    4. Tap on the Profile you would like to delete.
    5. At the bottom of the screen, tap Profile Deletion.
    6. Tap Delete to confirm deletion.


    Note: Only the Administrator is able to manage profiles.

    ]]>     -
    1. Open the Google Play Store app.
    2. Search for the %s app.
    3. Tap Update.

      ]]> -
      1. Tap your phone\'s Settings app.
      2. Tap System updates.
      3. Tap System updates and follow the instructions to update your Android operating system.

        ]]> - If you cannot find your question or would like to report a bug, contact us at admin@oppia.org.

        ]]>         + <p>%1$s <i>"O-pee-yah"</i> (Finnish) - "to learn"</p><p><br></p><p>%1$s\'s mission is to help anyone learn anything they want in an effective and enjoyable way.</p><p><br></p><p>By creating a set of free, high-quality, demonstrably effective lessons with the help of educators from around the world, %1$s aims to provide students with quality education — regardless of where they are or what traditional resources they have access to.</p><p><br></p><p>As a student, you can begin your learning adventure by browsing the topics listed on the Home Page!</p> + <p>An Administrator is the main user that manages profiles and settings for every profile on their account. They are most likely your parent, teacher, or guardian that created this profile for you.</p><p><br></p><p>Administrators have the ability to manage profiles, assign PINs, and change other settings under their account. Depending on your profile, Administrator permissions may be required for certain features such as changing your PIN, and more.</p><p><br></p><p>To see who your Administrator is, go to the Profile Chooser. The first profile listed and has "Administrator" written under their name is the Administrator.</p> + <p>If it is your first time creating a profile and you do not have a PIN:<ol><li>From the Profile Chooser, tap on <strong>Set up Multiple Profiles</strong>.</li><li>Create a PIN and <strong>Save</strong>.</li><li>Fill in all fields for the profile.<ol><li>(Optional) Upload a photo.</li><li>Enter a name.</li><li>(Optional) Assign a 3-digit PIN.</li></ol></li><li>Tap <strong>Create</strong>. This profile is added to your Profile Chooser!</li></ol></p><p>If you have created a profile before and have a PIN:<ol><li>From the Profile Chooser, tap on <strong>Add Profile</strong>.</li><li>Enter your PIN and tap <strong>Submit</strong>.</li><li>Fill in all fields for the profile.<ol><li>(Optional) Upload a photo.</li><li>Enter a name.</li><li>(Optional) Assign a 3-digit PIN.</li></ol></li><li>Tap <strong>Create</strong>. This profile is added to your Profile Chooser!</li></ol></p><p><br></p><p>Note: Only the <u>Administrator</u> is able to manage profiles.</p> + <p>The %s app currently supports English, Brazilian Portuguese, Arabic, Swahili, and Nigerian Pidgin. Choose one of these languages in the menu, under Options. To request the app in your language, please contact us at <strong>admin@oppia.org</strong>.</p> + <p><ol><li>From your %s app home screen, tap the menu in the top left corner.</li><li>Tap <strong>Share feedback</strong>.</li><li>Follow the instructions to report the bug or share feedback.</li></ol></p> + <p>%1$s\'s mission is to help learners gain necessary life skills. Math is an essential skill in everyday life. %1$s will be offering new lessons on science and other subjects soon!</p> + <p>Yes, %s will be offering new lessons on science and other subjects soon. Please check back for updates!</p> + <p>If the Exploration Player is not loading</p><p><br></p><p>Check to see if the app is up to date:</p><ul><li>Go to the Play Store and make sure the app is updated to its latest version</li></ul><p><br></p><p>Check your internet connection:</p><ul><li>If your internet connection is slow, try re-connecting to your Wi-Fi network or connecting to a different network.</li></ul><p>Ask the Administrator to check their device and internet connection:</p><ul><li>Get the Administrator to troubleshoot using the steps above</li></ul><p><br></p><p>Let us know if you still have issues with loading:</p><ul><li>Report a problem by contacting us at admin@oppia.org.</li></ul> + <p>If your audio is not playing</p><p><br></p><p>Check to see if the app is up to date:</p><ul><li>Go to the Play Store and make sure the app is updated to its latest version</li></ul><p><br></p><p>Check your internet connection:</p><ul><li>If your internet connection is slow, try re-connecting to your Wi-Fi network or connecting to a different network. Slow internet may cause the audio to load irregularly, making it difficult to play.</li></ul><p><br></p><p>Ask the Administrator to check their device and internet connection:</p><ul><li>Get the Administrator to troubleshoot using the steps above</li></ul><p><br></p><p>Let us know if you still have issues with loading:</p><ul><li>Report a problem by contacting us at admin@oppia.org.</li></ul> + <p>Once a profile is deleted:</p><ol><li>The profile cannot be recovered.</li><li>Profile information such as name, photos, and progress will be permanently deleted.</li></ol><p>To delete a profile (excluding the <u>Administrator</u>):</p><ol><li>From the Administrator\'s Home Page, tap on the menu button on the top left.</li><li>Tap on <strong>Administrator Controls</strong>.</li><li>Tap on <strong>Edit Profiles</strong>.</li><li>Tap on the Profile you would like to delete.</li><li>At the bottom of the screen, tap <strong>Profile Deletion</strong>.</li><li>Tap <strong>Delete</strong> to confirm deletion.</li></ol><p><br></p><p>Note: Only the <u>Administrator</u> is able to manage profiles.</p> + <p><ol><li>Open the Google Play Store app.</li><li>Search for the %s app.</li><li>Tap Update.</li></ol></p> + <p><ol><li>Tap your phone\'s Settings app.</li><li>Tap System updates.</li><li>Tap System updates and follow the instructions to update your Android operating system.</li></ol></p> + <p>If you cannot find your question or would like to report a bug, contact us at <strong>admin@oppia.org.</strong></p> Profile Edit Fragment Test Activity Administrator Controls Fragment Test Activity diff --git a/scripts/assets/file_content_validation_checks.textproto b/scripts/assets/file_content_validation_checks.textproto index 9a9918f2f64..d4e3597a664 100644 --- a/scripts/assets/file_content_validation_checks.textproto +++ b/scripts/assets/file_content_validation_checks.textproto @@ -116,6 +116,11 @@ file_content_checks { failure_message: "All plurals outside strings.xml must be marked as not translatable, or moved to strings.xml." exempted_file_patterns: "app/src/main/res/values.*?/strings\\.xml" } +file_content_checks { + file_path_regex: "app/src/main/res/values/strings\\.xml" + prohibited_content_regex: "CDATA" + failure_message: "CDATA isn't handled by Translatewiki correctly. Use escaped HTML, instead." +} file_content_checks { file_path_regex: ".+?\\.kt" prohibited_content_regex: "android.text.BidiFormatter" diff --git a/scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt b/scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt index 4e96123650e..5f47d4b81e1 100644 --- a/scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt +++ b/scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt @@ -222,6 +222,8 @@ class RegexPatternValidationCheckTest { private val referenceComputeIfAbsent = "computeIfAbsent won't desugar and requires Java 8 support (SDK 24+). Suggest using an atomic" + " Kotlin-specific solution, instead." + private val cdataShouldNotBeUsed = + "CDATA isn't handled by Translatewiki correctly. Use escaped HTML, instead." private val wikiReferenceNote = "Refer to https://github.com/oppia/oppia-android/wiki/Static-Analysis-Checks" + "#regexpatternvalidation-check for more details on how to fix this." @@ -2752,6 +2754,28 @@ class RegexPatternValidationCheckTest { ) } + @Test + fun testFileContent_includesCdataContentInStringsXml_fileContentIsNotCorrect() { + val prohibitedContent = + """ + Some nested HTML.

        ]]>         + """.trimIndent() + tempFolder.newFolder("testfiles", "app", "src", "main", "res", "values") + val stringFilePath = "app/src/main/res/values/strings.xml" + tempFolder.newFile("testfiles/$stringFilePath").writeText(prohibitedContent) + + val exception = assertThrows() { runScript() } + + assertThat(exception).hasMessageThat().contains(REGEX_CHECK_FAILED_OUTPUT_INDICATOR) + assertThat(outContent.toString().trim()) + .isEqualTo( + """ + $stringFilePath:1: $cdataShouldNotBeUsed + $wikiReferenceNote + """.trimIndent() + ) + } + /** Runs the regex_pattern_validation_check. */ private fun runScript() { main(File(tempFolder.root, "testfiles").absolutePath) From c82b71b82717e3c998fbdb7bfb052bd580d6d78e Mon Sep 17 00:00:00 2001 From: RD Rama Devi <122200035+Rd4dev@users.noreply.github.com> Date: Thu, 12 Sep 2024 15:05:07 +0530 Subject: [PATCH 3/5] Fix part of #5508: Limit APK/AAB Difference analysis reports in the PR Comment Thread (#5532) ## Explanation Fixes part of #5508 ### This PR includes Steps to locate the previous `stats.yml` workflow run, download its build artifact, and compare it with the current build log. If changes are detected, a comment will be uploaded to help minimize comment thread overload. **The implementation:** - Download the previous build log artifact (if available). - Run the script. - Compare the current generated build log with the previous build log artifact: - if no differences are found -> skip commenting. - if differences are found -> comment the current generated build log - if no previous artifact is found -> comment the current generated build log - This occurs in 2 instances: - 1. It's the first run of the PR. - 2. An error occurred during the previous stat check (since the previous build is from the second-to-last run ID). - Upload the current build log as an artifact (for the next stat run). - Comment/skip the stat report based on the comparison result. # ### Tested with a cloned PR _(with stats.yml implementation on develop)_ Tested PR: https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/pull/40 Reference for proof of implementation: - [x] should comment on initial run | [comment](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/pull/40#issuecomment-2336828924) | [stack trace](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/actions/runs/10762876260/job/29843752198#step:19:26) - [x] shouldn't comment when no change | [reference1](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/pull/40#issuecomment-2337237045) | [reference2](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/pull/40#issuecomment-2337278404) - [x] should comment on change | [comment](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/pull/40#issuecomment-2337251971) | [reference](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/pull/40#issuecomment-2337260315) - [x] comment on previous build fail (replicated!) | [comment](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/pull/40#issuecomment-2337297618) | [reference](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/pull/40#issuecomment-2337310429) ## Essential Checklist - [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)). --------- Co-authored-by: Ben Henning --- .github/workflows/build_tests.yml | 2 +- .github/workflows/main.yml | 10 +++--- .github/workflows/stats.yml | 60 ++++++++++++++++++++++++++++++- 3 files changed, 65 insertions(+), 7 deletions(-) diff --git a/.github/workflows/build_tests.yml b/.github/workflows/build_tests.yml index 7dde621a72f..7a024172368 100644 --- a/.github/workflows/build_tests.yml +++ b/.github/workflows/build_tests.yml @@ -153,7 +153,7 @@ jobs: run: | cp $GITHUB_WORKSPACE/bazel-bin/oppia.apk /home/runner/work/oppia-android/oppia-android/ - - uses: actions/upload-artifact@v2 + - uses: actions/upload-artifact@v4 with: name: oppia-bazel.apk path: /home/runner/work/oppia-android/oppia-android/oppia.apk diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 84cc12006a8..f5ba874970e 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -64,7 +64,7 @@ jobs: # We require 'sudo' to avoid an error of the existing android sdk. See https://github.com/actions/starter-workflows/issues/58 run: sudo ./gradlew --full-stacktrace :utility:testDebugUnitTest -Dorg.gradle.java.home=$JAVA_HOME - name: Upload Utility Test Reports - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v4 if: ${{ !cancelled() }} # IMPORTANT: Upload reports regardless of success or failure status with: name: utility reports @@ -77,7 +77,7 @@ jobs: # We require 'sudo' to avoid an error of the existing android sdk. See https://github.com/actions/starter-workflows/issues/58 run: sudo ./gradlew --full-stacktrace :data:testDebugUnitTest -Dorg.gradle.java.home=$JAVA_HOME - name: Upload Data Test Reports - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v4 if: ${{ !cancelled() }} # IMPORTANT: Upload reports regardless of success or failure status with: name: data reports @@ -90,7 +90,7 @@ jobs: # We require 'sudo' to avoid an error of the existing android sdk. See https://github.com/actions/starter-workflows/issues/58 run: sudo ./gradlew --full-stacktrace :domain:testDebugUnitTest -Dorg.gradle.java.home=$JAVA_HOME - name: Upload Domain Test Reports - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v4 if: ${{ !cancelled() }} # IMPORTANT: Upload reports regardless of success or failure status with: name: domain reports @@ -103,7 +103,7 @@ jobs: # We require 'sudo' to avoid an error of the existing android sdk. See https://github.com/actions/starter-workflows/issues/58 run: sudo ./gradlew --full-stacktrace :testing:testDebugUnitTest -Dorg.gradle.java.home=$JAVA_HOME - name: Upload Testing Test Reports - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v4 if: ${{ !cancelled() }} # IMPORTANT: Upload reports regardless of success or failure status with: name: testing reports @@ -144,7 +144,7 @@ jobs: run: | sudo ./gradlew --full-stacktrace :app:testDebugUnitTest --${{ matrix.shard }} -Dorg.gradle.java.home=$JAVA_HOME - name: Upload App Test Reports - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@v4 if: ${{ !cancelled() }} # IMPORTANT: Upload reports regardless of success or failure status with: name: app reports ${{ matrix.shard }} diff --git a/.github/workflows/stats.yml b/.github/workflows/stats.yml index de3ce5fadd5..74cbbeabbed 100644 --- a/.github/workflows/stats.yml +++ b/.github/workflows/stats.yml @@ -184,6 +184,63 @@ jobs: beta $(pwd)/oppia_beta_without_changes.aab $(pwd)/oppia_beta_with_changes.aab \ ga $(pwd)/oppia_ga_without_changes.aab $(pwd)/oppia_ga_with_changes.aab + - name: Find CI workflow run for PR + id: find-workflow-run + uses: actions/github-script@v7 + continue-on-error: true + with: + script: | + const { owner, repo } = context.repo; + const runsResponse = await github.rest.actions.listWorkflowRuns({ + owner, + repo, + workflow_id: 'stats.yml', + }); + + const runs = runsResponse.data.workflow_runs; + runs.sort((a, b) => new Date(b.created_at).getTime() - new Date(a.created_at).getTime()); + + const run = runs[1]; + if(!run) { + core.setFailed('Could not find a succesful workflow run'); + return; + } + console.log(run.id); + + core.setOutput('run-id', run.id); + + - name: Download previous build summary + uses: actions/download-artifact@v4 + with: + name: brief_build_summary_${{ matrix.prInfo.number }} + path: ./previous_build_logs + github-token: ${{ secrets.GITHUB_TOKEN }} + run-id: ${{ steps.find-workflow-run.outputs.run-id }} + continue-on-error: true # Ignore errors if the file doesn't exist (first run) + + - name: Compare current build summary with the previous one + id: build-comparison + run: | + if [ -f ./develop/brief_build_summary.log ]; then + echo "Comparing current and previous build summaries..." + if diff ./develop/brief_build_summary.log ./previous_build_logs/brief_build_summary.log > /dev/null; then + echo "No changes detected; skipping comment." + echo "skip_comment=true" >> $GITHUB_ENV + else + echo "Changes detected; proceeding with the comment." + echo "skip_comment=false" >> $GITHUB_ENV + fi + else + echo "No previous summary found; proceeding with the comment." + echo "skip_comment=false" >> $GITHUB_ENV + fi + + - name: Upload current build summary for future comparison + uses: actions/upload-artifact@v4 + with: + name: brief_build_summary_${{ matrix.prInfo.number }} + path: ./develop/brief_build_summary.log + # Reference: https://github.com/peter-evans/create-or-update-comment#setting-the-comment-body-from-a-file. # Also, for multi-line env values, see: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#multiline-strings. - name: Extract reports for uploading & commenting @@ -203,6 +260,7 @@ jobs: cp "$GITHUB_WORKSPACE/develop/full_build_summary.log" "$FULL_BUILD_SUMMARY_FILE_PATH" - name: Add build stats summary comment + if: ${{ env.skip_comment == 'false' }} env: PR_NUMBER: ${{ matrix.prInfo.number }} uses: peter-evans/create-or-update-comment@v1 @@ -210,7 +268,7 @@ jobs: issue-number: ${{ env.PR_NUMBER }} body: ${{ steps.compute-comment-body.outputs.comment_body }} - - uses: actions/upload-artifact@v2 + - uses: actions/upload-artifact@v4 with: name: ${{ env.FULL_BUILD_SUMMARY_FILE_NAME }} path: ${{ env.FULL_BUILD_SUMMARY_FILE_PATH }} From 65197bfad9a11db12838222b4050f6818fe87b8c Mon Sep 17 00:00:00 2001 From: Subhajit Mallick <153619690+subhajitxyz@users.noreply.github.com> Date: Mon, 16 Sep 2024 16:04:19 +0530 Subject: [PATCH 4/5] Fix #5485 Create means for verifying Fragment Arguments (#5527) ## Explanation Fixes #5485 Added test for 10 fragments arguments and saveInstanceState. ## Essential Checklist - [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: ...".) - [ ] 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 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 --------- Co-authored-by: Mr. 17 --- .../app/options/ReadingTextSizeFragment.kt | 3 +- .../options/ReadingTextSizeFragmentTest.kt | 43 ++++++++++++ .../resumelesson/ResumeLessonFragmentTest.kt | 37 ++++++++++ .../profile/ProfileListFragmentTest.kt | 27 ++++++++ .../profile/ProfileRenameFragmentTest.kt | 26 +++++++ .../profile/ProfileResetPinFragmentTest.kt | 67 +++++++++++++++++++ .../app/spotlight/SpotlightFragmentTest.kt | 21 ++++++ .../android/app/story/StoryFragmentTest.kt | 42 ++++++++++++ .../android/app/survey/SurveyFragmentTest.kt | 29 ++++++++ .../ThirdPartyDependencyListFragmentTest.kt | 28 ++++++++ .../revisioncard/RevisionCardFragmentTest.kt | 43 ++++++++++++ 11 files changed, 365 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/org/oppia/android/app/options/ReadingTextSizeFragment.kt b/app/src/main/java/org/oppia/android/app/options/ReadingTextSizeFragment.kt index 1cdae8c8578..02c9a456e74 100644 --- a/app/src/main/java/org/oppia/android/app/options/ReadingTextSizeFragment.kt +++ b/app/src/main/java/org/oppia/android/app/options/ReadingTextSizeFragment.kt @@ -63,7 +63,8 @@ class ReadingTextSizeFragment : InjectableFragment(), TextSizeRadioButtonListene readingTextSizeFragmentPresenter.onTextSizeSelected(selectedTextSize) } - private fun retrieveFragmentArguments(): ReadingTextSizeFragmentArguments { + /** Returns the [ReadingTextSizeFragmentArguments] stored in the fragment's arguments. */ + fun retrieveFragmentArguments(): ReadingTextSizeFragmentArguments { return checkNotNull(arguments) { "Expected arguments to be passed to ReadingTextSizeFragment" }.getProto(FRAGMENT_ARGUMENTS_KEY, ReadingTextSizeFragmentArguments.getDefaultInstance()) diff --git a/app/src/sharedTest/java/org/oppia/android/app/options/ReadingTextSizeFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/options/ReadingTextSizeFragmentTest.kt index 5e4b14be4aa..1883d2459bf 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/options/ReadingTextSizeFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/options/ReadingTextSizeFragmentTest.kt @@ -16,6 +16,7 @@ import androidx.test.espresso.matcher.ViewMatchers.isChecked import androidx.test.espresso.matcher.ViewMatchers.isRoot import androidx.test.espresso.matcher.ViewMatchers.withText import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth.assertThat import dagger.Component import org.hamcrest.Description import org.hamcrest.TypeSafeMatcher @@ -37,6 +38,7 @@ import org.oppia.android.app.application.testing.TestingBuildFlavorModule import org.oppia.android.app.devoptions.DeveloperOptionsModule import org.oppia.android.app.devoptions.DeveloperOptionsStarterModule import org.oppia.android.app.model.ProfileId +import org.oppia.android.app.model.ReadingTextSize.MEDIUM_TEXT_SIZE import org.oppia.android.app.model.ReadingTextSize.SMALL_TEXT_SIZE import org.oppia.android.app.player.state.itemviewmodel.SplitScreenInteractionModule import org.oppia.android.app.recyclerview.RecyclerViewMatcher.Companion.atPositionOnView @@ -194,6 +196,47 @@ class ReadingTextSizeFragmentTest { } } + @Test + fun testFragment_fragmentLoaded_verifyCorrectArgumentsPassed() { + launch(createReadingTextSizeActivityIntent()).use { scenario -> + testCoroutineDispatchers.runCurrent() + scenario.onActivity { activity -> + + val readingTextSizeFragment = activity.supportFragmentManager + .findFragmentById(R.id.reading_text_size_container) as ReadingTextSizeFragment + val receivedReadingTextSize = readingTextSizeFragment.retrieveFragmentArguments() + .readingTextSize + + assertThat(receivedReadingTextSize).isEqualTo(SMALL_TEXT_SIZE) + } + } + } + + @Test + fun testFragment_saveInstanceState_verifyCorrectStateRestored() { + launch(createReadingTextSizeActivityIntent()).use { scenario -> + testCoroutineDispatchers.runCurrent() + + scenario.onActivity { activity -> + val readingTextSizeFragment = activity.supportFragmentManager + .findFragmentById(R.id.reading_text_size_container) as ReadingTextSizeFragment + readingTextSizeFragment.readingTextSizeFragmentPresenter + .onTextSizeSelected(MEDIUM_TEXT_SIZE) + } + + scenario.recreate() + + scenario.onActivity { activity -> + val newReadingTextSizeFragment = activity.supportFragmentManager + .findFragmentById(R.id.reading_text_size_container) as ReadingTextSizeFragment + val restoredTopicIdList = + newReadingTextSizeFragment.readingTextSizeFragmentPresenter.getTextSizeSelected() + + assertThat(restoredTopicIdList).isEqualTo(MEDIUM_TEXT_SIZE) + } + } + } + private fun createReadingTextSizeActivityIntent() = ReadingTextSizeActivity.createReadingTextSizeActivityIntent(context, SMALL_TEXT_SIZE) diff --git a/app/src/sharedTest/java/org/oppia/android/app/resumelesson/ResumeLessonFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/resumelesson/ResumeLessonFragmentTest.kt index 1a28e628594..33da1eeed36 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/resumelesson/ResumeLessonFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/resumelesson/ResumeLessonFragmentTest.kt @@ -41,6 +41,7 @@ import org.oppia.android.app.model.ExplorationActivityParams import org.oppia.android.app.model.ExplorationCheckpoint import org.oppia.android.app.model.ProfileId import org.oppia.android.app.model.ReadingTextSize +import org.oppia.android.app.model.ResumeLessonFragmentArguments import org.oppia.android.app.player.state.itemviewmodel.SplitScreenInteractionModule import org.oppia.android.app.shim.ViewBindingShimModule import org.oppia.android.app.translation.testing.ActivityRecreatorTestModule @@ -99,6 +100,7 @@ import org.oppia.android.testing.time.FakeOppiaClockModule import org.oppia.android.util.accessibility.AccessibilityTestModule import org.oppia.android.util.caching.AssetModule import org.oppia.android.util.caching.testing.CachingTestModule +import org.oppia.android.util.extensions.getProto import org.oppia.android.util.gcsresource.GcsResourceModule import org.oppia.android.util.locale.LocaleProdModule import org.oppia.android.util.logging.EventLoggingConfigurationModule @@ -273,6 +275,41 @@ class ResumeLessonFragmentTest { } } + @Test + fun testFragment_fragmentLoaded_verifyCorrectArgumentsPassed() { + launch(createResumeLessonActivityIntent()).use { scenario -> + testCoroutineDispatchers.runCurrent() + scenario.onActivity { activity -> + + val resumeLessonFragment = activity.supportFragmentManager + .findFragmentById(R.id.resume_lesson_fragment_placeholder) as ResumeLessonFragment + val args = checkNotNull(resumeLessonFragment.arguments) { + "Expected arguments to be provided for fragment." + }.getProto( + ResumeLessonFragment.RESUME_LESSON_FRAGMENT_ARGUMENTS_KEY, + ResumeLessonFragmentArguments.getDefaultInstance() + ) + val receivedProfileId = args.profileId + val receivedClassroomId = args.classroomId + val receivedTopicId = args.topicId + val receivedStoryId = args.storyId + val receivedExplorationId = args.explorationId + val receivedParentScreen = args.parentScreen + val receivedCheckpoint = args.checkpoint + + assertThat(receivedProfileId) + .isEqualTo(ProfileId.newBuilder().apply { internalId = 1 }.build()) + assertThat(receivedClassroomId).isEqualTo(TEST_CLASSROOM_ID_1) + assertThat(receivedTopicId).isEqualTo(FRACTIONS_TOPIC_ID) + assertThat(receivedStoryId).isEqualTo(FRACTIONS_STORY_ID_0) + assertThat(receivedExplorationId).isEqualTo(FRACTIONS_EXPLORATION_ID_0) + assertThat(receivedParentScreen) + .isEqualTo(ExplorationActivityParams.ParentScreen.PARENT_SCREEN_UNSPECIFIED) + assertThat(receivedCheckpoint).isEqualTo(ExplorationCheckpoint.getDefaultInstance()) + } + } + } + private fun createResumeLessonActivityIntent(): Intent { return ResumeLessonActivity.createResumeLessonActivityIntent( context, diff --git a/app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileListFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileListFragmentTest.kt index 5b7bd78bb3e..742c20bd0f5 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileListFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileListFragmentTest.kt @@ -18,6 +18,7 @@ import androidx.test.espresso.matcher.ViewMatchers.isRoot import androidx.test.espresso.matcher.ViewMatchers.withId import androidx.test.espresso.matcher.ViewMatchers.withText import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth.assertThat import dagger.Component import org.hamcrest.Matchers.not import org.junit.After @@ -37,6 +38,7 @@ import org.oppia.android.app.application.ApplicationStartupListenerModule import org.oppia.android.app.application.testing.TestingBuildFlavorModule import org.oppia.android.app.devoptions.DeveloperOptionsModule import org.oppia.android.app.devoptions.DeveloperOptionsStarterModule +import org.oppia.android.app.model.ProfileListFragmentArguments import org.oppia.android.app.player.state.itemviewmodel.SplitScreenInteractionModule import org.oppia.android.app.recyclerview.RecyclerViewMatcher.Companion.atPosition import org.oppia.android.app.recyclerview.RecyclerViewMatcher.Companion.atPositionOnView @@ -86,6 +88,7 @@ import org.oppia.android.testing.time.FakeOppiaClockModule import org.oppia.android.util.accessibility.AccessibilityTestModule import org.oppia.android.util.caching.AssetModule import org.oppia.android.util.caching.testing.CachingTestModule +import org.oppia.android.util.extensions.getProto import org.oppia.android.util.gcsresource.GcsResourceModule import org.oppia.android.util.locale.LocaleProdModule import org.oppia.android.util.logging.EventLoggingConfigurationModule @@ -367,6 +370,30 @@ class ProfileListFragmentTest { } } + @Test + fun testFragment_fragmentLoaded_verifyCorrectArgumentsPassed() { + profileTestHelper.initializeProfiles() + launch(ProfileListActivity::class.java).use { scenario -> + testCoroutineDispatchers.runCurrent() + scenario.onActivity { activity -> + + val profileListFragment = activity.supportFragmentManager + .findFragmentById(R.id.profile_list_container) as ProfileListFragment + + val arguments = checkNotNull(profileListFragment.arguments) { + "Expected variables to be passed to ProfileListFragment" + } + val args = arguments.getProto( + ProfileListFragment.PROFILE_LIST_FRAGMENT_ARGUMENTS_KEY, + ProfileListFragmentArguments.getDefaultInstance() + ) + val receivedIsMultipane = args.isMultipane + + assertThat(receivedIsMultipane).isEqualTo(false) + } + } + } + // TODO(#59): Figure out a way to reuse modules instead of needing to re-declare them. @Singleton @Component( diff --git a/app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileRenameFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileRenameFragmentTest.kt index fd4bf50712b..1fc5d9316d3 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileRenameFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileRenameFragmentTest.kt @@ -21,6 +21,7 @@ import androidx.test.espresso.matcher.ViewMatchers.withId import androidx.test.espresso.matcher.ViewMatchers.withText import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.rule.ActivityTestRule +import com.google.common.truth.Truth.assertThat import dagger.Component import org.hamcrest.CoreMatchers.not import org.hamcrest.core.AllOf.allOf @@ -102,6 +103,7 @@ import org.oppia.android.util.networking.NetworkConnectionUtilDebugModule import org.oppia.android.util.parser.html.HtmlParserEntityTypeModule import org.oppia.android.util.parser.image.GlideImageLoaderModule import org.oppia.android.util.parser.image.ImageParsingModule +import org.oppia.android.util.profile.CurrentUserProfileIdIntentDecorator.extractCurrentUserProfileId import org.robolectric.annotation.Config import org.robolectric.annotation.LooperMode import javax.inject.Inject @@ -441,6 +443,30 @@ class ProfileRenameFragmentTest { } } + @Test + fun testFragment_fragmentLoaded_verifyCorrectArgumentsPassed() { + ActivityScenario.launch( + ProfileRenameActivity.createProfileRenameActivity( + context = context, + internalProfileId = 1 + ) + ).use { scenario -> + testCoroutineDispatchers.runCurrent() + scenario.onActivity { activity -> + + val profileRenameFragment = activity.supportFragmentManager + .findFragmentById(R.id.profile_rename_fragment_placeholder) as ProfileRenameFragment + val args = + checkNotNull(profileRenameFragment.arguments) { + "Expected arguments to be passed to ProfileRenameFragment" + } + val receivedProfileId = args.extractCurrentUserProfileId().internalId + + assertThat(receivedProfileId).isEqualTo(1) + } + } + } + // TODO(#59): Figure out a way to reuse modules instead of needing to re-declare them. @Singleton @Component( diff --git a/app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileResetPinFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileResetPinFragmentTest.kt index c306e9a6206..c0e1541d8a3 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileResetPinFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileResetPinFragmentTest.kt @@ -22,6 +22,7 @@ import androidx.test.espresso.matcher.ViewMatchers.withId import androidx.test.espresso.matcher.ViewMatchers.withText import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.rule.ActivityTestRule +import com.google.common.truth.Truth.assertThat import dagger.Component import org.hamcrest.CoreMatchers.allOf import org.hamcrest.CoreMatchers.not @@ -42,6 +43,7 @@ import org.oppia.android.app.application.ApplicationStartupListenerModule import org.oppia.android.app.application.testing.TestingBuildFlavorModule import org.oppia.android.app.devoptions.DeveloperOptionsModule import org.oppia.android.app.devoptions.DeveloperOptionsStarterModule +import org.oppia.android.app.model.ProfileResetPinFragmentArguments import org.oppia.android.app.player.state.itemviewmodel.SplitScreenInteractionModule import org.oppia.android.app.shim.ViewBindingShimModule import org.oppia.android.app.translation.testing.ActivityRecreatorTestModule @@ -92,6 +94,7 @@ import org.oppia.android.testing.time.FakeOppiaClockModule import org.oppia.android.util.accessibility.AccessibilityTestModule import org.oppia.android.util.caching.AssetModule import org.oppia.android.util.caching.testing.CachingTestModule +import org.oppia.android.util.extensions.getProto import org.oppia.android.util.gcsresource.GcsResourceModule import org.oppia.android.util.locale.LocaleProdModule import org.oppia.android.util.logging.EventLoggingConfigurationModule @@ -1006,6 +1009,70 @@ class ProfileResetPinFragmentTest { } } + @Test + fun testFragment_fragmentLoaded_verifyCorrectArgumentsPassed() { + ActivityScenario.launch( + ProfileResetPinActivity.createProfileResetPinActivity( + context = context, + profileId = 0, + isAdmin = true + ) + ).use { scenario -> + testCoroutineDispatchers.runCurrent() + scenario.onActivity { activity -> + + val profileResetPinFragment = activity.supportFragmentManager + .findFragmentById(R.id.profile_reset_pin_fragment_placeholder) as ProfileResetPinFragment + + val arguments = checkNotNull(profileResetPinFragment.arguments) { + "Expected arguments to be passed to ProfileResetPinFragment" + } + val args = + arguments.getProto( + ProfileResetPinFragment.PROFILE_RESET_PIN_FRAGMENT_ARGUMENTS_KEY, + ProfileResetPinFragmentArguments.getDefaultInstance() + ) + val receivedProfileResetPinProfileId = args.internalProfileId + val receivedProfileResetPinIsAdmin = args.isAdmin + + assertThat(receivedProfileResetPinProfileId).isEqualTo(0) + assertThat(receivedProfileResetPinIsAdmin).isEqualTo(true) + } + } + } + + @Test + fun testFragment_fragmentLoaded_whenIsAdminFalse_verifyCorrectArgumentsPassed() { + ActivityScenario.launch( + ProfileResetPinActivity.createProfileResetPinActivity( + context = context, + profileId = 0, + isAdmin = false + ) + ).use { scenario -> + testCoroutineDispatchers.runCurrent() + scenario.onActivity { activity -> + + val profileResetPinFragment = activity.supportFragmentManager + .findFragmentById(R.id.profile_reset_pin_fragment_placeholder) as ProfileResetPinFragment + + val arguments = checkNotNull(profileResetPinFragment.arguments) { + "Expected arguments to be passed to ProfileResetPinFragment" + } + val args = + arguments.getProto( + ProfileResetPinFragment.PROFILE_RESET_PIN_FRAGMENT_ARGUMENTS_KEY, + ProfileResetPinFragmentArguments.getDefaultInstance() + ) + val receivedProfileResetPinProfileId = args.internalProfileId + val receivedProfileResetPinIsAdmin = args.isAdmin + + assertThat(receivedProfileResetPinProfileId).isEqualTo(0) + assertThat(receivedProfileResetPinIsAdmin).isEqualTo(false) + } + } + } + // TODO(#59): Figure out a way to reuse modules instead of needing to re-declare them. @Singleton @Component( diff --git a/app/src/sharedTest/java/org/oppia/android/app/spotlight/SpotlightFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/spotlight/SpotlightFragmentTest.kt index 1e37adc5e68..9fb017111e3 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/spotlight/SpotlightFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/spotlight/SpotlightFragmentTest.kt @@ -14,6 +14,7 @@ import androidx.test.espresso.matcher.ViewMatchers.isDisplayed import androidx.test.espresso.matcher.ViewMatchers.withId import androidx.test.espresso.matcher.ViewMatchers.withText import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth.assertThat import dagger.Component import org.junit.After import org.junit.Before @@ -93,6 +94,7 @@ import org.oppia.android.util.networking.NetworkConnectionDebugUtilModule import org.oppia.android.util.networking.NetworkConnectionUtilDebugModule import org.oppia.android.util.parser.html.HtmlParserEntityTypeModule import org.oppia.android.util.parser.image.ImageParsingModule +import org.oppia.android.util.profile.CurrentUserProfileIdIntentDecorator.extractCurrentUserProfileId import org.robolectric.annotation.Config import org.robolectric.annotation.LooperMode import javax.inject.Inject @@ -339,6 +341,25 @@ class SpotlightFragmentTest { } } + @Test + fun testFragment_fragmentLoaded_verifyCorrectArgumentsPassed() { + TestPlatformParameterModule.forceEnableSpotlightUi(true) + launch( + createSpotlightFragmentTestActivity(context) + ).use { scenario -> + testCoroutineDispatchers.runCurrent() + + scenario.onActivity { activity -> + val spotlightFragment = activity.supportFragmentManager + .findFragmentByTag(SpotlightManager.SPOTLIGHT_FRAGMENT_TAG) as SpotlightFragment + val receivedInternalProfileId = spotlightFragment + .arguments?.extractCurrentUserProfileId()?.internalId ?: -1 + + assertThat(receivedInternalProfileId).isEqualTo(0) + } + } + } + private fun setUpTestApplicationComponent() { ApplicationProvider.getApplicationContext().inject(this) } diff --git a/app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt index c93e0f8460a..c8013552937 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt @@ -67,6 +67,7 @@ import org.oppia.android.app.customview.LessonThumbnailImageView import org.oppia.android.app.devoptions.DeveloperOptionsModule import org.oppia.android.app.devoptions.DeveloperOptionsStarterModule import org.oppia.android.app.model.ProfileId +import org.oppia.android.app.model.StoryFragmentArguments import org.oppia.android.app.player.exploration.ExplorationActivity import org.oppia.android.app.player.state.itemviewmodel.SplitScreenInteractionModule import org.oppia.android.app.recyclerview.RecyclerViewMatcher.Companion.atPosition @@ -132,6 +133,7 @@ import org.oppia.android.util.accessibility.AccessibilityTestModule import org.oppia.android.util.accessibility.FakeAccessibilityService import org.oppia.android.util.caching.AssetModule import org.oppia.android.util.caching.testing.CachingTestModule +import org.oppia.android.util.extensions.getProto import org.oppia.android.util.gcsresource.GcsResourceModule import org.oppia.android.util.locale.LocaleProdModule import org.oppia.android.util.logging.EventLoggingConfigurationModule @@ -144,6 +146,7 @@ import org.oppia.android.util.parser.html.HtmlParserEntityTypeModule import org.oppia.android.util.parser.image.ImageLoader import org.oppia.android.util.parser.image.ImageParsingModule import org.oppia.android.util.parser.image.ImageTransformation +import org.oppia.android.util.profile.CurrentUserProfileIdIntentDecorator.extractCurrentUserProfileId import org.robolectric.annotation.Config import org.robolectric.annotation.LooperMode import javax.inject.Inject @@ -772,6 +775,45 @@ class StoryFragmentTest { } } + @Test + fun testFragment_fragmentLoaded_verifyCorrectArgumentsPassed() { + launch(createFractionsStoryActivityIntent()).use { scenario -> + testCoroutineDispatchers.runCurrent() + + scenario.onActivity { activity -> + val storyFragment = activity.supportFragmentManager + .findFragmentById(R.id.story_fragment_placeholder) as StoryFragment + + val arguments = checkNotNull(storyFragment.arguments) { + "Expected arguments to be passed to StoryFragment." + } + val args = arguments.getProto( + StoryFragment.STORY_FRAGMENT_ARGUMENTS_KEY, + StoryFragmentArguments.getDefaultInstance() + ) + + val receivedInternalProfileId = arguments.extractCurrentUserProfileId().internalId + val receivedClassroomId = + checkNotNull(args.classroomId) { + "Expected classroomId to be passed to StoryFragment." + } + val receivedTopicId = + checkNotNull(args.topicId) { + "Expected topicId to be passed to StoryFragment." + } + val receivedStoryId = + checkNotNull(args.storyId) { + "Expected storyId to be passed to StoryFragment." + } + + assertThat(receivedInternalProfileId).isEqualTo(internalProfileId) + assertThat(receivedClassroomId).isEqualTo(TEST_CLASSROOM_ID_1) + assertThat(receivedTopicId).isEqualTo(FRACTIONS_TOPIC_ID) + assertThat(receivedStoryId).isEqualTo(FRACTIONS_STORY_ID_0) + } + } + } + @Config(qualifiers = "+sw600dp") @Test // TODO(#4212): Error -> No views in hierarchy found matching fun testStoryFragment_completedChapter_checkProgressDrawableIsCorrect() { diff --git a/app/src/sharedTest/java/org/oppia/android/app/survey/SurveyFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/survey/SurveyFragmentTest.kt index b600558c0da..332914c0c7c 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/survey/SurveyFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/survey/SurveyFragmentTest.kt @@ -48,6 +48,7 @@ import org.oppia.android.app.devoptions.DeveloperOptionsModule import org.oppia.android.app.devoptions.DeveloperOptionsStarterModule import org.oppia.android.app.model.ProfileId import org.oppia.android.app.model.ScreenName +import org.oppia.android.app.model.SurveyFragmentArguments import org.oppia.android.app.model.SurveyQuestionName import org.oppia.android.app.player.state.itemviewmodel.SplitScreenInteractionModule import org.oppia.android.app.recyclerview.RecyclerViewMatcher.Companion.atPositionOnView @@ -101,6 +102,7 @@ import org.oppia.android.testing.time.FakeOppiaClockModule import org.oppia.android.util.accessibility.AccessibilityTestModule import org.oppia.android.util.caching.AssetModule import org.oppia.android.util.caching.testing.CachingTestModule +import org.oppia.android.util.extensions.getProto import org.oppia.android.util.gcsresource.GcsResourceModule import org.oppia.android.util.locale.LocaleProdModule import org.oppia.android.util.logging.CurrentAppScreenNameIntentDecorator.extractCurrentAppScreenName @@ -113,6 +115,7 @@ import org.oppia.android.util.networking.NetworkConnectionUtilDebugModule import org.oppia.android.util.parser.html.HtmlParserEntityTypeModule import org.oppia.android.util.parser.image.GlideImageLoaderModule import org.oppia.android.util.parser.image.ImageParsingModule +import org.oppia.android.util.profile.CurrentUserProfileIdIntentDecorator.extractCurrentUserProfileId import org.robolectric.annotation.Config import org.robolectric.annotation.LooperMode import javax.inject.Inject @@ -535,6 +538,32 @@ class SurveyFragmentTest { } } + @Test + fun testFragment_fragmentLoaded_verifyCorrectArgumentsPassed() { + launch( + createSurveyActivityIntent() + ).use { scenario -> + testCoroutineDispatchers.runCurrent() + scenario.onActivity { activity -> + + val surveyFragment = activity.supportFragmentManager + .findFragmentById(R.id.survey_fragment_placeholder) as SurveyFragment + val args = surveyFragment.arguments!!.getProto( + SurveyFragment.SURVEY_FRAGMENT_ARGUMENTS_KEY, + SurveyFragmentArguments.getDefaultInstance() + ) + val receivedInternalProfileId = surveyFragment.arguments!! + .extractCurrentUserProfileId().internalId + val receivedTopicId = args.topicId!! + val receivedExplorationId = args.explorationId!! + + assertThat(receivedInternalProfileId).isEqualTo(0) + assertThat(receivedTopicId).isEqualTo(TEST_TOPIC_ID_0) + assertThat(receivedExplorationId).isEqualTo(TEST_EXPLORATION_ID_2) + } + } + } + private fun selectNpsAnswerAndMoveToNextQuestion(npsScore: Int) { onView( allOf( diff --git a/app/src/sharedTest/java/org/oppia/android/app/thirdparty/ThirdPartyDependencyListFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/thirdparty/ThirdPartyDependencyListFragmentTest.kt index 89403f4efe2..08c49683c59 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/thirdparty/ThirdPartyDependencyListFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/thirdparty/ThirdPartyDependencyListFragmentTest.kt @@ -18,6 +18,7 @@ import androidx.test.espresso.matcher.ViewMatchers.isRoot import androidx.test.espresso.matcher.ViewMatchers.withId import androidx.test.espresso.matcher.ViewMatchers.withText import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth.assertThat import dagger.Component import org.hamcrest.Matchers.allOf import org.junit.After @@ -39,6 +40,8 @@ import org.oppia.android.app.devoptions.DeveloperOptionsModule import org.oppia.android.app.devoptions.DeveloperOptionsStarterModule import org.oppia.android.app.help.thirdparty.LicenseListActivity import org.oppia.android.app.help.thirdparty.ThirdPartyDependencyListActivity +import org.oppia.android.app.help.thirdparty.ThirdPartyDependencyListFragment +import org.oppia.android.app.model.ThirdPartyDependencyListFragmentArguments import org.oppia.android.app.player.state.itemviewmodel.SplitScreenInteractionModule import org.oppia.android.app.recyclerview.RecyclerViewMatcher.Companion.atPosition import org.oppia.android.app.shim.ViewBindingShimModule @@ -86,6 +89,7 @@ import org.oppia.android.testing.time.FakeOppiaClockModule import org.oppia.android.util.accessibility.AccessibilityTestModule import org.oppia.android.util.caching.AssetModule import org.oppia.android.util.caching.testing.CachingTestModule +import org.oppia.android.util.extensions.getProto import org.oppia.android.util.gcsresource.GcsResourceModule import org.oppia.android.util.locale.LocaleProdModule import org.oppia.android.util.logging.EventLoggingConfigurationModule @@ -447,6 +451,30 @@ class ThirdPartyDependencyListFragmentTest { } } + @Test + fun testFragment_fragmentLoaded_verifyCorrectArgumentsPassed() { + launch(ThirdPartyDependencyListActivity::class.java).use { scenario -> + testCoroutineDispatchers.runCurrent() + scenario.onActivity { activity -> + + val thirdPartyDependencyListFragment = activity.supportFragmentManager + .findFragmentById(R.id.third_party_dependency_list_fragment_placeholder) + as ThirdPartyDependencyListFragment + + val arguments = checkNotNull(thirdPartyDependencyListFragment.arguments) { + "Expected arguments to be passed to ThirdPartyDependencyListFragment" + } + val args = arguments.getProto( + "ThirdPartyDependencyListFragment.arguments", + ThirdPartyDependencyListFragmentArguments.getDefaultInstance() + ) + val receivedIsMultipane = args?.isMultipane ?: false + + assertThat(receivedIsMultipane).isEqualTo(false) + } + } + } + private fun retrieveDependencyName(id: Int): String { return ApplicationProvider.getApplicationContext() .resources.getString(id) diff --git a/app/src/sharedTest/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragmentTest.kt index 29dde65b725..8be20c6dc55 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragmentTest.kt @@ -26,6 +26,7 @@ import androidx.test.espresso.matcher.ViewMatchers.isRoot import androidx.test.espresso.matcher.ViewMatchers.withId import androidx.test.espresso.matcher.ViewMatchers.withText import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth.assertThat import dagger.Component import dagger.Module import dagger.Provides @@ -58,6 +59,7 @@ import org.oppia.android.app.model.OptionsActivityParams import org.oppia.android.app.model.ProfileId import org.oppia.android.app.model.ReadingTextSize import org.oppia.android.app.model.RevisionCardActivityParams +import org.oppia.android.app.model.RevisionCardFragmentArguments import org.oppia.android.app.model.WrittenTranslationLanguageSelection import org.oppia.android.app.options.OptionsActivity import org.oppia.android.app.player.exploration.ExplorationActivity @@ -122,6 +124,7 @@ import org.oppia.android.util.accessibility.AccessibilityTestModule import org.oppia.android.util.caching.AssetModule import org.oppia.android.util.caching.LoadImagesFromAssets import org.oppia.android.util.caching.LoadLessonProtosFromAssets +import org.oppia.android.util.extensions.getProto import org.oppia.android.util.gcsresource.GcsResourceModule import org.oppia.android.util.locale.LocaleProdModule import org.oppia.android.util.logging.EventLoggingConfigurationModule @@ -133,6 +136,7 @@ import org.oppia.android.util.networking.NetworkConnectionUtilDebugModule import org.oppia.android.util.parser.html.HtmlParserEntityTypeModule import org.oppia.android.util.parser.image.GlideImageLoaderModule import org.oppia.android.util.parser.image.ImageParsingModule +import org.oppia.android.util.profile.CurrentUserProfileIdIntentDecorator.extractCurrentUserProfileId import org.robolectric.annotation.Config import org.robolectric.annotation.LooperMode import javax.inject.Inject @@ -810,6 +814,45 @@ class RevisionCardFragmentTest { } } + @Test + fun testFragment_fragmentLoaded_verifyCorrectArgumentsPassed() { + launch( + createRevisionCardActivityIntent( + context, + profileId.internalId, + FRACTIONS_TOPIC_ID, + subtopicId = 2, + FRACTIONS_SUBTOPIC_LIST_SIZE + ) + ).use { scenario -> + testCoroutineDispatchers.runCurrent() + scenario.onActivity { activity -> + + val revisionCardFragment = activity.supportFragmentManager + .findFragmentById(R.id.revision_card_fragment_placeholder) as RevisionCardFragment + val arguments = checkNotNull(revisionCardFragment.arguments) { + "Expected arguments to be passed to StoryFragment" + } + val args = arguments.getProto( + RevisionCardFragment.REVISION_CARD_FRAGMENT_ARGUMENTS_KEY, + RevisionCardFragmentArguments.getDefaultInstance() + ) + val receivedTopicId = + checkNotNull(args?.topicId) { + "Expected topicId to be passed to RevisionCardFragment" + } + val receivedSubtopicId = args?.subtopicId ?: -1 + val receivedProfileId = arguments.extractCurrentUserProfileId() + val receivedSubtopicListSize = args?.subtopicListSize ?: -1 + + assertThat(receivedTopicId).isEqualTo(FRACTIONS_TOPIC_ID) + assertThat(receivedSubtopicId).isEqualTo(2) + assertThat(receivedProfileId).isEqualTo(profileId) + assertThat(receivedSubtopicListSize).isEqualTo(FRACTIONS_SUBTOPIC_LIST_SIZE) + } + } + } + /** See the version in StateFragmentTest for documentation details. */ @Suppress("SameParameterValue") private fun openClickableSpan(text: String): ViewAction { From aadc1a1d944d92beed0c7532d2fc0d60b5644d37 Mon Sep 17 00:00:00 2001 From: "Mr. 17" Date: Mon, 16 Sep 2024 17:40:16 +0530 Subject: [PATCH 5/5] Fix #5344: Remove temporary functions from TopicListController (#5528) ## Explanation Fixes #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 - [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 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/classroom/ClassroomController.kt | 56 +++++--- .../domain/topic/TopicListController.kt | 133 +++--------------- .../classroom/ClassroomControllerTest.kt | 17 +++ 3 files changed, 69 insertions(+), 137 deletions(-) diff --git a/domain/src/main/java/org/oppia/android/domain/classroom/ClassroomController.kt b/domain/src/main/java/org/oppia/android/domain/classroom/ClassroomController.kt index 20cc3ee832e..dc99f75c5ce 100644 --- a/domain/src/main/java/org/oppia/android/domain/classroom/ClassroomController.kt +++ b/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 { + 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 { - return if (loadLessonProtosFromAssets) { - assetRepository.loadProtoFromLocalAssets( - assetName = "classrooms", - baseMessage = ClassroomIdList.getDefaultInstance() - ).classroomIdsList.map { classroomId -> - loadClassroomById(classroomId) - } - } else loadClassroomsFromJson() - } - private fun loadClassroomsFromJson(): List { // 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 { 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 index 4fa2ed9edef..b82f8f56f0f 100644 --- a/domain/src/main/java/org/oppia/android/domain/topic/TopicListController.kt +++ b/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 { - 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 { 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 { // 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 { - 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 { - // 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() - - // 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 = - loadClassrooms().flatMap { it.topicPrerequisitesMap.keys.toList() } + private fun loadCombinedTopicIdList(): List = + classroomController.getClassrooms().flatMap { it.topicPrerequisitesMap.keys.toList() } } internal fun createTopicThumbnailFromJson(topicJsonObject: JSONObject): LessonThumbnail { diff --git a/domain/src/test/java/org/oppia/android/domain/classroom/ClassroomControllerTest.kt b/domain/src/test/java/org/oppia/android/domain/classroom/ClassroomControllerTest.kt index 24129abc307..6c1cee650bf 100644 --- a/domain/src/test/java/org/oppia/android/domain/classroom/ClassroomControllerTest.kt +++ b/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)