-
-
Notifications
You must be signed in to change notification settings - Fork 492
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][MIG] website_sale_product_attribute_value_filter_existing: Migration to 16.0 #773
[16.0][MIG] website_sale_product_attribute_value_filter_existing: Migration to 16.0 #773
Conversation
… to 15.0 [UPD] Update website_sale_product_attribute_value_filter_existing.pot
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: e-commerce-15.0/e-commerce-15.0-website_sale_product_attribute_value_filter_existing Translate-URL: https://translation.odoo-community.org/projects/e-commerce-15-0/e-commerce-15-0-website_sale_product_attribute_value_filter_existing/
9c2b71b
to
d424e0f
Compare
6957932
to
d0966dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested. Good work, thanks! :) Some minor comments:
var domReady = new Promise(function (resolve) { | ||
$(resolve); | ||
}); | ||
var ready = Promise.all([domReady, session.is_bound]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I bet this isn't needed :)
var domReady = new Promise(function (resolve) { | |
$(resolve); | |
}); | |
var ready = Promise.all([domReady, session.is_bound]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, at the beginning, I didn't add this ready, keep it same as v15. but without this ready, the running tests somehow is not stable, 50% passed and 50% failed. So I searched and tried to add this ready to make sure this tests run when DOM is ready
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, when tours fail that randomly is quite annoying. Ok then 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I removed this redundant code and just keep step_delay
to make sure there is enough time between steps
{ | ||
test: true, | ||
url: "/shop", | ||
wait_for: ready, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait_for: ready, |
url_path="/", | ||
code="%s.run('%s')" % tour, | ||
ready="%s.tours['%s'].ready" % tour, | ||
step_delat=100, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it needed anyway?
step_delat=100, | |
step_delay=100, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I will keep this to make sure there is enough time between steps. Some tests in Odoo modules are still using this
@@ -0,0 +1,3 @@ | |||
* Go to Website Shop | |||
* Active product attributes filter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has changed a bit in v16 as now is a theme panel option that is enabled on either top or bottom in the layout. I'd move it to CONFIGURATION.rst as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, moved to CONFIGURATION.rst
and update more details. thanks for pointing out
d0966dd
to
bba2f62
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 👍
This PR has the |
/ocabot merge nobump |
This PR looks fantastic, let's merge it! |
/ocabot migration website_sale_product_attribute_value_filter_existing |
@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-773-by-pedrobaeza-bump-nobump. 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. |
/ocabot merge nobump |
What a great day to merge this nice PR. Let's do it! |
Congratulations, your PR was merged at 1718312. Thanks a lot for contributing to OCA. ❤️ |
No description provided.