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.

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

Media: Clean up linked post meta on attachment deletion #12321

Merged
merged 3 commits into from
Sep 19, 2022

Conversation

spacedmonkey
Copy link
Contributor

@spacedmonkey spacedmonkey commented Sep 15, 2022

Context

If you delete a muted / optimized version of a video, also delete the post meta to link to the original video.

Summary

Relevant Technical Choices

I created a util class so that the code can be shared. I thought about an abstract and traits to share code. A util class is what I needed up with.

I would like to avoid doing a meta query, but I can't think of a way around it, other than a raw database query.

To-do

User-facing changes

Testing Instructions

  • This is a non-user-facing change and requires no QA

This PR can be tested by following these steps:

  1. Add video
  2. Mute video
  3. Permanently delete muted video from WP media library
  4. Try to mute video again
  5. See no error

Reviews

Does this PR have a security-related impact?

No

Does this PR change what data or activity we track or use?

No

Does this PR have a legal-related impact?

No

Checklist

  • This PR addresses an existing issue and I have linked this PR to it in ZenHub
  • I have tested this code to the best of my abilities
  • I have verified accessibility to the best of my abilities (docs)
  • I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
  • This code is covered by automated tests (unit, integration, and/or e2e) to verify it works as intended (docs)
  • I have added documentation where necessary
  • I have added a matching Type: XYZ label to the PR

Fixes #12240

@spacedmonkey spacedmonkey added Group: Media Group: WordPress Changes related to WordPress or Gutenberg integration Pod: WP labels Sep 15, 2022
@spacedmonkey spacedmonkey self-assigned this Sep 15, 2022
@googleforcreators-bot
Copy link
Collaborator

googleforcreators-bot commented Sep 15, 2022

Plugin builds for 9e34261 are ready 🛎️!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 15, 2022

Size Change: 0 B

Total Size: 2.72 MB

ℹ️ View Unchanged
Filename Size
assets/css/carousel-view-rtl.css 702 B
assets/css/carousel-view.css 701 B
assets/css/web-stories-block-rtl.css 4.52 kB
assets/css/web-stories-block.css 4.56 kB
assets/css/web-stories-embed-rtl.css 318 B
assets/css/web-stories-embed.css 317 B
assets/css/web-stories-list-styles-rtl.css 2.36 kB
assets/css/web-stories-list-styles.css 2.39 kB
assets/css/web-stories-theme-style-twentyeleven-rtl.css 102 B
assets/css/web-stories-theme-style-twentyeleven.css 102 B
assets/css/web-stories-theme-style-twentyfifteen-rtl.css 251 B
assets/css/web-stories-theme-style-twentyfifteen.css 251 B
assets/css/web-stories-theme-style-twentyfourteen-rtl.css 287 B
assets/css/web-stories-theme-style-twentyfourteen.css 287 B
assets/css/web-stories-theme-style-twentyseventeen-rtl.css 288 B
assets/css/web-stories-theme-style-twentyseventeen.css 288 B
assets/css/web-stories-theme-style-twentysixteen-rtl.css 224 B
assets/css/web-stories-theme-style-twentysixteen.css 224 B
assets/css/web-stories-theme-style-twentyten-rtl.css 143 B
assets/css/web-stories-theme-style-twentyten.css 143 B
assets/css/web-stories-theme-style-twentytwelve-rtl.css 256 B
assets/css/web-stories-theme-style-twentytwelve.css 256 B
assets/css/web-stories-theme-style-twentytwenty-rtl.css 86 B
assets/css/web-stories-theme-style-twentytwenty.css 86 B
assets/css/web-stories-theme-style-twentytwentyone-rtl.css 326 B
assets/css/web-stories-theme-style-twentytwentyone.css 326 B
assets/css/web-stories-widget-rtl.css 482 B
assets/css/web-stories-widget.css 482 B
assets/css/wp-dashboard-rtl.css 657 B
assets/css/wp-dashboard.css 659 B
assets/css/wp-story-editor-rtl.css 737 B
assets/css/wp-story-editor.css 738 B
assets/js/1512.js 6.15 kB
assets/js/1814.js 7.45 kB
assets/js/2388.js 85.3 kB
assets/js/4319.js 34.9 kB
assets/js/4422.js 49.3 kB
assets/js/4484.js 84.7 kB
assets/js/5051.js 92.1 kB
assets/js/6753.js 113 kB
assets/js/9796.js 3.71 kB
assets/js/carousel-view.js 3.41 kB
assets/js/chunk-colorthief.js 2.64 kB
assets/js/chunk-ffmpeg.js 5.89 kB
assets/js/chunk-focus-visible.js 1.01 kB
assets/js/chunk-getStoryMarkup.js 12.4 kB
assets/js/chunk-html-to-image.js 4.5 kB
assets/js/chunk-opentype.js 96 B
assets/js/chunk-react-calendar.js 12.5 kB
assets/js/chunk-react-color.js 44.3 kB
assets/js/chunk-web-animations-js.js 14.6 kB
assets/js/chunk-web-stories-template-0-metaData.js 545 B
assets/js/chunk-web-stories-template-0.js 11.4 kB
assets/js/chunk-web-stories-template-1-metaData.js 539 B
assets/js/chunk-web-stories-template-1.js 9.61 kB
assets/js/chunk-web-stories-template-10-metaData.js 533 B
assets/js/chunk-web-stories-template-10.js 7.37 kB
assets/js/chunk-web-stories-template-11-metaData.js 539 B
assets/js/chunk-web-stories-template-11.js 9.09 kB
assets/js/chunk-web-stories-template-12-metaData.js 497 B
assets/js/chunk-web-stories-template-12.js 9.7 kB
assets/js/chunk-web-stories-template-13-metaData.js 525 B
assets/js/chunk-web-stories-template-13.js 7.4 kB
assets/js/chunk-web-stories-template-14-metaData.js 582 B
assets/js/chunk-web-stories-template-14.js 7.37 kB
assets/js/chunk-web-stories-template-15-metaData.js 544 B
assets/js/chunk-web-stories-template-15.js 9 kB
assets/js/chunk-web-stories-template-16-metaData.js 588 B
assets/js/chunk-web-stories-template-16.js 10.9 kB
assets/js/chunk-web-stories-template-17-metaData.js 539 B
assets/js/chunk-web-stories-template-17.js 9.2 kB
assets/js/chunk-web-stories-template-18-metaData.js 585 B
assets/js/chunk-web-stories-template-18.js 9.91 kB
assets/js/chunk-web-stories-template-19-metaData.js 501 B
assets/js/chunk-web-stories-template-19.js 10.8 kB
assets/js/chunk-web-stories-template-2-metaData.js 586 B
assets/js/chunk-web-stories-template-2.js 9.3 kB
assets/js/chunk-web-stories-template-20-metaData.js 548 B
assets/js/chunk-web-stories-template-20.js 9.01 kB
assets/js/chunk-web-stories-template-21-metaData.js 535 B
assets/js/chunk-web-stories-template-21.js 9.85 kB
assets/js/chunk-web-stories-template-22-metaData.js 525 B
assets/js/chunk-web-stories-template-22.js 7.83 kB
assets/js/chunk-web-stories-template-23-metaData.js 605 B
assets/js/chunk-web-stories-template-23.js 7.48 kB
assets/js/chunk-web-stories-template-24-metaData.js 516 B
assets/js/chunk-web-stories-template-24.js 11.7 kB
assets/js/chunk-web-stories-template-25-metaData.js 544 B
assets/js/chunk-web-stories-template-25.js 7.06 kB
assets/js/chunk-web-stories-template-26-metaData.js 601 B
assets/js/chunk-web-stories-template-26.js 7.27 kB
assets/js/chunk-web-stories-template-27-metaData.js 542 B
assets/js/chunk-web-stories-template-27.js 7.82 kB
assets/js/chunk-web-stories-template-28-metaData.js 532 B
assets/js/chunk-web-stories-template-28.js 9.07 kB
assets/js/chunk-web-stories-template-29-metaData.js 562 B
assets/js/chunk-web-stories-template-29.js 9.25 kB
assets/js/chunk-web-stories-template-3-metaData.js 539 B
assets/js/chunk-web-stories-template-3.js 8.42 kB
assets/js/chunk-web-stories-template-30-metaData.js 576 B
assets/js/chunk-web-stories-template-30.js 7.89 kB
assets/js/chunk-web-stories-template-31-metaData.js 502 B
assets/js/chunk-web-stories-template-31.js 10.3 kB
assets/js/chunk-web-stories-template-32-metaData.js 552 B
assets/js/chunk-web-stories-template-32.js 13.3 kB
assets/js/chunk-web-stories-template-33-metaData.js 492 B
assets/js/chunk-web-stories-template-33.js 9.07 kB
assets/js/chunk-web-stories-template-34-metaData.js 571 B
assets/js/chunk-web-stories-template-34.js 7.58 kB
assets/js/chunk-web-stories-template-35-metaData.js 566 B
assets/js/chunk-web-stories-template-35.js 8.91 kB
assets/js/chunk-web-stories-template-36-metaData.js 577 B
assets/js/chunk-web-stories-template-36.js 12.7 kB
assets/js/chunk-web-stories-template-37-metaData.js 528 B
assets/js/chunk-web-stories-template-37.js 6.71 kB
assets/js/chunk-web-stories-template-38-metaData.js 572 B
assets/js/chunk-web-stories-template-38.js 7.94 kB
assets/js/chunk-web-stories-template-39-metaData.js 589 B
assets/js/chunk-web-stories-template-39.js 8.08 kB
assets/js/chunk-web-stories-template-4-metaData.js 564 B
assets/js/chunk-web-stories-template-4.js 12.7 kB
assets/js/chunk-web-stories-template-40-metaData.js 557 B
assets/js/chunk-web-stories-template-40.js 10.2 kB
assets/js/chunk-web-stories-template-41-metaData.js 572 B
assets/js/chunk-web-stories-template-41.js 7.75 kB
assets/js/chunk-web-stories-template-42-metaData.js 521 B
assets/js/chunk-web-stories-template-42.js 7 kB
assets/js/chunk-web-stories-template-43-metaData.js 557 B
assets/js/chunk-web-stories-template-43.js 8.76 kB
assets/js/chunk-web-stories-template-44-metaData.js 584 B
assets/js/chunk-web-stories-template-44.js 11.1 kB
assets/js/chunk-web-stories-template-45-metaData.js 565 B
assets/js/chunk-web-stories-template-45.js 7.52 kB
assets/js/chunk-web-stories-template-46-metaData.js 531 B
assets/js/chunk-web-stories-template-46.js 5.22 kB
assets/js/chunk-web-stories-template-47-metaData.js 592 B
assets/js/chunk-web-stories-template-47.js 9.42 kB
assets/js/chunk-web-stories-template-48-metaData.js 555 B
assets/js/chunk-web-stories-template-48.js 9.09 kB
assets/js/chunk-web-stories-template-49-metaData.js 518 B
assets/js/chunk-web-stories-template-49.js 9.69 kB
assets/js/chunk-web-stories-template-5-metaData.js 556 B
assets/js/chunk-web-stories-template-5.js 9.94 kB
assets/js/chunk-web-stories-template-50-metaData.js 503 B
assets/js/chunk-web-stories-template-50.js 9.15 kB
assets/js/chunk-web-stories-template-51-metaData.js 526 B
assets/js/chunk-web-stories-template-51.js 10.4 kB
assets/js/chunk-web-stories-template-52-metaData.js 602 B
assets/js/chunk-web-stories-template-52.js 10.4 kB
assets/js/chunk-web-stories-template-53-metaData.js 553 B
assets/js/chunk-web-stories-template-53.js 5.78 kB
assets/js/chunk-web-stories-template-54-metaData.js 547 B
assets/js/chunk-web-stories-template-54.js 7.67 kB
assets/js/chunk-web-stories-template-55-metaData.js 573 B
assets/js/chunk-web-stories-template-55.js 7.13 kB
assets/js/chunk-web-stories-template-56-metaData.js 542 B
assets/js/chunk-web-stories-template-56.js 9.87 kB
assets/js/chunk-web-stories-template-57-metaData.js 527 B
assets/js/chunk-web-stories-template-57.js 14.9 kB
assets/js/chunk-web-stories-template-58-metaData.js 555 B
assets/js/chunk-web-stories-template-58.js 5.74 kB
assets/js/chunk-web-stories-template-59-metaData.js 590 B
assets/js/chunk-web-stories-template-59.js 8.96 kB
assets/js/chunk-web-stories-template-6-metaData.js 569 B
assets/js/chunk-web-stories-template-6.js 7.07 kB
assets/js/chunk-web-stories-template-60-metaData.js 510 B
assets/js/chunk-web-stories-template-60.js 9.51 kB
assets/js/chunk-web-stories-template-7-metaData.js 569 B
assets/js/chunk-web-stories-template-7.js 7.46 kB
assets/js/chunk-web-stories-template-8-metaData.js 569 B
assets/js/chunk-web-stories-template-8.js 8.93 kB
assets/js/chunk-web-stories-template-9-metaData.js 581 B
assets/js/chunk-web-stories-template-9.js 8.46 kB
assets/js/chunk-web-stories-templates.js 845 B
assets/js/chunk-web-stories-textset-0.js 5.06 kB
assets/js/chunk-web-stories-textset-1.js 6.65 kB
assets/js/chunk-web-stories-textset-2.js 7.65 kB
assets/js/chunk-web-stories-textset-3.js 15.1 kB
assets/js/chunk-web-stories-textset-4.js 4.15 kB
assets/js/chunk-web-stories-textset-5.js 5.47 kB
assets/js/chunk-web-stories-textset-6.js 5.28 kB
assets/js/chunk-web-stories-textset-7.js 10.2 kB
assets/js/generateBlurhash.worker.worker.js 1.1 kB
assets/js/imgareaselect.js 3.77 kB
assets/js/lightbox.js 550 B
assets/js/tinymce-button.js 2.85 kB
assets/js/web-stories-activation-notice.js 25.4 kB
assets/js/web-stories-block.js 18 kB
assets/js/web-stories-embed.js 20 B
assets/js/web-stories-widget.js 587 B
assets/js/wp-dashboard.js 64.1 kB
assets/js/wp-story-editor.js 1.36 MB

compressed-size-action

@swissspidy swissspidy changed the title Media: web_stories_muted_id not cleaned up on attachment deletion Media: Clean up linked post meta on attachment deletion Sep 15, 2022
@swissspidy
Copy link
Collaborator

Can we do delete_metadata( 'post', null, 'web_stories_optimized_id', $attachment_id, true ); instead?

The only difference is that this works on all post types, not just attachments. But since these are prefixed meta keys that ar only used by us and only for attachments, that's OK.

This will greatly simplify the code and we don't have to establish a Utils class or similar (which I am not keen on adding)

@spacedmonkey
Copy link
Contributor Author

Can we do delete_metadata( 'post', null, 'web_stories_optimized_id', $attachment_id, true ); instead?

I always for forget about that with the metadata api. That is way simpler. I have pushed a change to be do this.

@kkalarickal
Copy link

Error Reproduction:

I recreated the issue in main-branch and got the following error on console:
image.png

Bug Fix:

I switched over to #12321 branch and confirmed via test-web-story the following sequence worked:

  • create a story and adds a page with video embedded
  • confirm that story plays with sounds
  • press remove-audio button and confirm that a new muted video has been uploaded to media library
  • delete that muted video from the media library
  • create another page and embed a different video with audio into the page
  • now you can play audio, but if you try to "reduce audio" once again, there is NO console error and its possible to reduce audio a second time.

@kkalarickal
Copy link

Permutations:

In order to test around other media clean up events, I added background audio and then deleted the original file, to confirm there are no side effects.

@swissspidy swissspidy merged commit 23bb0c4 into main Sep 19, 2022
@swissspidy swissspidy deleted the fix/clear-post-meta branch September 19, 2022 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Group: Media Group: WordPress Changes related to WordPress or Gutenberg integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Media: web_stories_muted_id not cleaned up on attachment deletion
4 participants