Fix #1903 - added regex check to ensure analytics/crashlytics is disabled in develop #4944

chrislee115 · 2023-04-12T01:33:07Z

Explanation

Fix #1903

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).

seanlip · 2023-04-12T02:33:55Z

scripts/assets/file_content_validation_checks.textproto

@@ -465,3 +465,13 @@ file_content_checks {
  exempted_file_name: "scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt"
  exempted_file_name: "utility/src/main/java/org/oppia/android/util/parser/image/UrlImageParser.kt"
 }
+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".+$"


@chrislee115 Thanks for your PR! PTAL at the "Files Changed" tab on GitHub (https://github.com/oppia/oppia-android/pull/4944/files). Two questions:

Do you need a double backslash here?

Please add a newline at EOF.

Additionally, in your PR description, please describe how you validated the change and show screenshots as proof that it works correctly. Thanks!

Adding a newline EOF.
What is the double backslash are you referring to?
Also is there a preferred way to test scrips/assets? I'm not sure if I particularly saw anything relevant in the testing guide, but I may have missed something?

If you look at the other regexes in the file, you'll see that they use a double backslash \\ instead of a single one \, and that the single ones are highlighted by github in red. This might indicate that the backslash here is escaping the s rather than being treated as a literal backslash. You'll probably want to double-check what works through manual testing.

For (manual) testing, I'd suggest that you create a file with prohibited content, run the script, and check that it does the right thing. Show this proof in your PR description, so that it is possible to validate that your changes actually fix the issue.

Also, I'm not sure what's needed for this PR in terms of automated tests -- perhaps @BenHenning could comment. (But the above notes still apply!)

seanlip · 2023-04-12T04:26:13Z

@BenHenning Adding you for this comment: #4944 (comment) Thanks!

chrislee115 · 2023-04-12T22:17:59Z

I'm now running some tests by running

bazel test //scripts/src/javatests/org/oppia/android/scripts/regex:RegexPatternValidationCheckTest --cache_test_results=no

I'm getting a bit confused on how regex works here as some things that I thought would match are not matching properly.
I.e. this check passes:

file_content_checks {
  file_path_regex: "app/src/main/AndroidManifest.xml"
  prohibited_content_regex: "value="
  failure_message: "found value="
}

while this check fails:

file_content_checks {
  file_path_regex: "app/src/main/AndroidManifest.xml"
  prohibited_content_regex: "name="
  failure_message: "found name="
}

is there some reformatting that's happening to AndroidManifest.xml that I'm not aware of?

BenHenning · 2023-04-15T00:41:53Z

I'm now running some tests by running
bazel test //scripts/src/javatests/org/oppia/android/scripts/regex:RegexPatternValidationCheckTest --cache_test_results=no
I'm getting a bit confused on how regex works here as some things that I thought would match are not matching properly. I.e. this check passes:
file_content_checks {
  file_path_regex: "app/src/main/AndroidManifest.xml"
  prohibited_content_regex: "value="
  failure_message: "found value="
}
while this check fails:
file_content_checks {
  file_path_regex: "app/src/main/AndroidManifest.xml"
  prohibited_content_regex: "name="
  failure_message: "found name="
}
is there some reformatting that's happening to AndroidManifest.xml that I'm not aware of?

@chrislee115 when running your examples locally, it seems the second pattern causes testFileContent_activityDeclarationInManifest_withConfigChanges_fileContentIsNotCorrect to fail. Looking at the test file shows:

oppia-android/scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt

Lines 1476 to 1486 in 1591524

    
               val prohibitedContent = 
        
                 """ 
        
                   <?xml version="1.0" encoding="utf-8"?> 
        
                   <manifest package="org.oppia.android"> 
        
                     <application android:name=".app.application.OppiaApplication"> 
        
                       <activity 
        
                         android:name=".app.ExampleActivity" 
        
                         android:configChanges="orientation" /> 
        
                     </application> 
        
                   </manifest> 
        
                 """.trimIndent()

It seems that the problem you're running into is that an existing test has an unexpected failure for that check. This is correct since every test is setting up a test-only environment that runs against all regex checks. The second test check is failing if "name=" is present, which it is for that test's AndroidManifest.xml that it's creating.

BenHenning

Thanks @chrislee115! The checks look good. As @seanlip mentioned it would good to have tests added for these (as you're working on), and 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.

BenHenning · 2023-04-15T00:43:21Z

scripts/assets/file_content_validation_checks.textproto

+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"


Nit: please end with punctuation (ditto for the error below) for consistency with other errors.

chrislee115 · 2023-04-17T22:03:47Z

Thanks @chrislee115! The checks look good. As @seanlip mentioned it would good to have tests added for these (as you're working on), and 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.

#4949
I wrote another test in the RegexPatternValidationCheckTest.kt file to try to help debug, but I can't seem to match across two lines.
For example this doesn't match anything, is there something I'm missing here/

file_content_checks {
  file_path_regex: "app/src/main/AndroidManifest.xml"
  prohibited_content_regex: "meta-data\\s*android"
  failure_message: "meta-data and android newline"
}

chrislee115 · 2023-04-17T22:06:55Z

scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt

+  }
+
+  @Test
+  fun testFileContent_activityDeclarationInManifest_withFirebaseCrashlyticsEnabled_fileContentIsNotCorrect() {


New tests here and below
testFileContent_activityDeclarationInManifest_withFirebaseCrashlyticsEnabled_fileContentIsNotCorrect()
testFileContent_activityDeclarationInManifest_withFirebaseAnalyticsEnabled_fileContentIsNotCorrect()

oppiabot · 2023-04-26T17:03:31Z

Hi @chrislee115, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

chrislee115 · 2023-04-27T21:17:48Z

I'm still not sure how to fix this issue - are there other resources I can look at?

MohitGupta121 · 2023-04-28T03:27:35Z

@chrislee115 apologize that your query still not resolved. I think @BenHenning for sure help you with your query.

@BenHenning Can you please answer this discussion #4949 (comment) of @chrislee115 which blocking him to fix this regex issue. I answered his query as per my knowledge of regex but not solve his problem.

Thanks.

BenHenning · 2023-05-04T01:46:06Z

@chrislee115 apologize that your query still not resolved. I think @BenHenning for sure help you with your query.

@BenHenning Can you please answer this discussion #4949 (comment) of @chrislee115 which blocking him to fix this regex issue. I answered his query as per my knowledge of regex but not solve his problem.

Thanks.

Done! Thanks for the highlight.

oppiabot · 2023-05-11T02:26:54Z

Hi @chrislee115, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

chrislee115 · 2023-05-23T00:50:21Z

My apologies for the delay, was out of town - made another pull request based off this one #4995

…itly disab… (#4995)  ## 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 ##1903:" Adds a regex check that ensures that firebase analytics and crashlytics are explicitly disabled in AndroidManifest.xml, new pull requests based on /pull/4944 Manual testing: Case 1: firebase_analytis_collection_deactivated = false in AndroidManifest.xml; want = true (line 19) ![image](https://github.com/oppia/oppia-android/assets/44930615/0da2895b-55ea-4d17-acb9-00cf0e3dbfbe) Case 2: firebase_analytis_collection_deactivated = true not found in AndroidManifest.xml; want = explicit line (line 19) ![image](https://github.com/oppia/oppia-android/assets/44930615/99ebcd4e-bb10-4e49-a1a1-141d8e2e7ae9) Case 3: firebase_crashlytics_collection_enabled = true in AndroidManifest.xml; want = false (line 20) ![image](https://github.com/oppia/oppia-android/assets/44930615/0f459eb0-4c34-46b9-baff-3a5efd99e320) Case 4: firebase_crashlytics_collection_enabled = false not found in AndroidManifest.xml; want = explicit line (line 20) ![image](https://github.com/oppia/oppia-android/assets/44930615/92f5d167-dc12-4cb1-a940-9d836cf7e7db) Case 5: happy case ![image](https://github.com/oppia/oppia-android/assets/44930615/f38d4460-9d0a-4b5f-af7e-b2b8df31c3a8) ## 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". - [] 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)). --------- Co-authored-by: Ben Henning <[email protected]>

added regex check to ensure analytics/crashlytics is enabled in develop

9cd3140

chrislee115 requested a review from BenHenning as a code owner April 12, 2023 01:33

seanlip assigned BenHenning Apr 12, 2023

seanlip requested changes Apr 12, 2023

View reviewed changes

seanlip assigned chrislee115 and unassigned BenHenning Apr 12, 2023

added newline to EOF

b55db03

seanlip assigned BenHenning Apr 12, 2023

added double backslash to regex

d521845

BenHenning requested changes Apr 15, 2023

View reviewed changes

BenHenning removed their assignment Apr 15, 2023

adding automated tests, regex still broken

a9c7917

chrislee115 commented Apr 17, 2023

View reviewed changes

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Apr 26, 2023

oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Apr 27, 2023

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label May 11, 2023

oppiabot bot closed this May 18, 2023

chrislee115 mentioned this pull request May 23, 2023

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

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1903 - added regex check to ensure analytics/crashlytics is disabled in develop #4944

Fix #1903 - added regex check to ensure analytics/crashlytics is disabled in develop #4944

chrislee115 commented Apr 12, 2023 •

edited by adhiamboperes

Loading

seanlip Apr 12, 2023 •

edited

Loading

chrislee115 Apr 12, 2023

seanlip Apr 12, 2023

seanlip Apr 12, 2023 •

edited

Loading

seanlip commented Apr 12, 2023

chrislee115 commented Apr 12, 2023 •

edited

Loading

BenHenning commented Apr 15, 2023

BenHenning left a comment

BenHenning Apr 15, 2023

chrislee115 Apr 17, 2023

chrislee115 commented Apr 17, 2023

chrislee115 Apr 17, 2023

oppiabot bot commented Apr 26, 2023

chrislee115 commented Apr 27, 2023

MohitGupta121 commented Apr 28, 2023 •

edited

Loading

BenHenning commented May 4, 2023

oppiabot bot commented May 11, 2023

chrislee115 commented May 23, 2023

Fix #1903 - added regex check to ensure analytics/crashlytics is disabled in develop #4944

Fix #1903 - added regex check to ensure analytics/crashlytics is disabled in develop #4944

Conversation

chrislee115 commented Apr 12, 2023 • edited by adhiamboperes Loading

Explanation

Essential Checklist

seanlip Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

chrislee115 Apr 12, 2023

Choose a reason for hiding this comment

seanlip Apr 12, 2023

Choose a reason for hiding this comment

seanlip Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

seanlip commented Apr 12, 2023

chrislee115 commented Apr 12, 2023 • edited Loading

BenHenning commented Apr 15, 2023

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning Apr 15, 2023

Choose a reason for hiding this comment

chrislee115 Apr 17, 2023

Choose a reason for hiding this comment

chrislee115 commented Apr 17, 2023

chrislee115 Apr 17, 2023

Choose a reason for hiding this comment

oppiabot bot commented Apr 26, 2023

chrislee115 commented Apr 27, 2023

MohitGupta121 commented Apr 28, 2023 • edited Loading

BenHenning commented May 4, 2023

oppiabot bot commented May 11, 2023

chrislee115 commented May 23, 2023

chrislee115 commented Apr 12, 2023 •

edited by adhiamboperes

Loading

seanlip Apr 12, 2023 •

edited

Loading

seanlip Apr 12, 2023 •

edited

Loading

chrislee115 commented Apr 12, 2023 •

edited

Loading

MohitGupta121 commented Apr 28, 2023 •

edited

Loading