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

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

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

LuisAlejandroS
Copy link
Contributor

@LuisAlejandroS LuisAlejandroS commented Nov 20, 2024

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:

  • Ensures that the "Quick Add to Cart" button disappears when stock is zero and "Allow out of stock order" is unchecked.
  • Resolves compatibility issues with website_sale_hide_price module.

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

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Is it ready for review or did you open it in draft by mistake?

@LuisAlejandroS LuisAlejandroS changed the title [FIX]website_sale_hide_price: fix button visibility issue [16.0][FIX]website_sale_hide_price: fix button visibility issue Nov 20, 2024
@LuisAlejandroS LuisAlejandroS marked this pull request as ready for review November 20, 2024 14:49
Copy link

@Pablocce Pablocce left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@LuisAlejandroS LuisAlejandroS force-pushed the 16.0-imp-website_sale_hide_price branch 2 times, most recently from 1fe61b3 to cc3b092 Compare November 21, 2024 12:56
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

@pilarvargas-tecnativa could take a look?

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.

I don't see any difference in the behaviour of the module with this FIX, can you put an indicative image of the problem? I can't reproduce the problem you are referring to outside this PR.

@LuisAlejandroS
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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: #987.

@pedrobaeza pedrobaeza added this to the 16.0 milestone Nov 22, 2024
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Having a method that determines the visibility and override it makes it more inheritable that touching the QWeb template, so let's go with the change:

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

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 added a commit that referenced this pull request Nov 22, 2024
Signed-off-by pedrobaeza
@OCA-git-bot
Copy link
Contributor

@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
Copy link
Member

@LuisAlejandroS it seems the tests are failing.

@danielduqma
Copy link

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
Copy link
Member

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
Copy link

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
Copy link
Contributor

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
Copy link

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
Copy link
Member

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.

@LuisAlejandroS LuisAlejandroS force-pushed the 16.0-imp-website_sale_hide_price branch 3 times, most recently from 2583773 to 6498f80 Compare November 25, 2024 16:05
@LuisAlejandroS
Copy link
Contributor Author

@pedrobaeza Hi! I’ve made a small adjustment to ensure the tests for your module don’t break. Feel free to review it whenever you have the chance. Thanks in advance!

self.env["website"].get_current_website().website_show_price
and not self.website_hide_price
)
if config["test_enable"]:
Copy link
Member

Choose a reason for hiding this comment

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

You have to test also for a specific context (like website_sale_hide_price_test), for allowing to check the functionality in this module (if having tests).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pedrobaeza I’ve just made the changes. Could you please check if this works for you now? Thanks for your time!

Replaced view inheritance with Python logic in _website_show_quick_add
to fix an issue where the "Add to Cart" button visibility was
incorrectly overridden by this module. This update restores the
expected behavior by ensuring the inherited logic determines button
visibility.
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-986-by-pedrobaeza-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 4f271ce into OCA:16.0 Nov 26, 2024
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at da1a745. Thanks a lot for contributing to OCA. ❤️

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.

7 participants