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

[IMP] website_sale: showcase product videos on e-commerce #32298

Conversation

dharmrajsinh-jhala
Copy link
Contributor

Task - https://www.odoo.com/web#id=1945483&model=project.task&view_type=form&menu_id=5195

Pad - https://pad.odoo.com/p/r.d8a742a1ff985775a0dae53959b0dd9e

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Apr 1, 2019
@C3POdoo C3POdoo added the RD research & development, internal work label Apr 1, 2019
@dharmrajsinh-jhala dharmrajsinh-jhala force-pushed the master-product-video-ecommerce-dja branch from 2b8d4d2 to e635e5f Compare April 4, 2019 11:31
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Apr 4, 2019
@dharmrajsinh-jhala dharmrajsinh-jhala force-pushed the master-product-video-ecommerce-dja branch from e635e5f to 8681602 Compare April 10, 2019 12:01
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Apr 10, 2019
@dharmrajsinh-jhala dharmrajsinh-jhala force-pushed the master-product-video-ecommerce-dja branch 3 times, most recently from fb03947 to 8e3ddaa Compare April 10, 2019 12:51
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Apr 10, 2019
Copy link
Contributor

@JKE-be JKE-be left a comment

Choose a reason for hiding this comment

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

Here, some first remarks.
If you can take a look, thanks

@@ -17,3 +20,62 @@ class ProductImage(models.Model):

product_tmpl_id = fields.Many2one('product.template', "Product Template", index=True, ondelete='cascade')
product_variant_id = fields.Many2one('product.product', "Product Variant", index=True, ondelete='cascade')
video_url = fields.Char('Video URL',
help='URL of a video for showcasing your product.'
'You can provide URL from Youtube, Instagram, Vimeo, Dailymotion or Youku.')
Copy link
Contributor

Choose a reason for hiding this comment

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

any url is valid now

'''

# To detect if we have a valid URL or not
urlRegex = r'^(http:\/\/|https:\/\/|\/\/)[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?$'
Copy link
Contributor

Choose a reason for hiding this comment

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

embedded part to move into a utility method

validURL = re.search(urlRegex, image.video_url)

if not validURL:
image.embed_code = _('Provided URL is not valid. Please enter a valid URL.')
Copy link
Contributor

Choose a reason for hiding this comment

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

image.embed_code = False

@api.constrains('video_url')
def _check_valid_video_url(self):
for image in self:
if image.video_url:
Copy link
Contributor

Choose a reason for hiding this comment

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

if you have video_url, you enter line 46.
So embed_code was always set by L51 or L74 no?

Now if L51 we st to False, you need to check here '
if not image.embed_code or not ...'

@@ -613,6 +613,8 @@ a.no-decoration {
}

.carousel-control-prev, .carousel-control-next {
height: 70%;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try to use more generic position instead of 70 15 32 28 % ?

Copy link
Contributor

Choose a reason for hiding this comment

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

And have you tried the design as discussed yesterday:

e.g.
image

_render: function () {
this.$el.html(QWeb.render('productVideo', {
isValid: this.value && _.str.startsWith(this.value.trim(), '<iframe'),
embedCode: this.value,
Copy link
Contributor

Choose a reason for hiding this comment

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

if value only

else:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

by default no preview, if no video set

@dharmrajsinh-jhala dharmrajsinh-jhala force-pushed the master-product-video-ecommerce-dja branch from 8e3ddaa to f093555 Compare April 11, 2019 12:10
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Apr 11, 2019
@dharmrajsinh-jhala dharmrajsinh-jhala force-pushed the master-product-video-ecommerce-dja branch from f093555 to 570f410 Compare April 11, 2019 12:28
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Apr 11, 2019
@dharmrajsinh-jhala dharmrajsinh-jhala force-pushed the master-product-video-ecommerce-dja branch from 570f410 to 3ee1652 Compare April 16, 2019 05:08
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Apr 16, 2019
@dharmrajsinh-jhala dharmrajsinh-jhala force-pushed the master-product-video-ecommerce-dja branch 2 times, most recently from 7f9aaad to fded6e4 Compare April 16, 2019 06:18
@dharmrajsinh-jhala dharmrajsinh-jhala marked this pull request as ready for review April 16, 2019 06:22
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Apr 16, 2019
This commit adds a new file 'tools.py' that can contain
utility methods which can be used at more than one places.

For now it contains a method 'get_video_embed_url', that
will accept a URL for the video and return an iframe snippet
that can be embedded if the provided URL is valid (returns
False otherwise).

task-1945483
This commit adds facility to showcase product videos on it's
eCommerce page. They can be added to 'product.image' model,
same as extra images, thus allowing user to enter multiple
videos per template and per variant. Also, associated image
will be used as a thumbnail for the video on the product's
website page.

task-1945483
@dharmrajsinh-jhala dharmrajsinh-jhala force-pushed the master-product-video-ecommerce-dja branch from fded6e4 to b0e1ec8 Compare April 16, 2019 10:36
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Apr 16, 2019
@JKE-be
Copy link
Contributor

JKE-be commented Apr 16, 2019

@robodoo r+ rebase-ff

@JKE-be JKE-be changed the title [IMP] website_sale: Showcase Product Videos on eCommerece [IMP] website_sale: showcase product videos on e-commerce Apr 16, 2019
@robodoo
Copy link
Contributor

robodoo commented Apr 16, 2019

Merge method set to rebase and fast-forward

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Apr 16, 2019
robodoo pushed a commit that referenced this pull request Apr 16, 2019
This commit adds facility to showcase product videos on it's
eCommerce page. They can be added to 'product.image' model,
same as extra images, thus allowing user to enter multiple
videos per template and per variant. Also, associated image
will be used as a thumbnail for the video on the product's
website page.

task-1945483

closes #32298

Signed-off-by: Jérémy Kersten (jke) <jke@openerp.com>
@robodoo
Copy link
Contributor

robodoo commented Apr 16, 2019

Merged, thanks!

@robodoo robodoo closed this Apr 16, 2019
@rdeodoo rdeodoo deleted the master-product-video-ecommerce-dja branch April 16, 2019 13:58
Copy link
Contributor

@seb-odoo seb-odoo left a comment

Choose a reason for hiding this comment

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

Nice job, I like the new possibility.

Few remarks that could be improved in a new commit below.

Also, currently we are required to also upload an image when we set a video... It would be best if the image was filled with a frame of the video by default.

Finally, I'm not sure the issue, but when I tried on this runbot: http://501106-32298-b0e1ec-all.runbot14.odoo.com/shop/product/customizable-desk-9
I added a video there but the page is not loading correctly?

@@ -208,10 +208,25 @@
<h2><field name="name" placeholder="Image Name"/></h2>
<!-- Unfortunately for now we can't drag and drop kanban o2m, so we have to let the user input the sequence manually. -->
<label for="sequence" string="Sequence"/><br/>
<field name="sequence"/>
<field name="sequence"/><br/><br/>
<label for="sequence" string="Video URL"/><br/>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be label for=video_url I believe.

@@ -3,6 +3,7 @@
<!-- Layout and common templates -->
<template id="assets_backend" inherit_id="web.assets_backend">
<xpath expr="." position="inside">
<script type="text/javascript" src="/website_sale/static/src/js/website_sale_video_field_preview.js"></script>

This comment was marked as outdated.

@@ -102,7 +102,7 @@
<field name="website_style_ids" widget="many2many_tags" groups="base.group_no_one"/>
</group>
</group>
<group name="product_template_images" string="Extra Product Images">
<group name="product_template_images" string="Extra Product Images / Videos">
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally the name on the o2m on the product/template field should be renamed too (it is the title of the modal when adding a new video).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI 🤖 Robodoo has seen passing statuses RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants