Skip to content

Video preview isssue #30324 #31770

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: 2.4-develop
Choose a base branch
from
Open

Video preview isssue #30324 #31770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1be4239
Merge pull request #1 from magento/2.4-develop
gixid192 Jan 20, 2021
af8943f
fix old video preview thumbnail in add video modal
Jan 20, 2021
008597b
Merge branch '2.4-develop' into video-preview-isssue-#30324
gixid192 Jan 21, 2021
8f2a916
format code
gixid192 Jan 22, 2021
99eae8d
fix jscs rules
Jan 22, 2021
7e2ea61
update this
Jan 22, 2021
a0e2926
fix race condition saving video thumbnail
Feb 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions app/code/Magento/ProductVideo/view/adminhtml/web/js/get-video-information.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,10 +350,14 @@ define([

_VIDEO_URL_VALIDATE_TRIGGER: 'validate_video_url',

_CANCEL_VIDEO_INFORMATION_TRIGGER: 'cancelled_video_information',

_videoInformation: null,

_currentVideoUrl: null,

_shouldCancelVideoRequest: false,

/**
* @private
*/
Expand All @@ -366,6 +370,11 @@ define([
}, this
));
this.element.on(this._VIDEO_URL_VALIDATE_TRIGGER, $.proxy(this._onUrlValidateHandler, this));
this.element.on(this._CANCEL_VIDEO_INFORMATION_TRIGGER, $.proxy(
function () {
this._shouldCancelVideoRequest = true;
}, this
));
},

/**
Expand Down Expand Up @@ -395,6 +404,8 @@ define([
id,
googleapisUrl;

this._shouldCancelVideoRequest = false;

if (this._currentVideoUrl === url) {
return;
}
Expand Down Expand Up @@ -471,6 +482,10 @@ define([
return;
}

if (this._shouldCancelVideoRequest) {
return;
}

tmp = data.items[0];
uploadedFormatted = tmp.snippet.publishedAt.replace('T', ' ').replace(/\..+/g, '');
respData = {
Expand Down Expand Up @@ -502,6 +517,10 @@ define([

return null;
}

if (this._shouldCancelVideoRequest) {
return;
}
tmp = data;
respData = {
duration: this._formatVimeoDuration(tmp.duration),
Expand Down
9 changes: 9 additions & 0 deletions app/code/Magento/ProductVideo/view/adminhtml/web/js/new-video-dialog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,8 @@ define([

_gallery: null,

_isUpdatingVideo: false,

/**
* Bind events
* @private
Expand Down Expand Up @@ -335,6 +337,7 @@ define([
_onGetVideoInformationSuccess: function (e, data) {
var self = this;

this._isUpdatingVideo = true;
self.element.on('finish_update_video finish_create_video', $.proxy(function (element, playerData) {
if (!self._onlyVideoPlayer ||
!self._isEditPage && playerData.oldVideoId !== playerData.newVideoId ||
Expand Down Expand Up @@ -395,6 +398,9 @@ define([
data: 'remote_image=' + sourceUrl,
type: 'post',
success: $.proxy(function (result) {
if (!self._isUpdatingVideo) {
return;
}
this._tempPreviewImageData = result;
this._getPreviewImage().attr('src', sourceUrl).show();
this._blockActionButtons(false, true);
Expand Down Expand Up @@ -722,6 +728,7 @@ define([
modalTitleElement = modal.find('.modal-title');

if (!file) {
widget._videoUrlWidget.trigger('cancelled_video_information');
widget._blockActionButtons(true);

modal.find('.video-delete-button').hide();
Expand Down Expand Up @@ -1097,12 +1104,14 @@ define([

this._isEditPage = true;
this.imageData = null;
this._isUpdatingVideo = false;

if (this._previewImage) {
this._previewImage.remove();
this._previewImage = null;
}
this._tempPreviewImageData = null;
this._videoUrlWidget.trigger('cancelled_video_information');
Copy link

Choose a reason for hiding this comment

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

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @danielrussob,

  1. I'm not sure how to write test for this case. Could you suggest some ideas?
  2. 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.

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@gixid192 gixid192 Jan 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

this.element.trigger('reset');
newVideoForm = this.element.find(this._videoFormSelector);

Expand Down