Fix #5001: Set Up Firestore and Upload Free Form Responses #5098
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
Some of the production code was updated in the previous commit.
These scenarios are covered in other tests so I'm fine with not trying to make them work.
This commit sets up the logging infrastructure, including tests. Since Firestore is introduced, some tests in the app module may be broken, and will be fixed in the next commit. This commit soes not handle actual upload to firestore yet, because firebase auth is not yet properly configured.
The comment on timeout possibility and creating a sealed class for fakeauthenticationwrapperimp have not been addressed in this commit.
…ional-response-upload
|
Unassigning @adhiamboperes since a re-review was requested. @adhiamboperes, please make sure you have addressed all review comments. Thanks! |
This is important so that we can verify uploads and also test appcheck
BenHenning
approved these changes
Jan 26, 2024
There was a problem hiding this comment.
Thanks @adhiamboperes! Just had a few follow-up comments. Overall the PR LGTM (so I've approved it as such), but feel free to assign me to take another pass on the final deltas if you'd like (I'm fine with you resolving my follow-up comments, as well).
domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/FirestoreDataController.kt
Outdated
Show resolved
Hide resolved
domain/src/test/java/org/oppia/android/domain/auth/FirebaseAuthWrapperImplTest.kt
Outdated
Show resolved
Hide resolved
domain/src/test/java/org/oppia/android/domain/auth/FirebaseAuthWrapperImplTest.kt
Outdated
Show resolved
Hide resolved
testing/src/main/java/org/oppia/android/testing/FakeFirebaseAuthWrapperImpl.kt
Outdated
Show resolved
Hide resolved
testing/src/test/java/org/oppia/android/testing/FakeFirebaseAuthWrapperImplTest.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/devoptions/vieweventlogs/ViewEventLogsViewModel.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/auth/AuthenticationController.kt
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/auth/AuthenticationController.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/FirestoreDataController.kt
Outdated
Show resolved
Hide resolved
|
Hi @adhiamboperes, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks! |
- refactor FakeAuthState from sealed to enum class - refactor auth timeout to constant - fix remove uploaded logs logic - fix firebase authwrapper test set up and naming - refactor event log viewmodel list mapping
|
@BenHenning, I have resolved all the conversations as I was able to verify the changes myself. This PR is ready to be merged, thanks! |
This was referenced Feb 7, 2024
BenHenning
added a commit
that referenced
this pull request
Feb 8, 2024
## Explanation Fixes #5334 Fixes #3618 The root issue is that ``//instrumentation`` was updated in #5098 to depend on ``//testing``. However, the current dexer that we use in Bazel has troubles dexing certain dependencies (including Robolectric & Mockito), and since ``//testing`` transitively pulls in these test dependencies, the instrumentation test binaries fail to build. The main fix is isolating the new Firebase testing dependencies to their own package instead of needing to pull in ``//testing``. This resulted in some broad but trivial changes in other tests throughout the codebase. Note that I opted for just updating ``//testing`` rather than all affected deps lists to keep things a bit simpler (stricter deps will fix this in a later PR as part of the Bazel chain using automation to make things a lot simpler). This addresses issue #5334. To keep this from happening again in the future, new smoke build tests were added for instrumentation tests and the top-level ``//instrumentation:oppia_test`` to ensure that our ``ComputeAffectedTests`` utility correctly identifies and picks up these tests as part of the CI run when relevant (historically, instrumentation tests have been completely ignored in CI since they can't be run yet; note this doesn't include the instrumentation package's unit tests since these use Robolectric and _are_ included in CI runs). This addresses issue #3618. Note that this was verified as working using an initial commit to this PR that only added the new smoke tests and verifying that ``//instrumentation:oppia_test_binary_smoke_test`` is now picked up and fails, per the log output (& CI results): ``` BAZEL_TEST_TARGETS: //instrumentation:oppia_test_binary_smoke_test ``` Finally, note that there are a couple of new test targets added under ``//domain/src/test/java/org/oppia/android/domain/auth`` since it was noticed during the development of #5138 that these were missed in an earlier commit to develop (and should be run in both Bazel & Gradle test passes). ## 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 change only targets & fixes test-only infrastructure.
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
Fixes #5001.
This is PR 5 of 6 Planned PRs.
We are adding a whole new infrastructure for uploading data to Firebase Firestore.
Key Changes
FirestoreDataControllerthat handles Anonymous Authentication and logging to Firestore, based on connectivity status.This new controller provides functionality to retrieve the current signed in user and also sign in a user anonymously, and can be called from anywhere. It has a fake that is to be used in tests.
utilitymodule files toCODEOWNERSThe implementation relies on an AuthenticationWrapper to switch out the real and fake authentication controllers for testing purposes. To do this, I created a dagger module to provide the correct listener where needed, hence the test file changes in the app module tests.
Technical Decisions
FirestoreLogStorageCacheSizebased on the average length of a Google Review and the storage size computation specs defined by Firebase. I computed it based on 10 profiles on a device over a 3 month period.Dependency Updates
Proguard
Added new -dontwarns for the following:
-dontwarn javax.naming.**ReferenceEssential Checklist
Upload Screenshots
Firestore Upload
Querying Synced Firestore Data on BigQuery