Video preview isssue #30324 #31770

gixid192 · 2021-01-20T09:41:19Z

Summary

This PR fix the #30324
The error as I understand is caused by not cancelling the video-api request. More on description below.

Description (*)

When making a request to the server. While the request has not been fulfilled yet, the user closes the edit modal and then opens add-new modal.
At the time the request finished, the user is already on the add-new-video modal, so the data from the request now renders on this new modal
The code make sure that it only renders new data in the correct modals/pages.

Fixed Issues (if relevant)

#30324

Manual testing scenarios (*)

Add setTimeout in the ajax's success function (which call _onVimeoLoaded/_onYouTubeLoaded). Keep this timeout long enough to deplay the render function.
Follow the steps in original issue: Add and save new video -> open edit page -> close edit page -> click "Add Video" button to open add-new-video modal

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)

abc

m2-assistant · 2021-01-20T09:41:21Z

Hi @gixid192. 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

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests
Semantic Version Checker

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

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

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

gixid192 · 2021-01-20T11:27:06Z

@magento run all tests

gixid192 · 2021-01-21T02:46:21Z

@magento run all tests

gixid192 · 2021-01-22T08:21:44Z

@magento run all tests

gixid192 · 2021-01-22T10:29:38Z

@magento run all tests

gixid192 · 2021-01-25T02:51:08Z

@magento run Functional Tests B2B

danielrussob · 2021-01-25T18:17:44Z

@magento give me test instance

magento-deployment-service · 2021-01-25T18:17:47Z

Hi @danielrussob. Thank you for your request. I'm working on Magento instance for you.

danielrussob · 2021-01-25T18:17:58Z

@magento give me 2.4-develop instance

magento-deployment-service · 2021-01-25T18:18:00Z

Hi @danielrussob. Thank you for your request. I'm working on Magento instance for you.

magento-deployment-service · 2021-01-25T18:24:59Z

Hi @danielrussob, here is your Magento Instance: https://f2e1e753c097aa7081e0c62821e83d39.instances.magento-community.engineering
Admin access: https://f2e1e753c097aa7081e0c62821e83d39.instances.magento-community.engineering/admin_f6d9
Login: 72e551ca
Password: 9c6a52f49b09

magento-deployment-service · 2021-01-25T18:25:18Z

Hi @danielrussob, here is your Magento Instance: https://f2e1e753c097aa7081e0c62821e83d39-2-4-develop.instances.magento-community.engineering
Admin access: https://f2e1e753c097aa7081e0c62821e83d39-2-4-develop.instances.magento-community.engineering/admin_67dc
Login: dbc51683
Password: 9983cf449290

danielrussob · 2021-01-25T18:35:10Z

Issue Reproduced on vanilla Magento 2.4

danielrussob · 2021-01-25T18:38:39Z

Issued (just as described) are solved on new Magento instance

danielrussob · 2021-01-25T18:43:49Z

app/code/Magento/ProductVideo/view/adminhtml/web/js/new-video-dialog.js

@@ -1103,6 +1104,7 @@ define([
                this._previewImage = null;
            }
            this._tempPreviewImageData = null;
+            this._videoUrlWidget.trigger('cancelled_video_information');


Hello @gixid192 ,
thank you for your contribution.

I have two questions for you:

a. Can you write a test for this PR?
b. I have found a new bug, I tell you a step to reproduce the bug:

b.1. Navigate to Add New Product page in backend
b.2. Open Image And Videos section
b.3. Click on Add Video button
b.4. Enter Vimeo or YouTube video URL in Url field and wait until the Save button is enabled
b.5. Click on Save button
b.6. Click on Add Video Button
b.7. paste URL in Url input, click Title and quickly click on X to close
b.8. Press on Add Video button
b.9. See an Preview Image (bottom left corner) from the previous video

Hi @danielrussob,

I'm not sure how to write test for this case. Could you suggest some ideas?

Did you try this new case with the code in the PR or the original code? With the PR's code, it stop the render part right after you close X, so there will be no b.8/b.9
I tried it myself, couldn't reproduce the issue.

Hi,
for point 2, i have tested it on your PR, if you want we can schedule a chat on slack of Magento Community Engineering
to reproduce the issue (you can found me with name Daniel Russo)

for point 1, see comment of @sivaschenko

Hi,
So I tried again and reproduced the issue.

There's another ajax in the update image-placeholder:

magento2/app/code/Magento/ProductVideo/view/adminhtml/web/js/new-video-dialog.js

Line 393 in cd857f9

$.ajax({

I made the same approach and it fixed the problem! However, I don't understand the purpose of that _loadRemotePreview function.
The _loadRemotePreview function does 2 things:

Send request to server to save the image

Only show the preview_image if the request above success.

The thing is those 2 things are not related, it does not take data from the request to show for users (instead the preview_image is taken from video's api). Also, if the request fail, there is no error handle for it (and the preview_image still show)

Should we split this function into 2 small separated ones?

@danielrussob: I updated new code to fix the issue you reported, just write new code, no splitting; kindly have a look when you have time.

If you have options on this split into 2 functions, please let me know.

sivaschenko · 2021-01-26T11:36:35Z

@danielrussob @gixid192 I think that MFTF test covering this fix may not be reliable as the fixed behaviour depends on the server response time. I'd advise covering the fix with a javascript jasmine test. https://devdocs.magento.com/guides/v2.4/test/js/jasmine.html

gixid192 · 2021-01-27T10:56:04Z

@sivaschenko Thanks for the suggestions, I will try to cover the test, never write testes in Js before 😅

danielrussob · 2021-02-02T17:15:48Z

@magento give me 2.4-develop instance

magento-deployment-service · 2021-02-02T17:15:50Z

Hi @danielrussob, unfortunately there is no ability to deploy Magento instance at the moment. Please try again later.

gixid192 · 2021-02-18T03:48:20Z

@magento run all tests

gixid192 and others added 2 commits January 20, 2021 15:44

Merge pull request #1 from magento/2.4-develop

1be4239

abc

fix old video preview thumbnail in add video modal

af8943f

m2-community-project bot added Priority: P3 May be fixed according to the position in the backlog. Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels Jan 20, 2021

magento-engcom-team added Area: Frontend Component: ProductVideo Release Line: 2.4 labels Jan 20, 2021

m2-community-project bot added the Progress: pending review label Jan 20, 2021

Merge branch '2.4-develop' into video-preview-isssue-magento#30324

008597b

format code

8f2a916

Jimmy added 2 commits January 22, 2021 16:47

fix jscs rules

99eae8d

update this

7e2ea61

danielrussob self-requested a review January 25, 2021 18:38

danielrussob self-assigned this Jan 25, 2021

m2-community-project bot added Progress: review and removed Progress: pending review labels Jan 25, 2021

danielrussob requested changes Jan 25, 2021

View reviewed changes

m2-community-project bot added Progress: needs update Progress: review and removed Progress: review Progress: needs update labels Jan 25, 2021

fix race condition saving video thumbnail

a0e2926

Video preview isssue #30324 #31770

Are you sure you want to change the base?

Video preview isssue #30324 #31770

Uh oh!

Conversation

gixid192 commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Jan 20, 2021

Uh oh!

gixid192 commented Jan 20, 2021

Uh oh!

gixid192 commented Jan 21, 2021

Uh oh!

gixid192 commented Jan 22, 2021

Uh oh!

gixid192 commented Jan 22, 2021

Uh oh!

gixid192 commented Jan 25, 2021

Uh oh!

danielrussob commented Jan 25, 2021

Uh oh!

magento-deployment-service bot commented Jan 25, 2021

Uh oh!

danielrussob commented Jan 25, 2021

Uh oh!

magento-deployment-service bot commented Jan 25, 2021

Uh oh!

magento-deployment-service bot commented Jan 25, 2021

Uh oh!

magento-deployment-service bot commented Jan 25, 2021

Uh oh!

danielrussob commented Jan 25, 2021

Uh oh!

danielrussob commented Jan 25, 2021

Uh oh!

danielrussob Jan 25, 2021

Choose a reason for hiding this comment

Uh oh!

gixid192 Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

danielrussob Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

gixid192 Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gixid192 Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

sivaschenko commented Jan 26, 2021

Uh oh!

gixid192 commented Jan 27, 2021

Uh oh!

danielrussob commented Feb 2, 2021

Uh oh!

magento-deployment-service bot commented Feb 2, 2021

Uh oh!

gixid192 commented Feb 18, 2021

Uh oh!

Uh oh!

gixid192 commented Jan 20, 2021 •

edited

Loading

gixid192 Jan 27, 2021 •

edited

Loading