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

[17.0] [MIG] website_product_configurator: Migration to 17.0 #143

Open
wants to merge 267 commits into
base: 17.0
Choose a base branch
from

Conversation

bizzappdev
Copy link
Contributor

Dependency MR for module product_configurator #141
Dependency MR for module product_configurator_sale #142

PCatinean and others added 30 commits November 8, 2024 19:08
…igurator_mrp : correct method-name, claas-name, arguments
…, replace request.website.render with request.render
…production_lots, fix singleton error(product.config.step.line)
… website_product_configurator_mrp to uninstallable list
…pper function in session for next step and redirect to step if any incomplete step is there
@bizzappdev bizzappdev force-pushed the 17.0-mig-website_product_configurator-BAD branch 3 times, most recently from 1b752da to 92a1065 Compare November 22, 2024 05:40
@bizzappdev bizzappdev marked this pull request as ready for review November 22, 2024 05:46
@bizzappdev bizzappdev force-pushed the 17.0-mig-website_product_configurator-BAD branch from 92a1065 to 6e04543 Compare November 22, 2024 07:03
@rousseldenis
Copy link

/ocabot migration website_product_configurator

@bizzappdev bizzappdev force-pushed the 17.0-mig-website_product_configurator-BAD branch from 6e04543 to 1542af4 Compare November 25, 2024 09:12
Copy link

@ivantodorovich ivantodorovich left a comment

Choose a reason for hiding this comment

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

Thanks for the work done!

I've a few minor comments, most of which are not blocking

def setUp(self):
super(TestProductConfiguratorValues, self).setUp()
super().setUp()

Choose a reason for hiding this comment

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

We should change this to setUpClass instead.
Looks like the base class is doing it already: https://github.com/OCA/product-configurator/blob/e78af0c423193a6e466f22567377fdcb0e1b9e46/product_configurator/tests/common.py#L5C1-L7C29

@@ -5,11 +5,17 @@

class TestSaleOrder(TestProductConfiguratorValues):
def setUp(self):
super(TestSaleOrder, self).setUp()
super().setUp()

Choose a reason for hiding this comment

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

We should change this to setUpClass instead.

@@ -3,7 +3,7 @@

class TestResConfigSettings(TransactionCase):
def setUp(self):
super(TestResConfigSettings, self).setUp()
super().setUp()

Choose a reason for hiding this comment

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

We should change this to setUpClass instead.

@@ -0,0 +1,629 @@
/** @odoo-module **/

import Dialog from "@web/legacy/js/core/dialog";

Choose a reason for hiding this comment

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

nitpicking / not-blocking

This legacy Dialog is deprecated, and completely removed in 18.

IMO we should migrate to the new @web/core/dialog/dialog.


init: function () {
this._super.apply(this, arguments);
this.config_form = $("#product_config_form");

Choose a reason for hiding this comment

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

IMO this should be in start rather than init

And ideally we should use either vanilla-js or this.$

Comment on lines +89 to +90
-webkit-filter: brightness(90%) contrast(110%) grayscale(50%) opacity(50%);
-moz-filter: brightness(90%) contrast(110%) grayscale(50%) opacity(50%);

Choose a reason for hiding this comment

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

Suggested change
-webkit-filter: brightness(90%) contrast(110%) grayscale(50%) opacity(50%);
-moz-filter: brightness(90%) contrast(110%) grayscale(50%) opacity(50%);

not needed

Comment on lines +78 to +79
-webkit-filter: brightness(90%) contrast(110%) grayscale(50%) opacity(50%);
-moz-filter: brightness(90%) contrast(110%) grayscale(50%) opacity(50%);

Choose a reason for hiding this comment

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

Suggested change
-webkit-filter: brightness(90%) contrast(110%) grayscale(50%) opacity(50%);
-moz-filter: brightness(90%) contrast(110%) grayscale(50%) opacity(50%);

not needed

Comment on lines +71 to +72
-webkit-filter: none;
-moz-filter: none;

Choose a reason for hiding this comment

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

Suggested change
-webkit-filter: none;
-moz-filter: none;

not needed

Comment on lines +64 to +65
-webkit-filter: none;
-moz-filter: none;

Choose a reason for hiding this comment

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

Suggested change
-webkit-filter: none;
-moz-filter: none;

not needed

@@ -0,0 +1,3 @@
#Odoo Product Configurator

This module facilitates to configure product on website.

Choose a reason for hiding this comment

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

Could you replace this with proper "readme section files" under a readme directory? 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants