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.

Sign up for GitHub

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

Jump to bottom

[13.0] website_sale_attribute_filter_price: Migration to 13.0. #404

Merged
merged 5 commits into from
Jul 22, 2020
Merged

[13.0] website_sale_attribute_filter_price: Migration to 13.0. #404

merged 5 commits into from
Jul 22, 2020

Conversation

mamcode
Copy link
Member

@mamcode mamcode commented Jul 15, 2020

Standard migration.

@OCA-git-bot
Copy link
Contributor

Hi @Tardo,
some modules you are maintaining are being modified, check this out!

@mamcode
Copy link
Member Author

mamcode commented Jul 15, 2020

@Tardo Thanks for this module 👍 , can you review this PR please.

@mamcode mamcode mentioned this pull request Jul 15, 2020
Closed
22 tasks
HaraldPanten
HaraldPanten suggested changes Jul 17, 2020
View reviewed changes
Copy link

@HaraldPanten HaraldPanten 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 correct to only show the price range selector depending if user is logged in or not? That's a strange behaviour or I might be missing something. I checked V12 and it seems to work as your PR.

What do you think?
Captura de pantalla 2020-07-17 a las 13 12 02

@mamcode
Copy link
Member Author

mamcode commented Jul 20, 2020

@HaraldPanten You are right, I had not noticed that point and I also think that it should work without the user being logged in.

@Tardo Tardo mentioned this pull request Jul 21, 2020
Merged
Tardo
Tardo suggested changes Jul 21, 2020
View reviewed changes
Copy link
Member

@Tardo Tardo left a comment

Choose a reason for hiding this comment

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

Thanks for the work! please, update javascript to use es6.
The issue reported by @HaraldPanten is already fixed here: #411

website_sale_attribute_filter_price/__manifest__.py Outdated Show resolved Hide resolved
website_sale_attribute_filter_price/static/src/js/shop.js Outdated Show resolved Hide resolved
website_sale_attribute_filter_price/static/src/js/shop.js Outdated Show resolved Hide resolved
website_sale_attribute_filter_price/static/src/js/shop.js Outdated Show resolved Hide resolved
website_sale_attribute_filter_price/static/src/js/shop.js Outdated Show resolved Hide resolved
@Tardo
Copy link
Member

Tardo commented Jul 21, 2020

@mamcode please, cherry-pick this commit https://github.com/OCA/e-commerce/pull/411/commits to fix the issue reported by @HaraldPanten

@mamcode
Copy link
Member Author

mamcode commented Jul 21, 2020

@mamcode please, cherry-pick this commit https://github.com/OCA/e-commerce/pull/411/commits to fix the issue reported by @HaraldPanten

Thanks for this fix 👍

@pedrobaeza
Copy link
Member

Side effect

@pedrobaeza pedrobaeza reopened this Jul 21, 2020
HaraldPanten
HaraldPanten approved these changes Jul 22, 2020
View reviewed changes
Copy link

@HaraldPanten HaraldPanten left a comment

Choose a reason for hiding this comment

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

Now it's functionally working fine 👍

ivantodorovich
ivantodorovich approved these changes Jul 22, 2020
View reviewed changes
Copy link

@ivantodorovich ivantodorovich left a comment

Choose a reason for hiding this comment

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

LGTM 👍
good job @mamcode :)

Tardo
Tardo approved these changes Jul 22, 2020
View reviewed changes
Copy link
Member

@Tardo Tardo 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!

@pedrobaeza
Copy link
Member

Please squash commits and reorder them to be properly sorted.

@mamcode mamcode force-pushed the 13.0-mig-website_sale_attribute_filter_price branch from 5e62251 to dee2b6d Compare July 22, 2020 12:14
@mamcode
Copy link
Member Author

mamcode commented Jul 22, 2020

Please squash commits and reorder them to be properly sorted.

Pedro please review again. It's ok like that ?

@pedrobaeza
Copy link
Member

Selección_035

@mamcode mamcode force-pushed the 13.0-mig-website_sale_attribute_filter_price branch from dee2b6d to d9e600c Compare July 22, 2020 13:02
@mamcode
Copy link
Member Author

mamcode commented Jul 22, 2020

@pedrobaeza I have already ordered the commits in the log, please review again.

@pedrobaeza
Copy link
Member

I don't see that the squash is done...

@mamcode mamcode force-pushed the 13.0-mig-website_sale_attribute_filter_price branch from d9e600c to 5f438b0 Compare July 22, 2020 15:12
@mamcode
Copy link
Member Author

mamcode commented Jul 22, 2020

@pedrobaeza look again please...

@pedrobaeza
Copy link
Member

OK now!

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 13.0-ocabot-merge-pr-404-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit c8e196f into OCA:13.0 Jul 22, 2020
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 901362b. 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
Reviewers

@Tardo Tardo Tardo approved these changes

@ivantodorovich ivantodorovich ivantodorovich approved these changes

@HaraldPanten HaraldPanten HaraldPanten approved these changes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mamcode @OCA-git-bot @Tardo @pedrobaeza @ivantodorovich @HaraldPanten