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

Fix cloudinary upload error message #6525

Merged
merged 3 commits into from Jun 24, 2020
Merged

Fix cloudinary upload error message #6525

merged 3 commits into from Jun 24, 2020

Conversation

sudhirt4
Copy link
Contributor

@sudhirt4 sudhirt4 commented Jun 3, 2020

This is a minor pull request that fixes the propagation of error from cloudinary to strapi error middleware (err at : boom.Boomify).

Before (when entering invalid cloudinary config)

  AssertionError [ERR_ASSERTION]: Cannot wrap non-Error object

After

Error uploading to cloudinary: Unknown API key 4659829216368531
error Error: Upload to cloudinary failed

@codecov
Copy link

codecov bot commented Jun 3, 2020

Codecov Report

Merging #6525 into master will increase coverage by 0.13%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6525      +/-   ##
==========================================
+ Coverage   19.85%   19.98%   +0.13%     
==========================================
  Files         857      858       +1     
  Lines       12076    12097      +21     
  Branches     1963     1965       +2     
==========================================
+ Hits         2398     2418      +20     
- Misses       8093     8094       +1     
  Partials     1585     1585              
Flag Coverage Δ
#front 14.67% <50.00%> (+<0.01%) ⬆️
#unit 41.71% <95.00%> (+0.45%) ⬆️
Impacted Files Coverage Δ
...admin/src/containers/InstalledPluginsPage/index.js 7.14% <ø> (ø)
...kages/strapi-admin/admin/src/translations/index.js 100.00% <ø> (ø)
...-manager/admin/src/components/DynamicZone/index.js 0.00% <ø> (ø)
...in-content-manager/admin/src/translations/index.js 0.00% <ø> (ø)
...lder/admin/src/containers/FormModal/utils/forms.js 0.00% <0.00%> (ø)
...ntent-type-builder/admin/src/translations/index.js 0.00% <ø> (ø)
...ugin-documentation/admin/src/translations/index.js 0.00% <ø> (ø)
...trapi-plugin-email/admin/src/translations/index.js 0.00% <ø> (ø)
.../containers/InputModalStepper/InputModalStepper.js 0.00% <ø> (ø)
...-upload/admin/src/containers/ModalStepper/index.js 0.00% <ø> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6774667...9cc002d. Read the comment docs.

@alexandrebodin
Copy link
Member

Hi @sudhirt4, I don't think these errors should be sent back by the API. We should log them and send a upload error instead so we don't leak too many information

@alexandrebodin alexandrebodin self-requested a review June 15, 2020 12:40
@sudhirt4
Copy link
Contributor Author

@alexandrebodin
Updated the changes. Now limiting logs from from cloudinary to console while throwing a custom error message.

@@ -19,7 +19,8 @@ module.exports = {
{ resource_type: 'auto', public_id: file.hash, ...customConfig },
(err, image) => {
if (err) {
return reject(err);
console.log(`Error uploading to cloudinary: ${err.message}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here you will need to use strapi.log.error()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks for the improvement

@alexandrebodin alexandrebodin added source: core:upload Source is core/upload package issue: enhancement Issue suggesting an enhancement to an existing feature labels Jun 24, 2020
@alexandrebodin alexandrebodin added this to the 3.0.6 milestone Jun 24, 2020
@alexandrebodin alexandrebodin merged commit 0a38ec7 into strapi:master Jun 24, 2020
iicdii pushed a commit to iicdii/strapi that referenced this pull request Jul 2, 2020
Signed-off-by: Sudhir Shrestha <sudhirshrestha@live.com>
Signed-off-by: harimkims <harimkims@gmail.com>
gilfernandes pushed a commit to onepointconsulting/strapi that referenced this pull request Aug 13, 2020
Signed-off-by: Sudhir Shrestha <sudhirshrestha@live.com>
Signed-off-by: Gil Fernandes <gil.fernandes@onepointltd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: enhancement Issue suggesting an enhancement to an existing feature source: core:upload Source is core/upload package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants