From eeb09d197d91ce7b6f4aeef8782cd8ee3c4de678 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Sat, 16 Nov 2019 19:31:12 -0800 Subject: [PATCH 1/3] Add descriptions for fractions & ratios topics, and support HTML in topic descriptions. --- .../TopicOverviewFragmentPresenter.kt | 20 +++++++++++++++---- .../topic/overview/TopicOverviewViewModel.kt | 2 ++ .../res/layout/topic_overview_fragment.xml | 2 +- domain/src/main/assets/fractions_topic.json | 2 +- domain/src/main/assets/ratios_topic.json | 2 +- 5 files changed, 21 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/org/oppia/app/topic/overview/TopicOverviewFragmentPresenter.kt b/app/src/main/java/org/oppia/app/topic/overview/TopicOverviewFragmentPresenter.kt index e10cb8db3d1..5edfe80f96b 100644 --- a/app/src/main/java/org/oppia/app/topic/overview/TopicOverviewFragmentPresenter.kt +++ b/app/src/main/java/org/oppia/app/topic/overview/TopicOverviewFragmentPresenter.kt @@ -8,6 +8,7 @@ import androidx.fragment.app.Fragment import androidx.lifecycle.LiveData import androidx.lifecycle.Observer import androidx.lifecycle.Transformations +import org.oppia.app.R import org.oppia.app.databinding.TopicOverviewFragmentBinding import org.oppia.app.fragment.FragmentScope import org.oppia.app.model.Topic @@ -17,6 +18,7 @@ import org.oppia.app.viewmodel.ViewModelProvider import org.oppia.domain.topic.TopicController import org.oppia.util.data.AsyncResult import org.oppia.util.logging.Logger +import org.oppia.util.parser.HtmlParser import javax.inject.Inject /** The presenter for [TopicOverviewFragment]. */ @@ -26,11 +28,15 @@ class TopicOverviewFragmentPresenter @Inject constructor( private val fragment: Fragment, private val viewModelProvider: ViewModelProvider, private val logger: Logger, - private val topicController: TopicController + private val topicController: TopicController, + private val htmlParserFactory: HtmlParser.Factory ) { private val routeToTopicPlayListener = activity as RouteToTopicPlayListener private val topicOverviewViewModel = getTopicOverviewViewModel() private lateinit var topicId: String + private val htmlParser: HtmlParser by lazy { + htmlParserFactory.create(/* entityType= */ "topic", topicId) + } fun handleCreateView(inflater: LayoutInflater, container: ViewGroup?): View? { topicId = checkNotNull(fragment.arguments?.getString(TOPIC_ID_ARGUMENT_KEY)) { @@ -46,7 +52,7 @@ class TopicOverviewFragmentPresenter @Inject constructor( return binding.root } - fun seeMoreClicked(v: View) { + fun seeMoreClicked(@Suppress("UNUSED_PARAMETER") v: View) { routeToTopicPlayListener.routeToTopicPlayFragment() } @@ -57,8 +63,14 @@ class TopicOverviewFragmentPresenter @Inject constructor( private val topicLiveData: LiveData by lazy { getTopicList() } private fun subscribeToTopicLiveData() { - topicLiveData.observe(fragment, Observer { result -> - topicOverviewViewModel.topic.set(result) + topicLiveData.observe(fragment, Observer { topic -> + topicOverviewViewModel.topic.set(topic) + topicOverviewViewModel.topicDescription.set( + htmlParser.parseOppiaHtml( + topic.description, + fragment.requireView().findViewById(R.id.topic_description_text_view) + ) + ) }) } diff --git a/app/src/main/java/org/oppia/app/topic/overview/TopicOverviewViewModel.kt b/app/src/main/java/org/oppia/app/topic/overview/TopicOverviewViewModel.kt index 2b4f64c85c8..db595911f94 100644 --- a/app/src/main/java/org/oppia/app/topic/overview/TopicOverviewViewModel.kt +++ b/app/src/main/java/org/oppia/app/topic/overview/TopicOverviewViewModel.kt @@ -13,5 +13,7 @@ import javax.inject.Inject class TopicOverviewViewModel @Inject constructor() : ObservableViewModel() { val topic = ObservableField(Topic.getDefaultInstance()) + val topicDescription = ObservableField("") + var downloadStatusIndicatorDrawableResourceId = ObservableField(R.drawable.ic_available_offline_primary_24dp) } diff --git a/app/src/main/res/layout/topic_overview_fragment.xml b/app/src/main/res/layout/topic_overview_fragment.xml index bf7a59b8ebb..ff068258bc5 100644 --- a/app/src/main/res/layout/topic_overview_fragment.xml +++ b/app/src/main/res/layout/topic_overview_fragment.xml @@ -83,7 +83,7 @@ android:layout_marginTop="16dp" android:layout_marginEnd="24dp" android:gravity="start" - android:text="@{viewModel.topic.description}" + android:text="@{viewModel.topicDescription}" android:textColor="@color/oppiaPrimaryText" android:textSize="16sp" app:layout_constraintEnd_toEndOf="parent" diff --git a/domain/src/main/assets/fractions_topic.json b/domain/src/main/assets/fractions_topic.json index 8eb4ba83ea9..8a5e6417f83 100644 --- a/domain/src/main/assets/fractions_topic.json +++ b/domain/src/main/assets/fractions_topic.json @@ -1,7 +1,7 @@ { "topic": { "story_reference_schema_version": 1, - "description": "", + "description": "You'll often need to talk about part of an object or group. For example, a jar of milk might be half-full, or some of the eggs in a box might have broken. In these lessons, you'll learn to use fractions to describe situations like these.", "canonical_story_references": [ { "story_id": "wANbh4oOClga", diff --git a/domain/src/main/assets/ratios_topic.json b/domain/src/main/assets/ratios_topic.json index fac7c0fcb66..f5a7aad4f84 100644 --- a/domain/src/main/assets/ratios_topic.json +++ b/domain/src/main/assets/ratios_topic.json @@ -1,7 +1,7 @@ { "topic": { "story_reference_schema_version": 1, - "description": "", + "description": "Many everyday problems involve thinking about proportions. For example, if 30 apples cost 6 dollars, how much would 12 apples cost? In these lessons, you'll learn about how to use ratios and proportional reasoning to solve problems like this.", "canonical_story_references": [], "language_code": "en", "id": "omzF4oqgeTXd", From cb558aad5d9380fb6654e7e41a5350a63b58f920 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Sat, 16 Nov 2019 20:04:46 -0800 Subject: [PATCH 2/3] Add test to verify rich-text parsing. Small miscellaneous fixes. --- .../java/org/oppia/app/topic/TopicActivity.kt | 7 +- .../oppia/app/topic/TopicActivityPresenter.kt | 5 +- .../oppia/app/topic/TopicFragmentPresenter.kt | 4 +- .../overview/TopicOverviewFragmentTest.kt | 104 +++++++----------- 4 files changed, 48 insertions(+), 72 deletions(-) diff --git a/app/src/main/java/org/oppia/app/topic/TopicActivity.kt b/app/src/main/java/org/oppia/app/topic/TopicActivity.kt index f296540852a..334762af84c 100644 --- a/app/src/main/java/org/oppia/app/topic/TopicActivity.kt +++ b/app/src/main/java/org/oppia/app/topic/TopicActivity.kt @@ -17,18 +17,17 @@ const val TOPIC_ACTIVITY_TOPIC_ID_ARGUMENT_KEY = "TopicActivity.topic_id" class TopicActivity : InjectableAppCompatActivity(), RouteToQuestionPlayerListener, RouteToConceptCardListener, RouteToTopicPlayListener, RouteToStoryListener, RouteToExplorationListener { private lateinit var topicId: String - private lateinit var storyId: String + private var storyId: String? = null @Inject lateinit var topicActivityPresenter: TopicActivityPresenter override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) activityComponent.inject(this) - topicId = checkNotNull(intent?.getStringExtra(org.oppia.app.topic.TOPIC_ACTIVITY_TOPIC_ID_ARGUMENT_KEY) ?: "") - { + topicId = checkNotNull(intent?.getStringExtra(org.oppia.app.topic.TOPIC_ACTIVITY_TOPIC_ID_ARGUMENT_KEY)) { "Expected topic ID to be included in intent for TopicActivity." } - storyId = intent?.getStringExtra(TOPIC_ACTIVITY_STORY_ID_ARGUMENT_KEY) ?: "" + storyId = intent?.getStringExtra(TOPIC_ACTIVITY_STORY_ID_ARGUMENT_KEY) topicActivityPresenter.handleOnCreate(topicId, storyId) } diff --git a/app/src/main/java/org/oppia/app/topic/TopicActivityPresenter.kt b/app/src/main/java/org/oppia/app/topic/TopicActivityPresenter.kt index e738692436a..5f499845276 100644 --- a/app/src/main/java/org/oppia/app/topic/TopicActivityPresenter.kt +++ b/app/src/main/java/org/oppia/app/topic/TopicActivityPresenter.kt @@ -14,14 +14,15 @@ const val STORY_ID_ARGUMENT_KEY = "story_id" @ActivityScope class TopicActivityPresenter @Inject constructor(private val activity: AppCompatActivity) { - fun handleOnCreate(topicId: String, storyId: String) { + fun handleOnCreate(topicId: String, storyId: String?) { activity.setContentView(R.layout.topic_activity) if (getTopicFragment() == null) { val topicFragment = TopicFragment() val args = Bundle() args.putString(TOPIC_ID_ARGUMENT_KEY, topicId) - if (topicId.isNotEmpty() && storyId.isNotEmpty()) + if (storyId != null) { args.putString(STORY_ID_ARGUMENT_KEY, storyId) + } topicFragment.arguments = args activity.supportFragmentManager.beginTransaction().add( R.id.topic_fragment_placeholder, diff --git a/app/src/main/java/org/oppia/app/topic/TopicFragmentPresenter.kt b/app/src/main/java/org/oppia/app/topic/TopicFragmentPresenter.kt index 7c3d16834d2..ee5971372cf 100644 --- a/app/src/main/java/org/oppia/app/topic/TopicFragmentPresenter.kt +++ b/app/src/main/java/org/oppia/app/topic/TopicFragmentPresenter.kt @@ -60,8 +60,8 @@ class TopicFragmentPresenter @Inject constructor( viewPager.setCurrentItem(tab.ordinal, true) } - private fun setUpViewPager(viewPager: ViewPager, topicId: String?) { - val adapter = ViewPagerAdapter(fragment.fragmentManager!!, topicId!!, storyId) + private fun setUpViewPager(viewPager: ViewPager, topicId: String) { + val adapter = ViewPagerAdapter(fragment.childFragmentManager, topicId, storyId) viewPager.adapter = adapter tabLayout.setupWithViewPager(viewPager) tabLayout.getTabAt(0)!!.setText(fragment.getString(R.string.overview)).setIcon(tabIcons[0]) diff --git a/app/src/sharedTest/java/org/oppia/app/topic/overview/TopicOverviewFragmentTest.kt b/app/src/sharedTest/java/org/oppia/app/topic/overview/TopicOverviewFragmentTest.kt index a85200154b4..c054f364617 100644 --- a/app/src/sharedTest/java/org/oppia/app/topic/overview/TopicOverviewFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/app/topic/overview/TopicOverviewFragmentTest.kt @@ -1,116 +1,92 @@ package org.oppia.app.topic.overview -import android.app.Application -import android.content.Context import android.content.pm.ActivityInfo +import android.text.SpannedString +import android.text.style.StyleSpan +import android.widget.TextView import androidx.test.core.app.ActivityScenario +import androidx.test.core.app.ApplicationProvider import androidx.test.espresso.Espresso.onView import androidx.test.espresso.assertion.ViewAssertions.matches import androidx.test.espresso.matcher.ViewMatchers.isClickable import androidx.test.espresso.matcher.ViewMatchers.withId import androidx.test.espresso.matcher.ViewMatchers.withText import androidx.test.ext.junit.runners.AndroidJUnit4 -import androidx.test.rule.ActivityTestRule -import dagger.BindsInstance -import dagger.Component -import dagger.Module -import dagger.Provides -import kotlinx.coroutines.CoroutineDispatcher +import com.google.common.truth.Truth.assertThat import org.hamcrest.Matchers.containsString -import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.oppia.app.R import org.oppia.app.topic.TopicActivity import org.oppia.app.utility.EspressoTestsMatchers.withDrawable -import org.oppia.util.threading.BackgroundDispatcher -import org.oppia.util.threading.BlockingDispatcher -import javax.inject.Singleton +private const val TEST_TOPIC_ID = "GJ2rLXRKD5hw" +private const val TOPIC_NAME = "Fractions" +private const val TOPIC_DESCRIPTION = + "You'll often need to talk about part of an object or group. For example, a jar of milk might be half-full, or " + + "some of the eggs in a box might have broken. In these lessons, you'll learn to use fractions to describe " + + "situations like these." + +// NOTE TO DEVELOPERS: this test must be annotated with @LooperMode(LooperMode.MODE.PAUSED) to pass. /** Tests for [TopicOverviewFragment]. */ @RunWith(AndroidJUnit4::class) class TopicOverviewFragmentTest { - - private val topicThumbnail = R.drawable.lesson_thumbnail_graphic_child_with_cupcakes - - private val topicName = "Second Test Topic" - - private val topicDescription = - "A topic considering the various implications of having especially long topic descriptions. " + - "These descriptions almost certainly need to wrap, which should be interesting in the UI (especially on " + - "small screens). Consider also that there may even be multiple points pertaining to a topic, some of which " + - "may require expanding the description section in order to read the whole topic description." - - @get:Rule - var activityTestRule: ActivityTestRule = ActivityTestRule( - TopicActivity::class.java, /* initialTouchMode= */ true, /* launchActivity= */ false - ) + private val topicThumbnail = R.drawable.lesson_thumbnail_graphic_child_with_fractions_homework @Test fun testTopicOverviewFragment_loadFragment_checkTopicName_isCorrect() { - ActivityScenario.launch(TopicActivity::class.java).use { - onView(withId(R.id.topic_name_text_view)).check(matches(withText(containsString(topicName)))) + launchTopicActivityIntent(TEST_TOPIC_ID).use { + onView(withId(R.id.topic_name_text_view)).check(matches(withText(containsString(TOPIC_NAME)))) } } // TODO(#135): Update this test case to check on click of See More play tab is shown. @Test fun testTopicOverviewFragment_loadFragment_seeMoreIsClickable() { - ActivityScenario.launch(TopicActivity::class.java).use { + launchTopicActivityIntent(TEST_TOPIC_ID).use { onView(withId(R.id.see_more_text_view)).check(matches(isClickable())) } } @Test fun testTopicOverviewFragment_loadFragmentWithTestTopicId1_checkTopicDescription_isCorrect() { - ActivityScenario.launch(TopicActivity::class.java).use { - onView(withId(R.id.topic_description_text_view)).check(matches(withText(containsString(topicDescription)))) + launchTopicActivityIntent(TEST_TOPIC_ID).use { + onView(withId(R.id.topic_description_text_view)).check(matches(withText(containsString(TOPIC_DESCRIPTION)))) + } + } + + @Test + fun testTopicOverviewFragment_loadFragmentWithTestTopicId1_checkTopicDescription_hasRichText() { + launchTopicActivityIntent(TEST_TOPIC_ID).use { scenario -> + scenario.onActivity { activity -> + val descriptionTextView: TextView = activity.findViewById(R.id.topic_description_text_view) + val descriptionText = descriptionTextView.text as SpannedString + val spans = descriptionText.getSpans(0, descriptionText.length, StyleSpan::class.java) + assertThat(spans).isNotEmpty() + } } } @Test fun testTopicOverviewFragment_loadFragment_configurationChange_checkTopicThumbnail_isCorrect() { - ActivityScenario.launch(TopicActivity::class.java).use { + launchTopicActivityIntent(TEST_TOPIC_ID).use { onView(withId(R.id.topic_thumbnail_image_view)).check(matches(withDrawable(topicThumbnail))) } } @Test fun testTopicOverviewFragment_loadFragment_configurationChange_checkTopicName_isCorrect() { - activityTestRule.launchActivity(null) - activityTestRule.activity.requestedOrientation = ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE - onView(withId(R.id.topic_name_text_view)).check(matches(withText(containsString(topicName)))) - } + launchTopicActivityIntent(TEST_TOPIC_ID).use { scenario -> + scenario.onActivity { activity -> + activity.requestedOrientation = ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE + } - @Module - class TestModule { - @Provides - @Singleton - fun provideContext(application: Application): Context { - return application - } - - // TODO(#89): Introduce a proper IdlingResource for background dispatchers to ensure they all complete before - // proceeding in an Espresso test. This solution should also be interoperative with Robolectric contexts by using a - // test coroutine dispatcher. - - @Singleton - @Provides - @BackgroundDispatcher - fun provideBackgroundDispatcher(@BlockingDispatcher blockingDispatcher: CoroutineDispatcher): CoroutineDispatcher { - return blockingDispatcher + onView(withId(R.id.topic_name_text_view)).check(matches(withText(containsString(TOPIC_NAME)))) } } - @Singleton - @Component(modules = [TestModule::class]) - interface TestApplicationComponent { - @Component.Builder - interface Builder { - @BindsInstance - fun setApplication(application: Application): Builder - - fun build(): TestApplicationComponent - } + private fun launchTopicActivityIntent(topicId: String): ActivityScenario { + val intent = TopicActivity.createTopicActivityIntent(ApplicationProvider.getApplicationContext(), topicId) + return ActivityScenario.launch(intent) } } From b6ec77607803f02cb097e930814269e903673349 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 19 Nov 2019 16:57:46 -0800 Subject: [PATCH 3/3] Misc comment update & address reviewer comment by adding tests, and fix broken build. --- .../overview/TopicOverviewFragmentTest.kt | 2 +- ...QuestionAssessmentProgressControllerTest.kt | 18 ++++++++++++++++++ .../oppia/domain/topic/TopicControllerTest.kt | 18 ++++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/app/src/sharedTest/java/org/oppia/app/topic/overview/TopicOverviewFragmentTest.kt b/app/src/sharedTest/java/org/oppia/app/topic/overview/TopicOverviewFragmentTest.kt index 3c0b4904d97..0e18c005358 100644 --- a/app/src/sharedTest/java/org/oppia/app/topic/overview/TopicOverviewFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/app/topic/overview/TopicOverviewFragmentTest.kt @@ -28,7 +28,7 @@ private const val TOPIC_DESCRIPTION = "some of the eggs in a box might have broken. In these lessons, you'll learn to use fractions to describe " + "situations like these." -// NOTE TO DEVELOPERS: this test must be annotated with @LooperMode(LooperMode.MODE.PAUSED) to pass. +// NOTE TO DEVELOPERS: this test must be annotated with @LooperMode(LooperMode.MODE.PAUSED) to pass on Robolectric. /** Tests for [TopicOverviewFragment]. */ @RunWith(AndroidJUnit4::class) class TopicOverviewFragmentTest { diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt index d6c902cb5fd..e3927adcb20 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt @@ -31,6 +31,10 @@ import org.oppia.domain.topic.TEST_SKILL_ID_0 import org.oppia.domain.topic.TEST_SKILL_ID_1 import org.oppia.domain.topic.TEST_SKILL_ID_2 import org.oppia.util.data.AsyncResult +import org.oppia.util.logging.EnableConsoleLog +import org.oppia.util.logging.EnableFileLog +import org.oppia.util.logging.GlobalLogLevel +import org.oppia.util.logging.LogLevel import org.oppia.util.threading.BackgroundDispatcher import org.oppia.util.threading.BlockingDispatcher import org.robolectric.annotation.Config @@ -219,6 +223,20 @@ class QuestionAssessmentProgressControllerTest { fun provideBlockingDispatcher(@TestDispatcher testDispatcher: CoroutineDispatcher): CoroutineDispatcher { return testDispatcher } + + // TODO(#59): Either isolate these to their own shared test module, or use the real logging + // module in tests to avoid needing to specify these settings for tests. + @EnableConsoleLog + @Provides + fun provideEnableConsoleLog(): Boolean = true + + @EnableFileLog + @Provides + fun provideEnableFileLog(): Boolean = false + + @GlobalLogLevel + @Provides + fun provideGlobalLogLevel(): LogLevel = LogLevel.VERBOSE } @Module diff --git a/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt b/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt index 35499ca42d7..5cac14a9bf4 100644 --- a/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt @@ -210,6 +210,15 @@ class TopicControllerTest { assertThat(topic.skillCount).isEqualTo(3) } + @Test + fun testRetrieveTopic_fractionsTopic_hasCorrectDescription() { + val topicLiveData = topicController.getTopic(FRACTIONS_TOPIC_ID) + + val topic = topicLiveData.value!!.getOrThrow() + assertThat(topic.topicId).isEqualTo(FRACTIONS_TOPIC_ID) + assertThat(topic.description).contains("You'll often need to talk about") + } + @Test fun testRetrieveTopic_ratiosTopic_returnsCorrectTopic() { val topicLiveData = topicController.getTopic(RATIOS_TOPIC_ID) @@ -220,6 +229,15 @@ class TopicControllerTest { assertThat(topic.skillCount).isEqualTo(1) } + @Test + fun testRetrieveTopic_ratiosTopic_hasCorrectDescription() { + val topicLiveData = topicController.getTopic(RATIOS_TOPIC_ID) + + val topic = topicLiveData.value!!.getOrThrow() + assertThat(topic.topicId).isEqualTo(RATIOS_TOPIC_ID) + assertThat(topic.description).contains("Many everyday problems involve thinking about proportions") + } + @Test fun testRetrieveTopic_invalidTopic_returnsFailure() { val topicLiveData = topicController.getTopic("invalid_topic_id")