Add regex checks to ensure Firebase analytics + Crashlytics stay disabled in the codebase.

[BenHenning]

The Firebase activation parameters (https://github.com/oppia/oppia-android/blob/2d14066/app/src/main/AndroidManifest.xml#L17) shouldn't be enabled ever in develop. We should have a CI check to ensure these parameters are always off.

[BenHenning]

@rt4914 maybe a SLoP candidate issue?

[rt4914]

@rt4914 maybe a SLoP candidate issue?

Yeah looks like one. I have mentioned 5 points for now.

[Russ741]

Hey, may I give fixing this issue a shot?

[rt4914]

Hey, may I give fixing this issue a shot?

@Russ741 Sure. Thanks.

[Russ741]

@rt4914 I have a couple of questions, if you don't mind:

• My understanding is that we want to enforce that firebase_analytics_collection_deactivated is true and firebase_crashlytics_collection_enabled is false on the develop branch - is this correct?
• Do we still intend to have Firebase analytics enabled for releases? If so, how? (It looks like firebase_analytics_collection_deactivated can't be overridden at runtime)
• Just for my own curiosity, why do we want to have Firebase deactivated for developer builds? Is it just to keep our analytics uncontaminated with weird non-prod crashes, etc.?
• What kind of test would be appropriate? On one end, there's something crude like grep'ing AndroidManifest.xml, and on the other there's running the app and inspecting the FirebaseAnalytics object or something (it doesn't appear to have getters, though).

Also, if there's a good resource for me to read about our continuous integration setup, I'd very much appreciate a pointer to it.

[rt4914]

@rt4914 I have a couple of questions, if you don't mind:

• My understanding is that we want to enforce that firebase_analytics_collection_deactivated is true and firebase_crashlytics_collection_enabled is false on the develop branch - is this correct?
• Do we still intend to have Firebase analytics enabled for releases? If so, how? (It looks like firebase_analytics_collection_deactivated can't be overridden at runtime)
• Just for my own curiosity, why do we want to have Firebase deactivated for developer builds? Is it just to keep our analytics uncontaminated with weird non-prod crashes, etc.?
• What kind of test would be appropriate? On one end, there's something crude like grep'ing AndroidManifest.xml, and on the other there's running the app and inspecting the FirebaseAnalytics object or something (it doesn't appear to have getters, though).

Also, if there's a good resource for me to read about our continuous integration setup, I'd very much appreciate a pointer to it.

I think @vinitamurthi and @Sarthak2601 Would be able to answer this more correctly.

[Sarthak2601]

@rt4914 I have a couple of questions, if you don't mind:

• My understanding is that we want to enforce that firebase_analytics_collection_deactivated is true and firebase_crashlytics_collection_enabled is false on the develop branch - is this correct?

Yes, correct.

• Do we still intend to have Firebase analytics enabled for releases? If so, how? (It looks like firebase_analytics_collection_deactivated can't be overridden at runtime)

Yes, we would enable it for releases. We'd probably reverse these values for other build variants using productFlavors (or manually, depends on what the team decides). However, enabling it for releases is outside the scope of this issue.

• Just for my own curiosity, why do we want to have Firebase deactivated for developer builds? Is it just to keep our analytics uncontaminated with weird non-prod crashes, etc.?

Enabling data collection would mean collection of sensitive information of our contributors and the open source nature of the project makes it challenging for us to let them know as the information will be gathered even when they're just testing the app. To avoid this, we've deactivated the Firebase data collection.

• What kind of test would be appropriate? On one end, there's something crude like grep'ing AndroidManifest.xml, and on the other there's running the app and inspecting the FirebaseAnalytics object or something (it doesn't appear to have getters, though).

This issue tracks the addition of a CI based check that will ensure that the value of these flags is not changed. I think this can be done via a python script but I'll defer to @vinitamurthi here.

Also, if there's a good resource for me to read about our continuous integration setup, I'd very much appreciate a pointer to it.

I'm not sure if I know about the documentation but you can checkout this file to see the workflow.

[vinitamurthi]

I think @Sarthak2601 has answered most of the questions so I will talk about the last one:

What kind of test would be appropriate? On one end, there's something crude like grep'ing AndroidManifest.xml, and on the other there's running the app and inspecting the FirebaseAnalytics object or something (it doesn't appear to have getters, though).

Yes we want a check that can run in Circle CI. Basically we do want to just do a manifest check, but its not enough to just look up the current AndroidManifest.xml since multiple manifests can exist and the final app manifest may be different from what is specified in AndroidManifest.xml. So there are two ways I can think of :

1. Build the app and create the apk. Look at the AndroidManifest.xml file of the built APK and do a grep on that
2. You might be able to use the manifest merger tool to just figure out what the final manifest would be without actually building the APK. I haven't looked into this myself and I am not sure if we can correctly generate the same manifest as that which is created when building the apk, so I'm not sure how successful this approach will be, but its faster than building an APK so worth spending about 15-20 mins researching on this approach too.

Hope this helps!

[Russ741]

@vinitamurthi

Yes we want a check that can run in Circle CI.

I'm unfamiliar with Circle CI, so I did some reading about how it integrates with Github, and that suggests that it uses .circleci/config.yml.
I see said file in oppia/oppia, but not oppia/oppia-android (it appears to have been removed in #1449).

Do we use Circle CI for the Android client in some way that I'm missing?

[BenHenning]

This should actually be running on GitHub actions (not CircleCI). The existing actions check could be used as a baseline for this: https://github.com/oppia/oppia-android/tree/develop/.github/workflows.

That being said, it would be better to wait until #1724 is resolved since that will make this much easier. Sorry for the confusion--this issue wasn't well defined when it was filed.

[seanlip]

#1724 is resolved. Should we get this fixed? It is the last thing remaining in the Performance Metrics project.

[BenHenning]

@seanlip I was actually thinking we could repurpose this to conditionally allow Firebase to be enabled in developer builds (#4390). We would want to implement either that or this long-term, but not both. I think, given that, we could implement this more immediately (since it's a regex check), and then remove the check once #4390 is addressed.

[seanlip]

@BenHenning Either's OK with me -- could you please make the call on what to do here specifically and put it in the description at the top, so that folks know what to do? Also should we remove "[Blocked: #...]" from the title?

[seanlip]

Per @BenHenning, the aim of this issue should be to add two regex checks to ensure that Firebase analytics + Crashlytics lines in the manifest are set to "off". Once that's done, this issue can be closed.

Note for contributors: docs about the regex check system can be found in https://github.com/oppia/oppia-android/wiki/Static-Analysis-Checks#generic-regex-pattern-matching-against-file-contents

[chrislee115]

Can I be assigned this issue?

My approach:
add checks to file_content_validation_checks.textproto that looks something like (in develop branch):

file_content_checks {
filename_regex: ".+.xml"
prohibited_content_regex: "^\s*<meta-data\s+android:name\s*=\s*"firebase_analytics_collection_deactivated"\s*android:value\s*=\s*"false".+$"
failure_message: "Firebase analytics collection should always be deactivated in develop"
}
file_content_checks {
filename_regex: ".+.xml"
prohibited_content_regex: "^\s*<meta-data\s+android:name\s*=\s*"firebase_crashlytics_collection_enabled"\s*android:value\s*=\s*"true".+$"
failure_message: "Firebase crashlytics collection should always be deactivated in develop"
}

tested in regex101 -
https://regex101.com/r/l14O5J/1

aims to prohibit
firebase_analytics_collection_deactived=false
firebase_crashlytics_collection_enabled=true

[BenHenning]

That seems like a really solid approach @chrislee115. Assigning over to you.