Skip to content

Conversation

@adalpari
Copy link
Contributor

@adalpari adalpari commented Dec 22, 2025

Description

Add a visual progress bar to the Media Quota setting in Site Settings to follow the iOS UI. The quota preference now displays both the text summary (e.g., "50% of 3 GB") and a LinearProgressIndicator showing the storage usage percentage visually.

This change introduces a new QuotaPreference component that extends the standard Preference with a custom layout containing title, summary text, and a progress bar.

Screenshot 2025-12-22 at 11 35 51 Screenshot 2025-12-22 at 13 32 19

Testing instructions

Media quota progress bar:

  1. Open the app and navigate to a WordPress.com site
  2. Go to Site Settings
  3. Scroll down to the "Media" section
  • Verify the "Space available" preference shows the quota text (e.g., "50% of 3 GB")
  • Verify a progress bar is displayed below the text showing the usage percentage
  • Verify the progress bar fills according to the percentage used
  1. Long press on the preference
  • Verify the long press hint toast appears
  1. Test with a self-hosted site
  • Verify the Media quota section is not displayed (as expected)

@dangermattic
Copy link
Collaborator

dangermattic commented Dec 22, 2025

2 Warnings
⚠️ Class QuotaPreference is missing tests, but unit-tests-exemption label was set to ignore this.
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 22, 2025

App Icon📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack
FlavorJalapeno
Build TypeDebug
Versionpr22449-7b3f57c
Commit7b3f57c
Direct Downloadjetpack-prototype-build-pr22449-7b3f57c.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 22, 2025

App Icon📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress
FlavorJalapeno
Build TypeDebug
Versionpr22449-7b3f57c
Commit7b3f57c
Direct Downloadwordpress-prototype-build-pr22449-7b3f57c.apk
Note: Google Login is not supported on these builds.

@adalpari adalpari marked this pull request as ready for review December 22, 2025 12:46
@codecov
Copy link

codecov bot commented Dec 22, 2025

Codecov Report

❌ Patch coverage is 0% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.14%. Comparing base (7b29d97) to head (7b3f57c).
⚠️ Report is 7 commits behind head on trunk.

Files with missing lines Patch % Lines
.../org/wordpress/android/ui/prefs/QuotaPreference.kt 0.00% 44 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #22449      +/-   ##
==========================================
- Coverage   39.16%   39.14%   -0.02%     
==========================================
  Files        2202     2203       +1     
  Lines      106560   106604      +44     
  Branches    15119    15129      +10     
==========================================
  Hits        41734    41734              
- Misses      61328    61372      +44     
  Partials     3498     3498              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nbradbury
Copy link
Contributor

@adalpari Unrelated to this PR, I have a wp.com site with a BUSINESS_PLAN_ID (not a JETPACK_BUSINESS_PLAN_ID), and the media quota is hidden here. If I change the code not to hide the quota, I see this:

unlimited

WDYT about showing the quota for sites with unlimited media storage, perhaps in a separate PR? The progress bar isn't helpful, but just knowing how much storage you're using might be useful.

@nbradbury nbradbury self-assigned this Dec 22, 2025
@adalpari
Copy link
Contributor Author

@adalpari Unrelated to this PR, I have a wp.com site with a BUSINESS_PLAN_ID (not a JETPACK_BUSINESS_PLAN_ID), and the media quota is hidden here. If I change the code not to hide the quota, I see this:

Not sure to follow BUSINESS_PLAN_ID (not a JETPACK_BUSINESS_PLAN_ID). What condition should we change here?

if (!mSite.isJetpackConnected() || (mSite.getPlanId() != PlansConstants.JETPACK_BUSINESS_PLAN_ID
                                            && mSite.getPlanId() != PlansConstants.JETPACK_PREMIUM_PLAN_ID)) {
            removeJetpackMediaSettings();
        }

It would be a small change, so I can do it in this PR

@adalpari
Copy link
Contributor Author

@claude

@claude
Copy link
Contributor

claude bot commented Dec 22, 2025

Claude finished @adalpari's task in 2m 31s —— View job


Code Review Complete ✅

