Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Pause media upload when internet connection unavailable #19878

Merged
merged 19 commits into from
Jan 10, 2024
Merged

feat: Pause media upload when internet connection unavailable #19878

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
4107505
feat: Failed uploads while offline communicate paused state
dcalhoun Jan 3, 2024
a929e1f
build: Update Gutenberg ref
dcalhoun Jan 3, 2024
04af69f
build: Fix incorrect Gutenberg PR reference
dcalhoun Jan 3, 2024
578ee6a
build: Fix incorrect Gutenberg PR reference
dcalhoun Jan 3, 2024
b22021a
Add discrete onMediaUploadPaused handler to media uploads when offline
derekblank Jan 4, 2024
d8a7bff
refactor: Fix parameter name typo
dcalhoun Jan 4, 2024
8d46802
refactor: Annotate override of `onMediaUploadPaused` method
dcalhoun Jan 4, 2024
8ed6850
refactor: Remove unused `onMediaUploadFailed` error type parameter
dcalhoun Jan 4, 2024
ad1ca04
feat: Aztec `onMediaUploadPaused` passes onto `onMediaUploadFailed`
dcalhoun Jan 4, 2024
594f989
feat: Only consider failed media uploads to be paused
dcalhoun Jan 4, 2024
00df84c
feat: Track failed media upload event when pausing media uploads
dcalhoun Jan 4, 2024
b1f7e3b
feat: When lacking connectivity, pause completed upload w/o remote URLs
dcalhoun Jan 4, 2024
2a5075d
Merge pull request #19884 from wordpress-mobile/feat/update-image-blo…
dcalhoun Jan 4, 2024
51d141b
docs: Add release note
dcalhoun Jan 4, 2024
4c92efe
Merge branch 'trunk' of github.com:wordpress-mobile/WordPress-Android…
dcalhoun Jan 4, 2024
b97dbec
build: Update Gutenberg ref
dcalhoun Jan 4, 2024
92c2b99
Merge branch 'trunk' into feat/update-image-block-upload-visuals
derekblank Jan 10, 2024
eb6ccca
Merge branch 'trunk' of github.com:wordpress-mobile/WordPress-Android…
dcalhoun Jan 10, 2024
e3ad26b
build: Update Gutenberg ref
dcalhoun Jan 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions RELEASE-NOTES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
*** PLEASE FOLLOW THIS FORMAT: [<priority indicator, more stars = higher priority>] <description> [<PR URL>]

