Skip to content

Commit

Permalink
Fix #4042: Implement success criteria metrics for lesson checkpointing (
Browse files Browse the repository at this point in the history
#5336)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation
<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->

Fixes #4042

This PR implements the success criteria metrics required for lesson
checkpointing as described
[here](https://docs.google.com/document/d/1d8yjwz76mngtsPRxC7fubgLKg8mfA7kG1sWRWdbiaVw/edit#bookmark=id.2zyjd5vygmcv).
A high level overview of the implementation is documented on the issue
thread
[here](#4042 (comment)).

#### Changes to FakeAnalyticsEventLogger
- `fun getOldestEvents(count: Int): List<EventLog>` is necessary to
retrieve a list of a pre-defined count of oldest events from all the
logged events.
- `fun getLoggedEvent(predicate: (EventLog) -> Boolean): EventLog?`
serves to acquire a reference to a logged event when the context of the
event is known, but the event index is uncertain or potentially
surrounded by other events. Thus, extracting the event via index might
be difficult.

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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
<!-- Delete these section if this PR does not include UI-related
changes. -->
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

---------

Co-authored-by: Adhiambo Peres <[email protected]>
  • Loading branch information
theMr17 and adhiamboperes committed May 8, 2024
1 parent a818170 commit 98257db
Show file tree
Hide file tree
Showing 13 changed files with 813 additions and 38 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import org.oppia.android.app.viewmodel.ViewModelProvider
import org.oppia.android.databinding.ExplorationActivityBinding
import org.oppia.android.domain.exploration.ExplorationDataController
import org.oppia.android.domain.oppialogger.OppiaLogger
import org.oppia.android.domain.oppialogger.analytics.LearnerAnalyticsLogger
import org.oppia.android.domain.survey.SurveyGatingController
import org.oppia.android.domain.translation.TranslationController
import org.oppia.android.util.accessibility.AccessibilityService
Expand All @@ -60,6 +61,7 @@ class ExplorationActivityPresenter @Inject constructor(
private val fontScaleConfigurationUtil: FontScaleConfigurationUtil,
private val translationController: TranslationController,
private val oppiaLogger: OppiaLogger,
private val learnerAnalyticsLogger: LearnerAnalyticsLogger,
private val resourceHandler: AppLanguageResourceHandler,
private val surveyGatingController: SurveyGatingController
) {
Expand Down Expand Up @@ -328,7 +330,7 @@ class ExplorationActivityPresenter @Inject constructor(
return
}
// If checkpointing is enabled, get the current checkpoint state to show an appropriate dialog
// fragment.
// fragment and log lesson saved advertently event.
showDialogFragmentBasedOnCurrentCheckpointState()
}

Expand Down Expand Up @@ -514,9 +516,11 @@ class ExplorationActivityPresenter @Inject constructor(
} else {
when (checkpointState) {
CheckpointState.CHECKPOINT_SAVED_DATABASE_NOT_EXCEEDED_LIMIT -> {
learnerAnalyticsLogger.explorationAnalyticsLogger.value?.logLessonSavedAdvertently()
stopExploration(isCompletion = false)
}
CheckpointState.CHECKPOINT_SAVED_DATABASE_EXCEEDED_LIMIT -> {
learnerAnalyticsLogger.explorationAnalyticsLogger.value?.logLessonSavedAdvertently()
showProgressDatabaseFullDialogFragment()
}
else -> showUnsavedExplorationDialogFragment()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ import org.oppia.android.app.application.testing.TestingBuildFlavorModule
import org.oppia.android.app.devoptions.DeveloperOptionsModule
import org.oppia.android.app.devoptions.DeveloperOptionsStarterModule
import org.oppia.android.app.help.HelpActivity
import org.oppia.android.app.model.EventLog.Context.ActivityContextCase.LESSON_SAVED_ADVERTENTLY_CONTEXT
import org.oppia.android.app.model.ExplorationActivityParams
import org.oppia.android.app.model.OppiaLanguage
import org.oppia.android.app.model.ProfileId
Expand Down Expand Up @@ -128,6 +129,7 @@ import org.oppia.android.domain.topic.TEST_TOPIC_ID_0
import org.oppia.android.domain.translation.TranslationController
import org.oppia.android.domain.workmanager.WorkManagerConfigurationModule
import org.oppia.android.testing.BuildEnvironment
import org.oppia.android.testing.FakeAnalyticsEventLogger
import org.oppia.android.testing.OppiaTestRule
import org.oppia.android.testing.RunOn
import org.oppia.android.testing.TestLogReportingModule
Expand Down Expand Up @@ -222,6 +224,9 @@ class ExplorationActivityTest {
@Inject
lateinit var fakeAccessibilityService: FakeAccessibilityService

@Inject
lateinit var fakeAnalyticsEventLogger: FakeAnalyticsEventLogger

private val internalProfileId: Int = 0

@Before
Expand Down Expand Up @@ -1932,6 +1937,129 @@ class ExplorationActivityTest {
assertThat(explorationActivityTestRule.activity.isFinishing).isTrue()
}

@Test
fun testExpActivity_startNewExploration_pressBack_logsLessonSavedAdvertentlyEvent() {
setUpAudioForFractionLesson()
explorationActivityTestRule.launchActivity(
createExplorationActivityIntent(
internalProfileId,
FRACTIONS_TOPIC_ID,
FRACTIONS_STORY_ID_0,
FRACTIONS_EXPLORATION_ID_0,
shouldSavePartialProgress = true
)
)
explorationDataController.startPlayingNewExploration(
internalProfileId,
FRACTIONS_TOPIC_ID,
FRACTIONS_STORY_ID_0,
FRACTIONS_EXPLORATION_ID_0
)
testCoroutineDispatchers.runCurrent()

pressBack()
testCoroutineDispatchers.runCurrent()

val lessonSavedAdvertentlyEventCount = fakeAnalyticsEventLogger.countEvents {
it.context.activityContextCase == LESSON_SAVED_ADVERTENTLY_CONTEXT
}
assertThat(lessonSavedAdvertentlyEventCount).isEqualTo(1)
}

@Test
fun testExpActivity_startNewExploration_pressToolbarBackIcon_logsLessonSavedAdvertentlyEvent() {
setUpAudioForFractionLesson()
markAllSpotlightsSeen()
explorationActivityTestRule.launchActivity(
createExplorationActivityIntent(
internalProfileId,
FRACTIONS_TOPIC_ID,
FRACTIONS_STORY_ID_0,
FRACTIONS_EXPLORATION_ID_0,
shouldSavePartialProgress = true
)
)
explorationDataController.startPlayingNewExploration(
internalProfileId,
FRACTIONS_TOPIC_ID,
FRACTIONS_STORY_ID_0,
FRACTIONS_EXPLORATION_ID_0
)
testCoroutineDispatchers.runCurrent()

// Click on 'X' icon on toolbar.
onView(withContentDescription(R.string.nav_app_bar_navigate_up_description)).perform(click())
testCoroutineDispatchers.runCurrent()

explorationDataController.stopPlayingExploration(isCompletion = false)
testCoroutineDispatchers.runCurrent()

val lessonSavedAdvertentlyEventCount = fakeAnalyticsEventLogger.countEvents {
it.context.activityContextCase == LESSON_SAVED_ADVERTENTLY_CONTEXT
}
assertThat(lessonSavedAdvertentlyEventCount).isEqualTo(1)
}

@Test
fun testExpActivity_replayExploration_pressBack_doesNotLogLessonSavedAdvertentlyEvent() {
setUpAudioForFractionLesson()
explorationActivityTestRule.launchActivity(
createExplorationActivityIntent(
internalProfileId,
FRACTIONS_TOPIC_ID,
FRACTIONS_STORY_ID_0,
FRACTIONS_EXPLORATION_ID_0,
shouldSavePartialProgress = false
)
)
explorationDataController.replayExploration(
internalProfileId,
FRACTIONS_TOPIC_ID,
FRACTIONS_STORY_ID_0,
FRACTIONS_EXPLORATION_ID_0
)
testCoroutineDispatchers.runCurrent()

pressBack()
testCoroutineDispatchers.runCurrent()

val lessonSavedAdvertentlyEventCount = fakeAnalyticsEventLogger.countEvents {
it.context.activityContextCase == LESSON_SAVED_ADVERTENTLY_CONTEXT
}
assertThat(lessonSavedAdvertentlyEventCount).isEqualTo(0)
}

@Test
fun testExpActivity_replayExp_pressToolbarBackIcon_doesNotLogLessonSavedAdvertentlyEvent() {
setUpAudioForFractionLesson()
markAllSpotlightsSeen()
explorationActivityTestRule.launchActivity(
createExplorationActivityIntent(
internalProfileId,
FRACTIONS_TOPIC_ID,
FRACTIONS_STORY_ID_0,
FRACTIONS_EXPLORATION_ID_0,
shouldSavePartialProgress = false
)
)
explorationDataController.replayExploration(
internalProfileId,
FRACTIONS_TOPIC_ID,
FRACTIONS_STORY_ID_0,
FRACTIONS_EXPLORATION_ID_0
)
testCoroutineDispatchers.runCurrent()

// Click on 'X' icon on toolbar.
onView(withContentDescription(R.string.nav_app_bar_navigate_up_description)).perform(click())
testCoroutineDispatchers.runCurrent()

val lessonSavedAdvertentlyEventCount = fakeAnalyticsEventLogger.countEvents {
it.context.activityContextCase == LESSON_SAVED_ADVERTENTLY_CONTEXT
}
assertThat(lessonSavedAdvertentlyEventCount).isEqualTo(0)
}

@Test
@RunOn(TestPlatform.ROBOLECTRIC) // TODO(#3858): Enable for Espresso.
fun testExpActivity_englishContentLang_contentIsInEnglish() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,15 @@ class ExplorationProgressController @Inject constructor(
ControllerState(
ExplorationProgress(),
message.isRestart,
// The [message.explorationCheckpoint] is [ExplorationCheckpoint.getDefaultInstance()]
// in the following 3 cases.
// - New exploration is started.
// - Saved Exploration is restarted.
// - Completed exploration is replayed.
// The [message.explorationCheckpoint] will contain the exploration checkpoint
// only when a saved exploration is resumed.
isResume = message.explorationCheckpoint
!= ExplorationCheckpoint.getDefaultInstance(),
message.sessionId,
message.ephemeralStateFlow,
commandQueue,
Expand Down Expand Up @@ -634,6 +643,14 @@ class ExplorationProgressController @Inject constructor(
topPendingState.interaction, userAnswer, answerOutcome.labelledAsCorrectAnswer
)

// Log correct & incorrect answer submission in a resumed exploration.
if (isResume) {
if (answerOutcome.labelledAsCorrectAnswer)
explorationAnalyticsLogger.logResumeLessonSubmitCorrectAnswer()
else
explorationAnalyticsLogger.logResumeLessonSubmitIncorrectAnswer()
}

// Follow the answer's outcome to another part of the graph if it's different.
val ephemeralState = computeBaseCurrentEphemeralState()
when {
Expand Down Expand Up @@ -947,9 +964,11 @@ class ExplorationProgressController @Inject constructor(

deferred.invokeOnCompletion {
val checkpointState = if (it == null) {
explorationAnalyticsLogger.logProgressSavingSuccess()
deferred.getCompleted()
} else {
oppiaLogger.e("Lightweight checkpointing", "Failed to save checkpoint in exploration", it)
explorationAnalyticsLogger.logProgressSavingFailure()
// CheckpointState is marked as CHECKPOINT_UNSAVED because the deferred did not
// complete successfully.
CheckpointState.CHECKPOINT_UNSAVED
Expand Down Expand Up @@ -1094,6 +1113,7 @@ class ExplorationProgressController @Inject constructor(
private class ControllerState(
val explorationProgress: ExplorationProgress,
val isRestart: Boolean,
val isResume: Boolean,
val sessionId: String,
val ephemeralStateFlow: MutableStateFlow<AsyncResult<EphemeralState>>,
val commandQueue: SendChannel<ControllerMessage<*>>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,31 @@ class LearnerAnalyticsLogger @Inject constructor(
getExpectedStateLogger()?.logFinishExploration()
}

/** Logs that the current exploration progress has been saved successfully. */
fun logProgressSavingSuccess() {
getExpectedStateLogger()?.logProgressSavingSuccess()
}

/** Logs that the current exploration progress has failed to save. */
fun logProgressSavingFailure() {
getExpectedStateLogger()?.logProgressSavingFailure()
}

/** Logs that the user has left the lesson advertently (attempted to save). */
fun logLessonSavedAdvertently() {
getExpectedStateLogger()?.logLessonSavedAdvertently()
}

/** Logs that correct answer was submitted in a resumed lesson. */
fun logResumeLessonSubmitCorrectAnswer() {
getExpectedStateLogger()?.logResumeLessonSubmitCorrectAnswer()
}

/** Logs that incorrect answer was submitted in a resumed lesson. */
fun logResumeLessonSubmitIncorrectAnswer() {
getExpectedStateLogger()?.logResumeLessonSubmitIncorrectAnswer()
}

/**
* Begins analytics logging for the specified [newState], returning the [StateAnalyticsLogger]
* that can be used to log events for the [State].
Expand Down Expand Up @@ -306,6 +331,31 @@ class LearnerAnalyticsLogger @Inject constructor(
logStateEvent(EventBuilder::setFinishExplorationContext)
}

/** Logs that the current exploration progress has been saved successfully. */
internal fun logProgressSavingSuccess() {
logStateEvent(EventBuilder::setProgressSavingSuccessContext)
}

/** Logs that the current exploration progress has failed to save. */
internal fun logProgressSavingFailure() {
logStateEvent(EventBuilder::setProgressSavingFailureContext)
}

/** Logs that the user has left the lesson advertently (attempted to save). */
internal fun logLessonSavedAdvertently() {
logStateEvent(EventBuilder::setLessonSavedAdvertentlyContext)
}

/** Logs that correct answer was submitted in a resumed lesson. */
internal fun logResumeLessonSubmitCorrectAnswer() {
logStateEvent(EventBuilder::setResumeLessonSubmitCorrectAnswerContext)
}

/** Logs that incorrect answer was submitted in a resumed lesson. */
internal fun logResumeLessonSubmitIncorrectAnswer() {
logStateEvent(EventBuilder::setResumeLessonSubmitIncorrectAnswerContext)
}

/** Logs that this card has been started. */
fun logStartCard() {
logStateEvent(linkedSkillId, ::createCardContext, EventBuilder::setStartCardContext)
Expand Down
Loading

0 comments on commit 98257db

Please sign in to comment.