Fix #1903 added regex check to ensure analytics/crashlytics is explicitly disab…

[chrislee115]

Explanation

Fix #1903

Adds a regex check in file content validation to ensure Firebase analytics are disabled in the codebase. Both regex are tested in https://regex101.com/r/l14O5J/1, https://regex101.com/r/QVHGxi/1 for Firebase analytics, Crashlytics, respectively.

• "Fixes #Add regex checks to ensure Firebase analytics + Crashlytics stay disabled in the codebase. #1903:" Adds a regex check that ensures that firebase analytics and crashlytics are explicitly disabled in AndroidManifest.xml, new pull requests based on Fix #1903 - added regex check to ensure analytics/crashlytics is disabled in develop #4944

Manual testing:
Case 1: firebase_analytis_collection_deactivated = false in AndroidManifest.xml; want = true (line 19)

Case 2: firebase_analytis_collection_deactivated = true not found in AndroidManifest.xml; want = explicit line (line 19)

Case 3: firebase_crashlytics_collection_enabled = true in AndroidManifest.xml; want = false (line 20)

Case 4: firebase_crashlytics_collection_enabled = false not found in AndroidManifest.xml; want = explicit line (line 20)

Case 5: happy case

Essential Checklist

• 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: ...".)
• Any changes to scripts/assets files have their rationale included in the PR explanation.
• The PR follows the style guide.
• The PR does not contain any unnecessary code changes from Android Studio (reference).
• The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
• [] The PR is assigned to the appropriate reviewers (reference).

[adhiamboperes]

Hi @chrislee115, thanks for your PR!

Your changes look good, just a couple of things:

• You have a failing lint check on CI that you should fix. Ktlint usually runs pre-push, and it is likely that you missed this step while setting up your project. Please refer to Gradle Build Failure in Mac M1 #4842 (comment) on how to set this up, so that this will always fail locally before pushing.
• Please add a demonstration (e.g. screenshots) in the PR description showing these checks correctly failing when the values are reverted in the main AndroidManifest.xml file.

[oppiabot]

Unassigning @adhiamboperes since the review is done.

[oppiabot]

Hi @chrislee115, it looks like some changes were requested on this pull request by @adhiamboperes. PTAL. Thanks!

[chrislee115]

• You have a failing lint check on CI that you should fix. Ktlint usually runs pre-push, and it is likely that you missed this step while setting up your project. Please refer to Gradle Build Failure in Mac M1 #4842 (comment) on how to set this up, so that this will always fail locally before pushing.

Is there an equivalent link for Windows?

[adhiamboperes]

• You have a failing lint check on CI that you should fix. Ktlint usually runs pre-push, and it is likely that you missed this step while setting up your project. Please refer to Gradle Build Failure in Mac M1 #4842 (comment) on how to set this up, so that this will always fail locally before pushing.

Is there an equivalent link for Windows?

Please refer to wiki page, for windows instructions to run bash scripts/setup.sh.

[adhiamboperes]

• You have a failing lint check on CI that you should fix. Ktlint usually runs pre-push, and it is likely that you missed this step while setting up your project. Please refer to Gradle Build Failure in Mac M1 #4842 (comment) on how to set this up, so that this will always fail locally before pushing.

Is there an equivalent link for Windows?

Please refer to wiki page, for windows instructions to run bash scripts/setup.sh.

Alternatively, in android studio, go to the .github folder and find the static_checks.yml file. Search for the line that corresponds to the name of the job that failed, and you can copy and run the same script on your terminal if you have bazel set up.

[chrislee115]

Got it thanks! Sorry for the basic questions - As for testing the changes, should the scripts be run if I run

bazel build //:oppia

If so, it seems like my checks aren't properly working as the build is successfully completing.

[adhiamboperes]

Got it thanks! Sorry for the basic questions - As for testing the changes, should the scripts be run if I run

bazel build //:oppia

If so, it seems like my checks aren't properly working as the build is successfully completing.

Similar to the ktlint check(and any other static check), you can get the command for name: Regex Patterns Validation Check from the static_checks.yml and run it on your terminal. Reference

[chrislee115]

Updated the description! I was wondering if we should also prohibit the commented version of the correct lines. I.e.

  <!--  <meta-data android:name="firebase_analytics_collection_deactivated" android:value="true" /> -->

I'm not sure how feasible this is since matching beginning/end of strings appear to not work as intended and matching anything less granular might be too fragile - #4949 (comment)

[adhiamboperes]

Hi @chrislee115 , if you have finished making changes, please re-assign the PR over to the reviewers.

[chrislee115]

Added Screenshots in description for manual testing results + fixed kt lint changes in the kt file by shortening test case names.

[chrislee115]

I requested another review from you - is that the same as re-assigning? Sorry still new to this

[adhiamboperes]

I requested another review from you - is that the same as re-assigning? Sorry still new to this

No, you can assign by writing: "@adhiamboperes, PTAL" and oppiabot should assign the person mentioned.

[adhiamboperes]

Updated the description! I was wondering if we should also prohibit the commented version of the correct lines. I.e.

  <!--  <meta-data android:name="firebase_analytics_collection_deactivated" android:value="true" /> -->

I'm not sure how feasible this is since matching beginning/end of strings appear to not work as intended and matching anything less granular might be too fragile - #4949 (comment)

I will defer to @BenHenning on this.

[adhiamboperes]

Thanks @chrislee115, these changes look good to me.

[BenHenning]

Thanks @chrislee115! This looks great! LGTM.

[BenHenning]

Updated the description! I was wondering if we should also prohibit the commented version of the correct lines. I.e.

  <!--  <meta-data android:name="firebase_analytics_collection_deactivated" android:value="true" /> -->

I'm not sure how feasible this is since matching beginning/end of strings appear to not work as intended and matching anything less granular might be too fragile - #4949 (comment)

I will defer to @BenHenning on this.

I think it's fine to ignore this @adhiamboperes @chrislee115 since it's a bit of an edge case, and we still always have codeowners to check for such things happening. This seems like a completely reasonable stop-gap for #1903.

[BenHenning]

Updating to latest develop & enabling auto-merge.

[oppiabot]

Unassigning @BenHenning since they have already approved the PR.