OCA / e-commerce

Odoo E-Commerce server automation addons
GNU Affero General Public License v3.0
170 stars 494 forks source link

[16.0][FIX]website_sale_hide_price: fix button visibility issue #986

Open LuisAlejandroS opened 4 days ago

LuisAlejandroS commented 4 days ago

Description:

In the website_sale_hide_price module, button visibility for the "Quick Add to Cart" button in the ecommerce product list (Kanban or List View) was not behaving as expected when the stock was zero and the "Allow out of stock order" option was unchecked. This PR resolves the issue by replacing view inheritance with logic in Python.

Cases covered with this improvement:

Reproduction Steps (in Runboat):

  1. Create a product in the Sales > Products menu and set the stock to 0.
  2. Make sure the "Allow out of stock order" option is unchecked.
  3. Enable the website_sale_hide_price module.
  4. In the ecommerce frontend, go to the product listing or Kanban view.
  5. Verify that the "Quick Add to Cart" button is hidden when stock is zero.

Solution: The issue was fixed by modifying the _website_show_quick_add method in the product.template model to include logic for checking stock availability and the "Allow out of stock order" option in Python, instead of relying on view inheritance.

This approach ensures better maintainability and resolves the dependency issues that caused the button visibility error. FL-556-4678

LuisAlejandroS commented 2 days ago

image A previous feature ensured that when the "Continue Selling" checkbox was not checked and a product was out of stock, the "Add to Cart" button wouldn't display in the kanban or list views. This update restores that functionality, resolving conflicts caused by this module.

pilarvargas-tecnativa commented 2 days ago

Ok, now I understand the problem. Maybe with this solution you don't have to apply that condition to other buttons at template level. For example those in some snippets. Could you check if in the next versions there is this problem and bring the commit? Thanks!

LuisAlejandroS commented 2 days ago

I noticed this issue persists in version 17.0. To address it, I created a forward-port with the necessary adjustments. You can review the pull request here: https://github.com/OCA/e-commerce/pull/987.

OCA-git-bot commented 2 days ago

This PR looks fantastic, let's merge it! Prepared branch 16.0-ocabot-merge-pr-986-by-pedrobaeza-bump-patch, awaiting test results.

OCA-git-bot commented 2 days ago

@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-986-by-pedrobaeza-bump-patch.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

pedrobaeza commented 2 days ago

@LuisAlejandroS it seems the tests are failing.

danielduqma commented 2 days ago

Hi,

Tests that are failing are related to module website_sale_stock_list_preview. This module depends on website_sale_hide_price to be installed and improperly show the button.

In fact, if you just install website_sale_stock_list_preview (with no other modules from this repository) tests will also fail, because website_sale_stock doesn't render Add to cart button that website_sale_stock_list_preview expects to be shown but disabled. In forward port to 17.0, tests doesn't fail because website_sale_stock_list_preview is not yet migrated.

As you are the authors of this module, could you please take a look at this? Thanks!

Regards.

pedrobaeza commented 2 days ago

Well, this was not failing before your patch, and that's because views are enable on demand on tests, so you should find a way to coexist both modules. One option is to incorporate on the code a check on config["test_enable"] and then only act if there's a special context that means that you are testing this module, not others.

If you don't find a solution, we will need to revert the 17.0 PR for not having the problem on the future.

danielduqma commented 2 days ago

To be honest, I don't see that point. There is a clear bug in website_sale_hide_price that breaks a behaviour implemented in website_sale_stock from Odoo core, and it can be easily tested in both 16.0 and 17.0 versions. I think that we agree until here.

There is a third module, website_sale_stock_list_preview, that dependes on website_sale_hide_price to show a button that website_sale_stock hides. What you propose is to revert the fix in 17.0 (and willfully keep a bug) and include a test_enable check to make the module misbehave in tests just to make website_sale_stock_list_preview pass them. But website_sale_stock_list_preview goal will be affected anyway and has to be reviewed.

I didn't pretend you to fix website_sale_stock_list_preview it ASAP. I just asked to know if you were going to take a first look as Tecnativa is the author and all contributors are from Tecnativa.

pilarvargas-tecnativa commented 2 days ago

A solution at view level would be to incorporate in the t-if the existing conditions in the odoo base view + the conditions added by this module that cause the error when overwriting the whole t-if. At code level it is a good solution but it seems to cause conflict in this version. Or what @pedrobaeza said, add a context to the tests.

danielduqma commented 2 days ago

But we don't see why adding more conditions to the t-if is necessary, if it already has a method that is inheritable to ease integration with other modules, as it is done in website_sale_stock. I don't get why this PR is conflicting, just because it arises existing bugs in other modules and we are asked to mask them.

pedrobaeza commented 2 days ago

As said, it's not a problem in the other module, as how tests are executed, they are correct. The status quo is broken with this change.