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

[13.0][MIG] website_sale_tax_toggle: Migration #497

Merged
merged 18 commits into from
Mar 7, 2021

Conversation

sergio-teruel
Copy link
Contributor

@sergio-teruel sergio-teruel commented Feb 17, 2021

cc @Tecnativa TT27484

@CarlosRoca13 @chienandalu Can you review?

sergio-teruel and others added 17 commits February 17, 2021 12:04
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: e-commerce-12.0/e-commerce-12.0-website_sale_tax_toggle
Translate-URL: https://translation.odoo-community.org/projects/e-commerce-12-0/e-commerce-12-0-website_sale_tax_toggle/
This tour was not effective because:

- Steps did not have proper wait conditions.
- That made them run fast without actually testing the desired effect.
- That left many remaining requests that could made tests to fail randomly, as you can see in OCA#420 (comment).

Now it's shorter but more effective:

- It skips going to `/shop` at the start because it starts already there.
- It moves tour code to demo data.
- It adds proper checks to each trigger, to make sure it doesn't run before checking the result is OK.

@Tecnativa TT24410
Currently translated at 100.0% (2 of 2 strings)

Translation: e-commerce-12.0/e-commerce-12.0-website_sale_tax_toggle
Translate-URL: https://translation.odoo-community.org/projects/e-commerce-12-0/e-commerce-12-0-website_sale_tax_toggle/fr/
Currently translated at 100.0% (2 of 2 strings)

Translation: e-commerce-12.0/e-commerce-12.0-website_sale_tax_toggle
Translate-URL: https://translation.odoo-community.org/projects/e-commerce-12-0/e-commerce-12-0-website_sale_tax_toggle/ca/
@sergio-teruel sergio-teruel force-pushed the 13.0-mig-website_sale_tax_toggle branch 3 times, most recently from b1027df to 0bd1c13 Compare February 17, 2021 12:25
@sergio-teruel sergio-teruel changed the title [13.0][MIG][WIP] website_sale_tax_toggle: Migration [13.0][MIG] website_sale_tax_toggle: Migration Feb 17, 2021
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.

Tested 👍

@pedrobaeza pedrobaeza added this to the 13.0 milestone Feb 21, 2021
Copy link
Contributor

@CarlosRoca13 CarlosRoca13 left a comment

Choose a reason for hiding this comment

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

All working fine!! Just a non-blocking comment for tour loading

<!-- Copyright 2020 Tecnativa - Jairo Llopis
License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). -->
<data>
<template id="assets_frontend_demo" inherit_id="website.assets_frontend">
Copy link
Contributor

@CarlosRoca13 CarlosRoca13 Feb 22, 2021

Choose a reason for hiding this comment

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

Suggested change
<template id="assets_frontend_demo" inherit_id="website.assets_frontend">
<template id="assets_tests_demo" inherit_id="website.assets_tests">

On v13 exists the view assets_test. This assets are loaded just when testing, so you can place the file on templates and inherit from this view on data. Not need to place the assets on demo anymore

Copy link
Contributor

@CarlosRoca13 CarlosRoca13 left a comment

Choose a reason for hiding this comment

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

Don't saw in last review the js variable definition 😅

odoo.define("website_sale_tax_toggle.tax_toggle_button", function(require) {
"use strict";

var sAnimation = require("website.content.snippets.animation");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var sAnimation = require("website.content.snippets.animation");
const sAnimation = require("website.content.snippets.animation");

},
_onPublishBtnClick: function(ev) {
ev.preventDefault();
var $data = $(ev.currentTarget).parents(".js_tax_toggle_management:first");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var $data = $(ev.currentTarget).parents(".js_tax_toggle_management:first");
const $data = $(ev.currentTarget).parents(".js_tax_toggle_management:first");

Comment on lines 7 to 8
var tour = require("web_tour.tour");
var base = require("web_editor.base");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var tour = require("web_tour.tour");
var base = require("web_editor.base");
const tour = require("web_tour.tour");
const base = require("web_editor.base");

// Notice that it's important targeting the price as `span .oe_currency_value`
// or `.oe_price .oe_currency_value` to make sure this test is compatible
// with website_sale_b2x_alt_price module in this same repo.
var steps = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var steps = [
const steps = [

@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). 🤖

@sergio-teruel
Copy link
Contributor Author

@CarlosRoca13 Changes done!!!

<!-- Copyright 2020 Tecnativa - Jairo Llopis
License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). -->
<data>
<template id="assets_tests_demo" inherit_id="website.assets_tests_demo">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<template id="assets_tests_demo" inherit_id="website.assets_tests_demo">
<template id="assets_tests_demo" inherit_id="website.assets_tests">

@sergio-teruel
Copy link
Contributor Author

@CarlosRoca13 Sorry!! Changes done!!

@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). 🤖

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@pedrobaeza pedrobaeza mentioned this pull request Mar 7, 2021
22 tasks
@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 13.0-ocabot-merge-pr-497-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 3f40cae into OCA:13.0 Mar 7, 2021
@OCA-git-bot
Copy link
Contributor

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

@pedrobaeza pedrobaeza deleted the 13.0-mig-website_sale_tax_toggle branch March 7, 2021 14:57
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.

None yet

9 participants