Details

Reviewers

Commits

Summary

This code will initialize db on Android.
Context in ENG-2071

Test Plan

Tested on emulator and physical device.

Created malformed db (e.g. simulate bug described in ENG-1910)
Open the app - should be terminated (on emulator it's not visible, app will start again immediately, but it can be proved via logs)
Open again - app should delete database and perform soft-logout and continue running with proper structure

Diff Detail

Repository

rCOMM Comm

Branch

error-handling-4

Lint

No Lint Coverage

Unit

No Test Coverage

kamil held this revision as a draft.

remove unused import

kamil published this revision for review.Jan 10 2023, 8:14 AM

Looks pretty standard

This revision is now accepted and ready to land.Jan 11 2023, 7:10 AM

This revision now requires review to proceed.Jan 12 2023, 6:46 AM

tomek requested changes to this revision.Jan 13 2023, 8:31 AM

tomek added inline comments.

native/android/app/src/cpp/DatabaseInitializerJNIHelper.cpp
9–10	It is a little confusing to see this order of functions. Can we e.g. make `SQLiteQueryExecutor::initialize(sqliteFilePath);` a part of `initializeQueryExecutor`?
native/android/app/src/main/java/app/comm/android/MainApplication.java
110–111	We have some other places where we have a similar call. Is it safe to have a couple of them? Can we create a function that contains this duplicated logic?

This revision now requires changes to proceed.Jan 13 2023, 8:31 AM

native/android/app/src/main/java/app/comm/android/MainApplication.java
110–111	It should be safe: https://linear.app/comm/issue/ENG-2071#comment-0e46f47a The `initialize()` will do nothing if it is already initialized. The supplier won't be consumed too. I don't think that separating a duplicated "nearly-oneliner" is a game changer, but up to you.

make initialize part of initializeQueryExecutor

native/android/app/src/cpp/DatabaseInitializerJNIHelper.cpp
9–10	sure, done
native/android/app/src/main/java/app/comm/android/MainApplication.java
110–111	According to @bartek comment, I would rather leave it as it is, it is one command so we do not gain much but it might be complicated (see CommCoreJSIModulePackage) where `context` is different than in other two calls