Skip to content

Convert Media to Swift#25303

Open
kean wants to merge 2 commits intotrunkfrom
task/convert-media-to-swift
Open

Convert Media to Swift#25303
kean wants to merge 2 commits intotrunkfrom
task/convert-media-to-swift

Conversation

@kean
Copy link
Contributor

@kean kean commented Feb 25, 2026

Closes #25303

The changes are covered by the extended unit test suite introduced in #25287. I also manually tested it, including the more obscure scenarios that require persisting an error, etc.

@kean kean added this to the Someday milestone Feb 25, 2026
@kean kean added the Tech Debt label Feb 25, 2026
@kean kean force-pushed the task/convert-media-to-swift branch 2 times, most recently from 3614546 to 829e5d9 Compare February 25, 2026 17:15
@kean kean force-pushed the task/convert-media-to-swift branch from 829e5d9 to df9e95b Compare February 25, 2026 17:18
@kean kean requested review from crazytonyli and jkmassel February 25, 2026 17:19
@kean kean enabled auto-merge February 25, 2026 17:27
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Feb 25, 2026

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
ConfigurationRelease-Alpha
Build Number31150
VersionPR #25303
Bundle IDcom.jetpack.alpha
Commitba94b4d
Installation URL4b6l5sfb2ds3o
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Feb 25, 2026

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
ConfigurationRelease-Alpha
Build Number31150
VersionPR #25303
Bundle IDorg.wordpress.alpha
Commitba94b4d
Installation URL47d7a95sjl600
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Feb 25, 2026

🤖 Build Failure Analysis

This build has failures. Claude has analyzed them - check the build annotations for details.

@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@sonarqubecloud
Copy link

Copy link
Contributor

@jkmassel jkmassel left a comment

Choose a reason for hiding this comment

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

Looks ok to me, pretty mechanical changes.

Claude suggested some more test coverage:

**Suggestion: consider adding tests for the remaining untested public API surface (all pre-existing gaps, not regressions from this PR):**

- `willAttemptToUploadLater()` — returns `true` when failure count < `maxAutoUploadFailureCount`, `false` when >=
- `canRetry` — `true` when `absoluteLocalURL` is set, `false` when nil
- `link` — constructs `"{siteURL}/?p={mediaID}"`, returns `""` when blog URL or mediaID is nil
- `remoteStatus` — round-trip through `remoteStatusNumber` to `MediaRemoteStatus` enum
- `absoluteLocalURL` setter with `nil` — verifies setting to nil clears `localURL`
- `fixLocalMediaURLs` with nil content — exercises the `guard var content` early-return path
- `fixLocalMediaURLs` skips remote media — explicitly verifies that the `guard !media.hasRemote` path prevents URL rewriting for uploaded media (currently only tested indirectly via regex not matching `https://`)

I can see the benefit to these, but up to you.

import WordPressShared

extension AbstractPost {
/// When updating the app through the App Store or installing a new version
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense for absolute paths, but why would we store everything relevant to the documents directory? 🤔

@kean kean added this pull request to the merge queue Feb 25, 2026
@jkmassel jkmassel removed this pull request from the merge queue due to a manual request Feb 25, 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.

4 participants