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

[FIX] website_sale: fix pricelist GeoIP #32961

Closed

Conversation

Projects
None yet
4 participants
@rdeodoo
Copy link
Contributor

commented Apr 25, 2019

Before this commit:
If the user was GeoIP located and it's pricelist (property_product_pricelist)
was not allowed for it's GeoIP country, that pricelist would still be return.

This issue was detected as a test test_get_pricelist_available_geoip2 was not
working since forward ported in master (only on theme runbot or local..).

Before master (saas-12.4), property_product_pricelist would not be recomputed
once set, so the test was actually not working as expected.
Since master, reading property_product_pricelist just after setting it goes
through the compute method, resulting in different value than in previous
versions.

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo robodoo added the seen 🙂 label Apr 25, 2019

@C3POdoo C3POdoo added the RD label Apr 25, 2019

@rdeodoo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@qsm-odoo @JKE-be This will fix the design-theme runbot, I created a branch and it is green on it.

The test should be red in odoo/odoo, not only on design-theme but somehow it is not, that's the strange part.
Anyway, in local the test fails as on design-theme.

There is a change between master and previous versions which cause this issue. Indeed, before master, writing on property_product_pricelist and then reading the value won't trigger the compute method but return a cached value, while in master it does not (which is better and supposed to IMHO..).

@JKE-be The failing test did highlight the case which is fixed by this PR about partner_pl and GeoIP.
If you think the pl should still be returned in such a case, let me know. But I don't think it makes sense to return the partner_pl if it is not GeoIP compliant, as we don't return partner_pl if it is not website compliant.

[FIX] website_sale: fix pricelist GeoIP
Before this commit:
If the user was GeoIP located and it's pricelist (property_product_pricelist)
was not allowed for it's GeoIP country, that pricelist would still be return.

This issue was detected as a test `test_get_pricelist_available_geoip2` was not
working since forward ported in master (only on theme runbot or local..).

Before master (saas-12.4), `property_product_pricelist` would not be recomputed
once set, so the test was actually not working as expected.
Since master, reading `property_product_pricelist` just after setting it goes
through the compute method, resulting in different value than in previous
versions.

@rdeodoo rdeodoo force-pushed the odoo-dev:master-fix-theme-pricelist-error-rde branch from 364b93d to 5ddc0f8 Apr 26, 2019

@robodoo robodoo added the CI 🤖 label Apr 26, 2019

@JKE-be

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

robodoo pushed a commit that referenced this pull request Apr 26, 2019

[FIX] website_sale: fix pricelist GeoIP
Before this commit:
If the user was GeoIP located and it's pricelist (property_product_pricelist)
was not allowed for it's GeoIP country, that pricelist would still be return.

This issue was detected as a test `test_get_pricelist_available_geoip2` was not
working since forward ported in master (only on theme runbot or local..).

Before master (saas-12.4), `property_product_pricelist` would not be recomputed
once set, so the test was actually not working as expected.
Since master, reading `property_product_pricelist` just after setting it goes
through the compute method, resulting in different value than in previous
versions.

closes #32961

Signed-off-by: Jérémy Kersten (jke) <jke@openerp.com>
@robodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

Merged, thanks!

@robodoo robodoo closed this Apr 26, 2019

@rdeodoo rdeodoo deleted the odoo-dev:master-fix-theme-pricelist-error-rde branch Apr 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.