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

[IMP][13.0] pos_empty_home : Make it configurable and simplify code #642

Merged
merged 2 commits into from Jun 2, 2021

Conversation

ivantodorovich
Copy link
Contributor

@ivantodorovich ivantodorovich commented May 10, 2021

Hello, two commits here

    1. Simplify code by using qweb expressions, screens.js and .css files can be removed.
    1. Make empty home configurable in pos.config

The 2nd point is, specially, to be able to run test tours in order modules without this module affecting results, as it completely removes the product list when no category is selected.. which is something most tours would need.

In line with that, "Empty Home" is disabled by default in the demo pos.config : point_of_sale.pos_config_main, which is the one used by tests normally. It's otherwise enabled by default in all other pos.config, as this is what you'd expect when installing the module (also to avoid affecting users that already use this module).

@ivantodorovich
Copy link
Contributor Author

ping @hkapatel-initos, would you please help with a review here :)

@hkapatel-initos
Copy link
Contributor

ping @hkapatel-initos, would you please help with a review here :)
Yes I will review

class PosConfig(models.Model):
_inherit = "pos.config"

iface_empty_home = fields.Boolean(
Copy link
Contributor

Choose a reason for hiding this comment

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

I've reviewed your PR. it is good that the code has been reduced and LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks !

@ivantodorovich
Copy link
Contributor Author

ping @OCA/pos-maintainers , any take on this one?
Should be an easy review and It's kind of blocking to get another PRs tests to pass

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Makes sense. thanks for improving the CI.

/ocabot merge minor

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

@ivantodorovich
Copy link
Contributor Author

Thanks!

Would you please run the ocabot command again?

Unfortunately it's not triggered when written on the review comments

@legalsylvain
Copy link
Contributor

Weird.

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 13.0-ocabot-merge-pr-642-by-legalsylvain-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 7dbf243 into OCA:13.0 Jun 2, 2021
@OCA-git-bot
Copy link
Contributor

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

None yet

5 participants