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

Add PDF replacement feature #220

Merged

Conversation

hugosolar
Copy link
Contributor

Description of the Change

We've seen some situations where users want to replace a PDF file with an updated version (CV for example)
This PR adds to the plugin the ability to replace pdf files (for now) using a new method to copy the blob and replace it with the uploaded file.
There's a new method called copy_media_to_blob_storage in helpers and also a new copy_blob method in the API client
this uses the Copy Blob operation described here: https://learn.microsoft.com/en-us/rest/api/storageservices/Copy-Blob

There's also a new button added in the media uploader that holds the workflow to replace the document

Open discussion for addressing images

We should have a discussion on how to approach image replacements since an image of a different orientation can be uploaded and image sizes of the image to be replaced won't match, so I was thinking on two paths

  • Should we force the same image sizes from the replaced image so we can have backward compatibility in case those images are being called directly?
  • Should we discard previous images and just replace thumbnails with the ones generated by the new image?

How to test the Change

To test this:

  • Go to the media screen in the admin dashboard (wp-admin/upload.php)
  • Upload a PDF file
  • Select the file and you'll see a "replace this media" button
  • Click the button and select the file to upload (the view is restricted to just upload a file)
  • Once it's uploaded click on the "Replace media" button below
  • That will trigger the replacement of the main file and the thumbnails stored as meta fields

Changelog Entry

Added - New feature to replace PDF files at the blob storage level
Fixed - Bug fix. Fixed a bug with the transient generated for displaying progress

Credits

@hugosolar

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@hugosolar hugosolar self-assigned this Mar 27, 2024
@jeffpaul jeffpaul requested review from colegeissinger and removed request for dkotter and jeffpaul April 1, 2024 21:23
@jeffpaul jeffpaul added this to the 4.5.0 milestone Apr 1, 2024
@jeffpaul
Copy link
Member

jeffpaul commented Apr 1, 2024

@hugosolar looks like failing e2e tests, could you check on those please?

@hugosolar
Copy link
Contributor Author

@jeffpaul I think this is related to the constants passed to Cypress related to the account name & key

here https://github.com/hugosolar/windows-azure-storage/blob/022290b32e875986dc4b790be6891c56ad736c2d/tests/cypress/e2e/settings.test.js#L12

On a previous PR, I remember we faced the same situation and @Sidsector9 was able to fix it
let me know otherwise

@jeffpaul jeffpaul requested a review from Sidsector9 April 12, 2024 18:47
includes/class-windows-azure-helper.php Show resolved Hide resolved
includes/class-windows-azure-replace-media.php Outdated Show resolved Hide resolved
includes/class-windows-azure-replace-media.php Outdated Show resolved Hide resolved
includes/class-windows-azure-replace-media.php Outdated Show resolved Hide resolved
includes/class-windows-azure-replace-media.php Outdated Show resolved Hide resolved
includes/class-windows-azure-replace-media.php Outdated Show resolved Hide resolved
includes/class-windows-azure-replace-media.php Outdated Show resolved Hide resolved
includes/class-windows-azure-replace-media.php Outdated Show resolved Hide resolved
@rickalee
Copy link
Collaborator

@hugosolar Some feedback to address and need to address the E2E tests.

@hugosolar hugosolar requested a review from rickalee April 17, 2024 17:38
@jeffpaul jeffpaul merged commit b5fcb51 into 10up:develop Apr 23, 2024
2 of 5 checks passed
@hugosolar hugosolar mentioned this pull request May 27, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants