VideoPress: Fix VideoPress Attachment Deletions on wp.com #20832

roundhill · 2021-08-25T22:11:51Z

Fixes issue where deleting a VideoPress video would not actually delete the VideoPress entry on wp.com. Uses the pre_delete_attachment filter to attempt deletion of VideoPress video on wp.com.

There's a big change here in that the attachment will not get deleted unless wp.com successfully deleted the VideoPress video (unless the API returns a 404 which means it was already deleted or not found). This is to prevent any dangling data. One thing I'd like advice on here is how we can tell the user there was an error so they can take appropriate action. At a minimum it'd be great if we could add to the activity log so that happiness can help troubleshoot.

Changes proposed in this Pull Request:

Updates VideoPress attachment deletions to use the wp.com api to actually delete the video from our servers.

Jetpack product discussion

pxWta-Zz-p2

Does this pull request change what data or activity we track or use?

No

Testing instructions:

Select a video in the media library and copy it's guid.
Delete the video.
Check that the video is not found at https://videopress.com/v/{guid}
Repeat the above steps for videos in the Calypso media library.

Also test that attachment poster image is deleted:

Right click on a video thumbnail and copy the url for the poster image:

* Delete the video * Try to access the thumbnail url, it should also be deleted.

…video on wp.com.

github-actions · 2021-08-25T22:15:13Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

Next scheduled release: September 7, 2021.
Scheduled code freeze: August 30, 2021.

jgcaruso

Tested perfectly for the happy path, but when I tried to make things break it got into some funny situations.

I wonder if we check for specific errors, like 404 and allow the function to return null. This could help if the video was deleted on wpcom but the request timed out (or jetpack lost connection). The next time the user tries to delete the video we'd block them because its an error, but really if the video is gone from the server we can allow it to be removed from their site's media library.

jgcaruso · 2021-08-26T14:43:08Z

projects/plugins/jetpack/modules/videopress/class.jetpack-videopress.php

+
+		$guid = get_post_meta( $post->ID, 'videopress_guid', true );
+		if ( empty( $guid ) ) {
+			return false;


I wonder if this should return null too? If its a videopress video but the guid is missing, there is probably nothing anyone can do to help remove this, and/or it may have never finished uploading or transcoding.

Not 100% sure about this though. I suppose we could manually remove it based on blog id and post it, so maybe failing here is OK. OR, maybe our endpoint can optionally take blog id / post id somehow as a fallback so guid isn't actually required? Not sure how likely guid missing from here would be though.

I think that should be ok to return null here. I think the chance of this happening is very small so it may be best to just delete it to avoid confusion. I'll update it!

roundhill · 2021-08-26T15:24:37Z

I wonder if we check for specific errors, like 404 and allow the function to return null. This could help if the video was deleted on wpcom but the request timed out (or jetpack lost connection). The next time the user tries to delete the video we'd block them because its an error, but really if the video is gone from the server we can allow it to be removed from their site's media library.

I like this idea, at least for 404s 👍. I'll add a check for that and return null.

…check as well to allow the video to be deleted. Also cleaned up some linter errors.

jgcaruso

404 check works as expected!

Other than the latest doc block warning, everything looks good!

jeherve

This seems to test well for me. I was wondering if we should take that opportunity to also delete the poster image or images that were created for that video?

roundhill · 2021-08-31T14:54:00Z

This seems to test well for me. I was wondering if we should take that opportunity to also delete the poster image or images that were created for that video?

That's a good call - I assumed all of the thumbnails were stored on wp.com but I see some in the local media library.

…, if it exists it deletes it.

roundhill · 2021-08-31T20:23:17Z

That's a good call - I assumed all of the thumbnails were stored on wp.com but I see some in the local media library.

Ok the attachment poster image is also deleted now as well: 5d5713e. Updated the testing instructions on how to test for that.

jeherve

This seems to test well for me. 👍

…ctually-delete

* master: tools: Tool to report unreleased projects (#20874) Add max length for stacktrace in Slack notifications (#20913) Show HLS Playlist for the VideoPress video url, instead of fragmented mp4 file (#20839) VideoPress: Fix VideoPress Attachment Deletions on wp.com (#20832) Update various JS dependencies (#20890) Widget Visibility: add scaffolding for upcoming editor features. (#20910) Heartbeat: Fix typo (#20901) Social Icons: fix being able to remove icons (#20899) Update To-Test.md for Jetpack 10.1 (#20894) Update dependency size-limit to v5 (#20893) Release: start 10.2 cycle (#20896) Jetpack 10.1 Changelog (#20892) E2E tests: add report name option in Slack notification (#20887) Carousel: add more specific selectors for the carousel cursor (#20882) E2E Tests: Fix pre-connection test (#20888) Search: fix body scroll for overlay open/close (#20733) E2E Tests: Fix mailchimp test (#20855) E2E tests: improve error handling (#20884)

Use pre_delete_attachment filter to attempt deletion of VideoPress …

fe36a86

…video on wp.com.

roundhill added [Feature] VideoPress A feature to help you upload and insert videos on your site. [Status] Needs Team Review labels Aug 25, 2021

roundhill requested a review from jgcaruso August 25, 2021 22:11

github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Aug 25, 2021

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Aug 25, 2021

jeherve assigned roundhill Aug 26, 2021

jgcaruso suggested changes Aug 26, 2021

View reviewed changes

jgcaruso mentioned this pull request Aug 26, 2021

VideoPress: Deleted Videos Not Deleted #19405

Open

roundhill added 2 commits August 26, 2021 15:06

Return null instead of false if a guid is not found. Added a 404 …

d65aee3

…check as well to allow the video to be deleted. Also cleaned up some linter errors.

Adding changelog entry.

fff5d41

roundhill removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Aug 30, 2021

jgcaruso previously approved these changes Aug 30, 2021

View reviewed changes

roundhill dismissed jgcaruso’s stale review via c13265a August 30, 2021 21:45

roundhill requested a review from a team as a code owner August 30, 2021 21:45

matticbot added the Touches WP.com Files label Aug 30, 2021

roundhill force-pushed the update/videopress-actually-delete branch from c13265a to fff5d41 Compare August 30, 2021 21:48

matticbot removed the Touches WP.com Files label Aug 30, 2021

Adding params and return details to docblock.

3f1eaf0

roundhill removed the request for review from a team August 30, 2021 21:50

jeherve added the [Type] Bug When a feature is broken and / or not performing as intended label Aug 31, 2021

jeherve reviewed Aug 31, 2021

View reviewed changes

Added function that checks for a poster image attachment on the video…

5d5713e

…, if it exists it deletes it.

jeherve approved these changes Sep 1, 2021

View reviewed changes

jeherve added this to the jetpack/10.2 milestone Sep 1, 2021

Merge remote-tracking branch 'origin/master' into update/videopress-a…

2a6e763

…ctually-delete

roundhill removed the [Status] Needs Team Review label Sep 1, 2021

roundhill merged commit 23b890a into master Sep 1, 2021

roundhill deleted the update/videopress-actually-delete branch September 1, 2021 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VideoPress: Fix VideoPress Attachment Deletions on wp.com #20832

VideoPress: Fix VideoPress Attachment Deletions on wp.com #20832

roundhill commented Aug 25, 2021 •

edited

Loading

github-actions bot commented Aug 25, 2021 •

edited

Loading

jgcaruso left a comment

jgcaruso Aug 26, 2021 •

edited

Loading

roundhill Aug 26, 2021

roundhill commented Aug 26, 2021

jgcaruso left a comment

jeherve left a comment

roundhill commented Aug 31, 2021

roundhill commented Aug 31, 2021 •

edited

Loading

jeherve left a comment

VideoPress: Fix VideoPress Attachment Deletions on wp.com #20832

VideoPress: Fix VideoPress Attachment Deletions on wp.com #20832

Conversation

roundhill commented Aug 25, 2021 • edited Loading

Changes proposed in this Pull Request:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Aug 25, 2021 • edited Loading

jgcaruso left a comment

Choose a reason for hiding this comment

jgcaruso Aug 26, 2021 • edited Loading

Choose a reason for hiding this comment

roundhill Aug 26, 2021

Choose a reason for hiding this comment

roundhill commented Aug 26, 2021

jgcaruso left a comment

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

roundhill commented Aug 31, 2021

roundhill commented Aug 31, 2021 • edited Loading

jeherve left a comment

Choose a reason for hiding this comment

roundhill commented Aug 25, 2021 •

edited

Loading

github-actions bot commented Aug 25, 2021 •

edited

Loading

jgcaruso Aug 26, 2021 •

edited

Loading

roundhill commented Aug 31, 2021 •

edited

Loading