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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Cloudinary deletion of other files than images #3691

Conversation

@DKvistgaard
Copy link
Contributor

DKvistgaard commented Jul 29, 2019

Deleting other file types than images didn't work in the current delete method for the Cloudinary plugin. This is due to the destroy method not getting the correct resource_type value. It defaults to image (see the documentation here) so it tries to remove an image with the passed public_id even though it's an image, so it fails instead. Also we can't pass auto to the resource_type for this method, so that's why we need the resource_type saved on the file.

My PR is a:

  • 馃挜 Breaking change
  • 馃悰 Bug fix
  • 馃拝 Enhancement
  • 馃殌 New feature

Main update on the:

  • Admin
  • Documentation
  • Framework
  • Plugin

Manual testing done on the following databases:

  • Not applicable
  • MongoDB
  • MySQL
  • Postgres
  • SQLite
Fixed by adding a resource_type on the file which is required for the
detroy method. It defaults to 'image', so if it isn't defined then it
will try to remove an image with the passed public_id.
@@ -50,6 +50,10 @@
"type": "string",
"configurable": false
},
"resource_type": {

This comment has been minimized.

Copy link
@alexandrebodin

alexandrebodin Jul 30, 2019

Collaborator

This field won't make sense for a lot of other plugins. It's too specific to cloudinary.

We could add a json metadata field to handle those kind of necessary informations for multiple other plugins for example

This comment has been minimized.

Copy link
@DKvistgaard

DKvistgaard Aug 2, 2019

Author Contributor

Good idea. I'll make an update for that soon.

This comment has been minimized.

Copy link
@DKvistgaard

DKvistgaard Aug 5, 2019

Author Contributor

@alexandrebodin It looks like the public_id field is also Cloudinary specific. Should we also move this field into the metadata field (calling it provider_metadata)?

This comment has been minimized.

Copy link
@alexandrebodin

alexandrebodin Aug 5, 2019

Collaborator

sure we can move it in that field

This comment has been minimized.

Copy link
@DKvistgaard

DKvistgaard Aug 5, 2019

Author Contributor

Might not be a good idea anyway as it will remove the public_id data from previously uploaded images, so it won't be possible to delete those images after updating

Edit: Thinking you might already have thought of that as you've marked this PR as a breaking change

This comment has been minimized.

Copy link
@alexandrebodin

alexandrebodin Aug 5, 2019

Collaborator

Yep :) Will just give details on the changelog and communicaition once we release this breaking change. but it's for the best

To store necessary metadata for upload providers. In this case it's used
for public_id and resource_type for Cloudinary.
@DKvistgaard

This comment has been minimized.

Copy link
Contributor Author

DKvistgaard commented Aug 5, 2019

@alexandrebodin So, a question regarding the Travis CI tests: It's currently failing as it says that type "jsonb" does not exist for PostgreSQL. That also seems to be true as Travis CI uses version 9.2 as default and the jsonb type was introduced in version 9.4. So the question is if I should update the PostgreSQL version for Travis CI and then which version we prefer?

@alexandrebodin

This comment has been minimized.

Copy link
Collaborator

alexandrebodin commented Aug 5, 2019

@alexandrebodin So, a question regarding the Travis CI tests: It's currently failing as it says that type "jsonb" does not exist for PostgreSQL. That also seems to be true as Travis CI uses version 9.2 as default and the jsonb type was introduced in version 9.4. So the question is if I should update the PostgreSQL version for Travis CI and then which version we prefer?

You can just update postgresl in TRAVIS as we mention postgresql v10+ in the requirements anyway

Copy link
Collaborator

alexandrebodin left a comment

LGTM, I'll be merging this with an upcoming release that also introduces breaking changes to avoid having to migrate too many times ;)

@alexandrebodin alexandrebodin dismissed their stale review Aug 6, 2019

To avoid anyone merging it by error

@alexandrebodin

This comment has been minimized.

Copy link
Collaborator

alexandrebodin commented Aug 6, 2019

@DKvistgaard

This comment has been minimized.

Copy link
Contributor Author

DKvistgaard commented Aug 8, 2019

@DKvistgaard you can use https://github.com/strapi/strapi/pull/3728/files#diff-354f30a63fb0907d4ad57269548329e3 to fix your pg issue ;)

Thanks! Hadn't had the time to look into the details of the Travis CI build, so thanks a lot for the link 馃憤 馃槃

@alexandrebodin alexandrebodin requested a review from lauriejim Sep 6, 2019
@alexandrebodin

This comment has been minimized.

Copy link
Collaborator

alexandrebodin commented Sep 9, 2019

@DKvistgaard Hi, we finally merged the latest release with quite a few changes. If you can update your PR it would be awesome !

@DKvistgaard

This comment has been minimized.

Copy link
Contributor Author

DKvistgaard commented Sep 10, 2019

@DKvistgaard Hi, we finally merged the latest release with quite a few changes. If you can update your PR it would be awesome !

Perfect. I'll go through it ASAP

DKvistgaard added 2 commits Sep 10, 2019
鈥-delete-resource-type
@alexandrebodin alexandrebodin added this to In progress in PRs via automation Sep 20, 2019
@alexandrebodin alexandrebodin moved this from In progress to Reviewer approved in PRs Sep 20, 2019
@alexandrebodin alexandrebodin changed the base branch from master to releases/3.0.0-beta.17 Oct 4, 2019
@alexandrebodin alexandrebodin merged commit 1a49d1a into strapi:releases/3.0.0-beta.17 Oct 4, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
PRs automation moved this from Reviewer approved to Done Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.