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_sale_product_attribute_value_filter_existing: Migration to 17.0 #925

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

Conversation

lef-adhoc
Copy link
Contributor

No description provided.

@lef-adhoc lef-adhoc force-pushed the 17.0-mig-website_sale_product_attribute_value_filter_existing branch 4 times, most recently from d5e2cfd to 4f5f7cd Compare May 21, 2024 20:10
@lef-adhoc lef-adhoc mentioned this pull request May 21, 2024
27 tasks
@lef-adhoc lef-adhoc force-pushed the 17.0-mig-website_sale_product_attribute_value_filter_existing branch 2 times, most recently from ba27c27 to 846fa13 Compare May 24, 2024 18:09
@lef-adhoc lef-adhoc force-pushed the 17.0-mig-website_sale_product_attribute_value_filter_existing branch from 846fa13 to 5e39b07 Compare June 19, 2024 12:36
@lef-adhoc
Copy link
Contributor Author

@pilarvargas-tecnativa Hi, may you check this?

@pilarvargas-tecnativa
Copy link
Contributor

thanks!

@chienandalu can you take a look?

@lef-adhoc
Copy link
Contributor Author

@chienandalu Hi, do you have any news about this?

@theangryangel
Copy link
Member

LGTM, functionally tested.

@pilarvargas-tecnativa
Copy link
Contributor

@lef-adhoc please check pre-commit

@theangryangel
Copy link
Member

theangryangel commented Jul 19, 2024

Apologies, I have given a positive review, however, it seems that there is a caching issue that we had missed. One of our team is investigating to see if they can assist :)

@AndrzejGerasimukARCHIMEDES

@theangryangel
Hello

Any luck with resolving caching issue ?

Can You share in details what kind of problem did You find?

@theangryangel
Copy link
Member

We ended up needing to go a different direction to meet the customer's need, and I don't think we've come back to look at this. @GarethGloSystems can you remind me what we ran into?

@lef-adhoc lef-adhoc force-pushed the 17.0-mig-website_sale_product_attribute_value_filter_existing branch from 5e39b07 to 90c4906 Compare September 9, 2024 13:10
@lef-adhoc
Copy link
Contributor Author

@pilarvargas-tecnativa pre-commit is ready

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@GarethGloSystems
Copy link

odoo/odoo@29b6b85 added <t t-cache="a,attrib_set"> to the website_sale.product_attributes and website_sale.o_wsale_offcanvas templates.

The last time I investigated these changes were incompatible with this module.

@pedrobaeza
Copy link
Member

pedrobaeza commented Sep 9, 2024

AFAIK, you can remove that cache or expand it with the needed data using the inheritance system (with replace, or t-add).

Copy link
Contributor

@pilarvargas-tecnativa pilarvargas-tecnativa left a comment

Choose a reason for hiding this comment

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

Please add this fix #970

@lef-adhoc lef-adhoc force-pushed the 17.0-mig-website_sale_product_attribute_value_filter_existing branch from 90c4906 to ed4cfe0 Compare September 23, 2024 15:54
@lef-adhoc
Copy link
Contributor Author

@pilarvargas-tecnativa hi, can you check if everything is ok?

@pilarvargas-tecnativa
Copy link
Contributor

@pilarvargas-tecnativa hi, can you check if everything is ok?

@lef-adhoc Hi, but I see you have included the change in your migration commit, you should cherry-pick the commit with the fix to keep the history.

…ibute parent container

If only the attribute container is hidden, the parent container still occupies a space that is visible in the attribute list. To avoid this, apply the condition to the parent container and hide it completely.

TT50704
@lef-adhoc lef-adhoc force-pushed the 17.0-mig-website_sale_product_attribute_value_filter_existing branch from ed4cfe0 to 603718b Compare September 26, 2024 12:46
Copy link
Contributor

@pilarvargas-tecnativa pilarvargas-tecnativa left a comment

Choose a reason for hiding this comment

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

thanks

@lef-adhoc lef-adhoc force-pushed the 17.0-mig-website_sale_product_attribute_value_filter_existing branch from 603718b to 6e11f1d Compare September 26, 2024 12:50
@lef-adhoc
Copy link
Contributor Author

@pedrobaeza Hi, can we merge this?

@@ -3,7 +3,7 @@
{
"name": "Website Sale Attribute Value Existing",
"summary": "Allow hide attributes values not used in variants",
"version": "16.0.1.1.0",
"version": "17.0.1.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

This change should be in the migration commit, not in the pre-commit one

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@lef-adhoc lef-adhoc force-pushed the 17.0-mig-website_sale_product_attribute_value_filter_existing branch from 6e11f1d to aafd92d Compare October 21, 2024 13:22
@lef-adhoc
Copy link
Contributor Author

@chienandalu It is corrected

Copy link
Contributor

@pilarvargas-tecnativa pilarvargas-tecnativa left a comment

Choose a reason for hiding this comment

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

In the mobile menu all attributes are displayed even if only one of them is in use.
image
For the test I have created the attribute TEST, with the values 1, 2 and 3. I have used the value 2 in a product and in the filter panel it appears correct but in the mobile view panel it does not.

)
cls.product_attribute_value_blue = ProductAttributeValue.create(
{"name": "Test blue", "attribute_id": cls.product_attribute.id}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

The yellow attribute that is tested in the tour that is not present is not really present because it is not defined. That part needs to be corrected and the unused attribute needs to be added.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Successfully merging this pull request may close these issues.