Add Media Library upload data types and tracker events#25619
Open
crazytonyli wants to merge 2 commits into
Open
Add Media Library upload data types and tracker events#25619crazytonyli wants to merge 2 commits into
crazytonyli wants to merge 2 commits into
Conversation
Collaborator
Generated by 🚫 Danger |
Contributor
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
crazytonyli
force-pushed
the
task/media-v2-upload-format
branch
from
a34822c to
35848ab
Compare
crazytonyli
force-pushed
the
task/media-v2-upload-foundation
branch
from
1b5f4d8 to
26d3fb8
Compare
Contributor
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 32692 | |
| Version | PR #25619 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 655eccc | |
| Installation URL | 106j20csb5fk8 |
Contributor
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 32692 | |
| Version | PR #25619 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 655eccc | |
| Installation URL | 2i2d7pbu1kht0 |
crazytonyli
force-pushed
the
task/media-v2-upload-foundation
branch
from
26d3fb8 to
a4390f8
Compare
crazytonyli
force-pushed
the
task/media-v2-upload-format
branch
from
35848ab to
dad690b
Compare
crazytonyli
force-pushed
the
task/media-v2-upload-foundation
branch
2 times, most recently
from
4e19015 to
486c1bf
Compare
Module-side foundation for the V2 upload pipeline: the upload source and policy value types, the in-flight/failed/state models, the transport protocol over the wp_mobile upload call, and the new tracker event cases. Bumps wordpress-rs to 0.4.0 for the create-media API the transport calls.
crazytonyli
force-pushed
the
task/media-v2-upload-foundation
branch
from
486c1bf to
de0286a
Compare
jkmassel
requested changes
Jun 17, 2026
| // .audio / .document map to no event — V1 parity. | ||
| return | ||
| } | ||
| WPAppAnalytics.track(resolvedStat, properties: baseProperties, blog: blog) |
Contributor
There was a problem hiding this comment.
media_origin is dropped here. V1 attaches it to these same events via selectionMethod — full_screen_picker for photoLibrary/camera, document_picker for otherApps — but only the .stockPhotos branch sets it. Device/camera/otherApps adds lose the dimension vs V1.
Comment on lines
+150
to
+153
| let trackedEventNames = TestAnalyticsTracker.tracked.map(\.event) | ||
| let trackedStats = TestAnalyticsTracker.tracked.map(\.stat) | ||
| #expect(!trackedEventNames.contains("media_library_photo_added")) | ||
| #expect(!trackedStats.contains(.mediaLibraryAddedPhotoViaStockPhotos)) |
Contributor
There was a problem hiding this comment.
trackedEventNames is always "" (the adapter only does stat-based tracking), so this assertion can never fail. Assert nothing fired instead:
Suggested change
| let trackedEventNames = TestAnalyticsTracker.tracked.map(\.event) | |
| let trackedStats = TestAnalyticsTracker.tracked.map(\.stat) | |
| #expect(!trackedEventNames.contains("media_library_photo_added")) | |
| #expect(!trackedStats.contains(.mediaLibraryAddedPhotoViaStockPhotos)) | |
| #expect(TestAnalyticsTracker.tracked.isEmpty) |
|
|
||
| static let uploadBannerUploadingOnly = NSLocalizedString( | ||
| "mediaLibrary.upload.banner.uploadingOnly", | ||
| value: "Uploading %1$d items", |
Contributor
There was a problem hiding this comment.
Renders "Uploading 1 items" for a single upload — needs singular/plural handling (.stringsdict or a count == 1 branch).
| ) | ||
| static let uploadBannerFailedOnly = NSLocalizedString( | ||
| "mediaLibrary.upload.banner.failedOnly", | ||
| value: "%1$d upload failed", |
Contributor
There was a problem hiding this comment.
Inverse plural bug — "2 upload failed" for >1. Same singular/plural handling as the uploading banner.
| value: "All in-progress uploads will be cancelled. This can't be undone.", | ||
| comment: "Body of the confirmation alert shown before canceling every in-flight upload." | ||
| ) | ||
| static let cancel = NSLocalizedString( |
Contributor
There was a problem hiding this comment.
cancel maps to "Keep uploading", but the repo names alert buttons by their text (cf. notNow, cancellationAlertContinueEditing), so this reads as the destructive action — which is actually uploadBulkCancelAllConfirm ("Cancel uploads"). Suggest renaming:
Suggested change
| static let cancel = NSLocalizedString( | |
| static let keepUploading = NSLocalizedString( |
Attach media_origin to device/camera/file-picker added events for V1 parity, tighten the Image Playground analytics test, and fix the upload banner pluralization and the bulk-cancel button name.
Contributor
|
Version |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
This PR will be merged after #25618.