I've conducted a comprehensive review of this PR that adds a visual progress bar to the Media Quota setting. Overall, the implementation is solid and follows established patterns, but I've identified several areas for improvement.


✅ Strengths

  1. Good architectural choice: Creating a custom QuotaPreference that extends Preference is the right approach
  2. Proper resource management: Using obtainStyledAttributes() with recycle() correctly
  3. Clean separation: Layout, attributes, and logic are properly separated
  4. Bounds checking: The setProgress() method correctly uses coerceIn() to enforce 0-100 range
  5. Accessibility consideration: Alpha adjustment for disabled state is handled properly

🔴 Issues Found

1. Line Length Violations (Code Style - High Priority)

Location: QuotaPreference.kt:47-49, QuotaPreference.kt:55-57, QuotaPreference.kt:70-73

Multiple lines exceed the 120-character limit specified in CLAUDE.md. This will fail the Checkstyle/Detekt checks.

Recommendation: Break these lines more aggressively or use local variables to shorten them. See inline comment for example.

2. Deprecated API Usage

Location: QuotaPreference.kt:1, QuotaPreference.kt:7, QuotaPreference.kt:20-23

The entire class uses the deprecated android.preference.Preference API with a file-level suppression. While this matches existing code patterns in the project, it's worth noting this is technical debt.

Recommendation: This is acceptable for now since the entire settings system uses this API, but this should be tracked for future migration to AndroidX Preferences or Jetpack Compose.

3. Missing Null Safety in Layout Binding

Location: QuotaPreference.kt:46-65

The findViewById calls use safe-call operators (?.) but don't handle the case where views are missing. If the layout is misconfigured, the preference will silently fail to display correctly.

Recommendation: Consider logging a warning or error if required views are null, especially during development.

4. Progress Bar Color Not Explicitly Set

Location: quota_preference.xml:30-36

The progress bar relies on default theme colors. For consistency with iOS (as mentioned in the PR title), you should verify the progress indicator color matches the design.

Recommendation: Review with design team or explicitly set app:indicatorColor if a specific color is required.

5. No Maximum Progress Value Set

Location: quota_preference.xml:30-36

The LinearProgressIndicator doesn't explicitly set android:max="100", relying on the default. While the default is 100, it's better to be explicit. See inline comment.

6. Missing Edge Case Handling (Important)

Location: QuotaPreference.kt:83-86, SiteSettingsFragment.java:1586-1587

The code doesn't handle the edge case shown in the PR comments where a site has unlimited storage. The progress bar will show 100% filled, which is misleading. See inline comment for suggested approach.


⚠️ Potential Issues

7. Performance: Unnecessary Layout Invalidation

Location: QuotaPreference.kt:85

Calling notifyChanged() triggers a full rebind of the view. If setProgress() is called multiple times rapidly, this could cause performance issues. See inline comment for optimization.

8. Inconsistent Naming Convention

Location: quota_preference.xml vs other preference layouts

The layout file uses snake_case (quota_preference.xml) which is correct for Android resources, but verify this matches other preference layout naming patterns in the project.


📝 Testing Concerns

9. No Unit Tests

While the unit-tests-exemption label is set, this preference has enough logic (progress coercion, alpha calculation, attribute parsing) to warrant testing.

Recommendation: Consider adding tests for:

  • Progress value clamping (negative, 0, 50, 100, >100)
  • Attribute parsing from XML
  • Alpha values for enabled/disabled states

10. Missing Accessibility Testing Instructions

The testing instructions don't mention TalkBack or accessibility verification.

Recommendation: Add accessibility testing steps:

  • Enable TalkBack and verify the quota information is announced correctly
  • Verify the progress bar has appropriate content description

🔍 Security & Privacy

✅ No security concerns identified. The preference displays read-only quota information with no user input or data persistence.


📊 Documentation

11. Missing KDoc for Public API

Location: QuotaPreference.kt:83-92

While the code has some documentation, the class and public methods could benefit from more comprehensive KDoc.


🎯 Summary

Overall Assessment: This is a well-implemented feature that follows Android best practices. The main concerns are:

  1. Line length violations that will fail linting → Must fix before merging
  2. Edge case handling for unlimited storage plans → Should be addressed
  3. Missing tests → Acceptable with exemption, but not ideal

Recommendation: Address the line length issues before merging. Consider handling the unlimited storage case mentioned in the PR comments, either in this PR or as a follow-up.


💬 Response to PR Discussion

Regarding @nbradbury's comment about showing quota for unlimited storage plans:

  • The third screenshot shows this creates a misleading UI (full progress bar for "unlimited")
  • If you want to show this information, I recommend:
    • Hide the progress bar entirely for unlimited plans
    • Update the summary text to clearly indicate "unlimited"
    • Consider a different visual treatment (icon instead of progress bar?)

This change could be made in this PR if the logic check at SiteSettingsFragment.java:1099-1101 is updated, but the QuotaPreference class would need to support hiding the progress bar (see my inline comment for suggested implementation).


I've also added inline comments on specific lines for the most critical issues. Please review them for detailed suggestions and code examples.


@nbradbury
Copy link
Contributor

nbradbury commented Dec 22, 2025

What condition should we change here?

I think just add a check for && mSite.getPlanId() != PlansConstants.BUSINESS_PLAN_ID.

Update: It just occurred to me that if we make this change, we should avoid showing the long-press toast for unlimited plans.

@adalpari
Copy link
Contributor Author

1. Line Length Violations (Code Style - High Priority)

False positive

2. Deprecated API Usage

Location: QuotaPreference.kt:1, QuotaPreference.kt:7, QuotaPreference.kt:20-23

This is a knos fact, but since we are working with old preferences, it0s out of scope to refactor them

3. Missing Null Safety in Layout Binding

Location: QuotaPreference.kt:46-65

The findViewById calls use safe-call operators (?.) but don't handle the case where views are missing. If the layout is misconfigured, the preference will silently fail to display correctly.

Recommendation: Consider logging a warning or error if required views are null, especially during development.

4. Progress Bar Color Not Explicitly Set

Location: quota_preference.xml:30-36

The progress bar relies on default theme colors. For consistency with iOS (as mentioned in the PR title), you should verify the progress indicator color matches the design.

We want it to follow the theme colours. So, no color change is needed.

5. No Maximum Progress Value Set

I don't think this isnecessary.

6. Missing Edge Case Handling (Important)

Location: QuotaPreference.kt:83-86, SiteSettingsFragment.java:1586-1587

The code doesn't handle the edge case shown in the PR comments where a site has unlimited storage. The progress bar will show 100% filled, which is misleading. See inline comment for suggested approach.

Fixed!

@adalpari
Copy link
Contributor Author

What condition should we change here?

I think just add a check for && mSite.getPlanId() != PlansConstants.BUSINESS_PLAN_ID.

Update: It just occurred to me that if we make this change, we should avoid showing the long-press toast for unlimited plans.

Done! Can you check it?

@adalpari adalpari requested a review from nbradbury December 22, 2025 14:02
@nbradbury
Copy link
Contributor

Done! Can you check it?

The quota is now being shown for unlimited plans, but the long-press hint still appears. It looks like that's being set here.

@adalpari
Copy link
Contributor Author

Done! Can you check it?

The quota is now being shown for unlimited plans, but the long-press hint still appears. It looks like that's being set here.

Ahh, good catch! Let me see how I can solve it

@adalpari
Copy link
Contributor Author

Done! Can you check it?

The quota is now being shown for unlimited plans, but the long-press hint still appears. It looks like that's being set here.

I believe it's now solved: 7b3f57c

@sonarqubecloud
Copy link

Copy link
Contributor

@nbradbury nbradbury left a comment

Choose a reason for hiding this comment

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

Changes are good! :shipit:

@adalpari adalpari merged commit 5ed9159 into trunk Dec 23, 2025
23 checks passed
@adalpari adalpari deleted the task/CMM-1081-Unify-Media-quota-bar-with-iOS branch December 23, 2025 07:56
@adalpari adalpari changed the title CMM-1081 unify media quota bar with i os CMM-1081 unify media quota bar with iOs Jan 5, 2026
@adalpari adalpari changed the title CMM-1081 unify media quota bar with iOs CMM-1081 unify media quota bar with iOS Jan 5, 2026
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.

5 participants