Conversation
Generated by 🚫 Danger |
7f13942 to
8ce2bf2
Compare
8ce2bf2 to
bdf752e
Compare
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
6b8ad25 to
1c52bac
Compare
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
1d719b1 to
74146c2
Compare
1c52bac to
af3ce94
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #15215 +/- ##
============================================
+ Coverage 38.68% 38.69% +0.01%
- Complexity 10546 10558 +12
============================================
Files 2193 2195 +2
Lines 124761 124767 +6
Branches 17247 17246 -1
============================================
+ Hits 48258 48277 +19
+ Misses 71624 71609 -15
- Partials 4879 4881 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
af3ce94 to
45c1791
Compare
We don't need these to be stored in a database. More internally: paaHJt-9qo-p2 `MediaIdGenerator` will be necessary for a temporary id during the media upload
…ry cache `MediaCacheOperations` is created only to delegate as much as possible to a Kotlin class instead of modifying `MediaStore.java`
It wasn't used anywhere
As it's unused
We don't use database anymore for storing `MediaModel`
The constructor was used only in tests. This commit also refactors and moves `MediaTestUtils` to testFixtures
Refactor `MediaTestUtils` to use Java-friendly builder pattern
`fetch` was for me very misleading as, at least for me, it strongly indicates a remote source (like backend/API). This method only calls `FileUploadUtils.mediaModelFromLocalUri` which is purely local operation, to prepare a file for upload.
`MediaStore` can't conflict with FluxC anymore since 41b4c5d
and API wrappers
45c1791 to
cfe92ff
Compare
| return filterByMimeType(siteId, "application") | ||
| } | ||
|
|
||
| fun searchSiteImages(siteId: Int, searchTerm: String): List<MediaModel> { |
There was a problem hiding this comment.
We can't test search* methods because the app doesn't enable searching
As it's like this for years, we should probably update contract with MediaPicker library to not being forced to provide search methods if searching is hidden.
| fun addOrUpdate(localSiteId: Int, media: MediaModel) { | ||
| val currentList = cache[localSiteId] ?: emptyList() | ||
| val mutableList = currentList.toMutableList() | ||
| val existingIndex = mutableList.indexOfFirst { it.mediaId == media.mediaId } |
There was a problem hiding this comment.
This class operates on mediaId (remote id) because we only cache MediaModel that we fetched from remote.
| public MediaModel getSiteMediaWithId(@NonNull SiteModel siteModel, long mediaId) { | ||
| List<MediaModel> media = MediaSqlUtils.getSiteMediaWithId(siteModel, mediaId); | ||
| return media.size() > 0 ? media.get(0) : null; | ||
| media.setId(mMediaIdGenerator.generate(media.getFilePath()).getValue()); |
There was a problem hiding this comment.
instantiateMediaModel is only used for upload, hence a generated, local-only id. We should probably remove instantiateMediaModel method - there's no point to inject a heavy MediaStore only to call this constructor. I've added it to the list of improvements.
After all of the clean ups it came out clearly, that `mUploadState` isn't really used in any meaningful way in the Woo app. The progress and state of uploads is tracked via different mechanisms.
288a185 to
521e2e4
Compare
There was a problem hiding this comment.
Pull request overview
This pull request migrates MediaModel from being a database-backed ORM model to an in-memory POJO, introducing a new cache-based architecture. The change removes database persistence overhead and eliminates unused properties like upload state tracking, alt text, and thumbnail URLs.
Changes:
- Introduced
MediaLibraryCachefor in-memory storage of media items per site - Added
MediaCacheOperationsto handle filtering and searching operations - Implemented
TimestampMediaIdGeneratorfor generating unique local media IDs - Removed database operations from
MediaStoreand replaced with cache operations - Simplified
MediaModelby removing unused fields and upload state tracking - Updated database schema to drop the MediaModel table (migration 239)
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
MediaLibraryCache.kt |
New singleton cache using ConcurrentHashMap to store media lists per site |
MediaCacheOperations.kt |
Operations layer for filtering and searching cached media by mime type |
TimestampMediaIdGenerator.kt |
Generates local IDs using timestamp and filepath hash |
MediaIdGenerator.kt |
Interface for ID generation strategy |
MediaModule.kt |
Dagger module providing MediaIdGenerator and Clock dependencies |
MediaStore.java |
Replaced database operations with cache operations, simplified upload handling |
MediaModel.java |
Removed ORM annotations, upload state, alt text, thumbnail URL, and made most fields final |
WellSqlConfig.kt |
Added migration 239 to drop MediaModel table |
MediaTestUtils.kt |
New test fixture utilities with builder pattern for creating test media |
MediaLibraryCacheTest.kt |
Unit tests for cache operations |
TimestampMediaIdGeneratorTest.kt |
Unit tests for ID generation |
MediaStoreTest.java |
Updated to use cache instead of database |
MediaSqlUtils.java |
Deleted - database operations no longer needed |
RestUploadRequestBody.java |
Deleted - replaced by WPRestUploadRequestBody |
MediaWPComRestResponse.java |
Deleted - no longer used |
MediaResponseUtils.kt |
Deleted - no longer used |
WPRestUploadRequestBody.kt |
Removed alt_text parameter from upload |
MediaWPRESTResponse.kt |
Removed alt_text field from response parsing |
BaseWPV2MediaRestClient.kt |
Removed upload state setting on failure |
FileUploadUtils.kt |
Removed null MediaUploadState parameter |
FluxCModule.kt |
Added MediaModule to Dagger component |
| Product/Media test files | Updated to use MediaTestUtils builders |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
libs/fluxc/src/testFixtures/kotlin/org/wordpress/android/fluxc/media/MediaTestUtils.kt
Show resolved
Hide resolved
libs/fluxc/src/main/java/org/wordpress/android/fluxc/model/MediaModel.java
Show resolved
Hide resolved
libs/fluxc/src/test/java/org/wordpress/android/fluxc/store/MediaLibraryCacheTest.kt
Outdated
Show resolved
Hide resolved
libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/TimestampMediaIdGenerator.kt
Show resolved
Hide resolved
libs/fluxc/src/main/java/org/wordpress/android/fluxc/model/MediaModel.java
Show resolved
Hide resolved
libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/MediaStore.java
Show resolved
Hide resolved
libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/MediaLibraryCache.kt
Show resolved
Hide resolved
libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/MediaStore.java
Outdated
Show resolved
Hide resolved
libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/MediaLibraryCache.kt
Outdated
Show resolved
Hide resolved
libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/MediaLibraryCache.kt
Outdated
Show resolved
Hide resolved
… now Using a shared builder is both cleaner and safer.
I simply forgot to do this in 521e2e4
To maintain the equals-hashCode contract
In this tests, we invoke `addOrUpdate` 10 times, on 10 different threads, at the same moment. Without making this method atomic, `MediaLibraryCache` won't record all 10 updates. This test should fail.
This asserts that accessing a specific `, List<MediaModel>` is locked when one of the threads operates on it.
ParaskP7
left a comment
There was a problem hiding this comment.
👋 @wzieba !
I have reviewed and tested this PR as per the instructions, everything works as expected, good job! 🌟 x 🌟 ^ 🌟
I have left few questions (❓), a couple of suggestions (💡) and some minor (🔍) comments for you to consider. I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.
libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/MediaCacheOperations.kt
Outdated
Show resolved
Hide resolved
libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/MediaCacheOperations.kt
Outdated
Show resolved
Hide resolved
libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/MediaLibraryCache.kt
Outdated
Show resolved
Hide resolved
libs/fluxc/src/main/java/org/wordpress/android/fluxc/model/MediaModel.java
Outdated
Show resolved
Hide resolved
libs/fluxc/src/test/java/org/wordpress/android/fluxc/store/MediaLibraryCacheTest.kt
Outdated
Show resolved
Hide resolved
libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/MediaLibraryCache.kt
Outdated
Show resolved
Hide resolved
libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/TimestampMediaIdGenerator.kt
Show resolved
Hide resolved
libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/MediaLibraryCache.kt
Outdated
Show resolved
Hide resolved
Also move tests from `fluxc-tests` module to the appropriate module (where SUT is declared)
We actually don't ever call this method in production code. We don't have to, as it's only in-memory cache and identified via local site id (so when user changes sites, the old cache is not reused).
|
Version |
Closes: AINFRA-542: Migrate
MediaModelDescription
This PR migrates
MediaModelfrom being an ORM model to a stored in-memory POJO. This reduces configuration overhead, while not degrading user experience. See: p91TBi-dHl-p2It also removes a decent portion of unused properties and classes, that happened to not be used anymore.
The goal of this PR was not to improve every aspect of handling
MediaModelas it'd simply require too many changes for a single PR. The list of remaining improvements, while probably not exhousted, can be found in AINFRA-1764: Address remainingMedia*improvements in Woo AndroidTest Steps
We need to test media upload for two connection types: WPCOM and Application Password.
Scenarios to test
.tiffile https://svs.gsfc.nasa.gov/vis/a030000/a030800/a030877/frames/5760x3240_16x9_01p/)Images/gif
There are no UI changes.
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.