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

Point of sale issue fixes #29269

Closed
wants to merge 3 commits into
base: 12.0
from

Conversation

Projects
None yet
7 participants

@C3POdoo C3POdoo added the RD label Dec 5, 2018

@nim-odoo nim-odoo requested a review from len-odoo Jan 23, 2019

@len-odoo
Copy link
Contributor

len-odoo left a comment

Besides my comment, commit messages should be improved. I put down some rephrasing below.

[FIX] point_of_sale: raise error instead of traceback when no journal is set

If no journal is set for the POS, the session creation would crash.
We add raise an explicit error asking the user to configure either the payment
method or the chart of accounts.

[FIX] point_of_sale: do not load chat windows on the POS

When the mail_service is started, if there are any running chat sessions,
Chat_windows are created and opened on the point_of_sale frontend.
They also appear in the printed receipt.

To remove them from display we create a new pos_assets_backend template,
which inherits the web.assets_backend and removes the odoobot related assets.

addons/point_of_sale/models/pos_session.py Outdated
@@ -188,6 +188,8 @@ def create(self, values):
journals = Journal.with_context(ctx).search([('type', '=', 'cash')])
if not journals:
journals = Journal.with_context(ctx).search([('journal_user', '=', True)])
if not journals:
raise ValidationError(_("No payment method configured! \nEither Chart of Account not installed or no payment method configured for this POS"))

This comment has been minimized.

@len-odoo

len-odoo Jan 24, 2019

Contributor

"No payment method configured! \nEither no Chart of Account is installed or no payment method is configured for this POS."

Note that you should also export the translation when you add new translatable terms.

addons/point_of_sale/views/pos_templates.xml Outdated
@@ -1,5 +1,10 @@
<?xml version="1.0" encoding="utf-8"?>
<odoo>
<template id="pos_assets_backend" name="POS UI Backend Assets" inherit_id="web.assets_backend" primary="True">
<xpath expr="//script[contains(@src, '/mail/static/src/js/services/mail_service.js')]" position="replace"></xpath>
<xpath expr="//script[contains(@src, '/mail_bot/static/src/js/mailbot_service.js')]" position="replace"></xpath>

This comment has been minimized.

@mart-e

mart-e Feb 4, 2019

Contributor

will this crash if mail_bot is not installed?

This comment has been minimized.

@mart-e

mart-e Feb 4, 2019

Contributor

yep, it crashes

This comment has been minimized.

@len-odoo

len-odoo Feb 4, 2019

Contributor

It is in auto_install with only a depends on mail, so it should always be installed. But nothing prevents from uninstalling it. So a bridge module?

This comment has been minimized.

@mart-e

mart-e Feb 4, 2019

Contributor

or, new idea : how about disabling the chatter on the js side (not that I know how to do so)

This comment has been minimized.

@len-odoo

len-odoo Feb 4, 2019

Contributor

@alexkuhn is it possible to do what Martin suggested just above? i.e. disable clippy without removing it from the assets?

This comment has been minimized.

@msh-odoo

msh-odoo Feb 4, 2019

Contributor

@mart-e @len-odoo We removed these files from assets_backend and create new assets bundle and use it in POS UI template to not deploy mail service in pos UI, after checking I find that even if mail_bot service deployed it will not create any issue in pos UI but I removed mail service so that it is not deployed in pos UI otherwise if you have chat window open and if you load POS UI then those chat window will also be displayed POS UI.

This comment has been minimized.

@alexkuhn

alexkuhn Feb 4, 2019

Contributor

In v12, you cannot disable some services.
You'll need to add this feature on service provider mixin in order to ignore deployment of some services:
https://github.com/odoo/odoo/blob/12.0/addons/web/static/src/js/core/service_mixins.js#L57

@mart-e mart-e force-pushed the odoo-dev:12.0-fix-pos-issue-dpa branch to e457b84 Feb 4, 2019

@robodoo robodoo removed the CI 🤖 label Feb 4, 2019

mart-e added a commit to odoo-dev/odoo that referenced this pull request Feb 4, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 4, 2019

len-odoo added a commit to odoo-dev/odoo that referenced this pull request Feb 11, 2019

len-odoo added a commit to odoo-dev/odoo that referenced this pull request Feb 11, 2019

dpa-odoo and others added some commits Dec 5, 2018

[IMP] point_of_sale: raise error instead of traceback when no journal…
… is set

If no journal is set for the POS, the session creation would crash.
We add raise an explicit error asking the user to configure either the payment
method or the chart of accounts.

opw 1878185
[FIX] point_of_sale: do not load chat windows on the POS
When the mail_service is started, if there are any running chat sessions,
Chat_windows are created and opened on the point_of_sale frontend.
They also appear in the printed receipt.

To remove them from display we create a new pos_assets_backend template,
which inherits the web.assets_backend and removes the odoobot related assets.

opw 1878185
[I18N] point_of_sale: export source terms
Following changes made at #29269

@len-odoo len-odoo force-pushed the odoo-dev:12.0-fix-pos-issue-dpa branch from 644f09b to 03afd2f Feb 11, 2019

@robodoo robodoo removed the CI 🤖 label Feb 11, 2019

@len-odoo

This comment has been minimized.

Copy link
Contributor

len-odoo commented Feb 11, 2019

After checking everything:

  • fix does solve the issue. Note that it removes altogether the chatter from the POS session, but even if the o_mail_thread was in the source it was not visible (unless some other display bug would make it visible). It is not in display:none, just hidden below the POS div.
  • it does not cause any issue if the mail_bot is manually removed as nothing references it.

Other cosmetic/translation issues have been solved already.

@robodoo robodoo added the CI 🤖 label Feb 11, 2019

@len-odoo

This comment has been minimized.

Copy link
Contributor

len-odoo commented Feb 11, 2019

robodoo r+

@robodoo robodoo added the r+ 👌 label Feb 11, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 11, 2019

Because this PR has multiple commits, I need to know how to merge it:

  • merge to merge directly, using the PR as merge commit message
  • rebase-merge to rebase and merge, using the PR as merge commit message
  • rebase-ff to rebase and fast-forward
@len-odoo

This comment has been minimized.

Copy link
Contributor

len-odoo commented Feb 11, 2019

robodoo rebase-ff

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 11, 2019

Merge method set to rebase and fast-forward

robodoo pushed a commit that referenced this pull request Feb 11, 2019

@robodoo robodoo added merging 👷 and removed merging 👷 labels Feb 11, 2019

@robodoo robodoo added the merged 🎉 label Feb 11, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 11, 2019

Merged, thanks!

@robodoo robodoo closed this Feb 11, 2019

@len-odoo len-odoo deleted the odoo-dev:12.0-fix-pos-issue-dpa branch Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment