feat: Pause media upload when internet connection unavailable #19878
Conversation
This is an exploration. The approach taken, namely using the error type string, is not fully tested.
|
 |
App Name |  WordPress |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr19878-e3ad26b | |
| Commit | e3ad26b | |
| Direct Download | wordpress-prototype-build-pr19878-e3ad26b.apk |
|
 |
App Name |  Jetpack |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr19878-e3ad26b | |
| Commit | e3ad26b | |
| Direct Download | jetpack-prototype-build-pr19878-e3ad26b.apk |
WordPress/src/main/java/org/wordpress/android/ui/posts/editor/media/EditorMedia.kt
Outdated
Show resolved
Hide resolved
libs/editor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergEditorFragment.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@dcalhoun Thanks for adding this change! I have tested and it is working quite well to update the Image block visuals when offline, as intended. 👍 🙇
I've proposed taking it a step further in #19884 to add a truly discrete onMediaUploadPaused handler. That PR augments this one, and is in the spirit of what I was initially trying to accomplish in #19867.
Let me know your thoughts on using a more discrete paused handler -- if it's valuable, feel free to have full agency to merge or modify that PR here in the interest of optimizing time, or just take any parts that are useful. If not, I feel this PR itself is enough to approve and merge after the next release cut on Jan 8.
Ensure the implementation adheres to the relevant interface.
With the addition of a discrete paused method, we no longer utilize this parameter in the failed method.
We do not plan to add paused media support to the Aztec editor. Therefore, it likely makes more sense for the `onMediaUploadPaused` implementation to merely invoke the existing `onMediaUploadFailed` method.
Without checking for an error, there may be a possibility that successfully uploaded media is marked as "paused."
Even though we communicate a nuanced paused state to the user, we likely need to continue tracking this event as a failure, or consider adding a discrete pause analytic event.
Based upon existing code, we should consider completed uploads lacking remote URLs to be failures. Therefore, we should apply the new pause state to this context.
…ck-upload-visuals__discrete-paused-state Add discrete onMediaUploadPaused handler to media uploads when offline
… into feat/update-image-block-upload-visuals
| it["error_type"] = error.type.name | ||
| } | ||
| } | ||
| analyticsTrackerWrapper.track(EDITOR_UPLOAD_MEDIA_FAILED, properties) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Added #19878 (comment) about potentially removing the analytics event when the upload is paused. Otherwise, LGTM. 🚀
… into feat/update-image-block-upload-visuals
This comment was marked as off-topic.
This comment was marked as off-topic.
Related
Description
Pause media upload when internet connection is unavailable.
Testing Instructions
See WordPress/gutenberg#56937.
Regression Notes
Disrupting successful media uploads, breaking other media block types.
Uploaded media to various block types.
No, opted to test the JavaScript logic instead.
PR Submission Checklist
RELEASE-NOTES.txtif necessary.UI Changes Testing Checklist