From 2c6f8a87735ea15fc4fc702c547bcf4ed21d1a21 Mon Sep 17 00:00:00 2001 From: Manas <119405883+manas-yu@users.noreply.github.com> Date: Mon, 17 Feb 2025 14:45:00 +0530 Subject: [PATCH 1/8] Fix #4848 Content Description Generation for Content and SelectionInteractionContent ViewModels (#5704) ## Explanation Fix #4848 This PR addresses post-PR review issues from #5614 by refining content description generation. The key updates include: - The `getContentDescription` function is now utilized in `ContentViewModel` and `SelectionInteractionContentViewModel`, ensuring accurate content descriptions with the necessary `customTagHandlers` 1. `ContentViewModel`: Handles `CUSTOM_LIST_LI_TAG`, `CUSTOM_LIST_OL_TAG`, `CUSTOM_LIST_UL_TAG`, `CUSTOM_IMG_TAG`, `CUSTOM_CONCEPT_CARD_TAG`, and `CUSTOM_MATH_TAG` 2. `SelectionInteractionContentViewModel`: Handles `CUSTOM_IMG_TAG` - Improved handling of tags where content resides in attributes, such as anchor tags: `Click here`, ensuring proper extraction of meaningful descriptions - Handled edge cases in `CustomHtmlContentHandler` to improve `getContentDescription` logic, addressing inconsistencies and included comments for better understanding. ## 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)). --- .../state/StatePlayerRecyclerViewAssembler.kt | 54 +++++++++++-- .../state/itemviewmodel/ContentViewModel.kt | 33 ++++---- .../SelectionInteractionContentViewModel.kt | 22 +++++- .../SelectionInteractionViewModel.kt | 38 +++++++-- app/src/main/res/layout/content_item.xml | 2 +- .../item_selection_interaction_items.xml | 1 + .../multiple_choice_interaction_items.xml | 1 + .../app/player/state/StateFragmentTest.kt | 78 ++++++++++++++++++- .../parser/html/CustomHtmlContentHandler.kt | 34 ++++++-- .../android/util/parser/html/LiTagHandler.kt | 7 +- .../html/CustomHtmlContentHandlerTest.kt | 18 +++++ .../util/parser/html/LiTagHandlerTest.kt | 51 ------------ 12 files changed, 238 insertions(+), 101 deletions(-) diff --git a/app/src/main/java/org/oppia/android/app/player/state/StatePlayerRecyclerViewAssembler.kt b/app/src/main/java/org/oppia/android/app/player/state/StatePlayerRecyclerViewAssembler.kt index 8f1e627d77a..5126bd59028 100644 --- a/app/src/main/java/org/oppia/android/app/player/state/StatePlayerRecyclerViewAssembler.kt +++ b/app/src/main/java/org/oppia/android/app/player/state/StatePlayerRecyclerViewAssembler.kt @@ -1,5 +1,6 @@ package org.oppia.android.app.player.state +import android.app.Application import android.content.Context import android.view.LayoutInflater import android.view.View @@ -91,7 +92,18 @@ import org.oppia.android.databinding.SubmittedHtmlAnswerItemBinding import org.oppia.android.databinding.TextInputInteractionItemBinding import org.oppia.android.domain.translation.TranslationController import org.oppia.android.util.accessibility.AccessibilityService +import org.oppia.android.util.logging.ConsoleLogger +import org.oppia.android.util.parser.html.CUSTOM_CONCEPT_CARD_TAG +import org.oppia.android.util.parser.html.CUSTOM_IMG_TAG +import org.oppia.android.util.parser.html.CUSTOM_LIST_LI_TAG +import org.oppia.android.util.parser.html.CUSTOM_LIST_OL_TAG +import org.oppia.android.util.parser.html.CUSTOM_LIST_UL_TAG +import org.oppia.android.util.parser.html.CUSTOM_MATH_TAG +import org.oppia.android.util.parser.html.ConceptCardTagHandler import org.oppia.android.util.parser.html.HtmlParser +import org.oppia.android.util.parser.html.ImageTagHandler +import org.oppia.android.util.parser.html.LiTagHandler +import org.oppia.android.util.parser.html.MathTagHandler import org.oppia.android.util.threading.BackgroundDispatcher import javax.inject.Inject @@ -145,7 +157,9 @@ class StatePlayerRecyclerViewAssembler private constructor( private val hasConversationView: Boolean, private val resourceHandler: AppLanguageResourceHandler, private val translationController: TranslationController, - private var userAnswerState: UserAnswerState + private var userAnswerState: UserAnswerState, + private val consoleLogger: ConsoleLogger, + private val conceptCardTagHandlerFactory: ConceptCardTagHandler.Factory, ) : HtmlParser.CustomOppiaTagActionListener { /** * A list of view models corresponding to past view models that are hidden by default. These are @@ -180,6 +194,25 @@ class StatePlayerRecyclerViewAssembler private constructor( } } + private val displayLocale = resourceHandler.getDisplayLocale() + private val customTagHandlers = mapOf( + CUSTOM_LIST_LI_TAG to LiTagHandler(context, displayLocale), + CUSTOM_LIST_UL_TAG to LiTagHandler(context, displayLocale), + CUSTOM_LIST_OL_TAG to LiTagHandler(context, displayLocale), + CUSTOM_IMG_TAG to ImageTagHandler(consoleLogger), + CUSTOM_CONCEPT_CARD_TAG to ConceptCardTagHandler( + conceptCardTagHandlerFactory.createConceptCardLinkClickListener(), + consoleLogger + ), + // Pick an arbitrary line height since rendering doesn't actually happen. + CUSTOM_MATH_TAG to MathTagHandler( + consoleLogger, + context.assets, + 10.0f, + false, + context.applicationContext as Application, + ) + ) private val isSplitView = ObservableField(false) override fun onConceptCardLinkClicked(view: View, skillId: String) { @@ -350,7 +383,8 @@ class StatePlayerRecyclerViewAssembler private constructor( gcsEntityId, hasConversationView, isSplitView.get()!!, - playerFeatureSet.conceptCardSupport + playerFeatureSet.conceptCardSupport, + customTagHandlers ) } } @@ -913,7 +947,9 @@ class StatePlayerRecyclerViewAssembler private constructor( private val translationController: TranslationController, private val multiTypeBuilderFactory: BindableAdapter.MultiTypeBuilder.Factory, private val singleTypeBuilderFactory: BindableAdapter.SingleTypeBuilder.Factory, - private val userAnswerState: UserAnswerState + private val userAnswerState: UserAnswerState, + private val consoleLogger: ConsoleLogger, + private val conceptCardTagHandlerFactory: ConceptCardTagHandler.Factory, ) { private val adapterBuilder: BindableAdapter.MultiTypeBuilder ) : StateItemViewModel(ViewType.CONTENT) { private val underscoreRegex = Regex("(?<=\\s|[,.;?!])_{3,}(?=\\s|[,.;?!])") private val replacementText = "Blank" + /** Returns content description by extracting text from [htmlContent]. */ + fun getContentDescription(): String { + val contentDescription = CustomHtmlContentHandler.getContentDescription( + htmlContent.toString(), + imageRetriever = null, + customTagHandlers = customTagHandlers + ) + return replaceRegexWithBlank(contentDescription) + } + /** * Replaces "2+ underscores, with space/punctuation on both sides" in the input text with a * replacement string "blank", returning a Spannable. * Adjusts offsets to handle text length changes during replacements. */ - fun replaceRegexWithBlank(inputText: CharSequence): Spannable { - val spannableStringBuilder = SpannableStringBuilder(inputText) - val matches = underscoreRegex.findAll(inputText) - var lengthOffset = 0 - - for (match in matches) { - val matchStart = match.range.first + lengthOffset - val matchEnd = match.range.last + 1 + lengthOffset - spannableStringBuilder.replace(matchStart, matchEnd, replacementText) - - // Adjust offset due to change in length (difference between old and new text length) - lengthOffset += replacementText.length - (matchEnd - matchStart) - } - return spannableStringBuilder - } + private fun replaceRegexWithBlank(inputText: CharSequence): String = + underscoreRegex.replace(inputText, replacementText) } diff --git a/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionContentViewModel.kt b/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionContentViewModel.kt index 04fea533655..71eabdf43ea 100644 --- a/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionContentViewModel.kt +++ b/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionContentViewModel.kt @@ -2,7 +2,10 @@ package org.oppia.android.app.player.state.itemviewmodel import androidx.databinding.ObservableBoolean import org.oppia.android.app.model.SubtitledHtml +import org.oppia.android.app.model.WrittenTranslationContext import org.oppia.android.app.viewmodel.ObservableViewModel +import org.oppia.android.domain.translation.TranslationController +import org.oppia.android.util.parser.html.CustomHtmlContentHandler /** [ObservableViewModel] for MultipleChoiceInput values or ItemSelectionInput values. */ class SelectionInteractionContentViewModel( @@ -10,10 +13,27 @@ class SelectionInteractionContentViewModel( val hasConversationView: Boolean, private val itemIndex: Int, private val selectionInteractionViewModel: SelectionInteractionViewModel, - val isEnabled: ObservableBoolean + val isEnabled: ObservableBoolean, + val customTagHandlers: Map, + private val writtenTranslationContext: WrittenTranslationContext, + private val translationController: TranslationController, ) : ObservableViewModel() { var isAnswerSelected = ObservableBoolean() + /** Returns content description by extracting text from [htmlContent]. */ + fun getContentDescription(): String { + val contentSubtitledHtml = + translationController.extractString( + htmlContent, writtenTranslationContext + ) + return CustomHtmlContentHandler.getContentDescription( + contentSubtitledHtml, + imageRetriever = null, + customTagHandlers = customTagHandlers + ) + } + + /** Handles item click by updating the selection state based on user interaction. */ fun handleItemClicked() { val isCurrentlySelected = isAnswerSelected.get() val shouldNowBeSelected = diff --git a/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionViewModel.kt b/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionViewModel.kt index d5f4b7016c7..a0604b8d326 100644 --- a/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionViewModel.kt +++ b/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionViewModel.kt @@ -22,6 +22,10 @@ import org.oppia.android.app.player.state.answerhandling.InteractionAnswerReceiv import org.oppia.android.app.translation.AppLanguageResourceHandler import org.oppia.android.app.viewmodel.ObservableArrayList import org.oppia.android.domain.translation.TranslationController +import org.oppia.android.util.logging.ConsoleLogger +import org.oppia.android.util.parser.html.CUSTOM_IMG_TAG +import org.oppia.android.util.parser.html.CustomHtmlContentHandler +import org.oppia.android.util.parser.html.ImageTagHandler import javax.inject.Inject /** Corresponds to the type of input that should be used for an item selection interaction view. */ @@ -52,7 +56,8 @@ class SelectionInteractionViewModel private constructor( val writtenTranslationContext: WrittenTranslationContext, private val translationController: TranslationController, private val resourceHandler: AppLanguageResourceHandler, - userAnswerState: UserAnswerState + userAnswerState: UserAnswerState, + consoleLogger: ConsoleLogger ) : StateItemViewModel(ViewType.SELECTION_INTERACTION), InteractionAnswerHandler { private val interactionId: String = interaction.id @@ -81,9 +86,20 @@ class SelectionInteractionViewModel private constructor( ObservableBoolean(true) } } - val choiceItems: ObservableList = - computeChoiceItems(choiceSubtitledHtmls, hasConversationView, this, enabledItemsList) + private val customTagHandlers = mapOf( + CUSTOM_IMG_TAG to ImageTagHandler(consoleLogger) + ) + val choiceItems: ObservableList = + computeChoiceItems( + choiceSubtitledHtmls, + hasConversationView, + this, + enabledItemsList, + this@SelectionInteractionViewModel.writtenTranslationContext, + translationController, + customTagHandlers + ) private var pendingAnswerError: String? = null private val isAnswerAvailable = ObservableField(false) val errorMessage = ObservableField("") @@ -287,7 +303,8 @@ class SelectionInteractionViewModel private constructor( /** Implementation of [StateItemViewModel.InteractionItemFactory] for this view model. */ class FactoryImpl @Inject constructor( private val translationController: TranslationController, - private val resourceHandler: AppLanguageResourceHandler + private val resourceHandler: AppLanguageResourceHandler, + private val consoleLogger: ConsoleLogger ) : InteractionItemFactory { override fun create( entityId: String, @@ -310,7 +327,8 @@ class SelectionInteractionViewModel private constructor( writtenTranslationContext, translationController, resourceHandler, - userAnswerState + userAnswerState, + consoleLogger ) } } @@ -320,7 +338,10 @@ class SelectionInteractionViewModel private constructor( choiceSubtitledHtmls: List, hasConversationView: Boolean, selectionInteractionViewModel: SelectionInteractionViewModel, - enabledItemsList: List + enabledItemsList: List, + writtenTranslationContext: WrittenTranslationContext, + translationController: TranslationController, + customTagHandlers: Map ): ObservableArrayList { val observableList = ObservableArrayList() observableList += choiceSubtitledHtmls.mapIndexed { index, subtitledHtml -> @@ -329,7 +350,10 @@ class SelectionInteractionViewModel private constructor( hasConversationView = hasConversationView, itemIndex = index, selectionInteractionViewModel = selectionInteractionViewModel, - isEnabled = enabledItemsList[index] + isEnabled = enabledItemsList[index], + customTagHandlers, + writtenTranslationContext, + translationController ) } return observableList diff --git a/app/src/main/res/layout/content_item.xml b/app/src/main/res/layout/content_item.xml index a29a44814fe..192d0a42642 100644 --- a/app/src/main/res/layout/content_item.xml +++ b/app/src/main/res/layout/content_item.xml @@ -50,7 +50,7 @@ android:minWidth="48dp" android:minHeight="48dp" android:text="@{htmlContent}" - android:contentDescription="@{viewModel.replaceRegexWithBlank(htmlContent)}" + android:contentDescription="@{viewModel.getContentDescription()}" android:textColor="@color/component_color_shared_primary_text_color" android:textColorLink="@color/component_color_shared_link_text_color" android:textSize="16sp" diff --git a/app/src/main/res/layout/item_selection_interaction_items.xml b/app/src/main/res/layout/item_selection_interaction_items.xml index 7fe2e6a5ba5..c7537f23e0e 100755 --- a/app/src/main/res/layout/item_selection_interaction_items.xml +++ b/app/src/main/res/layout/item_selection_interaction_items.xml @@ -43,6 +43,7 @@ android:layout_toEndOf="@+id/item_selection_checkbox" android:fontFamily="sans-serif" android:text="@{htmlContent}" + android:contentDescription="@{viewModel.getContentDescription()}" android:textColor="@{viewModel.isEnabled ? @color/component_color_shared_item_selection_interaction_enabled_color : @color/component_color_shared_item_selection_interaction_disabled_color}" android:textSize="16sp" /> diff --git a/app/src/main/res/layout/multiple_choice_interaction_items.xml b/app/src/main/res/layout/multiple_choice_interaction_items.xml index dcbf7239e1f..997f88601e3 100755 --- a/app/src/main/res/layout/multiple_choice_interaction_items.xml +++ b/app/src/main/res/layout/multiple_choice_interaction_items.xml @@ -43,6 +43,7 @@ android:layout_toEndOf="@+id/multiple_choice_radio_button" android:fontFamily="sans-serif" android:text="@{htmlContent}" + android:contentDescription="@{viewModel.getContentDescription()}" android:textColor="@{viewModel.isAnswerSelected() ? @color/component_color_shared_selection_interaction_selected_text_color : @color/component_color_shared_selection_interaction_unselected_text_color}" android:textSize="16sp" /> diff --git a/app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt index 5f436a48dc7..50003cdc606 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt @@ -5230,16 +5230,80 @@ class StateFragmentTest { playThroughRatioExplorationState13() playThroughRatioExplorationState14() - val expectedDescription = "James turned the page, and saw a recipe for banana smoothie." + - " Yummy!\n\n2 cups of milk and 1 cup of banana puree \n\n“I can make this,” he said." + - " “We’ll need to mix milk and banana puree in the ratio Blank.”\n\nCan you complete" + - " James’s sentence? What is the ratio of milk to banana puree?”" + val expectedDescription = "James turned the page, and saw a recipe for banana smoothie. " + + "Yummy!\nImage illustrating 2 cups of milk and 1 cup of banana puree\n“I can make this," + + "” he said. “We’ll need to mix milk and banana puree in the ratio Blank.”\nCan you " + + "complete James’s sentence? What is the ratio of milk to banana puree?”" onView(withId(R.id.content_text_view)) .check(matches(withContentDescription(expectedDescription))) } } + @Test + fun testStateFragment_contentDescription_itemSelectionInteraction() { + setUpTestWithLanguageSwitchingFeatureOff() + launchForExploration(TEST_EXPLORATION_ID_2, shouldSavePartialProgress = false).use { + startPlayingExploration() + playThroughPrototypeState1() + playThroughPrototypeState2() + playThroughPrototypeState3() + playThroughPrototypeState4() + verifyViewTypeIsPresent(SELECTION_INTERACTION) + + onView( + atPositionOnView( + recyclerViewId = R.id.selection_interaction_recyclerview, + position = 2, + targetViewId = R.id.item_selection_contents_text_view + ) + ).check(matches(withContentDescription("Green"))) + + onView( + atPositionOnView( + recyclerViewId = R.id.selection_interaction_recyclerview, + position = 3, + targetViewId = R.id.item_selection_contents_text_view + ) + ).check(matches(withContentDescription("Blue"))) + } + } + + @Test + fun testStateFragment_contentDescription_multipleChoiceInteraction() { + setUpTestWithLanguageSwitchingFeatureOff() + launchForExploration(RATIOS_EXPLORATION_ID_0, shouldSavePartialProgress = false).use { + startPlayingExploration() + + playThroughRatioExplorationState1() + playThroughRatioExplorationState2() + playThroughRatioExplorationState3() + playThroughRatioExplorationState4() + playThroughRatioExplorationState5() + playThroughRatioExplorationState6() + playThroughRatioExplorationState7() + playThroughRatioExplorationState8() + playThroughRatioExplorationState9() + playThroughRatioExplorationState10() + playThroughRatioExplorationState11() + playThroughRatioExplorationState12() + playThroughRatioExplorationState13() + playThroughRatioExplorationState14() + playThroughRatioExplorationState15() + + val expectedDescription = "Image illustrating On the left, there is 1-unit wide glass " + + "filled with apple puree. On the right, there is 2-unit wide glass filled with milk." + + onView( + atPositionOnView( + recyclerViewId = R.id.selection_interaction_recyclerview, + position = 2, + targetViewId = R.id.multiple_choice_content_text_view + ) + ).check(matches(withContentDescription(expectedDescription))) + } + } + @Test fun testFragment_argumentsAreCorrect() { setUpTestWithLanguageSwitchingFeatureOff() @@ -5345,6 +5409,12 @@ class StateFragmentTest { clickContinueNavigationButton() } + private fun playThroughRatioExplorationState15() { + typeTextInput("2:1") + clickSubmitAnswerButton() + clickContinueNavigationButton() + } + private fun addShadowMediaPlayerException(dataSource: Any, exception: Exception) { val classLoader = StateFragmentTest::class.java.classLoader!! val shadowMediaPlayerClass = classLoader.loadClass("org.robolectric.shadows.ShadowMediaPlayer") diff --git a/utility/src/main/java/org/oppia/android/util/parser/html/CustomHtmlContentHandler.kt b/utility/src/main/java/org/oppia/android/util/parser/html/CustomHtmlContentHandler.kt index 25cc7bf29d3..0e9988be8b4 100644 --- a/utility/src/main/java/org/oppia/android/util/parser/html/CustomHtmlContentHandler.kt +++ b/utility/src/main/java/org/oppia/android/util/parser/html/CustomHtmlContentHandler.kt @@ -28,12 +28,14 @@ class CustomHtmlContentHandler private constructor( private val contentDescriptionBuilder = StringBuilder() private val tagContentDescriptions = mutableMapOf() private var isInListItem = false - private var pendingNewline = false private val blockTags = setOf("p", "ol", "ul", "li", "oppia-ul", "oppia-ol", "oppia-li", "div") + // Indicates if a newline should be added before the next text content for block elements. + private var pendingNewline = false override fun endElement(uri: String?, localName: String?, qName: String?) { originalContentHandler?.endElement(uri, localName, qName) - if (localName in blockTags) { + val tagName = qName ?: localName + if (tagName in blockTags) { isInListItem = false } currentTrackedTag = null @@ -57,7 +59,7 @@ class CustomHtmlContentHandler private constructor( contentDescriptionBuilder.append('\n') pendingNewline = false } - ch?.let { contentDescriptionBuilder.append(String(it, start, length)) } + ch?.let { contentDescriptionBuilder.appendRange(it, start, start + length) } } override fun endDocument() { @@ -69,10 +71,17 @@ class CustomHtmlContentHandler private constructor( // Defer custom tag management to the tag handler so that Android's element parsing takes // precedence. currentTrackedTag = TrackedTag(checkNotNull(localName), checkNotNull(atts)) - if (localName in blockTags) { + val tagName = qName ?: localName + if (tagName in blockTags) { pendingNewline = true isInListItem = true } + if (tagName == "a") { + val href = atts.getValue("href") + if (href != null) { + tagContentDescriptions[contentDescriptionBuilder.length] = "$href " + } + } originalContentHandler?.startElement(uri, localName, qName, atts) } @@ -148,6 +157,7 @@ class CustomHtmlContentHandler private constructor( val attributes: Attributes, val openTagIndex: Int ) + /** * Returns the complete content description for the processed HTML, including descriptions * from all custom tags. @@ -156,11 +166,16 @@ class CustomHtmlContentHandler private constructor( val rawDesc = buildString { var lastIndex = 0 tagContentDescriptions.entries.sortedBy { it.key }.forEach { (index, description) -> - if (index > lastIndex) { - append(contentDescriptionBuilder.substring(lastIndex, index)) + if (index > lastIndex && index <= contentDescriptionBuilder.length) { + append( + contentDescriptionBuilder.substring( + lastIndex, + minOf(index, contentDescriptionBuilder.length) + ) + ) } append(description) - lastIndex = index + lastIndex = minOf(index, contentDescriptionBuilder.length) } if (lastIndex < contentDescriptionBuilder.length) { append(contentDescriptionBuilder.substring(lastIndex)) @@ -168,6 +183,7 @@ class CustomHtmlContentHandler private constructor( } return rawDesc.replace(Regex("\n+"), "\n").trim() } + /** Handler interface for a custom tag and its attributes. */ interface CustomTagHandler { /** @@ -259,6 +275,9 @@ class CustomHtmlContentHandler private constructor( customTagHandlers: Map ): String where T : Html.ImageGetter, T : ImageRetriever { val handler = CustomHtmlContentHandler(customTagHandlers, imageRetriever) + + // Triggers the HTML parsing process, allowing CustomHtmlContentHandler to + // intercept and populate the contentDescriptionBuilder. HtmlCompat.fromHtml( "$html", HtmlCompat.FROM_HTML_MODE_LEGACY, @@ -267,6 +286,7 @@ class CustomHtmlContentHandler private constructor( ) return handler.getContentDescription() } + /** * Returns a new [Spannable] with HTML parsed from [html] using the specified [imageRetriever] * for handling image retrieval, and map of tags to [CustomTagHandler]s for handling custom diff --git a/utility/src/main/java/org/oppia/android/util/parser/html/LiTagHandler.kt b/utility/src/main/java/org/oppia/android/util/parser/html/LiTagHandler.kt index 28321115984..239979e9878 100644 --- a/utility/src/main/java/org/oppia/android/util/parser/html/LiTagHandler.kt +++ b/utility/src/main/java/org/oppia/android/util/parser/html/LiTagHandler.kt @@ -9,7 +9,6 @@ import android.text.Spanned import android.text.style.ImageSpan import androidx.core.view.ViewCompat import org.oppia.android.util.locale.OppiaLocale -import org.xml.sax.Attributes import java.util.Stack /** The custom
  • tag corresponding to [LiTagHandler]. */ @@ -28,7 +27,7 @@ const val CUSTOM_LIST_OL_TAG = "oppia-ol" class LiTagHandler( private val context: Context, private val displayLocale: OppiaLocale.DisplayLocale -) : CustomHtmlContentHandler.CustomTagHandler, CustomHtmlContentHandler.ContentDescriptionProvider { +) : CustomHtmlContentHandler.CustomTagHandler { private val pendingLists = Stack>() private val latestPendingList: ListTag<*, *>? get() = pendingLists.lastOrNull() @@ -366,8 +365,4 @@ class LiTagHandler( private fun > Spannable.addMark(mark: T) = setSpan(mark, length, length, Spanned.SPAN_MARK_MARK) } - - override fun getContentDescription(attributes: Attributes): String { - return "" - } } diff --git a/utility/src/test/java/org/oppia/android/util/parser/html/CustomHtmlContentHandlerTest.kt b/utility/src/test/java/org/oppia/android/util/parser/html/CustomHtmlContentHandlerTest.kt index 8b11dcba7f0..ab491cf396c 100644 --- a/utility/src/test/java/org/oppia/android/util/parser/html/CustomHtmlContentHandlerTest.kt +++ b/utility/src/test/java/org/oppia/android/util/parser/html/CustomHtmlContentHandlerTest.kt @@ -402,6 +402,24 @@ class CustomHtmlContentHandlerTest { ) } + @Test + fun testGetContentDescription_anchorTag_handlesCorrectly() { + val contentDescription = CustomHtmlContentHandler.getContentDescription( + html = + """ +

    Visit Example for more info.

    + Another Link + """.trimIndent(), + imageRetriever = mockImageRetriever, + customTagHandlers = mapOf() + ) + + assertThat(contentDescription).isEqualTo( + "Visit https://example.com Example for more info.\n" + + "https://another.com Another Link" + ) + } + private fun Spannable.getSpansFromWholeString(spanClass: KClass): Array = getSpans(/* start= */ 0, /* end= */ length, spanClass.javaObjectType) diff --git a/utility/src/test/java/org/oppia/android/util/parser/html/LiTagHandlerTest.kt b/utility/src/test/java/org/oppia/android/util/parser/html/LiTagHandlerTest.kt index 13246b7d2d8..3a2f2085326 100644 --- a/utility/src/test/java/org/oppia/android/util/parser/html/LiTagHandlerTest.kt +++ b/utility/src/test/java/org/oppia/android/util/parser/html/LiTagHandlerTest.kt @@ -165,57 +165,6 @@ class LiTagHandlerTest { .hasLength(4) } - @Test - fun testGetContentDescription_handlesNestedOrderedList() { - val displayLocale = createDisplayLocaleImpl(US_ENGLISH_CONTEXT) - val htmlString = "

    You should know the following before going on:

    " + - "The counting numbers (1, 2, 3, 4, 5 ….)" + - "How to tell whether one counting number is bigger or " + - "smaller than another Item 1 Item 2" + - "" - val liTaghandler = LiTagHandler(context, displayLocale) - val contentDescription = - CustomHtmlContentHandler.getContentDescription( - html = htmlString, - imageRetriever = mockImageRetriever, - customTagHandlers = mapOf( - CUSTOM_LIST_LI_TAG to liTaghandler, - CUSTOM_LIST_OL_TAG to liTaghandler - ) - ) - assertThat(contentDescription).isEqualTo( - "You should know the following before going on:\n" + - "The counting numbers (1, 2, 3, 4, 5 ….)\n" + - "How to tell whether one counting number is bigger or smaller than another \n" + - "Item 1 \n" + - "Item 2" - ) - } - - @Test - fun testGetContentDescription_handlesSimpleUnorderedList() { - val displayLocale = createDisplayLocaleImpl(US_ENGLISH_CONTEXT) - val htmlString = "

    You should know the following before going on:
    " + - "The counting numbers (1, 2, 3, 4, 5 ….)" + - "How to tell whether one counting number is bigger or " + - "smaller than another

    " - val liTaghandler = LiTagHandler(context, displayLocale) - val contentDescription = - CustomHtmlContentHandler.getContentDescription( - html = htmlString, - imageRetriever = mockImageRetriever, - customTagHandlers = mapOf( - CUSTOM_LIST_LI_TAG to liTaghandler, - CUSTOM_LIST_OL_TAG to liTaghandler - ) - ) - assertThat(contentDescription).isEqualTo( - "You should know the following before going on:\n" + - "The counting numbers (1, 2, 3, 4, 5 ….)\n" + - "How to tell whether one counting number is bigger or smaller than another" - ) - } - private fun createDisplayLocaleImpl(context: OppiaLocaleContext): DisplayLocaleImpl { val formattingLocale = androidLocaleFactory.createOneOffAndroidLocale(context) return DisplayLocaleImpl(context, formattingLocale, machineLocale, formatterFactory) From d7eaad8ef8ddff8f7aa7c88a7c33212bca927c3d Mon Sep 17 00:00:00 2001 From: Manas <119405883+manas-yu@users.noreply.github.com> Date: Tue, 18 Feb 2025 07:06:45 +0530 Subject: [PATCH 2/8] Fix #3708: Content Description Generation For ImageRegionSelectionInteraction (#5691) ## Explanation Fix #3708 This PR adds logic for dynamic content descriptions in **ImageRegionSelectionInteraction**, i.e. generating distinct descriptions for selected and not selected regions. For example: - **Selected:** "Image showing Region 3." - **Not Selected:** "Select Region 3 image." ## Before ![image](https://github.com/user-attachments/assets/ddce71d7-f092-431a-af12-2d8e7ec727f3) ![image](https://github.com/user-attachments/assets/93764cf6-a1b8-4fa0-81b0-62b36a6f09d9) ## After ![image](https://github.com/user-attachments/assets/b23193f2-de50-4e30-a970-f45296f29483) ![image](https://github.com/user-attachments/assets/98e7f591-85df-4cb4-bb66-d38c1fb4e808) ## 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)). --- .../ImageRegionSelectionInteractionView.kt | 12 ++-- .../app/utility/ClickableAreasImage.kt | 64 ++++++++++++++++--- app/src/main/res/values/strings.xml | 2 + ...ImageRegionSelectionInteractionViewTest.kt | 27 ++++++-- 4 files changed, 85 insertions(+), 20 deletions(-) diff --git a/app/src/main/java/org/oppia/android/app/player/state/ImageRegionSelectionInteractionView.kt b/app/src/main/java/org/oppia/android/app/player/state/ImageRegionSelectionInteractionView.kt index e7f2dbe3e2e..a113976602f 100644 --- a/app/src/main/java/org/oppia/android/app/player/state/ImageRegionSelectionInteractionView.kt +++ b/app/src/main/java/org/oppia/android/app/player/state/ImageRegionSelectionInteractionView.kt @@ -11,6 +11,7 @@ import androidx.fragment.app.FragmentManager import org.oppia.android.app.model.ImageWithRegions import org.oppia.android.app.model.UserAnswerState import org.oppia.android.app.shim.ViewBindingShim +import org.oppia.android.app.translation.AppLanguageResourceHandler import org.oppia.android.app.utility.ClickableAreasImage import org.oppia.android.app.utility.OnClickableAreaClickedListener import org.oppia.android.app.view.ViewComponentFactory @@ -46,6 +47,7 @@ class ImageRegionSelectionInteractionView @JvmOverloads constructor( @Inject lateinit var machineLocale: OppiaLocale.MachineLocale @Inject lateinit var accessibilityService: AccessibilityService @Inject lateinit var imageLoader: ImageLoader + @Inject lateinit var resourceHandler: AppLanguageResourceHandler private lateinit var entityId: String private lateinit var overlayView: FrameLayout @@ -64,8 +66,8 @@ class ImageRegionSelectionInteractionView @JvmOverloads constructor( maybeInitializeClickableAreas() } - fun setUserAnswerState(userAnswerrState: UserAnswerState) { - this.userAnswerState = userAnswerrState + fun setUserAnswerState(userAnswerState: UserAnswerState) { + this.userAnswerState = userAnswerState } fun setEntityId(entityId: String) { @@ -118,7 +120,8 @@ class ImageRegionSelectionInteractionView @JvmOverloads constructor( ::entityId.isInitialized && ::imageUrl.isInitialized && ::onRegionClicked.isInitialized && - ::overlayView.isInitialized + ::overlayView.isInitialized && + ::resourceHandler.isInitialized ) { loadImage() @@ -129,7 +132,8 @@ class ImageRegionSelectionInteractionView @JvmOverloads constructor( bindingInterface, isAccessibilityEnabled = accessibilityService.isScreenReaderEnabled(), clickableAreas, - userAnswerState + userAnswerState, + resourceHandler ) areasImage.addRegionViews() performAttachment(areasImage) diff --git a/app/src/main/java/org/oppia/android/app/utility/ClickableAreasImage.kt b/app/src/main/java/org/oppia/android/app/utility/ClickableAreasImage.kt index fa77fa271de..dd57c27f29a 100644 --- a/app/src/main/java/org/oppia/android/app/utility/ClickableAreasImage.kt +++ b/app/src/main/java/org/oppia/android/app/utility/ClickableAreasImage.kt @@ -13,6 +13,7 @@ import org.oppia.android.app.model.ImageWithRegions.LabeledRegion import org.oppia.android.app.model.UserAnswerState import org.oppia.android.app.player.state.ImageRegionSelectionInteractionView import org.oppia.android.app.shim.ViewBindingShim +import org.oppia.android.app.translation.AppLanguageResourceHandler import kotlin.math.roundToInt /** Helper class to handle clicks on an image along with highlighting the selected region. */ @@ -23,7 +24,8 @@ class ClickableAreasImage( bindingInterface: ViewBindingShim, private val isAccessibilityEnabled: Boolean, private val clickableAreas: List, - userAnswerState: UserAnswerState + userAnswerState: UserAnswerState, + private val resourceHandler: AppLanguageResourceHandler ) { private var imageLabel: String? = null private val defaultRegionView by lazy { bindingInterface.getDefaultRegion(parentView) } @@ -60,6 +62,16 @@ class ClickableAreasImage( // Remove any previously selected region excluding 0th index(image view) if (index > 0) { childView.setBackgroundResource(0) + if (childView.tag != null) { + val regionLabel = childView.tag as String + clickableAreas.find { it.label == regionLabel }?.let { clickableArea -> + updateRegionContentDescription( + childView, + clickableArea, + regionLabel == imageLabel + ) + } + } } } } @@ -112,8 +124,13 @@ class ClickableAreasImage( newView.isFocusable = true newView.isFocusableInTouchMode = true newView.tag = clickableArea.label + + val isInitiallySelected = clickableArea.label.equals(imageLabel) + updateRegionContentDescription(newView, clickableArea, isInitiallySelected) + newView.initializeToggleRegionTouchListener(clickableArea) - if (clickableArea.label.equals(imageLabel)) { + + if (isInitiallySelected) { showOrHideRegion(newView = newView, clickableArea = clickableArea) } if (isAccessibilityEnabled) { @@ -123,7 +140,7 @@ class ClickableAreasImage( showOrHideRegion(newView, clickableArea) } } - newView.contentDescription = clickableArea.contentDescription + parentView.addView(newView) } @@ -143,14 +160,18 @@ class ClickableAreasImage( } private fun showOrHideRegion(newView: View, clickableArea: LabeledRegion) { - resetRegionSelectionViews() - listener.onClickableAreaTouched( - NamedRegionClickedEvent( - clickableArea.label, - clickableArea.contentDescription + if (clickableArea.label != imageLabel) { + imageLabel = clickableArea.label + resetRegionSelectionViews() + newView.setBackgroundResource(R.drawable.selected_region_background) + + listener.onClickableAreaTouched( + NamedRegionClickedEvent( + clickableArea.label, + generateContentDescription(clickableArea, true) + ) ) - ) - newView.setBackgroundResource(R.drawable.selected_region_background) + } } private fun View.initializeShowRegionTouchListener() { @@ -172,4 +193,27 @@ class ClickableAreasImage( return@setOnTouchListener true } } + + private fun generateContentDescription( + clickableArea: LabeledRegion, + isSelected: Boolean + ): String = if (isSelected) { + resourceHandler.getStringInLocaleWithWrapping( + R.string.selected_image_region_selection_content_description, + clickableArea.label + ) + } else { + resourceHandler.getStringInLocaleWithWrapping( + R.string.unselected_image_region_selection_content_description, + clickableArea.label + ) + } + + private fun updateRegionContentDescription( + view: View, + clickableArea: LabeledRegion, + isSelected: Boolean + ) { + view.contentDescription = generateContentDescription(clickableArea, isSelected) + } } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 4d6299de34d..4eb884b7ab1 100755 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -191,6 +191,8 @@ Correct! Topic: %s %1$s %2$s! + Image showing %1$s. + Select %1$s image. 1 Chapter %s Chapters diff --git a/app/src/sharedTest/java/org/oppia/android/app/testing/ImageRegionSelectionInteractionViewTest.kt b/app/src/sharedTest/java/org/oppia/android/app/testing/ImageRegionSelectionInteractionViewTest.kt index 6b558212760..9690ec629d4 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/testing/ImageRegionSelectionInteractionViewTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/testing/ImageRegionSelectionInteractionViewTest.kt @@ -11,6 +11,7 @@ import androidx.test.espresso.action.ViewActions.click import androidx.test.espresso.assertion.ViewAssertions.matches import androidx.test.espresso.matcher.ViewMatchers.isDisplayed import androidx.test.espresso.matcher.ViewMatchers.isEnabled +import androidx.test.espresso.matcher.ViewMatchers.withContentDescription import androidx.test.espresso.matcher.ViewMatchers.withId import androidx.test.espresso.matcher.ViewMatchers.withTagValue import androidx.test.espresso.matcher.ViewMatchers.withText @@ -177,7 +178,7 @@ class ImageRegionSelectionInteractionViewTest { assertThat(regionClickedEvent.value) .isEqualTo( NamedRegionClickedEvent( - regionLabel = "Region 3", contentDescription = "You have selected Region 3" + regionLabel = "Region 3", contentDescription = "Image showing Region 3." ) ) } @@ -218,12 +219,26 @@ class ImageRegionSelectionInteractionViewTest { assertThat(regionClickedEvent.value) .isEqualTo( NamedRegionClickedEvent( - regionLabel = "Region 2", contentDescription = "You have selected Region 2" + regionLabel = "Region 2", contentDescription = "Image showing Region 2." ) ) } } + @Test + // TODO(#1611): Fix ImageRegionSelectionInteractionViewTest + @RunOn(TestPlatform.ESPRESSO) + fun testImageRegionSelectionInteractionView_initialContentDescriptionRegion3_isCorrect() { + launch(ImageRegionSelectionTestActivity::class.java).use { + onView( + allOf( + withTagValue(`is`("Region 3")), + withContentDescription("Select Region 3 image.") + ) + ).check(matches(isDisplayed())) + } + } + @Test // TODO(#1611): Fix ImageRegionSelectionInteractionViewTest @RunOn(TestPlatform.ESPRESSO) @@ -280,7 +295,7 @@ class ImageRegionSelectionInteractionViewTest { assertThat(regionClickedEvent.value) .isEqualTo( NamedRegionClickedEvent( - regionLabel = "Region 2", contentDescription = "You have selected Region 2" + regionLabel = "Region 2", contentDescription = "Image showing Region 2." ) ) } @@ -308,7 +323,7 @@ class ImageRegionSelectionInteractionViewTest { assertThat(regionClickedEvent.value) .isEqualTo( NamedRegionClickedEvent( - regionLabel = "Region 3", contentDescription = "You have selected Region 3" + regionLabel = "Region 3", contentDescription = "Image showing Region 3." ) ) } @@ -352,7 +367,7 @@ class ImageRegionSelectionInteractionViewTest { assertThat(regionClickedEvent.value) .isEqualTo( NamedRegionClickedEvent( - regionLabel = "Region 3", contentDescription = "You have selected Region 3" + regionLabel = "Region 3", contentDescription = "Image showing Region 3." ) ) } @@ -394,7 +409,7 @@ class ImageRegionSelectionInteractionViewTest { assertThat(regionClickedEvent.value) .isEqualTo( NamedRegionClickedEvent( - regionLabel = "Region 2", contentDescription = "You have selected Region 2" + regionLabel = "Region 2", contentDescription = "Image showing Region 2." ) ) } From 9ce673f5ffea73c28e4f1dbd75e13114a2cbf1bd Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 18 Feb 2025 21:50:21 -0800 Subject: [PATCH 3/8] Fix #5660: Ensure mobile-install works (#5664) ## Explanation Fixes #5660 I've verified with local testing that ``bazel mobile-install`` works with the APK generated for each AAB target (it ends in ``_binary``, e.g. ``oppia_dev_binary``). Thus, documentation has been updated to recommend this as the main way to install the local development version of the app as ``mobile-install`` provides substantial performance benefits (see https://bazel.build/docs/mobile-install). The ``install_*`` targets (e.g. ``install_oppia_dev``) have been removed since ``mobile-install`` should be used, instead. The existing Starlark rule & macro has been repurposed to generating a universal APK which, in turn, will be useful for specific situations where ``mobile-install`` breaks down or is not used (such as for emulator tests). ## 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 N/A -- this changes nothing about the build of the app, only installation pathways. --- .bazelrc | 3 + BUILD.bazel | 6 +- build_flavors.bzl | 8 +- oppia_android_application.bzl | 106 +++++++++++---------- wiki/Bazel-Setup-Instructions-for-Linux.md | 31 +++++- wiki/Bazel-Setup-Instructions-for-Mac.md | 31 +++++- 6 files changed, 127 insertions(+), 58 deletions(-) diff --git a/.bazelrc b/.bazelrc index 5ca45844311..a12cb1f82e4 100644 --- a/.bazelrc +++ b/.bazelrc @@ -17,3 +17,6 @@ build:ignore_build_warnings --//tools/kotlin:warn_mode=warning # Show all test output by default (for better debugging). test --test_output=all + +# Always start the app by default when using mobile-install. +mobile-install --start_app diff --git a/BUILD.bazel b/BUILD.bazel index 4c6d5a97c2a..0563e369bd5 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -131,9 +131,9 @@ package_group( ] ] -# Define all binary flavors that can be built. Note that these are AABs, not APKs, and can be -# be installed on a local device or emulator using a 'bazel run' command like so: -# bazel run //:install_oppia_dev +# Define all binary flavors that can be built. Note that these are AABs and thus cannot be installed +# directly. However, their binaries can be installed using mobile-install like so: +# bazel mobile-install //:oppia_dev_binary [ define_oppia_aab_binary_flavor(flavor = flavor) for flavor in AVAILABLE_FLAVORS diff --git a/build_flavors.bzl b/build_flavors.bzl index aa5493a391e..b0ed8e2169c 100644 --- a/build_flavors.bzl +++ b/build_flavors.bzl @@ -2,7 +2,7 @@ Macros & definitions corresponding to Oppia binary build flavors. """ -load("//:oppia_android_application.bzl", "declare_deployable_application", "oppia_android_application") +load("//:oppia_android_application.bzl", "generate_universal_apk", "oppia_android_application") load("//:version.bzl", "MAJOR_VERSION", "MINOR_VERSION", "OPPIA_ALPHA_KITKAT_VERSION_CODE", "OPPIA_ALPHA_VERSION_CODE", "OPPIA_BETA_VERSION_CODE", "OPPIA_DEV_KITKAT_VERSION_CODE", "OPPIA_DEV_VERSION_CODE", "OPPIA_GA_VERSION_CODE") # Defines the list of flavors available to build the Oppia app in. Note to developers: this list @@ -242,7 +242,7 @@ def define_oppia_aab_binary_flavor(flavor): This will define two targets: - //:oppia_ (the AAB) - - //:install_oppia_ (the installable binary target--see declare_deployable_application + - //:oppia__universal_apk (the installable binary target--see generate_universal_apk for details) Args: @@ -278,7 +278,7 @@ def define_oppia_aab_binary_flavor(flavor): shrink_resources = True if len(_FLAVOR_METADATA[flavor]["proguard_specs"]) != 0 else False, deps = _FLAVOR_METADATA[flavor]["deps"], ) - declare_deployable_application( - name = "install_oppia_%s" % flavor, + generate_universal_apk( + name = "oppia_%s_universal_apk" % flavor, aab_target = ":oppia_%s" % flavor, ) diff --git a/oppia_android_application.bzl b/oppia_android_application.bzl index d9951cefa18..35e07feca65 100644 --- a/oppia_android_application.bzl +++ b/oppia_android_application.bzl @@ -158,61 +158,57 @@ def _package_metadata_into_deployable_aab_impl(ctx): runfiles = ctx.runfiles(files = [output_aab_file]), ) -def _generate_apks_and_install_impl(ctx): - input_file = ctx.attr.input_file.files.to_list()[0] +def _generate_universal_apk_impl(ctx): + input_aab_file = ctx.attr.input_aab_file.files.to_list()[0] + output_apk_file = ctx.outputs.output_apk_file debug_keystore_file = ctx.attr.debug_keystore.files.to_list()[0] apks_file = ctx.actions.declare_file("%s_processed.apks" % ctx.label.name) - deploy_shell = ctx.actions.declare_file("%s_run.sh" % ctx.label.name) - # Reference: https://developer.android.com/studio/command-line/bundletool#generate_apks. + # Reference: https://developer.android.com/tools/bundletool#generate_apks. # See also the Bazel BUILD file for the keystore for details on its password and alias. - generate_apks_arguments = [ + generate_universal_apk_arguments = [ "build-apks", - "--bundle=%s" % input_file.path, + "--bundle=%s" % input_aab_file.path, "--output=%s" % apks_file.path, "--ks=%s" % debug_keystore_file.path, "--ks-pass=pass:android", "--ks-key-alias=androiddebugkey", "--key-pass=pass:android", + "--mode=universal", ] + # bundletool only generates an APKs file, so the universal APK still needs to be extracted. + # Reference: https://docs.bazel.build/versions/master/skylark/lib/actions.html#run. ctx.actions.run( outputs = [apks_file], - inputs = ctx.files.input_file + ctx.files.debug_keystore, + inputs = ctx.files.input_aab_file + ctx.files.debug_keystore, tools = [ctx.executable._bundletool_tool], executable = ctx.executable._bundletool_tool.path, - arguments = generate_apks_arguments, - mnemonic = "BuildApksFromDeployAab", - progress_message = "Preparing AAB deploy to device", + arguments = generate_universal_apk_arguments, + mnemonic = "GenerateUniversalAPK", + progress_message = "Generating universal APK from AAB", ) - # References: https://github.com/bazelbuild/bazel/issues/7390, - # https://developer.android.com/studio/command-line/bundletool#deploy_with_bundletool, and - # https://docs.bazel.build/versions/main/skylark/rules.html#executable-rules-and-test-rules. - # Note that the bundletool can be executed directly since Bazel creates a wrapper script that - # utilizes its own internal Java toolchain. - ctx.actions.write( - output = deploy_shell, - content = """ - #!/bin/sh - {0} install-apks --apks={1} - echo The APK should now be installed - """.format(ctx.executable._bundletool_tool.short_path, apks_file.short_path), - is_executable = True, + command = """ + # Extract APK to working directory. + unzip -q "$(pwd)/{0}" universal.apk + mv universal.apk "$(pwd)/{1}" + """.format(apks_file.path, output_apk_file.path) + + # Reference: https://docs.bazel.build/versions/main/skylark/lib/actions.html#run_shell. + ctx.actions.run_shell( + outputs = [output_apk_file], + inputs = [apks_file], + tools = [], + command = command, + mnemonic = "ExtractUniversalAPK", + progress_message = "Extracting universal APK from .apks file", ) - # Reference for including necessary runfiles for Java: - # https://github.com/bazelbuild/bazel/issues/487#issuecomment-178119424. - runfiles = ctx.runfiles( - files = [ - ctx.executable._bundletool_tool, - apks_file, - ], - ).merge(ctx.attr._bundletool_tool.default_runfiles) return DefaultInfo( - executable = deploy_shell, - runfiles = runfiles, + files = depset([output_apk_file]), + runfiles = ctx.runfiles(files = [output_apk_file]), ) _convert_apk_to_module_aab = rule( @@ -303,10 +299,13 @@ _package_metadata_into_deployable_aab = rule( implementation = _package_metadata_into_deployable_aab_impl, ) -_generate_apks_and_install = rule( +_generate_universal_apk = rule( attrs = { - "input_file": attr.label( - allow_single_file = True, + "input_aab_file": attr.label( + allow_single_file = [".aab"], + mandatory = True, + ), + "output_apk_file": attr.output( mandatory = True, ), "debug_keystore": attr.label( @@ -319,14 +318,19 @@ _generate_apks_and_install = rule( default = "//third_party:android_bundletool_binary", ), }, - executable = True, - implementation = _generate_apks_and_install_impl, + implementation = _generate_universal_apk_impl, ) def oppia_android_application(name, config_file, proguard_generate_mapping, **kwargs): """ Creates an Android App Bundle (AAB) binary with the specified name and arguments. + This generates a mobile-installable target that ends in '_binary'. For example, if there's an + Oppia Android application defined with the name 'oppia_dev' then its APK binary can be + mobile-installed using: + + bazel mobile-install //:oppia_dev_binary + Args: name: str. The name of the Android App Bundle to build. This will corresponding to the name of the generated .aab file. @@ -390,30 +394,34 @@ def oppia_android_application(name, config_file, proguard_generate_mapping, **kw tags = ["manual"], ) -def declare_deployable_application(name, aab_target): +def generate_universal_apk(name, aab_target): """ - Creates a new target that can be run with 'bazel run' to install an AAB file. + Creates a new 'bazel mobile-install'-able universal APK target for the provided AAB target. - Example: - declare_deployable_application( - name = "install_oppia_prod", + Example usage in a top-level BUILD.bazel file and CLI: + generate_universal_apk( + name = "oppia_prod_universal_apk", aab_target = "//:oppia_prod", ) - $ bazel run //:install_oppia_prod + $ bazel mobile-install //:oppia_prod_universal_apk + + Note that, sometimes, you may not want to use mobile-install such as for production builds that + may have functional disparity from incremental installations of the app. In those cases, it's + best to uninstall the app from the target device and install the APK directly using + 'adb install' as so (per the above example): - This will build (if necessary) and install the correct APK derived from the Android app bundle - on the locally attached emulator or device. Note that this command does not support targeting a - specific device so it will not work if more than one device is available via 'adb devices'. + $ adb install bazel-bin/oppia_prod_universal_apk.apk Args: name: str. The name of the runnable target to install an AAB file on a local device. aab_target: target. The target (declared via oppia_android_application) that should be made installable. """ - _generate_apks_and_install( + _generate_universal_apk( name = name, - input_file = aab_target, + input_aab_file = aab_target, + output_apk_file = "%s.apk" % name, debug_keystore = "@bazel_tools//tools/android:debug_keystore", tags = ["manual"], ) diff --git a/wiki/Bazel-Setup-Instructions-for-Linux.md b/wiki/Bazel-Setup-Instructions-for-Linux.md index 6410ca034b6..f0dceb51c9f 100644 --- a/wiki/Bazel-Setup-Instructions-for-Linux.md +++ b/wiki/Bazel-Setup-Instructions-for-Linux.md @@ -42,5 +42,34 @@ INFO: Build completed successfully, ... Note also that the ``oppia_dev.aab`` under the ``bazel-bin`` directory of your local copy of Oppia Android should be a fully functioning development version of the app that can be installed using bundle-tool. However, it's recommended to deploy Oppia to an emulator or connected device using the following Bazel command: ```sh -bazel run //:install_oppia_dev +bazel mobile-install //:oppia_dev_binary +``` + +``mobile-install`` is much faster for local development (especially for the developer flavor of the app) because it does more sophisticated dex regeneration detection for faster incremental installs. See https://bazel.build/docs/mobile-install for details. + +**Note**: If you run into a failure like the following when trying to use `mobile-install` to a device running SDK 34 or newer: + +``` +FATAL EXCEPTION: main + Process: org.oppia.android, PID: 9508 + java.lang.RuntimeException: Unable to instantiate application com.google.devtools.build.android.incrementaldeployment.StubApplication package org.oppia.android: java.lang.SecurityException: Writable dex file '/data/local/tmp/incrementaldeployment/org.oppia.android/dex/incremental_classes4.dex' is not allowed. + at android.app.LoadedApk.makeApplicationInner(LoadedApk.java:1466) + at android.app.LoadedApk.makeApplicationInner(LoadedApk.java:1395) + at android.app.ActivityThread.handleBindApplication(ActivityThread.java:6959) + at android.app.ActivityThread.-$$Nest$mhandleBindApplication(Unknown Source:0) + at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2236) + at android.os.Handler.dispatchMessage(Handler.java:106) + at android.os.Looper.loopOnce(Looper.java:205) + at android.os.Looper.loop(Looper.java:294) + at android.app.ActivityThread.main(ActivityThread.java:8177) + at java.lang.reflect.Method.invoke(Native Method) + at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:552) + at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:971) + Caused by: java.lang.SecurityException: Writable dex file '/data/local/tmp/incrementaldeployment/org.oppia.android/dex/incremental_classes4.dex' is not allowed. +``` + +Then you will need to use `adb install` directly: + +```sh +adb install bazel-bin/oppia_dev_binary.apk ``` diff --git a/wiki/Bazel-Setup-Instructions-for-Mac.md b/wiki/Bazel-Setup-Instructions-for-Mac.md index f7b3b2e7dfd..eaf97c4f39e 100644 --- a/wiki/Bazel-Setup-Instructions-for-Mac.md +++ b/wiki/Bazel-Setup-Instructions-for-Mac.md @@ -70,5 +70,34 @@ INFO: Build completed successfully, ... Note also that the ``oppia_dev.aab`` under the ``bazel-bin`` directory of your local copy of Oppia Android should be a fully functioning development version of the app that can be installed using bundle-tool. However, it's recommended to deploy Oppia to an emulator or connected device using the following Bazel command: ```sh -bazel run //:install_oppia_dev +bazel mobile-install //:oppia_dev_binary +``` + +``mobile-install`` is much faster for local development (especially for the developer flavor of the app) because it does more sophisticated dex regeneration detection for faster incremental installs. See https://bazel.build/docs/mobile-install for details. + +**Note**: If you run into a failure like the following when trying to use `mobile-install` to a device running SDK 34 or newer: + +``` +FATAL EXCEPTION: main + Process: org.oppia.android, PID: 9508 + java.lang.RuntimeException: Unable to instantiate application com.google.devtools.build.android.incrementaldeployment.StubApplication package org.oppia.android: java.lang.SecurityException: Writable dex file '/data/local/tmp/incrementaldeployment/org.oppia.android/dex/incremental_classes4.dex' is not allowed. + at android.app.LoadedApk.makeApplicationInner(LoadedApk.java:1466) + at android.app.LoadedApk.makeApplicationInner(LoadedApk.java:1395) + at android.app.ActivityThread.handleBindApplication(ActivityThread.java:6959) + at android.app.ActivityThread.-$$Nest$mhandleBindApplication(Unknown Source:0) + at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2236) + at android.os.Handler.dispatchMessage(Handler.java:106) + at android.os.Looper.loopOnce(Looper.java:205) + at android.os.Looper.loop(Looper.java:294) + at android.app.ActivityThread.main(ActivityThread.java:8177) + at java.lang.reflect.Method.invoke(Native Method) + at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:552) + at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:971) + Caused by: java.lang.SecurityException: Writable dex file '/data/local/tmp/incrementaldeployment/org.oppia.android/dex/incremental_classes4.dex' is not allowed. +``` + +Then you will need to use `adb install` directly: + +```sh +adb install bazel-bin/oppia_dev_binary.apk ``` From bcaec12a14ba2c6a769d673e7688fab12bca52fd Mon Sep 17 00:00:00 2001 From: Tanish Moral <134790673+TanishMoral11@users.noreply.github.com> Date: Thu, 20 Feb 2025 05:02:46 +0530 Subject: [PATCH 4/8] Fix part of #3000 : TODO Integrate Buildifier Linter into Pre-push Checks (#5674) ## Explanation Fixes part of #3000 This PR integrates the Buildifier linter into the pre-push checks to ensure proper formatting of Bazel build files. The addition of `buildifier_lint_check.sh` ensures that any changes to build files are automatically checked for proper formatting before being pushed. This helps maintain consistency and quality in the project's build files. - The `buildifier_lint_check.sh` script is added to the pre-push checks. - The script is executed along with other existing checks, such as `ktlint`, `checkstyle`, and `buf`, ensuring that all code formatting checks are run in sequence before a commit is pushed. - If any of the checks fail, the push is blocked until the issues are resolved. This change enhances the automated code quality checks and reduces the chances of improperly formatted Bazel files being committed to the repository. ## 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: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> --- scripts/pre-push.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/pre-push.sh b/scripts/pre-push.sh index ea2878926de..4a927e5cd93 100755 --- a/scripts/pre-push.sh +++ b/scripts/pre-push.sh @@ -6,14 +6,14 @@ source scripts/formatting.sh # - ktlint # - checkstyle # - buf +# - buildifier # - (others in the future) -if bash scripts/ktlint_lint_check.sh && bash scripts/checkstyle_lint_check.sh && bash scripts/buf_lint_check.sh ; then +if bash scripts/ktlint_lint_check.sh && bash scripts/checkstyle_lint_check.sh && bash scripts/buf_lint_check.sh && bash scripts/buildifier_lint_check.sh; then echo_success "All checks passed successfully" exit 0 else exit 1 fi -# TODO(#3000): Add Bazel Linter to the project # TODO(#970): Add XML Linter to the project From 6e349bc9f21a685861214c6164adbc1af78f30fa Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 19 Feb 2025 16:35:35 -0800 Subject: [PATCH 5/8] Fix #5012: Remove KitKat support (#5630) ## Explanation Fixes #5012 Removes references and code corresponding to KitKat (SDK version 19) since the app has been set to a minimum version of Lollipop (SDK 21) since #3910 (roughly--technically KitKat was still supported but we no longer shipped a KitKat version). Because of this, the changes in this PR are largely a no-op from a build perspective. More broadly, this change is motivated by a desire to decrease CI time (which was already reduce considerably in the recent merge of #5629) by removing extraneous app builds being done in CI: - ``//:oppia`` since we should really only use ``//:oppia_dev`` moving forward. - Dev and alpha KitKat builds (the latter of which takes a long time since only the alpha job was building 2 proguarded builds of the app and thus taking much more time than the beta and GA build jobs). Separately, ``build_tests.yml`` and ``unit_tests.yml`` were updated to no longer support caching since we disabled this behavior a while back (#2884) and are unlikely to reintroduce it due to high storage costs. Some other noteworthy changes: - The optional providing of ``Retrofit`` and Retrofit services has been reverted since it's no longer necessary (and corresponding tests have been removed since there's no longer any condition to verify). - Code that was gated to run below Lollipop has been removed since it can't execute except on KitKat devices. - Support for a main dex target list has been removed as we're unlikely to need it with native multidex support (which wasn't an option for KitKat builds), though we may choose to reintroduce it to speed up cold app starts since it _can potentially_ help improve time-to-splash app startup performance. - Updated manifest min SDK values to avoid potential developer confusion on the min SDK version supported (though, strictly speaking, this doesn't technically matter with the way the app builds). - This updates the default min version supported for OS deprecation. No additional work is needed to actually activate the deprecation notice on the KitKat version of the app since we no longer deploy it, and OS deprecation wasn't added to the last version that was deployed. - Three file content pattern failure messages were updated to no longer reference KitKat (though the value of these checks mostly still seems realized, so I think it's fine to keep them in). - Some additional licenses were added due to tooling around the dev AAB (even though these aren't dependencies that ship with the app). I think it's fine including extra licenses in this case. - The protobuf license was corrected to BSD 3-Clause as declared for the dependency and in the GitHub repository for the dependencies. - This bumps version codes due to removing two flavors (for KitKat dev and KitKat alpha). - ``NetworkModuleTest`` has had its tests redesigned and better filled in in order to pass the code coverage minimum requirement (since old tests were removed). Note that it's not quite perfect in what it's testing, but it is at least verifying the complete configuration of the ``Retrofit`` instance, and the singleton properties of the two services currently supported (verifying that the services are set up would require a lot more testing that seems outside the direct scope of this test, so it seems okay to ignore here). - As part of the previous item's work, ``NetworkLoggingInterceptor`` had an issue fixed where it could cause an OkHttp exception to be thrown when trying to process logs (due to reading the response body twice). Reverting this change will now cause breakage failures in ``NetworkModuleTest``. - ``NetworkModule``'s initialization order was changed for slightly better network request logging (see the new comment in that file for more context). Note that there are some workflow job removals and renames in this PR. Any required CI checks will be correspondingly updated once this PR is approved and ready to merge (to avoid blocking any other in-flight PRs). ## 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 This shouldn't have an side effects on non-KitKat UIs, and KitKat support itself is being removed in this PR (so there's nothing to demonstrate). --- .github/CODEOWNERS | 3 - .github/workflows/build_tests.yml | 349 +----------------- .github/workflows/unit_tests.yml | 79 +--- BUILD.bazel | 63 +--- app/src/main/AppAndroidManifest.xml | 2 +- app/src/main/DatabindingAdaptersManifest.xml | 2 +- app/src/main/DatabindingResourcesManifest.xml | 2 +- app/src/main/RecyclerviewAdaptersManifest.xml | 2 +- app/src/main/ViewModelManifest.xml | 2 +- app/src/main/ViewModelsManifest.xml | 2 +- app/src/main/ViewsManifest.xml | 2 +- .../application/AbstractOppiaApplication.kt | 9 - .../databinding/TextViewBindingAdapters.java | 4 +- .../OsDeprecationNoticeDialogFragmentTest.kt | 2 +- .../PlatformParameterIntegrationTest.kt | 20 +- build_flavors.bzl | 40 +- config/kitkat_main_dex_class_list.txt | 50 --- .../oppia/android/config/AndroidManifest.xml | 2 +- .../backends/gae/NetworkLoggingInterceptor.kt | 15 +- .../data/backends/gae/NetworkModule.kt | 56 ++- data/src/test/AndroidManifest.xml | 2 +- .../data/backends/gae/NetworkModuleTest.kt | 238 ++++++++++-- .../syncup/PlatformParameterSyncUpWorker.kt | 18 +- .../PlatformParameterModuleTest.kt | 2 +- ...rameterSyncUpWorkManagerInitializerTest.kt | 20 +- .../PlatformParameterSyncUpWorkerTest.kt | 20 +- .../src/javatests/AndroidManifest.xml | 2 +- .../file_content_validation_checks.textproto | 6 +- scripts/assets/maven_dependencies.textproto | 25 +- .../license/MavenDependenciesRetriever.kt | 2 +- .../license/MavenDependenciesListCheckTest.kt | 2 +- .../license/MavenDependenciesRetrieverTest.kt | 2 +- .../GenerateMavenDependenciesListTest.kt | 2 +- .../regex/RegexPatternValidationCheckTest.kt | 6 +- .../testing/network/RetrofitTestModule.kt | 10 +- .../PerformanceMetricsAssessorImpl.kt | 20 +- .../PlatformParameterConstants.kt | 4 +- .../android/util/statusbar/StatusBarColor.kt | 12 +- utility/src/test/AndroidManifest.xml | 2 +- version.bzl | 18 +- wiki/Bazel-Setup-Instructions-for-Windows.md | 8 +- ...-Bazel-Migration-Best-Practices-and-FAQ.md | 2 +- wiki/Oppia-Bazel-Setup-Instructions.md | 4 +- wiki/Troubleshooting-Installation.md | 6 +- 44 files changed, 374 insertions(+), 765 deletions(-) delete mode 100644 config/kitkat_main_dex_class_list.txt diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index fdf7a2db439..8531242a536 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -259,9 +259,6 @@ WORKSPACE @oppia/android-app-infrastructure-reviewers # Proguard configurations for Bazel builds. /config/proguard/ @oppia/android-dev-workflow-reviewers -# Configuration for KitKat-specific curated builds. -/config/kitkat_main_dex_class_list.txt @oppia/android-dev-workflow-reviewers - # Specific manifest files specifically required for Bazel builds. /app/src/main/AppAndroidManifest.xml @oppia/android-dev-workflow-reviewers /app/src/main/DatabindingAdaptersManifest.xml @oppia/android-dev-workflow-reviewers diff --git a/.github/workflows/build_tests.yml b/.github/workflows/build_tests.yml index 2ceff32af16..9ffd5cb6594 100644 --- a/.github/workflows/build_tests.yml +++ b/.github/workflows/build_tests.yml @@ -15,149 +15,6 @@ concurrency: cancel-in-progress: true jobs: - bazel_build_app: - name: Build Binary with Bazel - runs-on: ${{ matrix.os }} - strategy: - matrix: - os: [ubuntu-20.04] - env: - ENABLE_CACHING: false - CACHE_DIRECTORY: ~/.bazel_cache - steps: - - uses: actions/checkout@v2 - with: - fetch-depth: 0 - - - name: Set up JDK 11 - uses: actions/setup-java@v1 - with: - java-version: 11 - - - name: Set up Bazel - uses: abhinavsingh/setup-bazel@v3 - with: - version: 6.5.0 - - - name: Set up build environment - uses: ./.github/actions/set-up-android-bazel-build-environment - - # For reference on this & the later cache actions, see: - # https://github.com/actions/cache/issues/239#issuecomment-606950711 & - # https://github.com/actions/cache/issues/109#issuecomment-558771281. Note that these work - # with Bazel since Bazel can share the most recent cache from an unrelated build and still - # benefit from incremental build performance (assuming that actions/cache aggressively removes - # older caches due to the 5GB cache limit size & Bazel's large cache size). - - uses: actions/cache@v4 - id: cache - with: - path: ${{ env.CACHE_DIRECTORY }} - key: ${{ runner.os }}-${{ env.CACHE_DIRECTORY }}-bazel-binary-${{ github.sha }} - restore-keys: | - ${{ runner.os }}-${{ env.CACHE_DIRECTORY }}-bazel-binary- - ${{ runner.os }}-${{ env.CACHE_DIRECTORY }}-bazel-tests- - ${{ runner.os }}-${{ env.CACHE_DIRECTORY }}-bazel- - - # This check is needed to ensure that Bazel's unbounded cache growth doesn't result in a - # situation where the cache never updates (e.g. due to exceeding GitHub's cache size limit) - # thereby only ever using the last successful cache version. This solution will result in a - # few slower CI actions around the time cache is detected to be too large, but it should - # incrementally improve thereafter. - - name: Ensure cache size - env: - BAZEL_CACHE_DIR: ${{ env.CACHE_DIRECTORY }} - run: | - # See https://stackoverflow.com/a/27485157 for reference. - EXPANDED_BAZEL_CACHE_PATH="${BAZEL_CACHE_DIR/#\~/$HOME}" - CACHE_SIZE_MB=$(du -smc $EXPANDED_BAZEL_CACHE_PATH | grep total | cut -f1) - echo "Total size of Bazel cache (rounded up to MBs): $CACHE_SIZE_MB" - # Use a 4.5GB threshold since actions/cache compresses the results, and Bazel caches seem - # to only increase by a few hundred megabytes across changes for unrelated branches. This - # is also a reasonable upper-bound (local tests as of 2021-03-31 suggest that a full build - # of the codebase (e.g. //...) from scratch only requires a ~2.1GB uncompressed/~900MB - # compressed cache). - if [[ "$CACHE_SIZE_MB" -gt 4500 ]]; then - echo "Cache exceeds cut-off; resetting it (will result in a slow build)" - rm -rf $EXPANDED_BAZEL_CACHE_PATH - fi - - - name: Configure Bazel to use a local cache - env: - BAZEL_CACHE_DIR: ${{ env.CACHE_DIRECTORY }} - run: | - EXPANDED_BAZEL_CACHE_PATH="${BAZEL_CACHE_DIR/#\~/$HOME}" - echo "Using $EXPANDED_BAZEL_CACHE_PATH as Bazel's cache path" - echo "build --disk_cache=$EXPANDED_BAZEL_CACHE_PATH" >> $HOME/.bazelrc - shell: bash - - - name: Check Bazel environment - run: bazel info - - # See https://git-secret.io/installation for details on installing git-secret. Note that the - # apt-get method isn't used since it's much slower to update & upgrade apt before installation - # versus just directly cloning & installing the project. Further, the specific version - # shouldn't matter since git-secret relies on a future-proof storage mechanism for secrets. - # This also uses a different directory to install git-secret to avoid requiring root access - # when running the git secret command. - - name: Install git-secret (non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - shell: bash - run: | - cd $HOME - mkdir -p $HOME/gitsecret - git clone https://github.com/sobolevn/git-secret.git git-secret - cd git-secret && make build - PREFIX="$HOME/gitsecret" make install - echo "$HOME/gitsecret" >> $GITHUB_PATH - echo "$HOME/gitsecret/bin" >> $GITHUB_PATH - - - name: Decrypt secrets (non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - env: - GIT_SECRET_GPG_PRIVATE_KEY: ${{ secrets.GIT_SECRET_GPG_PRIVATE_KEY }} - run: | - cd $HOME - # NOTE TO DEVELOPERS: Make sure to never print this key directly to stdout! - echo $GIT_SECRET_GPG_PRIVATE_KEY | base64 --decode > ./git_secret_private_key.gpg - gpg --import ./git_secret_private_key.gpg - cd $GITHUB_WORKSPACE - git secret reveal - - # Note that caching only works on non-forks. - - name: Build Oppia binary (with caching, non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - env: - BAZEL_REMOTE_CACHE_URL: ${{ secrets.BAZEL_REMOTE_CACHE_URL }} - run: | - bazel build --remote_http_cache=$BAZEL_REMOTE_CACHE_URL --google_credentials=./config/oppia-dev-workflow-remote-cache-credentials.json -- //:oppia - - - name: Build Oppia binary (without caching, or on a fork) - if: ${{ env.ENABLE_CACHING == 'false' || github.event.pull_request.head.repo.full_name != 'oppia/oppia-android' }} - run: | - bazel build -- //:oppia - - # Note that caching only works on non-forks. - - name: Build Oppia KitKat binary (with caching, non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - env: - BAZEL_REMOTE_CACHE_URL: ${{ secrets.BAZEL_REMOTE_CACHE_URL }} - run: | - bazel build --remote_http_cache=$BAZEL_REMOTE_CACHE_URL --google_credentials=./config/oppia-dev-workflow-remote-cache-credentials.json -- //:oppia_kitkat - - - name: Build Oppia binary KitKat (without caching, or on a fork) - if: ${{ env.ENABLE_CACHING == 'false' || github.event.pull_request.head.repo.full_name != 'oppia/oppia-android' }} - run: | - bazel build -- //:oppia_kitkat - - - name: Copy Oppia dev APKs for uploading - run: | - cp $GITHUB_WORKSPACE/bazel-bin/oppia.apk /home/runner/work/oppia-android/oppia-android/ - - - uses: actions/upload-artifact@v4 - with: - name: oppia-bazel.apk - path: /home/runner/work/oppia-android/oppia-android/oppia.apk - build_oppia_dev_aab: name: Build Oppia AAB (developer flavors) runs-on: ${{ matrix.os }} @@ -165,7 +22,6 @@ jobs: matrix: os: [ubuntu-20.04] env: - ENABLE_CACHING: false CACHE_DIRECTORY: ~/.bazel_cache steps: - uses: actions/checkout@v2 @@ -236,61 +92,8 @@ jobs: - name: Check Bazel environment run: bazel info - # See https://git-secret.io/installation for details on installing git-secret. Note that the - # apt-get method isn't used since it's much slower to update & upgrade apt before installation - # versus just directly cloning & installing the project. Further, the specific version - # shouldn't matter since git-secret relies on a future-proof storage mechanism for secrets. - # This also uses a different directory to install git-secret to avoid requiring root access - # when running the git secret command. - - name: Install git-secret (non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - shell: bash - run: | - cd $HOME - mkdir -p $HOME/gitsecret - git clone https://github.com/sobolevn/git-secret.git git-secret - cd git-secret && make build - PREFIX="$HOME/gitsecret" make install - echo "$HOME/gitsecret" >> $GITHUB_PATH - echo "$HOME/gitsecret/bin" >> $GITHUB_PATH - - - name: Decrypt secrets (non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - env: - GIT_SECRET_GPG_PRIVATE_KEY: ${{ secrets.GIT_SECRET_GPG_PRIVATE_KEY }} - run: | - cd $HOME - # NOTE TO DEVELOPERS: Make sure to never print this key directly to stdout! - echo $GIT_SECRET_GPG_PRIVATE_KEY | base64 --decode > ./git_secret_private_key.gpg - gpg --import ./git_secret_private_key.gpg - cd $GITHUB_WORKSPACE - git secret reveal - - # Note that caching only works on non-forks. - - name: Build Oppia developer AAB (with caching, non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - env: - BAZEL_REMOTE_CACHE_URL: ${{ secrets.BAZEL_REMOTE_CACHE_URL }} - run: | - bazel build --remote_http_cache=$BAZEL_REMOTE_CACHE_URL --google_credentials=./config/oppia-dev-workflow-remote-cache-credentials.json -- //:oppia_dev - - - name: Build Oppia developer AAB (without caching, or on a fork) - if: ${{ env.ENABLE_CACHING == 'false' || github.event.pull_request.head.repo.full_name != 'oppia/oppia-android' }} - run: | - bazel build -- //:oppia_dev - - # Note that caching only works on non-forks. - - name: Build Oppia developer KitKat AAB (with caching, non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - env: - BAZEL_REMOTE_CACHE_URL: ${{ secrets.BAZEL_REMOTE_CACHE_URL }} - run: | - bazel build --remote_http_cache=$BAZEL_REMOTE_CACHE_URL --google_credentials=./config/oppia-dev-workflow-remote-cache-credentials.json -- //:oppia_dev_kitkat - - - name: Build Oppia developer KitKat AAB (without caching, or on a fork) - if: ${{ env.ENABLE_CACHING == 'false' || github.event.pull_request.head.repo.full_name != 'oppia/oppia-android' }} - run: | - bazel build -- //:oppia_dev_kitkat + - name: Build Oppia developer AAB + run: bazel build -- //:oppia_dev build_oppia_alpha_aab: name: Build Oppia AAB (alpha flavors) @@ -299,7 +102,6 @@ jobs: matrix: os: [ubuntu-20.04] env: - ENABLE_CACHING: false CACHE_DIRECTORY: ~/.bazel_cache steps: - uses: actions/checkout@v2 @@ -370,61 +172,8 @@ jobs: - name: Check Bazel environment run: bazel info - # See https://git-secret.io/installation for details on installing git-secret. Note that the - # apt-get method isn't used since it's much slower to update & upgrade apt before installation - # versus just directly cloning & installing the project. Further, the specific version - # shouldn't matter since git-secret relies on a future-proof storage mechanism for secrets. - # This also uses a different directory to install git-secret to avoid requiring root access - # when running the git secret command. - - name: Install git-secret (non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - shell: bash - run: | - cd $HOME - mkdir -p $HOME/gitsecret - git clone https://github.com/sobolevn/git-secret.git git-secret - cd git-secret && make build - PREFIX="$HOME/gitsecret" make install - echo "$HOME/gitsecret" >> $GITHUB_PATH - echo "$HOME/gitsecret/bin" >> $GITHUB_PATH - - - name: Decrypt secrets (non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - env: - GIT_SECRET_GPG_PRIVATE_KEY: ${{ secrets.GIT_SECRET_GPG_PRIVATE_KEY }} - run: | - cd $HOME - # NOTE TO DEVELOPERS: Make sure to never print this key directly to stdout! - echo $GIT_SECRET_GPG_PRIVATE_KEY | base64 --decode > ./git_secret_private_key.gpg - gpg --import ./git_secret_private_key.gpg - cd $GITHUB_WORKSPACE - git secret reveal - - # Note that caching only works on non-forks. - - name: Build Oppia alpha AAB (with caching, non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - env: - BAZEL_REMOTE_CACHE_URL: ${{ secrets.BAZEL_REMOTE_CACHE_URL }} - run: | - bazel build --compilation_mode=opt --remote_http_cache=$BAZEL_REMOTE_CACHE_URL --google_credentials=./config/oppia-dev-workflow-remote-cache-credentials.json -- //:oppia_alpha - - - name: Build Oppia alpha AAB (without caching, or on a fork) - if: ${{ env.ENABLE_CACHING == 'false' || github.event.pull_request.head.repo.full_name != 'oppia/oppia-android' }} - run: | - bazel build --compilation_mode=opt -- //:oppia_alpha - - # Note that caching only works on non-forks. - - name: Build Oppia alpha KitKat AAB (with caching, non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - env: - BAZEL_REMOTE_CACHE_URL: ${{ secrets.BAZEL_REMOTE_CACHE_URL }} - run: | - bazel build --compilation_mode=opt --remote_http_cache=$BAZEL_REMOTE_CACHE_URL --google_credentials=./config/oppia-dev-workflow-remote-cache-credentials.json -- //:oppia_alpha_kitkat - - - name: Build Oppia alpha KitKat AAB (without caching, or on a fork) - if: ${{ env.ENABLE_CACHING == 'false' || github.event.pull_request.head.repo.full_name != 'oppia/oppia-android' }} - run: | - bazel build --compilation_mode=opt -- //:oppia_alpha_kitkat + - name: Build Oppia alpha AAB + run: bazel build --compilation_mode=opt -- //:oppia_alpha build_oppia_beta_aab: name: Build Oppia AAB (beta flavor) @@ -433,7 +182,6 @@ jobs: matrix: os: [ubuntu-20.04] env: - ENABLE_CACHING: false CACHE_DIRECTORY: ~/.bazel_cache steps: - uses: actions/checkout@v2 @@ -504,48 +252,8 @@ jobs: - name: Check Bazel environment run: bazel info - # See https://git-secret.io/installation for details on installing git-secret. Note that the - # apt-get method isn't used since it's much slower to update & upgrade apt before installation - # versus just directly cloning & installing the project. Further, the specific version - # shouldn't matter since git-secret relies on a future-proof storage mechanism for secrets. - # This also uses a different directory to install git-secret to avoid requiring root access - # when running the git secret command. - - name: Install git-secret (non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - shell: bash - run: | - cd $HOME - mkdir -p $HOME/gitsecret - git clone https://github.com/sobolevn/git-secret.git git-secret - cd git-secret && make build - PREFIX="$HOME/gitsecret" make install - echo "$HOME/gitsecret" >> $GITHUB_PATH - echo "$HOME/gitsecret/bin" >> $GITHUB_PATH - - - name: Decrypt secrets (non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - env: - GIT_SECRET_GPG_PRIVATE_KEY: ${{ secrets.GIT_SECRET_GPG_PRIVATE_KEY }} - run: | - cd $HOME - # NOTE TO DEVELOPERS: Make sure to never print this key directly to stdout! - echo $GIT_SECRET_GPG_PRIVATE_KEY | base64 --decode > ./git_secret_private_key.gpg - gpg --import ./git_secret_private_key.gpg - cd $GITHUB_WORKSPACE - git secret reveal - - # Note that caching only works on non-forks. - - name: Build Oppia beta AAB (with caching, non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - env: - BAZEL_REMOTE_CACHE_URL: ${{ secrets.BAZEL_REMOTE_CACHE_URL }} - run: | - bazel build --compilation_mode=opt --remote_http_cache=$BAZEL_REMOTE_CACHE_URL --google_credentials=./config/oppia-dev-workflow-remote-cache-credentials.json -- //:oppia_beta - - - name: Build Oppia beta AAB (without caching, or on a fork) - if: ${{ env.ENABLE_CACHING == 'false' || github.event.pull_request.head.repo.full_name != 'oppia/oppia-android' }} - run: | - bazel build --compilation_mode=opt -- //:oppia_beta + - name: Build Oppia beta AAB + run: bazel build --compilation_mode=opt -- //:oppia_beta build_oppia_ga_aab: name: Build Oppia AAB (GA flavor) @@ -554,7 +262,6 @@ jobs: matrix: os: [ubuntu-20.04] env: - ENABLE_CACHING: false CACHE_DIRECTORY: ~/.bazel_cache steps: - uses: actions/checkout@v2 @@ -625,45 +332,5 @@ jobs: - name: Check Bazel environment run: bazel info - # See https://git-secret.io/installation for details on installing git-secret. Note that the - # apt-get method isn't used since it's much slower to update & upgrade apt before installation - # versus just directly cloning & installing the project. Further, the specific version - # shouldn't matter since git-secret relies on a future-proof storage mechanism for secrets. - # This also uses a different directory to install git-secret to avoid requiring root access - # when running the git secret command. - - name: Install git-secret (non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - shell: bash - run: | - cd $HOME - mkdir -p $HOME/gitsecret - git clone https://github.com/sobolevn/git-secret.git git-secret - cd git-secret && make build - PREFIX="$HOME/gitsecret" make install - echo "$HOME/gitsecret" >> $GITHUB_PATH - echo "$HOME/gitsecret/bin" >> $GITHUB_PATH - - - name: Decrypt secrets (non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - env: - GIT_SECRET_GPG_PRIVATE_KEY: ${{ secrets.GIT_SECRET_GPG_PRIVATE_KEY }} - run: | - cd $HOME - # NOTE TO DEVELOPERS: Make sure to never print this key directly to stdout! - echo $GIT_SECRET_GPG_PRIVATE_KEY | base64 --decode > ./git_secret_private_key.gpg - gpg --import ./git_secret_private_key.gpg - cd $GITHUB_WORKSPACE - git secret reveal - - # Note that caching only works on non-forks. - - name: Build Oppia GA AAB (with caching, non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && github.event.pull_request.head.repo.full_name == 'oppia/oppia-android' }} - env: - BAZEL_REMOTE_CACHE_URL: ${{ secrets.BAZEL_REMOTE_CACHE_URL }} - run: | - bazel build --compilation_mode=opt --remote_http_cache=$BAZEL_REMOTE_CACHE_URL --google_credentials=./config/oppia-dev-workflow-remote-cache-credentials.json -- //:oppia_ga - - - name: Build Oppia GA AAB (without caching, or on a fork) - if: ${{ env.ENABLE_CACHING == 'false' || github.event.pull_request.head.repo.full_name != 'oppia/oppia-android' }} - run: | - bazel build --compilation_mode=opt -- //:oppia_ga + - name: Build Oppia GA AAB + run: bazel build --compilation_mode=opt -- //:oppia_ga diff --git a/.github/workflows/unit_tests.yml b/.github/workflows/unit_tests.yml index 0ae969fd9f8..acd9d7795e8 100644 --- a/.github/workflows/unit_tests.yml +++ b/.github/workflows/unit_tests.yml @@ -106,7 +106,6 @@ jobs: max-parallel: 10 matrix: ${{ fromJson(needs.bazel_compute_affected_targets.outputs.matrix) }} env: - ENABLE_CACHING: false CACHE_DIRECTORY: ~/.bazel_cache steps: - uses: actions/checkout@v2 @@ -206,57 +205,7 @@ jobs: echo "build --disk_cache=$EXPANDED_BAZEL_CACHE_PATH" >> $HOME/.bazelrc shell: bash - # See explanation in bazel_build_app for how this is installed. - - name: Install git-secret (non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && ((github.ref == 'refs/heads/develop' && github.event_name == 'push') || (github.event.pull_request.head.repo.full_name == 'oppia/oppia-android')) }} - shell: bash - run: | - cd $HOME - mkdir -p $HOME/gitsecret - git clone https://github.com/sobolevn/git-secret.git git-secret - cd git-secret && make build - PREFIX="$HOME/gitsecret" make install - echo "$HOME/gitsecret" >> $GITHUB_PATH - echo "$HOME/gitsecret/bin" >> $GITHUB_PATH - - - name: Decrypt secrets (non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && ((github.ref == 'refs/heads/develop' && github.event_name == 'push') || (github.event.pull_request.head.repo.full_name == 'oppia/oppia-android')) }} - env: - GIT_SECRET_GPG_PRIVATE_KEY: ${{ secrets.GIT_SECRET_GPG_PRIVATE_KEY }} - run: | - cd $HOME - # NOTE TO DEVELOPERS: Make sure to never print this key directly to stdout! - echo $GIT_SECRET_GPG_PRIVATE_KEY | base64 --decode > ./git_secret_private_key.gpg - gpg --import ./git_secret_private_key.gpg - cd $GITHUB_WORKSPACE - git secret reveal - - # See https://www.cyberciti.biz/faq/unix-for-loop-1-to-10/ for for-loop reference. - - name: Build Oppia Tests (with caching, non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && ((github.ref == 'refs/heads/develop' && github.event_name == 'push') || (github.event.pull_request.head.repo.full_name == 'oppia/oppia-android')) }} - env: - BAZEL_REMOTE_CACHE_URL: ${{ secrets.BAZEL_REMOTE_CACHE_URL }} - BAZEL_TEST_TARGETS: ${{ env.BAZEL_TEST_TARGETS }} - run: | - # Attempt to build 5 times in case there are flaky builds. - # TODO(#3759): Remove this once there are no longer app test build failures. - i=0 - # Disable exit-on-first-failure. - set +e - while [ $i -ne 5 ]; do - i=$(( $i+1 )) - echo "Attempt $i/5 to build test targets" - bazel build --keep_going --remote_http_cache=$BAZEL_REMOTE_CACHE_URL --google_credentials=./config/oppia-dev-workflow-remote-cache-credentials.json -- $BAZEL_TEST_TARGETS - done - # Capture the error code of the final command run (which should be a success if there isn't a real build failure). - last_error_code=$? - # Reenable exit-on-first-failure. - set -e - # Exit only if the most recent exit was a failure (by using a subshell). - (exit $last_error_code) - - - name: Build Oppia Tests (without caching, or on a fork) - if: ${{ env.ENABLE_CACHING == 'false' || ((github.ref != 'refs/heads/develop' || github.event_name != 'push') && (github.event.pull_request.head.repo.full_name != 'oppia/oppia-android')) }} + - name: Build Oppia Tests env: BAZEL_TEST_TARGETS: ${{ env.BAZEL_TEST_TARGETS }} run: | @@ -276,30 +225,8 @@ jobs: set -e # Exit only if the most recent exit was a failure (by using a subshell). (exit $last_error_code) - - name: Run Oppia Tests (with caching, non-fork only) - if: ${{ env.ENABLE_CACHING == 'true' && ((github.ref == 'refs/heads/develop' && github.event_name == 'push') || (github.event.pull_request.head.repo.full_name == 'oppia/oppia-android')) }} - env: - BAZEL_REMOTE_CACHE_URL: ${{ secrets.BAZEL_REMOTE_CACHE_URL }} - BAZEL_TEST_TARGETS: ${{ env.BAZEL_TEST_TARGETS }} - run: | - # Attempt to build 5 times in case there are flaky builds. - # TODO(#3970): Remove this once there are no longer app test build failures. - i=0 - # Disable exit-on-first-failure. - set +e - while [ $i -ne 5 ]; do - i=$(( $i+1 )) - echo "Attempt $i/5 to run test targets" - bazel test --keep_going --remote_http_cache=$BAZEL_REMOTE_CACHE_URL --google_credentials=./config/oppia-dev-workflow-remote-cache-credentials.json -- $BAZEL_TEST_TARGETS - done - # Capture the error code of the final command run (which should be a success if there isn't a real build failure). - last_error_code=$? - # Reenable exit-on-first-failure. - set -e - # Exit only if the most recent exit was a failure (by using a subshell). - (exit $last_error_code) - - name: Run Oppia Tests (without caching, or on a fork) - if: ${{ env.ENABLE_CACHING == 'false' || ((github.ref != 'refs/heads/develop' || github.event_name != 'push') && (github.event.pull_request.head.repo.full_name != 'oppia/oppia-android')) }} + + - name: Run Oppia Tests env: BAZEL_TEST_TARGETS: ${{ env.BAZEL_TEST_TARGETS }} run: | diff --git a/BUILD.bazel b/BUILD.bazel index 0563e369bd5..0e1f0cecff6 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -1,11 +1,7 @@ # TODO(#1532): Rename file to 'BUILD' post-Gradle. load("@dagger//:workspace_defs.bzl", "dagger_rules") -load("//:build_flavors.bzl", "AVAILABLE_FLAVORS", "define_oppia_aab_binary_flavor", "transform_android_manifest") -load("//:version.bzl", "MAJOR_VERSION", "MINOR_VERSION", "OPPIA_DEV_KITKAT_VERSION_CODE", "OPPIA_DEV_VERSION_CODE") - -# This is exported here since config/ isn't a Bazel package. -exports_files(["config/kitkat_main_dex_class_list.txt"]) +load("//:build_flavors.bzl", "AVAILABLE_FLAVORS", "define_oppia_aab_binary_flavor") # Corresponds to being accessible to all Oppia targets. This should be used for production APIs & # modules that may be used both in production targets and in tests. @@ -73,63 +69,6 @@ package_group( ) # TODO(#1640): Move binary manifest to top-level package post-Gradle. -[ - transform_android_manifest( - name = "oppia_apk_%s_transformed_manifest" % apk_flavor_metadata["flavor"], - application_relative_qualified_class = ".app.application.dev.DeveloperOppiaApplication", - build_flavor = apk_flavor_metadata["flavor"], - input_file = "//app:src/main/AndroidManifest.xml", - major_version = MAJOR_VERSION, - minor_version = MINOR_VERSION, - output_file = "AndroidManifest_transformed_%s.xml" % apk_flavor_metadata["flavor"], - version_code = apk_flavor_metadata["version_code"], - ) - for apk_flavor_metadata in [ - { - "flavor": "oppia", - "version_code": OPPIA_DEV_VERSION_CODE, - }, - { - "flavor": "oppia_kitkat", - "version_code": OPPIA_DEV_KITKAT_VERSION_CODE, - }, - ] -] - -[ - android_binary( - name = apk_flavor_metadata["flavor"], - custom_package = "org.oppia.android", - enable_data_binding = True, - main_dex_list = apk_flavor_metadata.get("main_dex_list"), - manifest = "oppia_apk_%s_transformed_manifest" % apk_flavor_metadata["flavor"], - manifest_values = { - "applicationId": "org.oppia.android", - "minSdkVersion": "%d" % apk_flavor_metadata["min_sdk_version"], - "targetSdkVersion": "%d" % apk_flavor_metadata["target_sdk_version"], - }, - multidex = apk_flavor_metadata["multidex"], - deps = [ - "//app/src/main/java/org/oppia/android/app/application/dev:developer_application", - "//config/src/java/org/oppia/android/config:all_languages_config", - ], - ) - for apk_flavor_metadata in [ - { - "flavor": "oppia", - "min_sdk_version": 21, - "multidex": "native", - "target_sdk_version": 34, - }, - { - "flavor": "oppia_kitkat", - "main_dex_list": "//:config/kitkat_main_dex_class_list.txt", - "min_sdk_version": 21, - "multidex": "manual_main_dex", - "target_sdk_version": 34, - }, - ] -] # Define all binary flavors that can be built. Note that these are AABs and thus cannot be installed # directly. However, their binaries can be installed using mobile-install like so: diff --git a/app/src/main/AppAndroidManifest.xml b/app/src/main/AppAndroidManifest.xml index 0b5a672e4b7..b46ab704ac4 100644 --- a/app/src/main/AppAndroidManifest.xml +++ b/app/src/main/AppAndroidManifest.xml @@ -1,5 +1,5 @@ - diff --git a/app/src/main/DatabindingAdaptersManifest.xml b/app/src/main/DatabindingAdaptersManifest.xml index 783be2f9363..9ca28db49a7 100644 --- a/app/src/main/DatabindingAdaptersManifest.xml +++ b/app/src/main/DatabindingAdaptersManifest.xml @@ -1,5 +1,5 @@ - diff --git a/app/src/main/DatabindingResourcesManifest.xml b/app/src/main/DatabindingResourcesManifest.xml index b48bc109de3..b7d5ffe9e72 100644 --- a/app/src/main/DatabindingResourcesManifest.xml +++ b/app/src/main/DatabindingResourcesManifest.xml @@ -1,5 +1,5 @@ - diff --git a/app/src/main/RecyclerviewAdaptersManifest.xml b/app/src/main/RecyclerviewAdaptersManifest.xml index 8f917756650..f05de7c5ee8 100644 --- a/app/src/main/RecyclerviewAdaptersManifest.xml +++ b/app/src/main/RecyclerviewAdaptersManifest.xml @@ -1,5 +1,5 @@ - diff --git a/app/src/main/ViewModelManifest.xml b/app/src/main/ViewModelManifest.xml index 67e79d1f41d..8d8132db1e2 100644 --- a/app/src/main/ViewModelManifest.xml +++ b/app/src/main/ViewModelManifest.xml @@ -2,6 +2,6 @@ - diff --git a/app/src/main/ViewModelsManifest.xml b/app/src/main/ViewModelsManifest.xml index 83d6b023161..41ddc2727ca 100644 --- a/app/src/main/ViewModelsManifest.xml +++ b/app/src/main/ViewModelsManifest.xml @@ -2,6 +2,6 @@ - diff --git a/app/src/main/ViewsManifest.xml b/app/src/main/ViewsManifest.xml index eac7e6941c4..059e24dbebe 100644 --- a/app/src/main/ViewsManifest.xml +++ b/app/src/main/ViewsManifest.xml @@ -2,6 +2,6 @@ - diff --git a/app/src/main/java/org/oppia/android/app/application/AbstractOppiaApplication.kt b/app/src/main/java/org/oppia/android/app/application/AbstractOppiaApplication.kt index 11a3025ff14..610178c32cd 100644 --- a/app/src/main/java/org/oppia/android/app/application/AbstractOppiaApplication.kt +++ b/app/src/main/java/org/oppia/android/app/application/AbstractOppiaApplication.kt @@ -4,7 +4,6 @@ import android.annotation.SuppressLint import android.app.Application import android.os.Build import androidx.appcompat.app.AppCompatActivity -import androidx.appcompat.app.AppCompatDelegate import androidx.multidex.MultiDexApplication import androidx.work.Configuration import androidx.work.WorkManager @@ -39,14 +38,6 @@ abstract class AbstractOppiaApplication( @SuppressLint("ObsoleteSdkInt") // Incorrect warning. override fun onCreate() { super.onCreate() - if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) { - // Ensure vector drawables can be properly loaded on KitKat devices. Note that this can - // introduce memory issues, but it's an easier-to-maintain solution that replacing all image - // binding with custom hook-ins (especially when it comes to databinding which isn't - // configurable in how it loads drawables), or building a custom vector drawable->PNG pipeline - // in Bazel. - AppCompatDelegate.setCompatVectorFromResourcesEnabled(true) - } // The current WorkManager version doesn't work in SDK 31+, so disable it. // TODO(#4751): Re-enable WorkManager for S+. if (Build.VERSION.SDK_INT < Build.VERSION_CODES.S) { diff --git a/app/src/main/java/org/oppia/android/app/databinding/TextViewBindingAdapters.java b/app/src/main/java/org/oppia/android/app/databinding/TextViewBindingAdapters.java index 284e1332b2d..6b75951614e 100644 --- a/app/src/main/java/org/oppia/android/app/databinding/TextViewBindingAdapters.java +++ b/app/src/main/java/org/oppia/android/app/databinding/TextViewBindingAdapters.java @@ -45,7 +45,7 @@ public static void setProfileLastVisitedText(@NonNull TextView textView, long ti } // TODO(#4345): Add test for this method. - /** Binds an AndroidX KitKat-compatible drawable top to the specified text view. */ + /** Binds an AndroidX drawable top to the specified text view. */ @BindingAdapter("drawableTopCompat") public static void setDrawableTopCompat( @NonNull TextView imageView, @@ -56,7 +56,7 @@ public static void setDrawableTopCompat( ); } - /** Binds an AndroidX KitKat-compatible drawable end to the specified text view. */ + /** Binds an AndroidX drawable end to the specified text view. */ @BindingAdapter("drawableEndCompat") public static void setDrawableEndCompat( @NonNull TextView imageView, diff --git a/app/src/sharedTest/java/org/oppia/android/app/notice/OsDeprecationNoticeDialogFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/notice/OsDeprecationNoticeDialogFragmentTest.kt index 8c44a36dc31..c2ec1431c63 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/notice/OsDeprecationNoticeDialogFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/notice/OsDeprecationNoticeDialogFragmentTest.kt @@ -168,7 +168,7 @@ class OsDeprecationNoticeDialogFragmentTest { .onActionButtonClicked( DeprecationNoticeActionResponse.Dismiss( deprecationNoticeType = DeprecationNoticeType.OS_DEPRECATION, - deprecatedVersion = 19, + deprecatedVersion = 21, ) ) } diff --git a/app/src/test/java/org/oppia/android/app/testing/PlatformParameterIntegrationTest.kt b/app/src/test/java/org/oppia/android/app/testing/PlatformParameterIntegrationTest.kt index 55923caadbc..67b2d0b7270 100644 --- a/app/src/test/java/org/oppia/android/app/testing/PlatformParameterIntegrationTest.kt +++ b/app/src/test/java/org/oppia/android/app/testing/PlatformParameterIntegrationTest.kt @@ -15,7 +15,6 @@ import androidx.work.WorkInfo import androidx.work.WorkManager import androidx.work.impl.utils.SynchronousExecutor import androidx.work.testing.WorkManagerTestInitHelper -import com.google.common.base.Optional import com.google.common.truth.Truth.assertThat import dagger.Component import dagger.Module @@ -310,19 +309,17 @@ class PlatformParameterIntegrationTest { jsonPrefixNetworkInterceptor: JsonPrefixNetworkInterceptor, remoteAuthNetworkInterceptor: RemoteAuthNetworkInterceptor, @BaseUrl baseUrl: String - ): Optional { + ): Retrofit { val client = OkHttpClient.Builder() .addInterceptor(jsonPrefixNetworkInterceptor) .addInterceptor(remoteAuthNetworkInterceptor) .build() - return Optional.of( - Retrofit.Builder() - .baseUrl(baseUrl) - .addConverterFactory(MoshiConverterFactory.create()) - .client(client) - .build() - ) + return Retrofit.Builder() + .baseUrl(baseUrl) + .addConverterFactory(MoshiConverterFactory.create()) + .client(client) + .build() } @Provides @@ -332,9 +329,8 @@ class PlatformParameterIntegrationTest { @Provides fun provideMockPlatformParameterService( mockRetrofit: MockRetrofit - ): Optional { - val delegate = mockRetrofit.create(PlatformParameterService::class.java) - return Optional.of(MockPlatformParameterService(delegate)) + ): PlatformParameterService { + return MockPlatformParameterService(mockRetrofit.create(PlatformParameterService::class.java)) } } diff --git a/build_flavors.bzl b/build_flavors.bzl index b0ed8e2169c..19b9939c3fa 100644 --- a/build_flavors.bzl +++ b/build_flavors.bzl @@ -3,25 +3,18 @@ Macros & definitions corresponding to Oppia binary build flavors. """ load("//:oppia_android_application.bzl", "generate_universal_apk", "oppia_android_application") -load("//:version.bzl", "MAJOR_VERSION", "MINOR_VERSION", "OPPIA_ALPHA_KITKAT_VERSION_CODE", "OPPIA_ALPHA_VERSION_CODE", "OPPIA_BETA_VERSION_CODE", "OPPIA_DEV_KITKAT_VERSION_CODE", "OPPIA_DEV_VERSION_CODE", "OPPIA_GA_VERSION_CODE") +load("//:version.bzl", "MAJOR_VERSION", "MINOR_VERSION", "OPPIA_ALPHA_VERSION_CODE", "OPPIA_BETA_VERSION_CODE", "OPPIA_DEV_VERSION_CODE", "OPPIA_GA_VERSION_CODE") # Defines the list of flavors available to build the Oppia app in. Note to developers: this list # should be ordered by the development pipeline (i.e. features go through dev first, then other # flavors as they mature). AVAILABLE_FLAVORS = [ "dev", - "dev_kitkat", "alpha", - "alpha_kitkat", "beta", "ga", ] -# This file contains the list of classes that must be in the main dex list for the legacy multidex -# build used on KitKat devices. Generally, this is the main application class is needed so that it -# can load multidex support, plus any dependencies needed by that pipeline. -_MAIN_DEX_LIST_TARGET_KITKAT = "//:config/kitkat_main_dex_class_list.txt" - # keep sorted _PRODUCTION_PROGUARD_SPECS = [ "config/proguard/android-proguard-rules.pro", @@ -55,21 +48,6 @@ _FLAVOR_METADATA = { "version_code": OPPIA_DEV_VERSION_CODE, "application_class": ".app.application.dev.DeveloperOppiaApplication", }, - "dev_kitkat": { - "manifest": "//app:src/main/AndroidManifest.xml", - "min_sdk_version": 21, - "target_sdk_version": 34, - "multidex": "manual_main_dex", - "main_dex_list": _MAIN_DEX_LIST_TARGET_KITKAT, - "proguard_specs": [], # Developer builds are not optimized. - "production_release": False, - "deps": [ - "//app/src/main/java/org/oppia/android/app/application/dev:developer_application", - "//config/src/java/org/oppia/android/config:all_languages_config", - ], - "version_code": OPPIA_DEV_KITKAT_VERSION_CODE, - "application_class": ".app.application.dev.DeveloperOppiaApplication", - }, "alpha": { "manifest": "//app:src/main/AndroidManifest.xml", "min_sdk_version": 21, @@ -84,21 +62,6 @@ _FLAVOR_METADATA = { "version_code": OPPIA_ALPHA_VERSION_CODE, "application_class": ".app.application.alpha.AlphaOppiaApplication", }, - "alpha_kitkat": { - "manifest": "//app:src/main/AndroidManifest.xml", - "min_sdk_version": 21, - "target_sdk_version": 34, - "multidex": "manual_main_dex", - "main_dex_list": _MAIN_DEX_LIST_TARGET_KITKAT, - "proguard_specs": [], - "production_release": True, - "deps": [ - "//app/src/main/java/org/oppia/android/app/application/alpha:alpha_application", - "//config/src/java/org/oppia/android/config:all_languages_config", - ], - "version_code": OPPIA_ALPHA_KITKAT_VERSION_CODE, - "application_class": ".app.application.alpha.AlphaOppiaApplication", - }, "beta": { "manifest": "//app:src/main/AndroidManifest.xml", "min_sdk_version": 21, @@ -272,7 +235,6 @@ def define_oppia_aab_binary_flavor(flavor): "targetSdkVersion": "%d" % _FLAVOR_METADATA[flavor]["target_sdk_version"], }, multidex = _FLAVOR_METADATA[flavor]["multidex"], - main_dex_list = _FLAVOR_METADATA[flavor].get("main_dex_list"), proguard_generate_mapping = True if len(_FLAVOR_METADATA[flavor]["proguard_specs"]) != 0 else False, proguard_specs = _FLAVOR_METADATA[flavor]["proguard_specs"], shrink_resources = True if len(_FLAVOR_METADATA[flavor]["proguard_specs"]) != 0 else False, diff --git a/config/kitkat_main_dex_class_list.txt b/config/kitkat_main_dex_class_list.txt deleted file mode 100644 index ea2de317d21..00000000000 --- a/config/kitkat_main_dex_class_list.txt +++ /dev/null @@ -1,50 +0,0 @@ -androidx/multidex/BuildConfig.class -androidx/multidex/MultiDex$V19.class -androidx/multidex/MultiDex.class -androidx/multidex/MultiDexApplication.class -androidx/multidex/MultiDexExtractor$1.class -androidx/multidex/MultiDexExtractor$ExtractedDex.class -androidx/multidex/MultiDexExtractor.class -androidx/multidex/ZipUtil$CentralDirectory.class -androidx/multidex/ZipUtil.class -androidx/work/Configuration$Provider.class -androidx/work/Configuration.class -androidx/work/WorkManager.class -javax/inject/Provider.class -kotlin/Function.class -kotlin/jvm/functions/Function0.class -kotlin/jvm/internal/FunctionBase.class -kotlin/jvm/internal/Intrinsics.class -kotlin/jvm/internal/Lambda.class -kotlin/Lazy.class -kotlin/LazyKt.class -kotlin/LazyKt__LazyJVMKt.class -kotlin/LazyKt__LazyKt.class -kotlin/SynchronizedLazyImpl.class -kotlin/UNINITIALIZED_VALUE.class -org/oppia/android/app/activity/ActivityComponent.class -org/oppia/android/app/activity/ActivityComponentFactory.class -org/oppia/android/app/activity/ActivityComponentImpl$Builder.class -org/oppia/android/app/activity/ActivityComponentImpl.class -org/oppia/android/app/application/ApplicationComponent$Builder.class -org/oppia/android/app/application/ApplicationComponent.class -org/oppia/android/app/application/ApplicationInjector.class -org/oppia/android/app/application/ApplicationInjectorProvider$DefaultImpls.class -org/oppia/android/app/application/ApplicationInjectorProvider.class -org/oppia/android/app/application/ApplicationModule.class -org/oppia/android/app/application/dev/DaggerDeveloperApplicationComponent$ActivityComponentImplBuilder.class -org/oppia/android/app/application/dev/DaggerDeveloperApplicationComponent$Builder.class -org/oppia/android/app/application/dev/DaggerDeveloperApplicationComponent.class -org/oppia/android/app/application/dev/DeveloperOppiaApplication.class -org/oppia/android/app/translation/AppLanguageActivityInjector.class -org/oppia/android/app/translation/AppLanguageActivityInjectorProvider.class -org/oppia/android/app/translation/AppLanguageApplicationInjector.class -org/oppia/android/app/translation/AppLanguageApplicationInjectorProvider.class -org/oppia/android/app/utility/datetime/DateTimeUtil$Injector.class -org/oppia/android/domain/locale/LocaleApplicationInjector.class -org/oppia/android/domain/locale/LocaleApplicationInjectorProvider.class -org/oppia/android/domain/oppialogger/ApplicationStartupListener.class -org/oppia/android/util/data/DataProvidersInjector.class -org/oppia/android/util/data/DataProvidersInjectorProvider.class -org/oppia/android/util/system/OppiaClockInjector.class -org/oppia/android/util/system/OppiaClockInjectorProvider.class diff --git a/config/src/java/org/oppia/android/config/AndroidManifest.xml b/config/src/java/org/oppia/android/config/AndroidManifest.xml index 58158ec68c4..0b7883ee463 100644 --- a/config/src/java/org/oppia/android/config/AndroidManifest.xml +++ b/config/src/java/org/oppia/android/config/AndroidManifest.xml @@ -1,5 +1,5 @@ - + diff --git a/data/src/main/java/org/oppia/android/data/backends/gae/NetworkLoggingInterceptor.kt b/data/src/main/java/org/oppia/android/data/backends/gae/NetworkLoggingInterceptor.kt index 97aab7d61a9..43e7749dd5e 100755 --- a/data/src/main/java/org/oppia/android/data/backends/gae/NetworkLoggingInterceptor.kt +++ b/data/src/main/java/org/oppia/android/data/backends/gae/NetworkLoggingInterceptor.kt @@ -14,6 +14,7 @@ import java.io.IOException import java.lang.Exception import javax.inject.Inject import javax.inject.Singleton +import kotlin.text.Charsets /** * Interceptor on top of Retrofit to log network requests and responses. @@ -42,7 +43,15 @@ class NetworkLoggingInterceptor @Inject constructor( return try { val response = chain.proceed(request) - val responseBody = response.body?.string() + // The response body needs to be cloned for reading otherwise it will throw since the body can + // only be read fully at most one time and other interceptors in the chain may read it. See + // https://stackoverflow.com/a/33862068 or OkHttp's HttpLoggingInterceptor for a reference. + val responseBody = response.body + val requestLength = responseBody?.contentLength()?.takeIf { it != -1L } + val responseBodyText = + responseBody?.source()?.also { + it.request(requestLength ?: Long.MAX_VALUE) + }?.buffer?.clone()?.readString(Charsets.UTF_8) CoroutineScope(backgroundDispatcher).launch { _logNetworkCallFlow.emit( @@ -50,7 +59,7 @@ class NetworkLoggingInterceptor @Inject constructor( .setRequestUrl(request.url.toString()) .setHeaders(request.headers.toString()) .setResponseStatusCode(response.code) - .setBody(responseBody ?: "") + .setBody(responseBodyText ?: "") .build() ) } @@ -62,7 +71,7 @@ class NetworkLoggingInterceptor @Inject constructor( .setRequestUrl(request.url.toString()) .setHeaders(request.headers.toString()) .setResponseStatusCode(response.code) - .setErrorMessage(responseBody ?: "") + .setErrorMessage(responseBodyText ?: "") .build() ) } diff --git a/data/src/main/java/org/oppia/android/data/backends/gae/NetworkModule.kt b/data/src/main/java/org/oppia/android/data/backends/gae/NetworkModule.kt index 0ac58d8fe72..0570dac652c 100644 --- a/data/src/main/java/org/oppia/android/data/backends/gae/NetworkModule.kt +++ b/data/src/main/java/org/oppia/android/data/backends/gae/NetworkModule.kt @@ -1,8 +1,5 @@ package org.oppia.android.data.backends.gae -import android.annotation.SuppressLint -import android.os.Build -import com.google.common.base.Optional import dagger.Module import dagger.Provides import okhttp3.OkHttpClient @@ -19,58 +16,51 @@ import javax.inject.Singleton */ @Module class NetworkModule { - @SuppressLint("ObsoleteSdkInt") // AS warning is incorrect in this context. @OppiaRetrofit @Provides @Singleton fun provideRetrofitInstance( - jsonPrefixNetworkInterceptor: JsonPrefixNetworkInterceptor, remoteAuthNetworkInterceptor: RemoteAuthNetworkInterceptor, networkLoggingInterceptor: NetworkLoggingInterceptor, + jsonPrefixNetworkInterceptor: JsonPrefixNetworkInterceptor, @BaseUrl baseUrl: String - ): Optional { - // TODO(#1720): Make this a compile-time dep once Hilt provides it as an option. - return if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) { - val client = OkHttpClient.Builder() - .addInterceptor(jsonPrefixNetworkInterceptor) - .addInterceptor(remoteAuthNetworkInterceptor) - .addInterceptor(networkLoggingInterceptor) - .build() - - Optional.of( - Retrofit.Builder() - .baseUrl(baseUrl) - .addConverterFactory(MoshiConverterFactory.create()) - .client(client) - .build() + ): Retrofit { + return Retrofit.Builder().apply { + baseUrl(baseUrl) + addConverterFactory(MoshiConverterFactory.create()) + client( + OkHttpClient.Builder().apply { + // This is in a specific order. The auth modifies a request, so it happens first. The + // prefix remover executes other interceptors before changing the response, so it's + // registered last so that the network logging interceptor receives a response with the + // XSSI prefix correctly removed. + addInterceptor(remoteAuthNetworkInterceptor) + addInterceptor(networkLoggingInterceptor) + addInterceptor(jsonPrefixNetworkInterceptor) + }.build() ) - } else Optional.absent() + }.build() } @Provides @Singleton fun provideFeedbackReportingService( - @OppiaRetrofit retrofit: Optional - ): Optional { - return retrofit.map { it.create(FeedbackReportingService::class.java) } + @OppiaRetrofit retrofit: Retrofit + ): FeedbackReportingService { + return retrofit.create(FeedbackReportingService::class.java) } @Provides @Singleton fun providePlatformParameterService( - @OppiaRetrofit retrofit: Optional - ): Optional { - return retrofit.map { it.create(PlatformParameterService::class.java) } + @OppiaRetrofit retrofit: Retrofit + ): PlatformParameterService { + return retrofit.create(PlatformParameterService::class.java) } // Provides the API key to use in authenticating remote messages sent or received. This will be - // replaced with a secret key in production. + // replaced with a secret key in production builds. @Provides @NetworkApiKey fun provideNetworkApiKey(): String = "" - - private companion object { - private fun Optional.map(mapFunc: (T) -> V): Optional = - transform { mapFunc(checkNotNull(it)) } // Payload should never actually be null. - } } diff --git a/data/src/test/AndroidManifest.xml b/data/src/test/AndroidManifest.xml index 2078d324983..cca8e4c298f 100644 --- a/data/src/test/AndroidManifest.xml +++ b/data/src/test/AndroidManifest.xml @@ -1,5 +1,5 @@ - diff --git a/data/src/test/java/org/oppia/android/data/backends/gae/NetworkModuleTest.kt b/data/src/test/java/org/oppia/android/data/backends/gae/NetworkModuleTest.kt index e30664000e8..bffa94db228 100644 --- a/data/src/test/java/org/oppia/android/data/backends/gae/NetworkModuleTest.kt +++ b/data/src/test/java/org/oppia/android/data/backends/gae/NetworkModuleTest.kt @@ -2,26 +2,46 @@ package org.oppia.android.data.backends.gae import android.app.Application import android.content.Context -import android.os.Build import androidx.test.core.app.ApplicationProvider +import androidx.test.core.content.pm.ApplicationInfoBuilder +import androidx.test.core.content.pm.PackageInfoBuilder import androidx.test.ext.junit.runners.AndroidJUnit4 -import com.google.common.base.Optional import com.google.common.truth.Truth.assertThat +import com.squareup.moshi.Json +import com.squareup.moshi.JsonClass +import com.squareup.moshi.JsonDataException import dagger.BindsInstance import dagger.Component import dagger.Module import dagger.Provides +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.async +import kotlinx.coroutines.flow.take +import kotlinx.coroutines.flow.toList +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import org.junit.After import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.oppia.android.data.backends.gae.api.FeedbackReportingService import org.oppia.android.data.backends.gae.api.PlatformParameterService +import org.oppia.android.testing.assertThrows import org.oppia.android.testing.robolectric.RobolectricModule +import org.oppia.android.testing.threading.BackgroundTestDispatcher +import org.oppia.android.testing.threading.TestCoroutineDispatcher +import org.oppia.android.testing.threading.TestCoroutineDispatchers import org.oppia.android.testing.threading.TestDispatcherModule +import org.robolectric.Shadows import org.robolectric.annotation.Config import org.robolectric.annotation.LooperMode +import retrofit2.Call import retrofit2.Retrofit +import retrofit2.http.GET +import java.net.HttpURLConnection import javax.inject.Inject +import javax.inject.Provider import javax.inject.Singleton /** Tests for [NetworkModule]. */ @@ -29,58 +49,212 @@ import javax.inject.Singleton @LooperMode(LooperMode.Mode.PAUSED) @Config(application = NetworkModuleTest.TestApplication::class) class NetworkModuleTest { - @field:[Inject NetworkApiKey] - lateinit var networkApiKey: String + @field:[Inject NetworkApiKey] lateinit var networkApiKey: String + @field:[Inject OppiaRetrofit] lateinit var retrofit: Retrofit + @field:[Inject OppiaRetrofit] lateinit var retrofitProvider: Provider + @Inject lateinit var context: Context + @Inject lateinit var mockWebServer: MockWebServer + @Inject lateinit var platformParameterService: PlatformParameterService + @Inject lateinit var feedbackReportingServiceProvider: Provider + @Inject lateinit var platformParameterServiceProvider: Provider + @Inject lateinit var testCoroutineDispatchers: TestCoroutineDispatchers + @Inject lateinit var networkLoggingInterceptor: NetworkLoggingInterceptor + + @field:[Inject BackgroundTestDispatcher] + lateinit var backgroundTestDispatcher: TestCoroutineDispatcher @Before fun setUp() { setUpTestApplicationComponent() + setUpApplicationForContext() + } + + @After + fun tearDown() { + mockWebServer.shutdown() + } + + @Test + fun testInjectedRetrofit_hasMockBaseUrl() { + val baseUrl = retrofit.baseUrl().toUrl().toString() + + assertThat(baseUrl).isEqualTo(mockWebServer.url("/").toUrl().toString()) + } + + @Test + fun testInjectedRetrofit_doesNotHaveOppiaBaseUrl() { + val baseUrl = retrofit.baseUrl().toUrl().toString() + + // The URL should point to a local development server since a MockWebServer is being used. + assertThat(baseUrl).doesNotContain("oppia.org") + } + + @Test + fun testInjectedRetrofit_secondInjection_returnsSingletonInstance() { + val firstInjection = retrofitProvider.get() + val secondInjection = retrofitProvider.get() + + // Multiple injections should yield the same instance due to it being a singleton. + assertThat(firstInjection).isEqualTo(secondInjection) + } + + @Test + fun testRetrofit_withTestService_responseWithoutXssiPrefix_nonJsonResponse_callSucceeds() { + val service = retrofit.create(TestService::class.java) + setUpTestServiceResponse(json = "{}") + + val parametersCall = service.fetchNothing().execute() + + assertThat(parametersCall.isSuccessful).isTrue() } @Test - @Config(sdk = [Build.VERSION_CODES.LOLLIPOP]) - fun testRetrofitInstance_lollipop_isProvided() { - assertThat(getTestApplication().getRetrofit()).isPresent() + fun testRetrofit_withTestService_malformedJsonResponse_throwsJsonDataException() { + val service = retrofit.create(TestService::class.java) + setUpTestServiceResponse(json = "{}") + + val exception = assertThrows() { service.fetchTestObject().execute() } + + // Verify that Moshi deserialization fails correctly on malformed JSON responses. + assertThat(exception).hasMessageThat().contains("Required value 'field1' missing") } @Test - @Config(sdk = [Build.VERSION_CODES.LOLLIPOP]) - fun testFeedbackReportingService_lollipop_isProvided() { - assertThat(getTestApplication().getFeedbackReportingService()).isPresent() + fun testRetrofit_withTestService_responseWithoutXssiPrefix_jsonResponse_returnsCorrectObject() { + val service = retrofit.create(TestService::class.java) + setUpTestServiceResponse(json = "{\"field1\":\"asdf\",\"field2\":1}") + + val testObject = service.fetchTestObject().execute().body() + + // Verify that Moshi deserialization works correctly. + assertThat(testObject?.field1).isEqualTo("asdf") + assertThat(testObject?.field2).isEqualTo(1) } @Test - @Config(sdk = [Build.VERSION_CODES.LOLLIPOP]) - fun testPlatformParameterService_lollipop_isProvided() { - assertThat(getTestApplication().getPlatformParameterService()).isPresent() + fun testRetrofit_withTestService_responseWithXssiPrefix_jsonResponse_returnsCorrectObject() { + val service = retrofit.create(TestService::class.java) + setUpTestObjectServiceResponse(field1 = "field val", field2 = 3) + + val testObject = service.fetchTestObject().execute().body() + + // Verify that the XSSI prefix is correctly removed. + assertThat(testObject?.field1).isEqualTo("field val") + assertThat(testObject?.field2).isEqualTo(3) } @Test - fun testNetworkApiKey_isEmpty() { + fun testRetrofit_withTestService_sendsCorrectAuthHeaderContext() { + val service = retrofit.create(TestService::class.java) + setUpTestObjectServiceResponse(field1 = "field val", field2 = 3) + + service.fetchTestObject().execute() + + val request = mockWebServer.takeRequest() + assertThat(request.getHeader("api_key")).isEmpty() // Verifies presence, but value is empty. + assertThat(request.getHeader("app_package_name")).isEqualTo("org.oppia.android.data") + assertThat(request.getHeader("app_version_name")).isEqualTo(TEST_APP_VERSION_NAME) + assertThat(request.getHeader("app_version_code")).isEqualTo("$TEST_APP_VERSION_CODE") + } + + @Test + @ExperimentalCoroutinesApi + fun testRetrofit_withTestService_logsSuccessfulResponse() { + val service = retrofit.create(TestService::class.java) + setUpTestObjectServiceResponse(field1 = "field val", field2 = 3) + // Collect requests. + val firstRequestsDeferred = CoroutineScope(backgroundTestDispatcher).async { + networkLoggingInterceptor.logNetworkCallFlow.take(1).toList() + } + testCoroutineDispatchers.advanceUntilIdle() // Ensure the flow is subscribed before emit(). + + service.fetchTestObject().execute() + testCoroutineDispatchers.advanceUntilIdle() + + val firstRequest = firstRequestsDeferred.getCompleted().single() + assertThat(firstRequest.responseStatusCode).isEqualTo(HttpURLConnection.HTTP_OK) + assertThat(firstRequest.body).isEqualTo("{\"field1\":\"field val\",\"field2\":3}") + } + + @Test + fun testInjectedFeedbackReportingService_secondInjection_returnsSingletonInstance() { + val firstInjection = feedbackReportingServiceProvider.get() + val secondInjection = feedbackReportingServiceProvider.get() + + // Multiple injections should yield the same instance due to it being a singleton. + assertThat(firstInjection).isEqualTo(secondInjection) + } + + @Test + fun testInjectedPlatformParameterService_secondInjection_returnsSingletonInstance() { + val firstInjection = platformParameterServiceProvider.get() + val secondInjection = platformParameterServiceProvider.get() + + // Multiple injections should yield the same instance due to it being a singleton. + assertThat(firstInjection).isEqualTo(secondInjection) + } + + @Test + fun testInjectedNetworkApiKey_isEmptyByDefault() { // The network API key is empty by default on developer builds. assertThat(networkApiKey).isEmpty() } + private fun setUpTestObjectServiceResponse(field1: String, field2: Int) { + setUpTestServiceResponse(json = "$XSSI_PREFIX\n{\"field1\":\"$field1\",\"field2\":$field2}") + } + + private fun setUpTestServiceResponse(json: String) { + mockWebServer.enqueue(MockResponse().setBody(json)) + } + private fun getTestApplication() = ApplicationProvider.getApplicationContext() private fun setUpTestApplicationComponent() { getTestApplication().inject(this) } + private fun setUpApplicationForContext() { + val packageManager = Shadows.shadowOf(context.packageManager) + val applicationInfo = + ApplicationInfoBuilder.newBuilder() + .setPackageName(context.packageName) + .build() + val packageInfo = + PackageInfoBuilder.newBuilder() + .setPackageName(context.packageName) + .setApplicationInfo(applicationInfo) + .build() + packageInfo.versionName = TEST_APP_VERSION_NAME + @Suppress("DEPRECATION") // versionCode is needed to test production code. + packageInfo.versionCode = TEST_APP_VERSION_CODE + packageManager.installPackage(packageInfo) + } + @Module class TestModule { @Provides @Singleton - fun provideContext(application: Application): Context { - return application - } + fun provideContext(application: Application): Context = application + + @Provides + @Singleton + fun provideMockWebServer() = MockWebServer().also { it.start() } + + @Provides + @BaseUrl + fun provideNetworkBaseUrl(mockWebServer: MockWebServer): String = + mockWebServer.url("/").toUrl().toString() + + @Provides + @XssiPrefix + fun provideXssiPrefix() = XSSI_PREFIX } @Singleton @Component( modules = [ - TestModule::class, NetworkModule::class, NetworkConfigProdModule::class, - TestDispatcherModule::class, RobolectricModule::class + TestModule::class, NetworkModule::class, TestDispatcherModule::class, RobolectricModule::class ] ) @@ -93,9 +267,6 @@ class NetworkModuleTest { } fun inject(networkModuleTest: NetworkModuleTest) - @OppiaRetrofit fun getRetrofit(): Optional - fun getFeedbackReportingService(): Optional - fun getPlatformParameterService(): Optional } class TestApplication : Application() { @@ -108,9 +279,26 @@ class NetworkModuleTest { fun inject(networkModuleTest: NetworkModuleTest) { component.inject(networkModuleTest) } + } + + interface TestService { + @GET("test_path/test_object_handler") + // TODO(#76): Update return payload for handling storage failures once retry policy is defined. + fun fetchTestObject(): Call + + @GET("test_path/test_nothing_handler") + fun fetchNothing(): Call + } + + @JsonClass(generateAdapter = true) + data class TestMoshiObject( + @Json(name = "field1") val field1: String, + @Json(name = "field2") val field2: Int + ) - fun getRetrofit() = component.getRetrofit() - fun getFeedbackReportingService() = component.getFeedbackReportingService() - fun getPlatformParameterService() = component.getPlatformParameterService() + private companion object { + private const val XSSI_PREFIX = ")]}'" + private const val TEST_APP_VERSION_NAME = "oppia-android-test-0123456789" + private const val TEST_APP_VERSION_CODE = 1 } } diff --git a/domain/src/main/java/org/oppia/android/domain/platformparameter/syncup/PlatformParameterSyncUpWorker.kt b/domain/src/main/java/org/oppia/android/domain/platformparameter/syncup/PlatformParameterSyncUpWorker.kt index 50422db85a2..bfbd6bc3905 100644 --- a/domain/src/main/java/org/oppia/android/domain/platformparameter/syncup/PlatformParameterSyncUpWorker.kt +++ b/domain/src/main/java/org/oppia/android/domain/platformparameter/syncup/PlatformParameterSyncUpWorker.kt @@ -3,7 +3,6 @@ package org.oppia.android.domain.platformparameter.syncup import android.content.Context import androidx.work.ListenableWorker import androidx.work.WorkerParameters -import com.google.common.base.Optional import com.google.common.util.concurrent.ListenableFuture import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope @@ -29,7 +28,7 @@ class PlatformParameterSyncUpWorker private constructor( context: Context, params: WorkerParameters, private val platformParameterController: PlatformParameterController, - private val platformParameterService: Optional, + private val platformParameterService: PlatformParameterService, private val oppiaLogger: OppiaLogger, private val exceptionsController: ExceptionsController, @BackgroundDispatcher private val backgroundDispatcher: CoroutineDispatcher @@ -85,19 +84,16 @@ class PlatformParameterSyncUpWorker private constructor( /** * Synchronously executes the network request to get platform parameters from the Oppia backend. */ - private fun makeNetworkCallForPlatformParameters(): Optional>?> { - return platformParameterService.transform { service -> - service?.getPlatformParametersByVersion( - applicationContext.getVersionName() - )?.execute() - } + private fun makeNetworkCallForPlatformParameters(): Response>? { + return platformParameterService.getPlatformParametersByVersion( + applicationContext.getVersionName() + ).execute() } /** Extracts platform parameters from the remote service and stores them in the cache store. */ private suspend fun refreshPlatformParameters(): Result { return try { - val optionalResponse = makeNetworkCallForPlatformParameters() - val response = optionalResponse.orNull() + val response = makeNetworkCallForPlatformParameters() if (response != null) { val responseBody = checkNotNull(response.body()) val platformParameterList = parseNetworkResponse(responseBody) @@ -126,7 +122,7 @@ class PlatformParameterSyncUpWorker private constructor( /** Creates an instance of [PlatformParameterSyncUpWorker] by properly injecting dependencies. */ class Factory @Inject constructor( private val platformParameterController: PlatformParameterController, - private val platformParameterService: Optional, + private val platformParameterService: PlatformParameterService, private val oppiaLogger: OppiaLogger, private val exceptionsController: ExceptionsController, @BackgroundDispatcher private val backgroundDispatcher: CoroutineDispatcher diff --git a/domain/src/test/java/org/oppia/android/domain/platformparameter/PlatformParameterModuleTest.kt b/domain/src/test/java/org/oppia/android/domain/platformparameter/PlatformParameterModuleTest.kt index 53ccda14f55..d3700feb18a 100644 --- a/domain/src/test/java/org/oppia/android/domain/platformparameter/PlatformParameterModuleTest.kt +++ b/domain/src/test/java/org/oppia/android/domain/platformparameter/PlatformParameterModuleTest.kt @@ -261,7 +261,7 @@ class PlatformParameterModuleTest { private companion object { private const val TEST_APP_VERSION_NAME = "oppia-android-test-0123456789" private const val TEST_APP_VERSION_CODE = 125L - private const val TEST_LOWEST_SUPPORTED_API_LEVEL = 19 + private const val TEST_LOWEST_SUPPORTED_API_LEVEL = 21 private const val TEST_ENABLE_APP_AND_OS_DEPRECATION_DEFAULT_VALUE = false } } diff --git a/domain/src/test/java/org/oppia/android/domain/platformparameter/syncup/PlatformParameterSyncUpWorkManagerInitializerTest.kt b/domain/src/test/java/org/oppia/android/domain/platformparameter/syncup/PlatformParameterSyncUpWorkManagerInitializerTest.kt index e3283289cb4..c4ab1078e3d 100644 --- a/domain/src/test/java/org/oppia/android/domain/platformparameter/syncup/PlatformParameterSyncUpWorkManagerInitializerTest.kt +++ b/domain/src/test/java/org/oppia/android/domain/platformparameter/syncup/PlatformParameterSyncUpWorkManagerInitializerTest.kt @@ -13,7 +13,6 @@ import androidx.work.NetworkType import androidx.work.WorkManager import androidx.work.testing.SynchronousExecutor import androidx.work.testing.WorkManagerTestInitHelper -import com.google.common.base.Optional import com.google.common.truth.Truth.assertThat import dagger.BindsInstance import dagger.Component @@ -205,19 +204,17 @@ class PlatformParameterSyncUpWorkManagerInitializerTest { jsonPrefixNetworkInterceptor: JsonPrefixNetworkInterceptor, remoteAuthNetworkInterceptor: RemoteAuthNetworkInterceptor, @BaseUrl baseUrl: String - ): Optional { + ): Retrofit { val client = OkHttpClient.Builder() .addInterceptor(jsonPrefixNetworkInterceptor) .addInterceptor(remoteAuthNetworkInterceptor) .build() - return Optional.of( - Retrofit.Builder() - .baseUrl(baseUrl) - .addConverterFactory(MoshiConverterFactory.create()) - .client(client) - .build() - ) + return Retrofit.Builder() + .baseUrl(baseUrl) + .addConverterFactory(MoshiConverterFactory.create()) + .client(client) + .build() } @Provides @@ -227,9 +224,8 @@ class PlatformParameterSyncUpWorkManagerInitializerTest { @Provides fun provideMockPlatformParameterService( mockRetrofit: MockRetrofit - ): Optional { - val delegate = mockRetrofit.create(PlatformParameterService::class.java) - return Optional.of(MockPlatformParameterService(delegate)) + ): PlatformParameterService { + return MockPlatformParameterService(mockRetrofit.create(PlatformParameterService::class.java)) } } diff --git a/domain/src/test/java/org/oppia/android/domain/platformparameter/syncup/PlatformParameterSyncUpWorkerTest.kt b/domain/src/test/java/org/oppia/android/domain/platformparameter/syncup/PlatformParameterSyncUpWorkerTest.kt index 82e756c79e6..48455479275 100644 --- a/domain/src/test/java/org/oppia/android/domain/platformparameter/syncup/PlatformParameterSyncUpWorkerTest.kt +++ b/domain/src/test/java/org/oppia/android/domain/platformparameter/syncup/PlatformParameterSyncUpWorkerTest.kt @@ -14,7 +14,6 @@ import androidx.work.WorkInfo import androidx.work.WorkManager import androidx.work.testing.SynchronousExecutor import androidx.work.testing.WorkManagerTestInitHelper -import com.google.common.base.Optional import com.google.common.truth.Truth.assertThat import dagger.BindsInstance import dagger.Component @@ -464,19 +463,17 @@ class PlatformParameterSyncUpWorkerTest { jsonPrefixNetworkInterceptor: JsonPrefixNetworkInterceptor, remoteAuthNetworkInterceptor: RemoteAuthNetworkInterceptor, @BaseUrl baseUrl: String - ): Optional { + ): Retrofit { val client = OkHttpClient.Builder() .addInterceptor(jsonPrefixNetworkInterceptor) .addInterceptor(remoteAuthNetworkInterceptor) .build() - return Optional.of( - Retrofit.Builder() - .baseUrl(baseUrl) - .addConverterFactory(MoshiConverterFactory.create()) - .client(client) - .build() - ) + return Retrofit.Builder() + .baseUrl(baseUrl) + .addConverterFactory(MoshiConverterFactory.create()) + .client(client) + .build() } @Provides @@ -486,9 +483,8 @@ class PlatformParameterSyncUpWorkerTest { @Provides fun provideMockPlatformParameterService( mockRetrofit: MockRetrofit - ): Optional { - val delegate = mockRetrofit.create(PlatformParameterService::class.java) - return Optional.of(MockPlatformParameterService(delegate)) + ): PlatformParameterService { + return MockPlatformParameterService(mockRetrofit.create(PlatformParameterService::class.java)) } } diff --git a/instrumentation/src/javatests/AndroidManifest.xml b/instrumentation/src/javatests/AndroidManifest.xml index 9f6f103645b..602f7c5095f 100644 --- a/instrumentation/src/javatests/AndroidManifest.xml +++ b/instrumentation/src/javatests/AndroidManifest.xml @@ -2,7 +2,7 @@ { return bazelClient - .retrieveThirdPartyMavenDepsListForBinary("//:oppia") + .retrieveThirdPartyMavenDepsListForBinary("//:oppia_dev") .map { it.removePrefix(MAVEN_PREFIX) } } diff --git a/scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesListCheckTest.kt b/scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesListCheckTest.kt index 7040ef42b1c..ff10efad3db 100644 --- a/scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesListCheckTest.kt +++ b/scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesListCheckTest.kt @@ -951,7 +951,7 @@ class MavenDependenciesListCheckTest { build.appendText( """ android_binary( - name = "oppia", + name = "oppia_dev", manifest = "AndroidManifest.xml", deps = depsList ) diff --git a/scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesRetrieverTest.kt b/scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesRetrieverTest.kt index 4eabf3e26dd..591b7702272 100644 --- a/scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesRetrieverTest.kt +++ b/scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesRetrieverTest.kt @@ -1477,7 +1477,7 @@ class MavenDependenciesRetrieverTest { build.appendText( """ android_binary( - name = "oppia", + name = "oppia_dev", manifest = "AndroidManifest.xml", deps = depsList ) diff --git a/scripts/src/javatests/org/oppia/android/scripts/maven/GenerateMavenDependenciesListTest.kt b/scripts/src/javatests/org/oppia/android/scripts/maven/GenerateMavenDependenciesListTest.kt index ca789a2ada5..f6c6061aa33 100644 --- a/scripts/src/javatests/org/oppia/android/scripts/maven/GenerateMavenDependenciesListTest.kt +++ b/scripts/src/javatests/org/oppia/android/scripts/maven/GenerateMavenDependenciesListTest.kt @@ -1334,7 +1334,7 @@ class GenerateMavenDependenciesListTest { build.appendText( """ android_binary( - name = "oppia", + name = "oppia_dev", manifest = "AndroidManifest.xml", deps = depsList ) 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 60b415c68db..1a6160e0443 100644 --- a/scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt +++ b/scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt @@ -70,7 +70,7 @@ class RegexPatternValidationCheckTest { "All plurals outside strings.xml must be marked as not translatable, or moved to strings.xml." private val importingAndroidBidiFormatterErrorMessage = "Do not use Android's BidiFormatter directly. Instead, use AndroidX's BidiFormatter for" + - " KitKat compatibility." + " better compatibility." private val importingAndroidXBidiFormatterErrorMessage = "Do not use AndroidX's BidiFormatter directly. Instead, use the wrapper utility" + " OppiaBidiFormatter so that tests can verify that formatting actually occurs on select" + @@ -120,7 +120,7 @@ class RegexPatternValidationCheckTest { " properly in SDK <21 environments." private val useJava8OptionalErrorMessage = "Prefer using com.google.common.base.Optional (Guava's Optional) since desugaring has some" + - " incompatibilities between Bazel & KitKat builds." + " incompatibilities between different SDK targeted builds." private val useJavaCalendarErrorMessage = "Don't use Calendar directly. Instead, use OppiaClock and/or OppiaLocale for" + " calendar-specific operations." @@ -133,7 +133,7 @@ class RegexPatternValidationCheckTest { private val doNotUseKotlinDelegatesErrorMessage = "Don't use Delegates; use a lateinit var or nullable primitive var default-initialized to" + " null, instead. Delegates uses reflection internally, have a non-trivial initialization" + - " cost, and can cause breakages on KitKat devices. See #3939 for more context." + " cost, and can cause breakages on some devices. See #3939 for more context." private val screenNameNotPresentErrorMessage = "Please add a Screen Name for this activity. To do this, add a value in the ScreenName enum " + "of screens.proto and add that name to your activity using " + diff --git a/testing/src/main/java/org/oppia/android/testing/network/RetrofitTestModule.kt b/testing/src/main/java/org/oppia/android/testing/network/RetrofitTestModule.kt index 70c570568c2..2d83fa143c9 100644 --- a/testing/src/main/java/org/oppia/android/testing/network/RetrofitTestModule.kt +++ b/testing/src/main/java/org/oppia/android/testing/network/RetrofitTestModule.kt @@ -1,6 +1,5 @@ package org.oppia.android.testing.network -import com.google.common.base.Optional import dagger.Module import dagger.Provides import org.oppia.android.data.backends.gae.OppiaRetrofit @@ -14,7 +13,7 @@ import javax.inject.Singleton * network failures to avoid unwanted test flakes. Tests may override this behavior if they * wish to test uncertain network conditions. * - * Note that Retrofit may be absent from the builds in certain environments (such as KitKat). + * Note that Retrofit may be absent from the builds in certain environments. * Attempting to inject [MockRetrofit] will verify that Retrofit is present, or otherwise fail. Do * not inject [MockRetrofit] for tests where Retrofit is supposed to be absent. */ @@ -22,13 +21,10 @@ import javax.inject.Singleton class RetrofitTestModule { @Provides @Singleton - fun provideMockRetrofit(@OppiaRetrofit retrofit: Optional): MockRetrofit { - check(retrofit.isPresent) { - "Expected Retrofit to be present in order to create a MockRetrofit" - } + fun provideMockRetrofit(@OppiaRetrofit retrofit: Retrofit): MockRetrofit { val behavior = NetworkBehavior.create() behavior.setFailurePercent(0) - return MockRetrofit.Builder(retrofit.get()).apply { + return MockRetrofit.Builder(retrofit).apply { networkBehavior(behavior) }.build() } diff --git a/utility/src/main/java/org/oppia/android/util/logging/performancemetrics/PerformanceMetricsAssessorImpl.kt b/utility/src/main/java/org/oppia/android/util/logging/performancemetrics/PerformanceMetricsAssessorImpl.kt index 7c8ec0998f7..c1a5ace4e60 100644 --- a/utility/src/main/java/org/oppia/android/util/logging/performancemetrics/PerformanceMetricsAssessorImpl.kt +++ b/utility/src/main/java/org/oppia/android/util/logging/performancemetrics/PerformanceMetricsAssessorImpl.kt @@ -3,7 +3,6 @@ package org.oppia.android.util.logging.performancemetrics import android.app.ActivityManager import android.content.Context import android.net.TrafficStats -import android.os.Build import android.os.Process import android.system.Os import android.system.OsConstants @@ -110,17 +109,12 @@ class PerformanceMetricsAssessorImpl @Inject constructor( /** Returns the number of processors that are currently online/available. */ private fun getNumberOfOnlineCores(): Int { - return if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) { - // Returns the number of processors currently available in the system. This may be less than - // the total number of configured processors because some of them may be offline. It must also - // be noted that a similar OsConstant, _SC_NPROCESSORS_CONF also exists which provides the - // total number of configured processors in the system. This value is similar to that - // returned from Runtime.getRuntime().availableProcessors(). - // Reference: https://man7.org/linux/man-pages/man3/sysconf.3.html - Os.sysconf(OsConstants._SC_NPROCESSORS_ONLN).toInt() - } else { - // Returns the maximum number of processors available. This value is never smaller than one. - Runtime.getRuntime().availableProcessors() - } + // Returns the number of processors currently available in the system. This may be less than the + // total number of configured processors because some of them may be offline. It must also be + // noted that a similar OsConstant, _SC_NPROCESSORS_CONF also exists which provides the total + // number of configured processors in the system. This value is similar to that returned from + // Runtime.getRuntime().availableProcessors(). Reference: + // https://man7.org/linux/man-pages/man3/sysconf.3.html + return Os.sysconf(OsConstants._SC_NPROCESSORS_ONLN).toInt() } } diff --git a/utility/src/main/java/org/oppia/android/util/platformparameter/PlatformParameterConstants.kt b/utility/src/main/java/org/oppia/android/util/platformparameter/PlatformParameterConstants.kt index 4648120ab79..1e96868bf72 100644 --- a/utility/src/main/java/org/oppia/android/util/platformparameter/PlatformParameterConstants.kt +++ b/utility/src/main/java/org/oppia/android/util/platformparameter/PlatformParameterConstants.kt @@ -182,9 +182,9 @@ const val LOWEST_SUPPORTED_API_LEVEL = "lowest_supported_api_level" * Default value for the platform parameter that contains an integer indicating the lowest * supported Android API Level. * - * The current minimum supported API level is 19 (KitKat). + * The current minimum supported API level is 21 (Lollipop). */ -const val LOWEST_SUPPORTED_API_LEVEL_DEFAULT_VALUE = 19 +const val LOWEST_SUPPORTED_API_LEVEL_DEFAULT_VALUE = 21 /** * Qualifier for the platform parameter that controls the time interval in days between showing diff --git a/utility/src/main/java/org/oppia/android/util/statusbar/StatusBarColor.kt b/utility/src/main/java/org/oppia/android/util/statusbar/StatusBarColor.kt index 512d6a1c35e..fce60604b1c 100644 --- a/utility/src/main/java/org/oppia/android/util/statusbar/StatusBarColor.kt +++ b/utility/src/main/java/org/oppia/android/util/statusbar/StatusBarColor.kt @@ -15,14 +15,12 @@ class StatusBarColor { * @param statusBarLight passed Boolean true if the status bar theme is light, else passed Boolean false */ fun statusBarColorUpdate(colorId: Int, activity: AppCompatActivity, statusBarLight: Boolean) { - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) { - if (statusBarLight && Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { - // TODO(#3616): Migrate to the proper SDK 30+ APIs. - @Suppress("DEPRECATION") // The code is correct for targeted versions of Android. - activity.window.decorView.systemUiVisibility = View.SYSTEM_UI_FLAG_LIGHT_STATUS_BAR - } - activity.window.statusBarColor = ContextCompat.getColor(activity, colorId) + if (statusBarLight && Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { + // TODO(#3616): Migrate to the proper SDK 30+ APIs. + @Suppress("DEPRECATION") // The code is correct for targeted versions of Android. + activity.window.decorView.systemUiVisibility = View.SYSTEM_UI_FLAG_LIGHT_STATUS_BAR } + activity.window.statusBarColor = ContextCompat.getColor(activity, colorId) } } } diff --git a/utility/src/test/AndroidManifest.xml b/utility/src/test/AndroidManifest.xml index b028fea9297..d0e75614795 100644 --- a/utility/src/test/AndroidManifest.xml +++ b/utility/src/test/AndroidManifest.xml @@ -1,5 +1,5 @@ - diff --git a/version.bzl b/version.bzl index e022394e9fd..beb76d6a4e0 100644 --- a/version.bzl +++ b/version.bzl @@ -1,20 +1,18 @@ """ Defines the latest version of the Oppia Android app. -Note that version codes must be ordered such that dev > alpha, and kitkat < lollipop+. This will -ensure that the Play Store provides users with the correct version of the app in situations where -their device qualifies for more than one choice. +Note that version codes must be ordered such that dev > alpha. This will ensure that the Play Store +provides users with the correct version of the app in situations where their device qualifies for +more than one choice. More unstable flavors of the app are higher version codes since they represent "newer" versions of -the app (that are potentially not broadly released yet). +the app (that potentially contain changes or features that are not yet ready for broad release). """ MAJOR_VERSION = 0 MINOR_VERSION = 14 -OPPIA_DEV_VERSION_CODE = 178 -OPPIA_DEV_KITKAT_VERSION_CODE = 177 -OPPIA_ALPHA_VERSION_CODE = 176 -OPPIA_ALPHA_KITKAT_VERSION_CODE = 175 -OPPIA_BETA_VERSION_CODE = 174 -OPPIA_GA_VERSION_CODE = 173 +OPPIA_DEV_VERSION_CODE = 182 +OPPIA_ALPHA_VERSION_CODE = 181 +OPPIA_BETA_VERSION_CODE = 180 +OPPIA_GA_VERSION_CODE = 179 diff --git a/wiki/Bazel-Setup-Instructions-for-Windows.md b/wiki/Bazel-Setup-Instructions-for-Windows.md index 287264e2331..6145791a6a3 100644 --- a/wiki/Bazel-Setup-Instructions-for-Windows.md +++ b/wiki/Bazel-Setup-Instructions-for-Windows.md @@ -174,7 +174,7 @@ scripts/setup.sh At this point, your system should be able to build Oppia Android. To verify, try building the APK (from your subsystem terminal -- note that this & all other Bazel commands must be run from the root of the ‘oppia-android’ directory otherwise they will fail): ```sh -bazel build //:oppia +bazel build //:oppia_dev ``` (Note that this command may take 10-20 minutes to complete depending on the performance of your machine). @@ -182,10 +182,8 @@ bazel build //:oppia If everything is working, you should see output like the following: ``` -Target //:oppia up-to-date: - bazel-bin/oppia_deploy.jar - bazel-bin/oppia_unsigned/apk - bazel-bin/oppia/apk +Target //:oppia_dev up-to-date: + bazel-bin/oppia_dev.aab INFO: Elapsed time: ... INFO: 1 process... INFO: Build completed successfully, ... diff --git a/wiki/Gradle--Bazel-Migration-Best-Practices-and-FAQ.md b/wiki/Gradle--Bazel-Migration-Best-Practices-and-FAQ.md index 98ff63f4278..cded43f920e 100644 --- a/wiki/Gradle--Bazel-Migration-Best-Practices-and-FAQ.md +++ b/wiki/Gradle--Bazel-Migration-Best-Practices-and-FAQ.md @@ -24,7 +24,7 @@ This is not always possible, since there might be other files relying on the sam - Look at other BUILD.bazel files in the codebase for an idea on how to lay out packages. - Migrating files will always involve introducing new libraries, and then hooking those up to the top-level module libraries. There are ways of making this a bit easier: - While performing the migration, focus on making sure the new library builds first, e.g.: ``bazel build ///src/main/java/org/oppia/app//:all``. - - After the new libraries contain all of the files they need to, their dependencies are correct, and they build, make sure the app also builds by running: ``bazel build //:oppia``. + - After the new libraries contain all of the files they need to, their dependencies are correct, and they build, make sure the app also builds by running: ``bazel build //:oppia_dev``. ## FAQ diff --git a/wiki/Oppia-Bazel-Setup-Instructions.md b/wiki/Oppia-Bazel-Setup-Instructions.md index b1c8609c519..7cec86630c7 100644 --- a/wiki/Oppia-Bazel-Setup-Instructions.md +++ b/wiki/Oppia-Bazel-Setup-Instructions.md @@ -31,13 +31,13 @@ After the installation completes you can build the app using Bazel. **Move your command line head to the `~/opensource/oppia-android`**, then run the below bazel command: ``` -bazel build //:oppia +bazel build //:oppia_dev ``` #### Building + installing the app ``` -bazel build //:oppia && adb install -r bazel-bin/oppia.apk +bazel run //:install_oppia_dev ``` #### Running specific module (app) Robolectric tests diff --git a/wiki/Troubleshooting-Installation.md b/wiki/Troubleshooting-Installation.md index f23f9280987..2ae7f5fe449 100644 --- a/wiki/Troubleshooting-Installation.md +++ b/wiki/Troubleshooting-Installation.md @@ -64,9 +64,9 @@ Here are some general troubleshooting tips for oppia-android. The specific platf 1. No matching toolchains (sdk_toolchain_type) ``` - ERROR: While resolving toolchains for target //:oppia: no matching toolchains found for types + ERROR: While resolving toolchains for target //:oppia_dev: no matching toolchains found for types @bazel_tools//tools/android:sdk_toolchain_type - ERROR: Analysis of target '//:oppia' failed; build aborted: no matching toolchains found for types + ERROR: Analysis of target '//:oppia_dev' failed; build aborted: no matching toolchains found for types @bazel_tools//tools/android:sdk_toolchain_type INFO: Elapsed time: 12.805s INFO: 0 processes. @@ -119,7 +119,7 @@ After successfully running the above commands, build the app using Bazel by runn ``` bazel clean --expunge - bazel build //:oppia --noexperimental_check_desugar_deps + bazel build //:oppia_dev --noexperimental_check_desugar_deps ``` The `--noexperimental_check_desugar_deps` flag is explained in the [bazel blog](https://blog.bazel.build/2018/12/19/bazel-0.21.html#android). From 958def53dc88c526da336a3b5b1095247e3636e4 Mon Sep 17 00:00:00 2001 From: jainv4156 <123441149+jainv4156@users.noreply.github.com> Date: Thu, 20 Feb 2025 18:49:48 +0530 Subject: [PATCH 6/8] Fixmpart of #4865: Refactor walkthrough to use profileId (#5708) ## Explanation Fixes part of https://github.com/oppia/oppia-android/issues/4865 ## 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". - [ ] 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 --- .../org/oppia/android/app/walkthrough/WalkthroughActivity.kt | 3 +-- .../app/walkthrough/end/WalkthroughFinalFragmentPresenter.kt | 3 +-- .../welcome/WalkthroughWelcomeFragmentPresenter.kt | 4 +--- .../oppia/android/app/walkthrough/WalkthroughActivityTest.kt | 5 ++++- .../android/app/walkthrough/WalkthroughFinalFragmentTest.kt | 5 ++++- .../app/walkthrough/WalkthroughTopicListFragmentTest.kt | 4 +++- .../app/walkthrough/WalkthroughWelcomeFragmentTest.kt | 3 ++- 7 files changed, 16 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/org/oppia/android/app/walkthrough/WalkthroughActivity.kt b/app/src/main/java/org/oppia/android/app/walkthrough/WalkthroughActivity.kt index d3d7a7081e3..9a3ff29965a 100644 --- a/app/src/main/java/org/oppia/android/app/walkthrough/WalkthroughActivity.kt +++ b/app/src/main/java/org/oppia/android/app/walkthrough/WalkthroughActivity.kt @@ -45,8 +45,7 @@ class WalkthroughActivity : companion object { - fun createWalkthroughActivityIntent(context: Context, internalProfileId: Int): Intent { - val profileId = ProfileId.newBuilder().setInternalId(internalProfileId).build() + fun createWalkthroughActivityIntent(context: Context, profileId: ProfileId): Intent { return Intent(context, WalkthroughActivity::class.java).apply { decorateWithUserProfileId(profileId) decorateWithScreenName(WALKTHROUGH_ACTIVITY) diff --git a/app/src/main/java/org/oppia/android/app/walkthrough/end/WalkthroughFinalFragmentPresenter.kt b/app/src/main/java/org/oppia/android/app/walkthrough/end/WalkthroughFinalFragmentPresenter.kt index 078d79af431..e54e8438b40 100644 --- a/app/src/main/java/org/oppia/android/app/walkthrough/end/WalkthroughFinalFragmentPresenter.kt +++ b/app/src/main/java/org/oppia/android/app/walkthrough/end/WalkthroughFinalFragmentPresenter.kt @@ -45,8 +45,7 @@ class WalkthroughFinalFragmentPresenter @Inject constructor( /* attachToRoot= */ false ) this.topicId = topicId - val internalProfileId = activity.intent?.extractCurrentUserProfileId()?.internalId ?: -1 - profileId = ProfileId.newBuilder().setInternalId(internalProfileId).build() + profileId = activity.intent?.extractCurrentUserProfileId() ?: ProfileId.getDefaultInstance() walkthroughFinalViewModel = WalkthroughFinalViewModel() diff --git a/app/src/main/java/org/oppia/android/app/walkthrough/welcome/WalkthroughWelcomeFragmentPresenter.kt b/app/src/main/java/org/oppia/android/app/walkthrough/welcome/WalkthroughWelcomeFragmentPresenter.kt index 6327a374eb7..413dfa863ae 100644 --- a/app/src/main/java/org/oppia/android/app/walkthrough/welcome/WalkthroughWelcomeFragmentPresenter.kt +++ b/app/src/main/java/org/oppia/android/app/walkthrough/welcome/WalkthroughWelcomeFragmentPresenter.kt @@ -36,7 +36,6 @@ class WalkthroughWelcomeFragmentPresenter @Inject constructor( private lateinit var binding: WalkthroughWelcomeFragmentBinding private val routeToNextPage = activity as WalkthroughFragmentChangeListener private lateinit var walkthroughWelcomeViewModel: WalkthroughWelcomeViewModel - private var internalProfileId: Int = -1 private lateinit var profileId: ProfileId private lateinit var profileName: String @@ -48,8 +47,7 @@ class WalkthroughWelcomeFragmentPresenter @Inject constructor( /* attachToRoot= */ false ) - internalProfileId = activity.intent?.extractCurrentUserProfileId()?.internalId ?: -1 - profileId = ProfileId.newBuilder().setInternalId(internalProfileId).build() + profileId = activity.intent?.extractCurrentUserProfileId() ?: ProfileId.getDefaultInstance() walkthroughWelcomeViewModel = WalkthroughWelcomeViewModel() binding.let { diff --git a/app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughActivityTest.kt b/app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughActivityTest.kt index 5939e42ae93..919172da856 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughActivityTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughActivityTest.kt @@ -35,6 +35,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.ProfileId import org.oppia.android.app.model.ScreenName import org.oppia.android.app.player.state.itemviewmodel.SplitScreenInteractionModule import org.oppia.android.app.shim.ViewBindingShimModule @@ -127,8 +128,10 @@ class WalkthroughActivityTest { @Test fun testActivity_createIntent_verifyScreenNameInIntent() { + val profileId = ProfileId.newBuilder().setInternalId(1).build() + val currentScreenName = WalkthroughActivity.createWalkthroughActivityIntent( - context, 1 + context, profileId ).extractCurrentAppScreenName() assertThat(currentScreenName).isEqualTo(ScreenName.WALKTHROUGH_ACTIVITY) diff --git a/app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughFinalFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughFinalFragmentTest.kt index cff6fdf44d9..5e5455ca39e 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughFinalFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughFinalFragmentTest.kt @@ -38,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.ProfileId import org.oppia.android.app.model.WalkthroughFinalFragmentArguments import org.oppia.android.app.player.state.itemviewmodel.SplitScreenInteractionModule import org.oppia.android.app.recyclerview.RecyclerViewMatcher.Companion.atPositionOnView @@ -138,7 +139,9 @@ class WalkthroughFinalFragmentTest { Intents.release() } - private fun createWalkthroughActivityIntent(profileId: Int): Intent { + private fun createWalkthroughActivityIntent(internalProfileId: Int): Intent { + val profileId = ProfileId.newBuilder().setInternalId(internalProfileId).build() + return WalkthroughActivity.createWalkthroughActivityIntent( context, profileId diff --git a/app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughTopicListFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughTopicListFragmentTest.kt index 7bf8581cafe..c3c87bf0125 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughTopicListFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughTopicListFragmentTest.kt @@ -38,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.ProfileId import org.oppia.android.app.player.state.itemviewmodel.SplitScreenInteractionModule import org.oppia.android.app.recyclerview.RecyclerViewMatcher.Companion.atPositionOnView import org.oppia.android.app.shim.ViewBindingShimModule @@ -138,7 +139,8 @@ class WalkthroughTopicListFragmentTest { Intents.release() } - private fun createWalkthroughActivityIntent(profileId: Int): Intent { + private fun createWalkthroughActivityIntent(internalProfileId: Int): Intent { + val profileId = ProfileId.newBuilder().setInternalId(internalProfileId).build() return WalkthroughActivity.createWalkthroughActivityIntent( context, profileId diff --git a/app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughWelcomeFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughWelcomeFragmentTest.kt index 97974f3a2d6..a5e44f0a06a 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughWelcomeFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughWelcomeFragmentTest.kt @@ -141,7 +141,8 @@ class WalkthroughWelcomeFragmentTest { ApplicationProvider.getApplicationContext().inject(this) } - private fun createWalkthroughActivityIntent(profileId: Int): Intent { + private fun createWalkthroughActivityIntent(internalProfileId: Int): Intent { + val profileId = ProfileId.newBuilder().setInternalId(internalProfileId).build() return WalkthroughActivity.createWalkthroughActivityIntent( context, profileId From f13ac52b6a0a81706566d5709921bbbba9ec4a98 Mon Sep 17 00:00:00 2001 From: Ayush Yadav <143514610+theayushyadav11@users.noreply.github.com> Date: Fri, 21 Feb 2025 10:57:17 +0530 Subject: [PATCH 7/8] Fixes #4132: Added tests for MathParsingErrorSubject. (#5684) Fixes #4132: This pull request focuses on adding comprehensive tests for the `MathParsingErrorSubject` class and making necessary adjustments to the related files. The most important changes include the removal of an exemption for `MathParsingErrorSubject.kt`, the addition of a new test file for `MathParsingErrorSubject`, and the creation of a corresponding Bazel test target. ### Test Enhancements: * [`testing/src/main/java/org/oppia/android/testing/math/MathParsingErrorSubject.kt`](diffhunk://#diff-c53693be09949f0e5f37517762c04d2ac4f8eb09049987bfcfcaca1243d51d3aL46-L47): Removed the TODO comment indicating the need for tests. * [`testing/src/test/java/org/oppia/android/testing/math/MathParsingErrorSubjectTest.kt`](diffhunk://#diff-1110331e8184259d41c97f5541a1ab1b71ec94e2942f49e0b6c7df0533bd049eR1-R293): Added a new test file containing various unit tests for the `MathParsingErrorSubject` class. ### Configuration Updates: * [`testing/src/test/java/org/oppia/android/testing/math/BUILD.bazel`](diffhunk://#diff-8a86bceb0e654069d42dcb4e76a52f65aeea3bc7d76a9cdff53f5e4bdde93f08R70-R85): Added a new Bazel test target for `MathParsingErrorSubjectTest`. ### Cleanup: * [`scripts/assets/test_file_exemptions.textproto`](diffhunk://#diff-6ed782dd71126d847e7bac39eca30f830be55b72aa10b7e944612f2463003e24L4105-L4108): Removed the test file exemption for `MathParsingErrorSubject.kt`. #### Screenshot of the pasing tests: ![Screenshot from 2025-02-01 19-58-33](https://github.com/user-attachments/assets/fa770881-5b11-4bc0-907e-a63dbbe4e2ca) ## 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: theayushyadav11 Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> --- scripts/assets/test_file_exemptions.textproto | 4 - .../testing/math/MathParsingErrorSubject.kt | 2 - .../oppia/android/testing/math/BUILD.bazel | 16 + .../math/MathParsingErrorSubjectTest.kt | 300 ++++++++++++++++++ 4 files changed, 316 insertions(+), 6 deletions(-) create mode 100644 testing/src/test/java/org/oppia/android/testing/math/MathParsingErrorSubjectTest.kt diff --git a/scripts/assets/test_file_exemptions.textproto b/scripts/assets/test_file_exemptions.textproto index 02d22431745..2b4731054d5 100644 --- a/scripts/assets/test_file_exemptions.textproto +++ b/scripts/assets/test_file_exemptions.textproto @@ -4102,10 +4102,6 @@ test_file_exemption { exempted_file_path: "testing/src/main/java/org/oppia/android/testing/math/ComparableOperationSubject.kt" test_file_not_required: true } -test_file_exemption { - exempted_file_path: "testing/src/main/java/org/oppia/android/testing/math/MathParsingErrorSubject.kt" - test_file_not_required: true -} test_file_exemption { exempted_file_path: "testing/src/main/java/org/oppia/android/testing/mockito/MockitoKotlinHelper.kt" test_file_not_required: true diff --git a/testing/src/main/java/org/oppia/android/testing/math/MathParsingErrorSubject.kt b/testing/src/main/java/org/oppia/android/testing/math/MathParsingErrorSubject.kt index 9c62f183aca..8a577d8265d 100644 --- a/testing/src/main/java/org/oppia/android/testing/math/MathParsingErrorSubject.kt +++ b/testing/src/main/java/org/oppia/android/testing/math/MathParsingErrorSubject.kt @@ -43,8 +43,6 @@ import org.oppia.android.util.math.MathParsingError.UnbalancedParenthesesError import org.oppia.android.util.math.MathParsingError.UnnecessarySymbolsError import org.oppia.android.util.math.MathParsingError.VariableInNumericExpressionError -// TODO(#4132): Add tests for this class. - /** * Truth subject for verifying properties of [MathParsingError]s. * diff --git a/testing/src/test/java/org/oppia/android/testing/math/BUILD.bazel b/testing/src/test/java/org/oppia/android/testing/math/BUILD.bazel index 63cb143e73e..cdc91cd8b0f 100644 --- a/testing/src/test/java/org/oppia/android/testing/math/BUILD.bazel +++ b/testing/src/test/java/org/oppia/android/testing/math/BUILD.bazel @@ -96,3 +96,19 @@ oppia_android_test( "//third_party:robolectric_android-all", ], ) + +oppia_android_test( + name = "MathParsingErrorSubjectTest", + srcs = ["MathParsingErrorSubjectTest.kt"], + custom_package = "org.oppia.android.testing.math", + test_class = "org.oppia.android.testing.math.MathParsingErrorSubjectTest", + test_manifest = "//testing:test_manifest", + deps = [ + "//model/src/main/proto:math_java_proto_lite", + "//testing/src/main/java/org/oppia/android/testing/math:math_parsing_error_subject", + "//testing/src/main/java/org/oppia/android/testing/robolectric:test_module", + "//third_party:com_google_truth_truth", + "//third_party:junit_junit", + "//third_party:robolectric_android-all", + ], +) diff --git a/testing/src/test/java/org/oppia/android/testing/math/MathParsingErrorSubjectTest.kt b/testing/src/test/java/org/oppia/android/testing/math/MathParsingErrorSubjectTest.kt new file mode 100644 index 00000000000..847a2b804d5 --- /dev/null +++ b/testing/src/test/java/org/oppia/android/testing/math/MathParsingErrorSubjectTest.kt @@ -0,0 +1,300 @@ +package org.oppia.android.testing.math + +import org.junit.Assert.assertThrows +import org.junit.Test +import org.junit.runner.RunWith +import org.junit.runners.JUnit4 +import org.oppia.android.app.model.MathBinaryOperation +import org.oppia.android.app.model.MathExpression +import org.oppia.android.app.model.Real +import org.oppia.android.testing.math.MathParsingErrorSubject.Companion.assertThat +import org.oppia.android.util.math.MathParsingError.DisabledVariablesInUseError +import org.oppia.android.util.math.MathParsingError.EquationHasTooManyEqualsError +import org.oppia.android.util.math.MathParsingError.EquationIsMissingEqualsError +import org.oppia.android.util.math.MathParsingError.EquationMissingLhsOrRhsError +import org.oppia.android.util.math.MathParsingError.ExponentIsVariableExpressionError +import org.oppia.android.util.math.MathParsingError.ExponentTooLargeError +import org.oppia.android.util.math.MathParsingError.FunctionNameIncompleteError +import org.oppia.android.util.math.MathParsingError.GenericError +import org.oppia.android.util.math.MathParsingError.HangingSquareRootError +import org.oppia.android.util.math.MathParsingError.InvalidFunctionInUseError +import org.oppia.android.util.math.MathParsingError.MultipleRedundantParenthesesError +import org.oppia.android.util.math.MathParsingError.NestedExponentsError +import org.oppia.android.util.math.MathParsingError.NoVariableOrNumberAfterBinaryOperatorError +import org.oppia.android.util.math.MathParsingError.NoVariableOrNumberBeforeBinaryOperatorError +import org.oppia.android.util.math.MathParsingError.NumberAfterVariableError +import org.oppia.android.util.math.MathParsingError.SingleRedundantParenthesesError +import org.oppia.android.util.math.MathParsingError.SpacesBetweenNumbersError +import org.oppia.android.util.math.MathParsingError.SubsequentBinaryOperatorsError +import org.oppia.android.util.math.MathParsingError.SubsequentUnaryOperatorsError +import org.oppia.android.util.math.MathParsingError.TermDividedByZeroError +import org.oppia.android.util.math.MathParsingError.UnbalancedParenthesesError +import org.oppia.android.util.math.MathParsingError.UnnecessarySymbolsError +import org.oppia.android.util.math.MathParsingError.VariableInNumericExpressionError + +/** Tests for [MathParsingErrorSubject]. */ +@RunWith(JUnit4::class) +class MathParsingErrorSubjectTest { + + @Test + fun testMathParsingErrorSubject_hasSpaceBetweenNumbersError() { + val error = SpacesBetweenNumbersError + assertThat(error).isSpacesBetweenNumbers() + } + + @Test + fun testMathParsingErrorSubject_hasSpaceBetweenNumbersError_fails() { + val error = UnbalancedParenthesesError + assertThrows(AssertionError::class.java) { + assertThat(error).isSpacesBetweenNumbers() + } + } + + @Test + fun testMathParsingErrorSubject_hasUnbalancedParenthesesError() { + val error = UnbalancedParenthesesError + assertThat(error).isUnbalancedParentheses() + } + + @Test + fun testMathParsingErrorSubject_hasUnbalancedParenthesesError_fails() { + val error = SpacesBetweenNumbersError + assertThrows(AssertionError::class.java) { + assertThat(error).isUnbalancedParentheses() + } + } + + @Test + fun testMathParsingErrorSubject_hasSingleRedundantParenthesesWitDetails() { + val constant = Real.newBuilder().setInteger(5).build() + val expression = MathExpression.newBuilder().setConstant(constant).build() + val group = MathExpression.newBuilder().setGroup(expression).build() + val error = SingleRedundantParenthesesError("(5)", group) + + assertThat(error).isSingleRedundantParenthesesThat().apply { + hasRawExpressionThat().isEqualTo("(5)") + hasExpressionThat().hasStructureThatMatches { + group { + constant { + hasExpressionThat().evaluatesToIntegerThat().isEqualTo(5) + } + } + } + } + } + + @Test + fun testMathParsingErrorSubject_hasMultipleRedundantParenthesesWithDetails() { + val constant = Real.newBuilder().setInteger(5).build() + val expression = MathExpression.newBuilder().setConstant(constant).build() + val groupOne = MathExpression.newBuilder().setGroup(expression).build() + val groupTwo = MathExpression.newBuilder().setGroup(groupOne).build() + val error = MultipleRedundantParenthesesError("((5))", groupTwo) + + assertThat(error).isMultipleRedundantParenthesesThat().apply { + hasRawExpressionThat().isEqualTo("((5))") + hasExpressionThat().hasStructureThatMatches { + group { + group { + constant { + hasExpressionThat().evaluatesToIntegerThat().isEqualTo(5) + } + } + } + } + } + } + + @Test + fun testMathParsingErrorSubject_matchesUnnecessarySymbol() { + val error = UnnecessarySymbolsError("@") + assertThat(error).isUnnecessarySymbolWithSymbolThat().isEqualTo("@") + } + + @Test + fun testMathParsingErrorSubject_matchesUnnecessarySymbol_fails() { + val error = UnnecessarySymbolsError("@") + assertThrows(AssertionError::class.java) { + assertThat(error).isUnnecessarySymbolWithSymbolThat().isEqualTo("#") + } + } + + @Test + fun testMathParsingErrorSubject_isNumberAfterVariableError() { + val number = Real.newBuilder().setInteger(5).build() + val error = NumberAfterVariableError(number, "x") + + assertThat(error).isNumberAfterVariableThat().apply { + hasNumberThat().isIntegerThat().isEqualTo(5) + hasVariableThat().isEqualTo("x") + } + } + + @Test + fun testMathParsingErrorSubject_isNumberAfterVariableError_fails() { + val number = Real.newBuilder().setInteger(5).build() + val error = NumberAfterVariableError(number, "x") + assertThrows(AssertionError::class.java) { + assertThat(error).isNumberAfterVariableThat().apply { + hasNumberThat().isIntegerThat().isEqualTo(5) + hasVariableThat().isEqualTo("y") + } + } + } + + @Test + fun testMathParsingErrorSubject_isSubsequentBinaryOperatorsError() { + val error = SubsequentBinaryOperatorsError("x", "+") + assertThat(error).isSubsequentBinaryOperatorsThat().apply { + hasFirstOperatorThat().isEqualTo("x") + hasSecondOperatorThat().isEqualTo("+") + } + } + + @Test + fun testMathParsingErrorSubject_isSubsequentBinaryOperatorsError_fails() { + val error = SubsequentBinaryOperatorsError("x", "+") + assertThrows(AssertionError::class.java) { + assertThat(error).isSubsequentBinaryOperatorsThat().apply { + hasFirstOperatorThat().isEqualTo("y") + hasSecondOperatorThat().isEqualTo("-") + } + } + } + + @Test + fun testMathParsingErrorSubject_isSubsequentUnaryOperatorsError() { + val error = SubsequentUnaryOperatorsError + assertThat(error).isSubsequentUnaryOperators() + } + + @Test + fun testMathParsingErrorSubject_isNoVarOrNumBeforeBinaryOperator() { + val operator = MathBinaryOperation.Operator.ADD + val error = NoVariableOrNumberBeforeBinaryOperatorError(operator, "+") + assertThat(error).isNoVarOrNumBeforeBinaryOperatorThat().apply { + hasOperatorThat().isEqualTo(operator) + hasOperatorSymbolThat().isEqualTo("+") + } + } + + @Test + fun testMathParsingErrorSubject_isNoVarOrNumBeforeBinaryOperator_fails() { + val operator = MathBinaryOperation.Operator.ADD + val error = NoVariableOrNumberBeforeBinaryOperatorError(operator, "+") + assertThrows(AssertionError::class.java) { + assertThat(error).isNoVarOrNumBeforeBinaryOperatorThat().apply { + hasOperatorThat().isEqualTo(MathBinaryOperation.Operator.SUBTRACT) + hasOperatorSymbolThat().isEqualTo("-") + } + } + } + + @Test + fun testMathParsingErrorSubject_isNoVariableOrNumberAfterBinaryOperator() { + val operator = MathBinaryOperation.Operator.ADD + val error = NoVariableOrNumberAfterBinaryOperatorError(operator, "+") + assertThat(error).isNoVariableOrNumberAfterBinaryOperatorThat().apply { + hasOperatorThat().isEqualTo(operator) + hasOperatorSymbolThat().isEqualTo("+") + } + } + + @Test + fun testMathParsingErrorSubject_isNoVariableOrNumberAfterBinaryOperator_fails() { + val operator = MathBinaryOperation.Operator.ADD + val error = NoVariableOrNumberAfterBinaryOperatorError(operator, "+") + assertThrows(AssertionError::class.java) { + assertThat(error).isNoVariableOrNumberAfterBinaryOperatorThat().apply { + hasOperatorThat().isEqualTo(MathBinaryOperation.Operator.SUBTRACT) + hasOperatorSymbolThat().isEqualTo("-") + } + } + } + + @Test + fun testMathParsingErrorSubject_isExponentIsVariableExpressionError() { + val error = ExponentIsVariableExpressionError + assertThat(error).isExponentIsVariableExpression() + } + + @Test + fun testMathParsingErrorSubject_isExponentTooLargeError() { + val error = ExponentTooLargeError + assertThat(error).isExponentTooLarge() + } + + @Test + fun testMathParsingErrorSubject_isNestedExponentsError() { + val error = NestedExponentsError + assertThat(error).isNestedExponents() + } + + @Test + fun testMathParsingErrorSubject_isHangingSquareRootError() { + val error = HangingSquareRootError + assertThat(error).isHangingSquareRoot() + } + + @Test + fun testMathParsingErrorSubject_isTermDividedByZeroError() { + val error = TermDividedByZeroError + assertThat(error).isTermDividedByZero() + } + + @Test + fun testMathParsingErrorSubject_isVariableInNumericExpressionError() { + val error = VariableInNumericExpressionError + assertThat(error).isVariableInNumericExpression() + } + + @Test + fun testMathParsingErrorSubject_isDisabledVariablesInUseWithVariablesError() { + val error = DisabledVariablesInUseError(listOf("x", "y")) + assertThat(error).isDisabledVariablesInUseWithVariablesThat().containsExactly("x", "y") + } + + @Test + fun testMathParsingErrorSubject_isDisabledVariablesInUseWithVariablesError_fails() { + val error = DisabledVariablesInUseError(listOf("x", "y")) + assertThrows(AssertionError::class.java) { + assertThat(error).isDisabledVariablesInUseWithVariablesThat().containsExactly("x", "z") + } + } + + @Test + fun testMathParsingErrorSubject_isEquationIsMissingEqualsError() { + val error = EquationIsMissingEqualsError + assertThat(error).isEquationIsMissingEquals() + } + + @Test + fun testMathParsingErrorSubject_isEquationHasTooManyEqualsError() { + val error = EquationHasTooManyEqualsError + assertThat(error).isEquationHasTooManyEquals() + } + + @Test + fun testMathParsingErrorSubject_isEquationMissingLhsOrRhsError() { + val error = EquationMissingLhsOrRhsError + assertThat(error).isEquationMissingLhsOrRhs() + } + + @Test + fun testMathParsingErrorSubject_isInvalidFunctionInUseWithNameError() { + val error = InvalidFunctionInUseError("sin") + assertThat(error).isInvalidFunctionInUseWithNameThat().isEqualTo("sin") + } + + @Test + fun testMathParsingErrorSubject_isFunctionNameIncompleteError() { + val error = FunctionNameIncompleteError + assertThat(error).isFunctionNameIncomplete() + } + + @Test + fun testMathParsingErrorSubject_isGenericError() { + val error = GenericError + assertThat(error).isGenericError() + } +} From 469fe9c1a6b949299f8f4d58051388fed47fba83 Mon Sep 17 00:00:00 2001 From: "translatewiki.net" Date: Mon, 24 Feb 2025 18:37:24 +0530 Subject: [PATCH 8/8] Localisation updates from https://translatewiki.net. (#5709) Translation updates --- app/src/main/res/values-ar/strings.xml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/src/main/res/values-ar/strings.xml b/app/src/main/res/values-ar/strings.xml index 5f928cf9c0f..2e7d667849e 100644 --- a/app/src/main/res/values-ar/strings.xml +++ b/app/src/main/res/values-ar/strings.xml @@ -196,6 +196,8 @@ %s ج.ب صحيح! الموضوع: %s + الصورة تظهر %1$s. + حدد الصورة %1$s. و لا فصل فصلًا واحدًا