24.1
-----
* [**] Image block media uploads display a custom error message when there is no internet connection [https://github.com/wordpress-mobile/WordPress-Android/pull/19878]

24.0
-----
* [*] Filter media types when sharing files to the editor [https://github.com/wordpress-mobile/WordPress-Android/pull/19754]
Expand Down
10 changes: 9 additions & 1 deletion WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3682,6 +3682,11 @@ public void onMediaUploaded(OnMediaUploaded event) {
return;
}

if (event.isError() && !NetworkUtils.isNetworkAvailable(this)) {
mEditorMedia.onMediaUploadPaused(mEditorMediaUploadListener, event.media, event.error);
return;
}

// event for unknown media, ignoring
if (event.media == null) {
AppLog.w(AppLog.T.MEDIA, "Media event carries null media object, not recognized");
Expand All @@ -3700,7 +3705,10 @@ public void onMediaUploaded(OnMediaUploaded event) {
mEditorMedia.onMediaUploadError(mEditorMediaUploadListener, event.media, event.error);
} else if (event.completed) {
// if the remote url on completed is null, we consider this upload wasn't successful
if (TextUtils.isEmpty(event.media.getUrl())) {
if (TextUtils.isEmpty(event.media.getUrl()) && !NetworkUtils.isNetworkAvailable(this)) {
MediaError error = new MediaError(MediaErrorType.GENERIC_ERROR);
mEditorMedia.onMediaUploadPaused(mEditorMediaUploadListener, event.media, error);
} else if (TextUtils.isEmpty(event.media.getUrl())) {
MediaError error = new MediaError(MediaErrorType.GENERIC_ERROR);
mEditorMedia.onMediaUploadError(mEditorMediaUploadListener, event.media, error);
} else {
Expand Down
12 changes: 12 additions & 0 deletions WordPress/src/main/java/org/wordpress/android/ui/posts/editor/media/EditorMedia.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,18 @@ class EditorMedia @Inject constructor(
listener.onMediaUploadFailed(media.id.toString())
}

fun onMediaUploadPaused(listener: EditorMediaUploadListener, media: MediaModel, error: MediaError) = launch {
val properties: Map<String, Any?> = withContext(bgDispatcher) {
analyticsUtilsWrapper
.getMediaProperties(media.isVideo, null, media.filePath)
.also {
it["error_type"] = error.type.name
}
}
analyticsTrackerWrapper.track(EDITOR_UPLOAD_MEDIA_FAILED, properties)
Copy link
Contributor

@derekblank derekblank Jan 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This will still send an EDITOR_UPLOAD_MEDIA_FAILED event to analytics when the upload is paused. I believe we'd want to remove these lines, but wanted to double check.

Copy link
Member Author

Choose a reason for hiding this comment

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

My thought is that we should still track these as errors (or, at least, track something). I referenced this in #19884 (comment). I imagine knowing the rate of this event is still useful for analysis.

Will you expand on why you think we should not?

Copy link
Contributor

@derekblank derekblank Jan 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Will you expand on why you think we should not?

My reasoning was that it might incorrectly affect the metrics outlined in the pitch, which were aiming to reduce instances of media_service_upload_failed and media_service_upload_response_error. I haven't yet checked to see if media_service_upload_failed results are actually affected by analyticsTrackerWrapper.track(EDITOR_UPLOAD_MEDIA_FAILED, properties), but it seems like it might be.

When the media upload is paused, I think we would expect it to retry and continue when the connection is resumed. If the upload then reached a legitimate failure, it would be captured by onMediaUploadFailed, which would fire the analytics failed event. Does that sound correct?

Either way, I don't think it should be a blocker for this PR. I've created an issue on the project board to discuss adding a paused analytics event.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for elaborating. Your rationale makes complete sense. I had not considered the impact on the planned project metrics.

I agree that we should follow up to remove or change this analytic event. We should also verify that the iOS implementation is correctly set up as well. I'll update your newly created issue to note this.

listener.onMediaUploadPaused(media.id.toString())
}

sealed class AddMediaToPostUiState(
val editorOverlayVisibility: Boolean,
val progressDialogUiState: ProgressDialogUiState
Expand Down
2 changes: 1 addition & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ ext {
automatticRestVersion = '1.0.8'
automatticStoriesVersion = '2.4.0'
automatticTracksVersion = '3.3.0'
gutenbergMobileVersion = 'v1.110.0'
gutenbergMobileVersion = '6491-31ff640dc82d1c7224f6791e23a4456ca034d04e'
wordPressAztecVersion = 'v1.9.0'
wordPressFluxCVersion = 'trunk-67afc3a620b4ad56b981d21637db6b3509b07ab6'
wordPressLoginVersion = '1.10.0'
Expand Down
6 changes: 6 additions & 0 deletions libs/editor/src/main/java/org/wordpress/android/editor/AztecEditorFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1431,6 +1431,12 @@ public void onMediaUploadFailed(final String localMediaId) {
mUploadingMediaProgressMax.remove(localMediaId);
}

@Override
public void onMediaUploadPaused(final String localMediaId) {
// Aztec does not leverage the paused media state, only the Gutenberg editor
onMediaUploadFailed(localMediaId);
}

@Override
public void onVideoInfoRequested(final AztecAttributes attrs) {
// VideoPress special case here
Expand Down
1 change: 1 addition & 0 deletions libs/editor/src/main/java/org/wordpress/android/editor/EditorMediaUploadListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@ public interface EditorMediaUploadListener {
void onMediaUploadProgress(String localId, float progress);
void onMediaUploadFailed(String localId);
void onGalleryMediaUploadSucceeded(long galleryId, long remoteId, int remaining);
void onMediaUploadPaused(String localId);
}
4 changes: 4 additions & 0 deletions ...itor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergContainerFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,10 @@ public void mediaFileUploadFailed(final int mediaId) {
mWPAndroidGlueCode.mediaFileUploadFailed(mediaId);
}

public void mediaFileUploadPaused(final int mediaId) {
mWPAndroidGlueCode.mediaFileUploadPaused(mediaId);
}

public void mediaFileUploadSucceeded(final int mediaId, final String mediaUrl, final int serverMediaId) {
mWPAndroidGlueCode.mediaFileUploadSucceeded(mediaId, mediaUrl, serverMediaId, new WritableNativeMap());
}
Expand Down
13 changes: 12 additions & 1 deletion .../editor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergEditorFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -845,7 +845,11 @@ private void updateFailedMediaState() {
for (String mediaId : mFailedMediaIds) {
// upload progress should work on numeric mediaIds only
if (!TextUtils.isEmpty(mediaId) && TextUtils.isDigitsOnly(mediaId)) {
getGutenbergContainerFragment().mediaFileUploadFailed(Integer.valueOf(mediaId));
if (NetworkUtils.isNetworkAvailable(getActivity())) {
getGutenbergContainerFragment().mediaFileUploadFailed(Integer.valueOf(mediaId));
} else {
getGutenbergContainerFragment().mediaFileUploadPaused(Integer.valueOf(mediaId));
}
} else {
getGutenbergContainerFragment().mediaFileSaveFailed(mediaId);
}
Expand Down Expand Up @@ -1496,6 +1500,13 @@ public void onMediaUploadFailed(final String localMediaId) {
mUploadingMediaProgressMax.remove(localMediaId);
}

@Override
public void onMediaUploadPaused(final String localMediaId) {
getGutenbergContainerFragment().mediaFileUploadPaused(Integer.valueOf(localMediaId));
mFailedMediaIds.add(localMediaId);
mUploadingMediaProgressMax.remove(localMediaId);
}

@Override
public void onGalleryMediaUploadSucceeded(final long galleryId, long remoteMediaId, int remaining) {
}
Expand Down
Loading