[DO NOT MERGE] Fix part of #4064: Add new learner analytics events [Blocked: #4257] #4258
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BenHenning
changed the title
This was referenced Mar 24, 2022
BenHenning
changed the base branch from
develop
to
learner-analytics-pt5-integrate-domain
…alytics-pt6-new-events
This one is for audio voiceover playing.
…alytics-pt6-new-events
6 tasks
BenHenning
added a commit
that referenced
this pull request
May 5, 2022
## Explanation Fix part of #4064 This PR integrates the constituent Dagger modules introduced in #4267 broadly in the app so that new logs can be easily added without changing a bunch of files (see #4270). Some notes on the history of this PR: - This PR is a rebase of #4258 to remove dependencies on #2173 (which has since been merged into developer) for a much cleaner history. - This PR is pulling out elements from #4118, #4247, and #4248 which contained completed work by @Sarthak2601. - This PR introduces new logging that didn't exist in the original PRs (#4118, #4247, and #4248) This PR broadly adds all of the analytics event described in the TDD above (& its corresponding PRD), needed for an upcoming user study with the app. It *does* enable actual logging if analytics were to be enabled in the Android manifest (even though the UI side isn't implemented yet--that's planned for #4271). ## 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 PR doesn't make UI changes, and existing flows shouldn't be affected. Commits: * strings for learner analytics * platform parameter impl for learner analytics * nit * nit * event action enum update * addition of contexts * nit * controller level logging and contexts * nit * nit fixes. * nit fixes. * event bundle modifications * sync status, logging identifiers, profile update, lifecycle owner * ui impl: part 1 -- basic * admin control strings * strings correction * strings correction * device id correction * exhaustive when fix. * exhaustive when fix. * todo formatting * nits. * nits. * collapsed contexts, added spacing, added comments * event action removal + nits * tests + dev options event logs fixes post event action removal * nits * removal of method for event action formatted string * nits, null context changes. * nits * reserved fixes and help index fix * bazel imports * bazel build fixes * test fixes * nit * logging identifier controller, module + uuid wrapper, real impl * logging identifier controller tests, fake uuid, tests * sync status manager + fake * logging methods, test setup * profile management, tests * sync status update. * lifecycle observer * Post-merge fixes + Bazel support. * Lots of reorganizing & changes. New tests and documentation have also been added. More broadly, this changes the device ID computation, but actually breaks it so more work will be needed in subsequent commits. * Lint fixes. * Post-merge fix (proper merge of maven_install). * Lint fixes (includes post-merge cleanups). * Lots of stuff. Restructured a lot of the UI, addressed most failing static checks (except KDocs and lint which will be in the follow-up commit), added tests, fixed copying, and generally finished the UI. Sync status seems broken, and it's not yet clear whether events are actually being logged (I need to investigate this). Analytics are disabled in local testing, so that might also be the reason for logs being stuck in an uploading state. * Documentation + lint fixes. This also changes the contract of ClipboardController. * Finish remaining planned tests. * Move over changes from learner-analytics-proto. * Manually pull in changes from 3d6c716. Note that this is operating on a different base). * Post-merge fixes. These at least ensure that the app can build, but many tests will still fail (which is fine seeing as much of this code is going to be split up soon, anyway). Rebase version: app build is no longer guaranteed. * Lint fixes. * Undo all learner analytics changes. I'll be pulling in specific components in specific PRs to organize the changes across 4 PRs. Note that I took this approach to preserve the history from the earlier commits. Those changes will still be included in this PR chain, just a bit awkwardly (i.e. it'll look like I introduced them originally, but that distinction is lost during the squash-and-merge, anyway). * Manually pull in non-app module changes. A bunch of work is still needed to finish these, and I'm still trying to figure out whether I can de-couple the module changes to make reviewing a bit nicer. * Post-merge fixes. All tests verified as building & passing. * Add sync status for no connectivity case. * Remove unnecessary sync manager. * Copy over changes from #4263. These are the domain changes needed for finishing learner analytics support. Cleanup, documentation, and testing all still need to be completed. * Add domain changes for AudioPlayerController. These originate from #4263. * Add missing Javadoc from #4263. * Add module deps for learner analytics components. This ensures that the new domain components introduced in the previous branch can be used throughout the app in both production and test components. * Post-merge fixes from different branch. * Remove all UserIdProdModule references. The module was removed during development in favor of a more integrated approach to managing the logging-related IDs. * Post-merge fix. * Remove references to missing module per #4263. * Add files changes from manual merges/cherry-picks. * Add event log callsites. * Add missing event log from #4263. This one is for audio voiceover playing. * Finish tests & documentation. This also renames 'device ID' to be 'installation ID' for more correctness. * Lint fixes. * Post-merge fixes. * Lint fixes. * Remove files incorrectly readded. * Add follow-up tests to improve coverage. * Post-merge & lint fixes. * Implement tests to verify event logging. These tests only verify event logging at the domain layer, though technically these events will also be logged from UI-based user interactions. Tests to verify that's the case will be added in #4271 as not all aspects of logging are properly hooked up in the UI layer as of this PR. * Update domain's Gradle Mockito version to Bazel's. * Add missing tests & needed behaviors. Fix broken app test. * Fix OS-specific issue in ClipboardController. Co-authored-by: Sarthak Agarwal <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: