[WIP] 9.0 website_base_multi_image #252

sergio-teruel · 2016-09-07T11:17:55Z

This module extends the functionality of 'base_multi_image' module to show image data in website

Port from v8

@Tecnativa

sergio-teruel · 2016-09-07T11:21:20Z

This PR will be merged with website_multi_image when it has been ported to 9.0

… post form

rafaelbn · 2016-09-10T06:29:59Z

Hi @sergio-teruel , travis fails

yajo

Good start, thanks!

yajo · 2016-09-15T15:17:55Z

website_base_multi_image/README.rst

+
+.. image:: https://odoo-community.org/website/image/ir.attachment/5784_f2813bd/datas
+   :alt: Try me on Runbot
+   :target: https://runbot.odoo-community.org/runbot/{id}


Put right id

yajo · 2016-09-15T15:18:06Z

website_base_multi_image/README.rst

+========================
+
+This module extends the functionality of 'base_multi_image' module
+to show winery data in website


yajo · 2016-09-15T15:18:14Z

website_base_multi_image/README.rst

+Bug Tracker
+===========
+
+Bugs are tracked on `GitHub Issues <https://github.com/OCA/xxx/issues>`_.


yajo · 2016-09-15T15:18:35Z

website_base_multi_image/README.rst

+Bugs are tracked on `GitHub Issues <https://github.com/OCA/xxx/issues>`_.
+In case of trouble, please check there if your issue has already been reported.
+If you spotted it first, help us smashing it by providing a detailed and welcomed feedback
+`here <https://github.com/OCA/website/issues/new?body=module:%mypopwine_website_portal_purchase_winery%0Aversion:%209.0%0A%0A**Steps%20to%20reproduce**%0A-%20...%0A%0A**Current%20behavior**%0A%0A**Expected%20behavior**>`_.


Use newer template.

yajo · 2016-09-15T15:18:44Z

website_base_multi_image/README.rst

+mission is to support the collaborative development of Odoo features and
+promote its widespread use.
+
+To contribute to this module, please visit http://odoo-community.org.


yajo · 2016-09-15T15:45:43Z

website_base_multi_image/static/src/xml/website_multi_image.xml

+
+    <t t-name="website_base_multi_image.image_preview">
+        <img id="image_preview"
+             class="img img-responsive product_detail_img"


product? is this not a base module?

yajo · 2016-09-15T15:45:56Z

website_base_multi_image/static/src/xml/website_multi_image.xml

+
+    <t t-name="website_base_multi_image.image_preview_thumb">
+        <div class="col-md-3 mt8 mb8">
+            <img class="img img-responsive winery-image-thumb"


yajo · 2016-09-15T15:46:23Z

website_base_multi_image/static/src/xml/website_multi_image.xml

+        </div>
+    </t>
+
+</templates>


yajo · 2016-09-15T15:47:48Z

website_base_multi_image/views/assets.xml

+<template id="assets_frontend" inherit_id="website.assets_frontend">
+    <xpath expr=".">
+        <link rel="stylesheet"
+              href="/website_base_multi_image/static/src/css/theme.css"/>


This is not a theme, could you change the file name?

yajo · 2016-09-15T15:48:21Z

website_base_multi_image/views/layout.xml

+<odoo>
+
+<template id="base_multi_image_fields">
+    <t t-set="owner_model" t-value="owner_model or 'product.template'"/>


This is base module, so add no defaults please. Let it die if variable is missing.

…ain image

yajo

I understand this is a base addon... Do you think it could include some demo data to see it working?

yajo · 2016-09-19T08:45:01Z

website_base_multi_image/views/layout.xml

+
+<template id="image_main">
+    <label>Image</label>
+    <div class="carousel-inner container" style="padding: 0;">


No inline styles please.

yajo · 2016-09-19T08:46:38Z

website_base_multi_image/static/src/css/style.css

+    width: 130px;
+    float: left;
+    margin-top: 5px;
+}*/


Do not leave useless code around please.

yajo · 2016-09-19T08:46:57Z

website_base_multi_image/controllers/main.py

+        result = {}
+        try:
+            secondary_images.write({'sequence': 20})
+        except Exception, e:


Exception as e

yajo · 2016-09-19T08:54:29Z

website_base_multi_image/controllers/main.py

+        ])
+        result = {}
+        try:
+            secondary_images.write({'sequence': 20})


I think you can drop this controller and use /web/dataset/resequence instead.

yajo · 2016-09-19T08:59:11Z

website_base_multi_image/controllers/main.py

+    #------------------------------------------------------
+    # remove images
+    #------------------------------------------------------
+    @http.route('/website/image/remove', type='json', auth='user')


This one controller could be replaced by a JS call to unlink too. Not so hard to do, and I believe it's good to get used to use built-in standard RPC methods instead of creating a controller for everything.

lasley · 2016-09-20T17:30:48Z

website_base_multi_image/models/image.py

+        @return: List of images to add
+        """
+        images_to_add = []
+        for key in post.keys():


Why not for key, image in post.iteritems(). IMO it would make the list check and new_image assignment block much cleaner.

lasley · 2016-09-20T17:31:45Z

website_base_multi_image/models/image.py

+        """
+        images_to_add = []
+        for key in post.keys():
+            if 'new_image_' in key:


key.startswith('new_image_') would better match with the documented/intended logic

lasley · 2016-09-20T17:32:57Z

website_base_multi_image/models/image.py

+                    'file_db_store': base64.encodestring(new_image.read()),
+                    'owner_model': model_name,
+                    'name': new_image.filename,
+                    'extension': 'jpg',


mimetype?

Sorry, but base_multi_image.image does not have mimetype field.

I think @lasley means that you should guess the mimetype from the incoming bytes and then choose the right extension instead of hardcoding jpg, right?

lasley · 2016-09-20T17:33:18Z

website_base_multi_image/static/src/css/style.css

+}
+
+#base_multi_image .carousel-indicators .active {
+   /*display: none;*/


Please remove this (see @yajo useless code comment)

lasley · 2016-09-20T17:35:05Z

website_base_multi_image/static/src/js/website_multi_image.js

+var _t = core._t;
+var $ = require("$");
+
+animation.registry.website_base_multi_image =


IMO this is really hard to read with 3 namespaces in the same tab level

lasley · 2016-09-20T17:38:06Z

website_base_multi_image/static/src/js/website_multi_image.js

+        });
+    },
+    image_preview: function (event) {
+        var image = event.target.files[0];


event.currentTarget

lasley · 2016-09-20T17:38:17Z

website_base_multi_image/static/src/js/website_multi_image.js

+        reader.readAsDataURL(image);
+    },
+    reader_load: function (event) {
+        var filename = event.target.fileName;


event.currentTarget

lasley · 2016-09-20T17:40:29Z

website_base_multi_image/static/src/js/website_multi_image.js

+        e.stopPropagation();
+        var self = this;
+        self.notify_clear();
+        var $a = $(e.target);


e.currentTarget

lasley · 2016-09-20T17:41:51Z

website_base_multi_image/static/src/js/website_multi_image.js

+    image_make_main: function(e){
+        var self = this;
+        self.notify_clear();
+        var $a = $(e.target);


e.currentTarget

lasley · 2016-09-20T17:43:49Z

website_base_multi_image/static/src/xml/website_multi_image.xml

+        </div>
+    </t>
+
+</templates>


Add newline

This seems fixed.

lasley · 2016-09-20T17:45:39Z

Thanks for the submission @sergio-teruel - comments inline

yajo

security flaw detected 🔓 😊

yajo · 2016-09-21T08:20:35Z

website_base_multi_image/security/ir.model.access.csv

@@ -0,0 +1,3 @@
+id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink
+access_portal_supplier_base_multi_image,access_portal_supplier_base_multi_image,base_multi_image.model_base_multi_image_image,base.group_portal,1,1,1,0


So any portal users can modify any image? 👎

write/create/unlink perms must be ir.rule-constrained, and I guess that should be made per submodule, so delete this please.

…eate and edit ACL's

sergio-teruel · 2016-09-21T15:45:32Z

@lasley @yajo thanks for review, changes done but it is work in progress... 😄

yajo · 2016-09-22T07:53:17Z

website_base_multi_image/security/ir.model.access.csv

@@ -1,3 +1,3 @@
 id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink
-access_portal_supplier_base_multi_image,access_portal_supplier_base_multi_image,base_multi_image.model_base_multi_image_image,base.group_portal,1,1,1,0
+access_portal_base_multi_image,access_portal_base_multi_image,base_multi_image.model_base_multi_image_image,base.group_portal,1,0,0,0
 access_public_base_multi_image,access_public_base_multi_image,base_multi_image.model_base_multi_image_image,base.group_public,1,0,0,0


You could use a single rule for this, with no group.
BTW keep in mind you are giving access to every multi image in the database. No matter if it belongs to a partner, product, human resource, or whatever...
Given this is a website_base module, I think it would be a good idea to add a "published" button to the model, and a ir.rule that constrains public access to published images.

rafaelbn · 2016-10-04T09:28:44Z

Hi all, @sergio-teruel what's pending to this module?

sergio-teruel · 2016-10-04T09:38:16Z

@rafaelbn is pending to do a zoom controller.

rafaelbn · 2016-11-11T23:29:26Z

Hi @sergio-teruel , are the changes requested by @lasley and @yajo done?

lasley

My comments look attended to, and functional seems good.

IMO we should add zoom controller, and whatever else is left in terms of featureset to roadmap in the interest of getting merge.

Although more test coverage would be nice 😉

yajo

Many improvements required, including security ones. I know this is WIP, but at least you know now where to start. Good work in general OTOH 😉.

yajo · 2016-11-14T09:08:42Z

website_base_multi_image/models/image.py

+                    'file_db_store': base64.encodestring(new_image.read()),
+                    'owner_model': model_name,
+                    'name': new_image.filename,
+                    'extension': 'jpg',


I think @lasley means that you should guess the mimetype from the incoming bytes and then choose the right extension instead of hardcoding jpg, right?

yajo · 2016-11-14T09:09:03Z

website_base_multi_image/README.rst

+
+To use this module, you need to:
+
+#. Go to ...


yajo · 2016-11-14T09:09:45Z

website_base_multi_image/demo/website_base_multi_image_demo.xml

@@ -0,0 +1,29 @@
+<?xml version="1.0" encoding="utf-8"?>
+<odoo>
+    <data noupdate="0">


yajo · 2016-11-14T09:10:58Z

website_base_multi_image/models/image.py

+    @api.model
+    def get_images_to_add(self, post, model_name):
+        """
+        @param post: dict with images in format 'new_image_xxx'


'new_image_xxx' is not a dict format nor an image format, I don't understand this doc line. Besides, add a general description for what the method does in its 1st line please.

yajo · 2016-11-14T09:12:22Z

website_base_multi_image/models/image.py

+                else:
+                    new_image = image
+                images_to_add.append((0, 0, {
+                    'storage': 'db',


You could get this from context (self.env.context.get("default_storage", "db")) or from a method argument. Not a blocker.

yajo · 2016-11-14T09:18:36Z

website_base_multi_image/security/ir.model.access.csv

@@ -0,0 +1,3 @@
+id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink
+access_portal_base_multi_image,access_portal_base_multi_image,base_multi_image.model_base_multi_image_image,base.group_portal,1,0,0,0
+access_public_base_multi_image,access_public_base_multi_image,base_multi_image.model_base_multi_image_image,base.group_public,1,0,0,0


You are still giving access to public and portal users to every image in the database.

As this is right now, this is a 👎 blocking security issue. I think the best fix you could apply is something similar to what I do here for base_custom_info: giving access to images by checking access to their owners.

That IMP should go direclty into base_multi_image, BTW.

yajo · 2016-11-14T09:19:20Z

website_base_multi_image/static/src/css/style.css

+    height: 0px;
+    width: 110px;
+    margin-left: 5px;
+}


All this line would become more readable/reusable if written into less/sass. Not blocking 😉

yajo · 2016-11-14T09:20:03Z

website_base_multi_image/static/src/xml/website_multi_image.xml

+        </div>
+    </t>
+
+</templates>


This seems fixed.

yajo · 2016-11-14T09:20:52Z

website_base_multi_image/views/layout.xml

+    <div class="carousel-inner container" style="padding: 0;">
+        <t t-foreach="srcs" t-as="src">
+            <div t-attf-class="item #{src_index == index and 'active' or ''}"
+                 t-attf-data-slide-to="#{src_index}">


I'd use t-att-data-slide-to="src_index" instead.

rafaelbn · 2016-12-16T01:11:19Z

Hi @sergio-teruel about #252 (comment) zoom controller, I think better to add that issue to known issues and merger this as is. Still pending yajo's comments but IMHO this is OK

rafaelbn · 2017-06-09T00:32:25Z

@yajo please could you review again ? If any change needed it could be very nice is you make it directly. Thanks for help.

pedrobaeza · 2017-07-21T11:07:47Z

@sergio-teruel please attend @yajo's comments.

pedrobaeza · 2018-02-23T11:39:37Z

Closing as the project is no longer active and v10 includes native support for this.

sergio-teruel added 3 commits July 18, 2016 11:35

[WIP][9.0] website_base_multi_image: Develop

850e460

[WIP][9.0] website_base_multi_image: Fix image main width and height

ae9103a

[WIP][9.0] website_base_multi_image: Adds public group ACL's

52c1e0b

sergio-teruel added 2 commits September 7, 2016 17:42

[WIP][9.0] website_base_multi_image: Create method to add images from…

e70a9c4

… post form

[WIP][9.0] website_base_multi_image: Render image if exists

ee2d903

rafaelbn added the needs review label Sep 10, 2016

rafaelbn added this to the 9.0 milestone Sep 10, 2016

rafaelbn added work in progress and removed needs review labels Sep 10, 2016

yajo requested changes Sep 15, 2016

View reviewed changes

[WIP][9.0] website_base_multi_image: Allow delete images and select m…

4ff4a7a

…ain image

yajo requested changes Sep 19, 2016

View reviewed changes

[WIP][9.0] website_base_multi_image: Remove unnecessary controllers

233dcf2

sergio-teruel force-pushed the 9.0-PR-website_base_multi_image branch from e3a54fd to 233dcf2 Compare September 19, 2016 19:56

[WIP][9.0] website_base_multi_image: Add layout with images on bottom

2ebdc95

lasley suggested changes Sep 20, 2016

View reviewed changes

yajo requested changes Sep 21, 2016

View reviewed changes

[WIP][9.0] website_base_multi_image: Adds demo page. Delete write, cr…

9c55698

…eate and edit ACL's

yajo requested changes Sep 22, 2016

View reviewed changes

[WIP][9.0] website_base_multi_image: Allow hidden section labels

67f1d36

lasley approved these changes Nov 12, 2016

View reviewed changes

yajo requested changes Nov 14, 2016

View reviewed changes

rafaelbn added the needs review label Dec 16, 2016

rafaelbn removed the work in progress label Dec 16, 2016

rafaelbn approved these changes Dec 16, 2016

View reviewed changes

yajo self-assigned this Jun 9, 2017

andriusdaunoravicius approved these changes Jul 21, 2017

View reviewed changes

pedrobaeza closed this Feb 23, 2018

		@@ -0,0 +1,3 @@
		id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink
		access_portal_supplier_base_multi_image,access_portal_supplier_base_multi_image,base_multi_image.model_base_multi_image_image,base.group_portal,1,1,1,0

[WIP] 9.0 website_base_multi_image #252

[WIP] 9.0 website_base_multi_image #252

Conversation

sergio-teruel commented Sep 7, 2016 • edited by rafaelbn

sergio-teruel commented Sep 7, 2016

rafaelbn commented Sep 10, 2016

yajo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yajo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasley Sep 20, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasley Sep 20, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasley commented Sep 20, 2016

yajo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergio-teruel commented Sep 21, 2016

yajo Sep 22, 2016 • edited

Choose a reason for hiding this comment

rafaelbn commented Oct 4, 2016

sergio-teruel commented Oct 4, 2016

rafaelbn commented Nov 11, 2016

lasley left a comment • edited

Choose a reason for hiding this comment

yajo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yajo Nov 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelbn commented Dec 16, 2016

rafaelbn commented Jun 9, 2017

pedrobaeza commented Jul 21, 2017

pedrobaeza commented Feb 23, 2018

sergio-teruel commented Sep 7, 2016 •

edited by rafaelbn

lasley Sep 20, 2016 •

edited

lasley Sep 20, 2016 •

edited

yajo Sep 22, 2016 •

edited

lasley left a comment •

edited

yajo Nov 14, 2016 •

edited