Migration to version 11.0 by bizzappdev · Pull Request #12 · OCA/apps-store

bizzappdev · 2018-09-05T10:22:52Z

Migration to version 11.0 as per the request at #4

oca-clabot · 2018-09-05T10:22:54Z

Hey @bizzappdev, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/website.cla
Here is a list of the users:

Divya Modi (no github login found)

Appreciation of efforts,
OCA CLAbot

bizzappdev · 2018-09-05T12:18:57Z

@elicoidal here is a PR for the v11.
Also, I would like to inform that BizzAppDev Company did the ECLA so IMO all the commits from @bizzappdev.com have to have the CLA. I man not sure. let me know if I have to do something special.

there was one bug/error (from interface-github) which was causing the Odoo module/version do not have the icon so the products did not have Icons.

the Bug will be solved once the OCA/interface-git#19 will be approved and merged.

elicoidal · 2018-09-06T00:56:35Z

@bizzappdev thanks for the PR.
For the ECLA, I will have a look but I need a github login to add it to your account (it is missing from the bot message).
cc @gdgellatly @OCA/board review welcome

elicoidal

@bizzappdev Thanks for your contribution!
No real technical review on my side. Some questions though

elicoidal · 2018-09-06T00:57:39Z

@@ -0,0 +1,27 @@
+# Copyright 2017 BizzAppDev


add 2018 in all copyrights

elicoidal · 2018-09-06T00:58:21Z

@@ -0,0 +1,88 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<odoo>
+    <data>


no need <data>

add copyright

elicoidal · 2018-09-06T01:05:40Z

+
+    @api.multi
+    def generate_zip_file(self):
+        product1 = self.env.ref('apps_download.product_product_100')


I am a little bit confused by this piece of code: what do you intend to do with the tests data?
I would suggest at least some docstrings to the function and short comments in the code.

elicoidal · 2018-09-06T01:07:50Z

@@ -0,0 +1,4 @@
+


remove line

elicoidal · 2018-09-06T01:13:51Z

+            },
+            {
+                content: "Select Author",
+                trigger: ".dropdown_author_by .dropdown-menu a:contains('OCA')",


I prefer not to have OCA hard-coded.
We should set up somewhere the organization and use it here

elicoidal · 2018-09-06T01:15:49Z

+<?xml version="1.0" encoding="UTF-8"?>
+<odoo>
+    <data>
+        <template id="assets_frontend" name="Website Apps Store" inherit_id="website.assets_frontend">


please improve readbility with one line per attribute

elicoidal · 2018-09-06T01:16:11Z

+            <xpath expr='//a[hasclass("js_check_product")]' position="attributes">
+                <attribute name="t-if">not product.odoo_module_id</attribute>
+            </xpath>
+            <!--xpath expr="//hr" position="attributes">


remove dead code

bizzappdev · 2018-09-06T15:45:23Z

@elicoidal All requests have been taken care.

bizzappdev · 2018-09-06T15:46:03Z

might be @pedrobaeza or @RoelAdriaans can do a technical review?

elicoidal · 2018-09-06T23:35:36Z

@OCA/board @gdgellatly Technical review welcome.

max3903

@elicoidal Apart from splitting the README.rst, LGTM
👍

yvaucher

Hello,

Thanks for the porting.

I'm a bit concerned about the purpose of apps_download module. It seems like it is a Proof of concept with demo data. What is in folder demo is not loaded as demo but really seems to be demo data.

I would advise to put the load of this file under demo key and make sure the method can be used without thoses data.

Otherwise you will find few other tiny remarks on the python code.

yvaucher · 2018-09-14T13:06:17Z

+    ],
+    "summary": "Product Download for Appstore",
+    "data": [
+        'demo/product_template_demo.xml',


Shouldn't this be under demo key loading this setup only in case of an instance with without_demo=False ?

yvaucher · 2018-09-14T13:27:59Z

            if self._check_related_to_module(product):
                attribute = self._get_version_attribute(values)
-                version = self._get_version_with_attribute(
+                version_ids = self._get_version_with_attribute(


Here you renamed version to version_ids but _get_version_with_attribute returns you a record set.
To not mix with actual integer ids, I suggest you to rename version_ids to versions

yvaucher · 2018-09-14T13:28:30Z

-                    'odoo_module_version_id': version.id,
-                })
+                for version in version_ids:
+                    module_id = version.module_id


s/module_id/module

It preferable to not use suffix _id when the variable is a record set.

yvaucher · 2018-09-14T13:44:49Z

+            url = "/shop/category/%s" % slug(category)
+
+        category_all = request.env['product.public.category'].search([])
+        versions = request.env['product.attribute.value'].search([])


Shouldn't this be filtered by attribute_id = ref('apps_product_creator.attribute_odoo_version') ? If an other product.attribute was created it would be confusing. product.attribute can be a lot of things.

elicoidal · 2018-09-17T00:27:57Z

+-->
+<odoo>
+        <record id="ir_cron_scheduler_generate_zip" model="ir.cron">
+            <field name="name">Generate all product zip</field>


Generate ZIP files for all products

elicoidal · 2018-09-17T00:30:32Z

+                    <page string="Module Dependency">
+                        <separator string="Modules Dependency" colspan="4"/>
+                        <field name="dependent_product_ids" colspan="4" nolabel="1"/>
+                        <button string="Get all Dependency" type="object" name="generate_zip_file"/>


Dependencies

elicoidal · 2018-09-17T00:32:15Z

+var base = require("web_editor.base");
+
+    Tour.register('download_zip', {
+        name: "Download Product Zip",


Download ZIP file

elicoidal · 2018-09-18T00:01:33Z

@bizzappdev have you had a chance to check the review?
@gdgellatly do we have a possibility to test it in our newly migrated v11?

gdgellatly · 2018-09-18T07:13:51Z

@elicoidal I think @sbidoul and @pedrobaeza are organizing the test instance. Just been a bit delayed with holidays.

sbidoul · 2018-09-18T09:38:27Z

@gdgellatly the test environment is ready now.

bizzappdev · 2018-09-18T09:41:14Z

@elicoidal all the requested changes are done.

elicoidal · 2018-09-18T10:20:37Z

@yvaucher could you please have a second look?

yvaucher · 2018-09-19T18:58:39Z

@bizzappdev I added more question on your reply, I get that you need some setup for the PhantomJS tests, but it seems it adds an action and a cron that won't work as they can only launch the test version of the method, or is it supposed to be overwritten somewhere?

Don't hesitate also to write a more complete Pull request's description if some context must be known by the reviewers.

yvaucher

Here is an example on how to mock the definition of the module_path for your test purpose.

Didn't tested it but should give you a proper way to do it.

yvaucher · 2018-09-20T12:07:57Z

        super(TestUi, self).setUp()
        self.tour = "odoo.__DEBUG__.services['web_tour.tour']"

-    def test_download_zip_tour(self):


You probably want to do the following:

In generate_zip_files extract this in an other method name _get_module_path:

module_path = product.module_path + '/'\ + product.odoo_module_version_id.technical_name

to get it like that:

module_path = product._get_module_path()

Then you need to mock this method to return your special test path:

[...] import mock mock_get_module_path = ( 'openerp.addons.apps_download.models.product.ProductProduct._get_module_path' ) # it might be odoo.addons in v11 to check [...] def test_download_zip_tour(self): with mock.patch(mock_get_module_path) as mock_func: mock_func.return_value = <your test path string> self.phantom_js('/shop', ...)

bizzappdev · 2018-09-20T16:02:11Z

@yvaucher Thank you so much, I was not aware of the mock. we have implemented that as per your example.

bizzappdev · 2018-09-24T08:23:23Z

@yvaucher anything else has to be taken care?

yvaucher · 2018-09-28T14:13:22Z

@bizzappdev thanks for the change, LGTM

elicoidal · 2018-09-28T15:57:26Z

Thanks all!
We will set it up during the code sprint

bizzappdev · 2018-09-29T09:15:11Z

@elicoidal yes sure, that will be great. I will be also around.

Divya Modi added 3 commits August 31, 2018 19:26

[WIP]partial commit for migration of apps-store

f11ab4c

[FIX]small changes

2d82bd9

[FIX]remove console from js file

461897a

pedrobaeza added this to the 11.0 milestone Sep 5, 2018

[FIX]remove coding from files

34ba90c

[FIX]correct version number

2659eee

elicoidal added the needs review label Sep 6, 2018

elicoidal reviewed Sep 6, 2018

View reviewed changes

[FIX] Changes as per suggested

1c9c898

max3903 approved these changes Sep 10, 2018

View reviewed changes

yvaucher requested changes Sep 14, 2018

View reviewed changes

elicoidal reviewed Sep 17, 2018

View reviewed changes

[FIX] add changes as per requested

c1fa5c2

[FIX] remove phantomjs for download zip file

7ec2242

yvaucher requested changes Sep 20, 2018

View reviewed changes

yvaucher added the needs fixing label Sep 20, 2018

Divya Modi and others added 2 commits September 20, 2018 19:26

[FIX] made changes as per suggested

94bfc00

[FIX] solve problem of travis for flake-8

eaf0f19

[FIX]Proper path for the phantomjs

dc39ebf

yvaucher approved these changes Sep 28, 2018

View reviewed changes

yvaucher merged commit 2317ae6 into OCA:11.0 Sep 28, 2018

pedrobaeza mentioned this pull request Sep 28, 2018

Migration to version 11.0 #4

Closed

2 tasks

elicoidal mentioned this pull request Sep 29, 2018

[RFC] Apps Store front end #7

Closed

Uh oh!

Conversation

bizzappdev commented Sep 5, 2018

Uh oh!

oca-clabot commented Sep 5, 2018

Uh oh!

bizzappdev commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elicoidal commented Sep 6, 2018

Uh oh!

elicoidal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bizzappdev commented Sep 6, 2018

Uh oh!

bizzappdev commented Sep 6, 2018

Uh oh!

elicoidal commented Sep 6, 2018

Uh oh!

max3903 left a comment

Choose a reason for hiding this comment

Uh oh!

yvaucher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elicoidal commented Sep 18, 2018

Uh oh!

gdgellatly commented Sep 18, 2018

Uh oh!

sbidoul commented Sep 18, 2018

Uh oh!

bizzappdev commented Sep 18, 2018

Uh oh!

elicoidal commented Sep 18, 2018

Uh oh!

yvaucher commented Sep 19, 2018

Uh oh!

yvaucher left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

bizzappdev commented Sep 20, 2018

bizzappdev commented Sep 5, 2018 •

edited

Loading

yvaucher left a comment •

edited

Loading

yvaucher commented Sep 28, 2018 •

edited

Loading