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] point_of_sale: from pos UI print sale details only for current … #32789

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@kebeclibre
Copy link
Contributor

commented Apr 18, 2019

…config

Have two pos config opened, with at least one connected to a POS box with a printer
Do some sales on both
Click on the printer icon in the pos UI to print today's sales

Before this commit, the ticket that was printed contained sales for
both point of sales

After this commit, it only contains sales of the point of sale from which
the print request has been sent

OPW 1958885

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

@kebeclibre kebeclibre requested review from mart-e, pimodoo and qle-odoo Apr 18, 2019

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

@C3POdoo C3POdoo added the OE label Apr 18, 2019

@mart-e

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

I understand your point but it was on purpose to mix the orders of all PoS.
The idea was to have a ticket with all orders made at the end of the day. This is apparently a frequent need for restaurant : you don't really want to know which PoS was used, you want to know how much food you sold today.
If you need to have a more precise report for only one PoS, you can use the backend report which uses the same method but called via a wizard.

I however agree the purpose of this button is not very clear but it was done in stable, after the freeze so we tried to keep minimal changes.

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

[FIX] point_of_sale: from pos UI print sale details only for current …
…config

Have two pos config opened, with at least one connected to a POS box with a printer
Do some sales on both
Click on the printer icon in the pos UI to print today's sales

Before this commit, the ticket that was printed contained sales for
both point of sales

After this commit, it only contains sales of the point of sale from which
the print request has been sent

OPW 1958885

@kebeclibre kebeclibre force-pushed the odoo-dev:12.0-sale-details-pos-lpe branch from 0e5e88e to 4c363d9 Apr 18, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Apr 18, 2019

@qle-odoo qle-odoo requested a review from switch87 Apr 19, 2019

@switch87

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

I did not test the commit yet but I agree with @kebeclibre that this should be per POS.

@mart-e The restaurant argument is true, but at the moment "vanilla" odoo is not ready for multi-pos restaurant setup without the oca add-ons, so I'm pro this fix (after testing next week)

@mart-e

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

Then for master as this is a change of behaviour. Keep in mind that if you need a per-pos report, you can get one in pdf from the backend. For stable a workaround exists.

@switch87
Copy link
Contributor

left a comment

some little change requests

@switch87

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@kebeclibre Can you do the requested changes and change the destination of the pr to 11.0?

@switch87

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@mart-e You should not be able to get a print-out of the sales from other stores / restaurants that run on the same database from within a PoS session. This should be done from the back-end and not the other way around.

@kebeclibre

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@switch87
The problem here is less functional than technical.
We all agree that the current feature is misleading, but it has this behavior since v10.0 (well, v9.0 actually). Furthermore, we agree on what the feature should become.
Changing it will confuse even more the users. And I'll get tickets to revert this change.
So, as the first rule of bugfixing is "Do no harm", I won't merge this PR, in any version.
What should really be done, is a clarification/extension of the feature in master

@kebeclibre kebeclibre closed this Apr 23, 2019

@robodoo robodoo added closed 💔 and removed CI 🤖 labels Apr 23, 2019

@switch87

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@switch87
The problem here is less functional than technical.
We all agree that the current feature is misleading, but it has this behavior since v10.0 (well, v9.0 actually). Furthermore, we agree on what the feature should become.
Changing it will confuse even more the users. And I'll get tickets to revert this change.
So, as the first rule of bugfixing is "Do no harm", I won't merge this PR, in any version.
What should really be done, is a clarification/extension of the feature in master

Ok, I will refer to this pr for future development 👍

@switch87 switch87 deleted the odoo-dev:12.0-sale-details-pos-lpe branch Apr 30, 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.