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

Adding failure callback to ui file uploader #27092

Conversation

bartoszkubicki
Copy link

@bartoszkubicki bartoszkubicki commented Feb 29, 2020

Description (*)

Magento_Ui file uploader works fine. However, recently when I have used it for custom file upload, and when I had error while uploading file there was no info what happened. Uploader just stopped uploading. After debugging it turned out it was 413 from nginx, and next steps were clear (bumping post, upload and client body size). I think if we have an error we should get info in dev tools console.

Magento_Downloadable file uploader in adminhtml is heavily based on this uploader, so whole thing can be investigated and tested while creating downloadable product.
By the way, I had refactored formatting a little bit.

Manual testing scenarios (*)

  1. Open downloadable product form
  2. Try to upload huge file exceeding nginx and php limitations (especially client_max_body_size)
  3. There is no info, uploader just stops working.

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

Resolved issues:

  1. resolves [Issue] Adding failure callback to ui file uploader #29557: Adding failure callback to ui file uploader

@m2-assistant
Copy link

m2-assistant bot commented Feb 29, 2020

Hi @bartoszkubicki. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

Copy link
Contributor

@lenaorobei lenaorobei left a comment

Choose a reason for hiding this comment

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

Hi @bartoszkubicki!
Could you please address build failures.

@bartoszkubicki
Copy link
Author

@lenaorobei I have fixed some issue, but now I am stuck. Unit tests are marked as failed, but there is no info what exactly failed even if I visit allure report (and I am writing js not php!).

Functional tests failed because of strange looking communicate - something about browser and selenium - but it doesn't seem to be connected to code changes.

Static tests are failed because of copy-paste in some module I didn't touched. What are we supposed to do with this one?

@lenaorobei
Copy link
Contributor

@bartoszkubicki sorry for the late response. I'm going to update your branch to see new build results. Most likely static failures are related to the es6.

@lenaorobei
Copy link
Contributor

@magento run all tests

@lenaorobei
Copy link
Contributor

@magento run Static Tests

Copy link
Contributor

@lenaorobei lenaorobei left a comment

Choose a reason for hiding this comment

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

Here is the build console log:

Error Message
Parsing error: The keyword 'let' is reserved
Stacktrace
line 78, col 13, Error - Parsing error: The keyword 'let' is reserved

Please avoid the use of ES6 syntax. Thank you.

@bartoszkubicki
Copy link
Author

@lenaorobei Is there any reason why anyone should not use es6? Why there is such a statement in static test?

@lenaorobei
Copy link
Contributor

I think @DrewML is the best person to answer this question.

@sidolov
Copy link
Contributor

sidolov commented Aug 14, 2020

@magento create issue

@sidolov sidolov added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. labels Aug 14, 2020
@bartoszkubicki
Copy link
Author

@lenaorobei I was curious why such a standard is kept. ECMA 6 seems to be quite supported in all main browsers.

@lenaorobei
Copy link
Contributor

@bartoszkubicki historically this decision is was made to keep IE compatibility. Now it requires revision and reconsideration, but that's the rule for now.

@bartoszkubicki bartoszkubicki force-pushed the ui-file-uploader-failure-callback branch from af081f2 to 4251151 Compare August 20, 2020 22:15
@bartoszkubicki
Copy link
Author

bartoszkubicki commented Aug 20, 2020

@lenaorobei Didn't know that we are going to deal with such a archeology ;)

Code corrected, hope this time all checks will pass.

@bartoszkubicki bartoszkubicki force-pushed the ui-file-uploader-failure-callback branch from 4251151 to db3554a Compare August 20, 2020 22:19
@lenaorobei
Copy link
Contributor

@magento run all tests

@bartoszkubicki
Copy link
Author

@gabrieldagama you were faster :) I was going to look at that this weeekend. So everything is ready?

@engcom-Hotel
Copy link
Contributor

@magento run all tests

@engcom-Delta engcom-Delta self-assigned this Oct 5, 2020
@engcom-Delta engcom-Delta moved this from Ready for Testing to Testing in Progress in Pull Requests Dashboard Oct 5, 2020
@engcom-Delta
Copy link
Contributor

✔️ QA passed
Result according Manual testing scenarios:
Before:
#27092Main

✔️ After:
Additional information about error in dev console
#27092PR

@engcom-Delta
Copy link
Contributor

Note: Functional Tests B2B , Functional Tests CE and Functional Tests EE are failed

@engcom-Delta engcom-Delta moved this from Testing in Progress to Extended Testing (optional) in Pull Requests Dashboard Oct 5, 2020
@gabrieldagama
Copy link
Contributor

The failing tests are not related to this PR.

@engcom-Hotel engcom-Hotel moved this from Extended Testing (optional) to Ready for Testing in Pull Requests Dashboard Oct 7, 2020
@engcom-Delta engcom-Delta moved this from Ready for Testing to Testing in Progress in Pull Requests Dashboard Oct 7, 2020
@engcom-Delta engcom-Delta added the QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope label Oct 7, 2020
@engcom-Delta
Copy link
Contributor

Note: Automation tests are passed

@engcom-Delta engcom-Delta moved this from Testing in Progress to Merge in Progress in Pull Requests Dashboard Oct 7, 2020
@magento-engcom-team magento-engcom-team merged commit a38f936 into magento:2.4-develop Oct 7, 2020
@m2-assistant
Copy link

m2-assistant bot commented Oct 7, 2020

Hi @bartoszkubicki, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@ghost ghost moved this from Merge in Progress to Recently Merged in Pull Requests Dashboard Oct 7, 2020
@bartoszkubicki
Copy link
Author

@lenaorobei Please back-port it to magento 2.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Frontend Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Component: Ui Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: accept QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Release Line: 2.4 Severity: S2 Major restrictions or short-term circumventions are required until a fix is available.
Projects
Pull Requests Dashboard
  
Recently Merged
Development

Successfully merging this pull request may close these issues.

[Issue] Adding failure callback to ui file uploader
8 participants