Skip to content

Conversation

@avazirna
Copy link
Contributor

@avazirna avazirna commented Feb 25, 2025

Product Description

Google issued a new policy that states that READ_MEDIA_IMAGES and READ_MEDIA_VIDEO permissions can only be used by apps whose core functionality requires broad access to images and videos stored in the device. These permissions were introduced in Android 13 and as such, this policy mostly affects that version and later. For apps that don't require this level of access, the recommended approach is to use a system picker, like the Android photo picker, which is available in Android 13 onward. Additionally, Google is committed to making the photo picker the default behaviour and is working to make it available to older versions too:

  • Android 11 and 12 - through Google System updates
  • Android 4.4 to 10 - via a backported version distributed through Google Play Services.

This PR implements the necessary changes to comply with this policy considering that CommCare doesn't need broad access to media files.
Note to reviewers: this work also involved assessing whether to adopt the ActivityResult API, since startActivityForResult is now deprecated. However, these two approaches can't be mixed, meaning that migrating would require the definition of ActivitResult contracts for every option in the FormEntryActivity.onAtivityResult method.

Safety Assurance

Safety story

This changes the current approach when selecting media files to upload in CommCare, it sets preference to system photo and video pickers that can handle the ACTION_PICK_IMAGE action, falling back to ACTION_GET_CONTENT when an appropriate component is not available, so is not supposed to break anything. Moreover, these change were tested on Android 5, 6, 8, 9, 10, 11, 12 and 13

QA Plan

A QA label has been added to ensure that this gets tested on both the minimum (Android 5) and maximum (Android 14) supported versions.

Labels and Review

  • Do we need to enhance the manual QA test coverage ? If yes, the "QA Note" label is set correctly
  • Does the PR introduce any major changes worth communicating ? If yes, the "Release Note" label is set and a "Release Note" is specified in PR description.
  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

cross-request: dimagi/commcare-core#1462

@coderabbitai
Copy link

coderabbitai bot commented Feb 25, 2025

📝 Walkthrough

Walkthrough

The pull request removes media permissions for reading images and videos from the Android manifest, permissions utility functions, and instrumentation tests, narrowing permissions to only audio and notifications. A new service declaration for com.google.android.gms.metadata.ModuleDependencies is added with its enabled and exported attributes set to false, and an intent filter defined to listen for specific dependency actions. The dependency for androidx.appcompat is upgraded from version 1.2.0 to 1.7.0 in the build configuration. Additionally, the intent creation for selecting images and videos in the corresponding widgets has been refactored. Both the ImageWidget and VideoWidget classes now use a new utility method from the WidgetUtils class, which encapsulates the media intent creation logic and checks device support for the photo picker.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Widget
    participant WidgetUtils
    participant PackageManager
    participant System
    User->>Widget: Initiate media selection (image/video)
    Widget->>WidgetUtils: createPickMediaIntent(context, mimeType)
    WidgetUtils->>PackageManager: Check photo picker support
    alt Photo Picker Supported
        WidgetUtils-->>Widget: Return Intent with MediaStore.ACTION_PICK_IMAGES
    else
        WidgetUtils-->>Widget: Return Intent with Intent.ACTION_GET_CONTENT
    end
    Widget->>System: Launch media selection
Loading

Suggested labels

Release Note, High Risk

Suggested reviewers

  • shubham1g5

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d3f29f1 and cfbf901.

📒 Files selected for processing (7)
  • app/AndroidManifest.xml (1 hunks)
  • app/build.gradle (1 hunks)
  • app/instrumentation-tests/src/org/commcare/androidTests/BaseTest.kt (0 hunks)
  • app/src/org/commcare/utils/Permissions.java (1 hunks)
  • app/src/org/commcare/views/widgets/ImageWidget.java (1 hunks)
  • app/src/org/commcare/views/widgets/VideoWidget.java (1 hunks)
  • app/src/org/commcare/views/widgets/WidgetUtils.java (2 hunks)
💤 Files with no reviewable changes (1)
  • app/instrumentation-tests/src/org/commcare/androidTests/BaseTest.kt
🔇 Additional comments (11)
app/src/org/commcare/views/widgets/VideoWidget.java (1)

68-69: Good implementation of the intent creation change.

The update to use WidgetUtils.createPickMediaIntent is a good approach for adapting to Google's new media permission policy in Android 13. This change properly delegates the media picker intent creation to a utility method which can handle differences between Android versions.

app/build.gradle (1)

76-76:

Details

❓ Verification inconclusive

Verify compatibility with this major AppCompat version update.

The update from appcompat 1.2.0 to 1.7.0 is quite significant (five minor versions). While this update is necessary to support the photo picker API for the new media permission strategy, it could introduce unexpected behavioral changes elsewhere in the application.


🏁 Script executed:

#!/bin/bash
# Find potential AppCompat-dependent implementations that might be affected by the version update

echo "Searching for AppCompatActivity usages:"
rg "extends\s+AppCompatActivity" -t java

echo -e "\nSearching for other AppCompat dependencies:"
rg "import\s+androidx\.appcompat" -t java --files-with-matches | sort | uniq | head -n 20

Length of output: 4204


Ensure Compatibility with AppCompat 1.7.0 Update

The update from AppCompat 1.2.0 to 1.7.0 introduces significant changes—especially with its support for the photo picker API and new media permission strategy. Our search shows extensive use of AppCompat (e.g., multiple activities like UnrecoverableErrorActivity, SeatAppActivity, TargetMismatchErrorActivity, and several others extend AppCompatActivity), so it's important to ensure that these changes do not inadvertently affect the app’s UI behavior or lifecycle management.

  • Action Items:
    • Review and Test: Manually verify that all activities and components dependent on AppCompat (as listed in the search output) behave correctly with the new version.
    • Check UI/Permission Flows: Pay special attention to UI rendering and permission-related behaviors in the context of the new photo picker API.
    • Regression Testing: Ensure that any behavioral changes introduced by moving to 1.7.0 do not negatively impact existing functionality.

The code snippet remains:

    implementation 'androidx.appcompat:appcompat:1.7.0'

Please double-check these areas to maintain app stability after the update.

app/src/org/commcare/views/widgets/ImageWidget.java (1)

141-142: Good implementation of the intent creation change.

The update to use WidgetUtils.createPickMediaIntent is a good approach for adapting to Google's new media permission policy in Android 13. This change properly delegates the media picker intent creation to a utility method which can handle differences between Android versions.

app/src/org/commcare/utils/Permissions.java (3)

106-109: Successfully removed unnecessary media permissions.

The permissions have been correctly reduced to align with Google's new policy for Android 13+. Only keeping the READ_MEDIA_AUDIO and POST_NOTIFICATIONS permissions is appropriate since the app is now using the system picker for images and videos.


122-123: Successfully updated required permissions.

The required permissions have been correctly reduced to only include READ_MEDIA_AUDIO for Android 13+, aligning with the overall permission reduction strategy.


97-115:

Details

❓ Verification inconclusive

Verify the app functionality without image and video permissions.

While removing these permissions aligns with Google's requirements, ensure that all app functionality that previously relied on direct media access has been updated to use the system picker.


🏁 Script executed:

#!/bin/bash
# Look for any places where direct file access for images/videos might be occurring

echo "Checking for direct file access patterns for images:"
rg "\\.(jpg|jpeg|png|gif|bmp)" --type java

echo -e "\nChecking for direct file access patterns for videos:"
rg "\\.(mp4|3gp|mkv|avi|mov)" --type java

echo -e "\nChecking for image/video content type usage:"
rg "image/|video/" --type java

Length of output: 10868


Action Required: Verify that All Media Access Uses the System Picker

I reviewed the code references for direct image/video file access. The search results show several occurrences of file extensions and media content types—mostly in test files and utility classes (e.g., in FormUploadUtil.java, MediaLayout.java, ImageWidget.java, and VideoWidget.java)—but no evidence of active, direct file access in the production code. These patterns appear to be used for constants and testing rather than bypassing the intended system picker integration.

Please ensure that every functionality previously relying on custom image or video access has been updated to use the system picker (as indicated by the use of intents with content types like "image/*" and "video/*"). A manual verification of the media flow in the app is recommended to confirm that removed permissions do not adversely affect any user-facing features.

app/src/org/commcare/views/widgets/WidgetUtils.java (3)

3-7: Appropriate imports for the new media picker functionality.

These imports support the new photo picker functionality, which is a good approach to adapt to Android 13's changes in media permissions.


93-98: Robust device capability check for photo picker support.

Good implementation of capability detection using PackageManager to check if the photo picker action can be resolved, ensuring compatibility across different Android versions.


82-91:

Details

✅ Verification successful

Good implementation of the photo picker intent creation.

The method correctly uses the modern MediaStore.ACTION_PICK_IMAGES when available, falling back to ACTION_GET_CONTENT for older devices. The @SuppressLint("InlinedApi") annotation appropriately handles the API level warning.


🏁 Script executed:

#!/bin/bash
# Check if the createPickMediaIntent method is used in the codebase

rg -A 1 "createPickMediaIntent" --type java

Length of output: 782


Implementation Confirmed and Approved

The createPickMediaIntent method in app/src/org/commcare/views/widgets/WidgetUtils.java (lines 82-91) correctly adapts its behavior based on device support by using MediaStore.ACTION_PICK_IMAGES when available and falling back to Intent.ACTION_GET_CONTENT otherwise. The use of the @SuppressLint("InlinedApi") annotation is appropriate for handling API level warnings.

Additionally, our search confirms that this method is consistently used in both app/src/org/commcare/views/widgets/VideoWidget.java and app/src/org/commcare/views/widgets/ImageWidget.java, reinforcing that the implementation is integral to the media picking functionality across the codebase.

Great work!

app/AndroidManifest.xml (2)

30-38:

Details

❓ Verification inconclusive

Verify that READ_MEDIA_AUDIO permission is sufficient for your use case.

The manifest now only includes READ_MEDIA_AUDIO but no longer has READ_MEDIA_IMAGES or READ_MEDIA_VIDEO permissions, which aligns with Google's policy. Ensure that audio functionality still works as expected and that you're using the photo picker for all image and video selection.


🏁 Script executed:

#!/bin/bash
# Check for references to media permissions in the codebase
rg "READ_MEDIA_(IMAGES|VIDEO)" --type java

Length of output: 44


Verify Media Permissions & Photo Picker Integration

The grep search for READ_MEDIA_(IMAGES|VIDEO) in Java files returned no results, suggesting that the removed permissions aren’t referenced in Java code. However, please manually verify that these permissions aren’t used elsewhere (e.g., in Kotlin or XML files) and confirm that audio functionality operates correctly with only READ_MEDIA_AUDIO while the photo picker is fully handling image and video selection.


176-184:

Details

✅ Verification successful

Good addition of the ModuleDependencies service for the photo picker.

This service declaration properly configures the Google GMS module dependency needed for the photo picker functionality. The service is correctly set as not enabled and not exported for security, with the appropriate intent filter and metadata.


🌐 Web query:

What does the ModuleDependencies service do in Android and how is it related to the photo picker?

💡 Result:

The ModuleDependencies service in Android enables backward compatibility for newer features like the Photo Picker on older Android devices by leveraging Google Play services. Here's how it works:

Core Functionality

  • Triggers dynamic module installation: Adding this service to your app's AndroidManifest.xml prompts Google Play services to install a backported version of the Photo Picker module on devices running Android 4.4 (KitKat) and newer[2][4][10].
  • No direct implementation: The service itself doesn't contain Photo Picker logic but acts as a declaration that your app requires this component.

Implementation Example

<service android:name="com.google.android.gms.metadata.ModuleDependencies"
    android:enabled="false"
    android:exported="false"
    tools:ignore="MissingClass">
    <intent-filter>
        <action android:name="com.google.android.gms.metadata.MODULE_DEPENDENCIES" />
    </intent-filter>
    <meta-data 
        android:name="photopicker_activity:0:required" 
        android:value="" />
</service>

This configuration:

  1. Declares dependency on Photo Picker (photopicker_activity)[4]
  2. Works with Android versions back to KitKat (API 19)[2]
  3. Requires no permissions for media access[9]

Benefits for Photo Picker Integration

  • Unified API surface: Developers can use modern PickVisualMedia APIs while supporting older devices[10]
  • Automatic updates: Google Play services handles module maintenance
  • Storage permission elimination: Users grant access only to selected media[9]

When combined with the Activity Results API, this allows permission-less media selection in just ~20 lines of code[2], while maintaining compatibility across 97% of active Android devices[4].

Citations:


ModuleDependencies Service Configuration Verified for the Photo Picker

The service declaration in app/AndroidManifest.xml correctly configures the dependency required by Google Play Services—enabling dynamic installation of the backported Photo Picker module on devices running Android KitKat (API 19) and newer. The service is appropriately marked as disabled and non-exported for security, includes the necessary intent filter, and defines the metadata (photopicker_activity:0:required) to signal its dependency.

No further changes are needed.

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@avazirna avazirna marked this pull request as ready for review February 25, 2025 16:13
@avazirna avazirna requested review from OrangeAndGreen, pm-dimagi and shubham1g5 and removed request for OrangeAndGreen February 25, 2025 16:13
@avazirna
Copy link
Contributor Author

@damagatchi retest this please

try {
((AppCompatActivity)getContext()).startActivityForResult(i,
((AppCompatActivity)getContext()).startActivityForResult(
WidgetUtils.createPickMediaIntent (getContext(), "image/*"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code formatting seems off

try {
((AppCompatActivity)getContext()).startActivityForResult(i,
((AppCompatActivity)getContext()).startActivityForResult(
WidgetUtils.createPickMediaIntent (getContext(), "video/*"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code formatting.

@avazirna avazirna requested a review from shubham1g5 February 26, 2025 15:42
shubham1g5
shubham1g5 previously approved these changes Feb 26, 2025
@avazirna
Copy link
Contributor Author

@damagatchi retest this please

In AppCompat 1.4+, ActivityController.visible() triggers a layout pass which is
triggering OnGlobalLayoutListener().onGlobalLayout() and causing the
DemoUserRestoreTest.demoUserRestoreAndUpdateTest() unit test to fail
@avazirna
Copy link
Contributor Author

avazirna commented Mar 3, 2025

@damagatchi retest this please

shubham1g5
shubham1g5 previously approved these changes Mar 3, 2025
@avazirna
Copy link
Contributor Author

avazirna commented Mar 3, 2025

@damagatchi retest this please

1 similar comment
@avazirna
Copy link
Contributor Author

avazirna commented Mar 3, 2025

@damagatchi retest this please

@shubham1g5 shubham1g5 added this to the 2.56 milestone Mar 3, 2025
@avazirna
Copy link
Contributor Author

avazirna commented Mar 4, 2025

@damagatchi retest this please

1 similar comment
@avazirna
Copy link
Contributor Author

avazirna commented Mar 4, 2025

@damagatchi retest this please

@avazirna
Copy link
Contributor Author

avazirna commented Mar 4, 2025

@damagatchi retest this please

@avazirna avazirna requested a review from shubham1g5 March 4, 2025 11:13
@avazirna avazirna merged commit df0c184 into master Mar 4, 2025
6 of 10 checks passed
@avazirna avazirna deleted the remove-media-permissions branch March 4, 2025 11:31
@coderabbitai coderabbitai bot mentioned this pull request Mar 12, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants