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

Master pos robustness gpe #29427

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@switch87
Copy link
Contributor

switch87 commented Dec 11, 2018

prevent editing in pos_config with active session:
point_of_sale:
active
module_pos_restaurant
journal_ids
sequence_ids
pickint_type_id
journal_id
pos_restaurant:
floor_ids
is_table_management
pos_hr:
employee_ids

prevent deletion and deactivation of models used in pos session:
point_of_sale:
account_account
account_journal
ir_sequence
product_template
res_partner
res_users
stock_picking_type
pos_restaurant:
restaurant_floor
pos_hr:
employee_ids

If locally stored session information does not match with the available
database information, show a popup windows explaining there is a
conflict in the data at hand. The popup gives the user the oportunity to
save the local storage data and clear the local storage so they can
continue selling.

Account:
Restrict removing accounts that are being used by journals.

task: https://www.odoo.com/web#id=1879971&action=333&active_id=1428&model=project.task&view_type=form&menu_id=4720

@switch87 switch87 force-pushed the odoo-dev:master-pos-robustness-gpe branch Jan 10, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 10, 2019

@mart-e
Copy link
Contributor

mart-e left a comment

Try to specify which object is blocking in an error message.
Check your write conditions, you are mixing active and delete operations.
On the js part, don't forget to translate your error messages.

Show resolved Hide resolved addons/account/models/account_account.py Outdated
Show resolved Hide resolved addons/point_of_sale/models/account_journal.py Outdated
Show resolved Hide resolved addons/point_of_sale/models/account_journal.py Outdated
Show resolved Hide resolved addons/point_of_sale/models/ir_sequence.py Outdated
Show resolved Hide resolved addons/point_of_sale/models/pos_config.py Outdated
Show resolved Hide resolved addons/point_of_sale/models/pos_config.py Outdated
Show resolved Hide resolved addons/point_of_sale/models/pos_config.py Outdated
addons/point_of_sale/static/src/js/chrome.js Outdated
json_error: function(err){
var self = this;
var body = "There are conflicts between the localStorage and the loaded data. " +
"Possibly there where changes made in the database after first initialisation of this PoS session.\n\n" +

This comment has been minimized.

@mart-e

mart-e Jan 10, 2019

Contributor

Avoid using terms like "localStorage" in error messages, only developers understand that.
Don't forget to translate the error messages.
"Changes were made in the..." instead of "Possible there where..."

This comment has been minimized.

@switch87

switch87 Jan 15, 2019

Author Contributor

@mart-e I replaced local storage with browser cache in my soon to be pushed commit. It's still a little technical but more commonly known I think.

Show resolved Hide resolved addons/point_of_sale/static/src/js/models.js Outdated
@@ -12,6 +12,7 @@

<script type="text/javascript" src="/point_of_sale/static/lib/backbone/backbone.js"></script>
<script type="text/javascript" src="/point_of_sale/static/lib/waitfont.js"></script>
<script type="text/javascript" src="/point_of_sale/static/lib/FileSaver.js"></script>

This comment has been minimized.

@mart-e

mart-e Jan 10, 2019

Contributor

Do we really need another js library, can't we reimplement the few needed methods? (real question)

This comment has been minimized.

@switch87

switch87 Jan 10, 2019

Author Contributor

I thought about that myself, but did not look into it to be honest, will try to do it 👍

This comment has been minimized.

@mart-e

mart-e Feb 13, 2019

Contributor

@switch87 you no longer use it, right? We can remove it now?

This comment has been minimized.

@switch87

switch87 Feb 13, 2019

Author Contributor

@switch87 switch87 force-pushed the odoo-dev:master-pos-robustness-gpe branch Jan 15, 2019

@robodoo robodoo removed the CI 🤖 label Jan 15, 2019

@switch87 switch87 force-pushed the odoo-dev:master-pos-robustness-gpe branch 4 times, most recently to 36d9b0d Jan 16, 2019

@robodoo robodoo added the CI 🤖 label Jan 16, 2019

@switch87

This comment has been minimized.

Copy link
Contributor Author

switch87 commented Jan 17, 2019

@mart-e I did the requested changes

@switch87

This comment has been minimized.

Copy link
Contributor Author

switch87 commented Feb 11, 2019

@mart-e If your review is ok, you may also merge this. (don't forget to update in the internal task)

@mart-e
Copy link
Contributor

mart-e left a comment

I have made a few comments and pushed some changes, still need another review

@@ -339,6 +339,15 @@ def unlink(self):
partner_prop_acc = self.env['ir.property'].search([('value_reference', 'in', values)], limit=1)
if partner_prop_acc:
raise UserError(_('You cannot remove/deactivate an account which is set on a customer or vendor.'))
for account in self:
journal_ids = self.env['account.journal'].search([
('default_credit_account_id', '=',

This comment has been minimized.

@mart-e

mart-e Feb 12, 2019

Contributor

strange way to indent you domain, put one clause per line

('default_credit_account_id', '=',
account.id), ('default_debit_account_id', '=', account.id)
]).ids
if len(journal_ids):

This comment has been minimized.

@mart-e

mart-e Feb 12, 2019

Contributor

no need for len, if journal_ids: is fine

]).ids
if len(journal_ids):
raise UserError(_("You cannot delete Account that are used by a Journal.\n")\
+ _("Account: ") + str(account.id) + "\n"\

This comment has been minimized.

@mart-e

mart-e Feb 12, 2019

Contributor

better specify the name instead of the id, even if we risk to have duplicates, it is more userfriendly

def write(self, vals):
for journal in self:
pos_session_ids = self.env['pos.session'].search([('state', '!=', 'closed'), '|', ('config_id.journal_id', '=', journal.id), ('config_id.journal_ids', 'in', journal.id)]).ids
if vals.get('active', False) and len(pos_session_ids):

This comment has been minimized.

@mart-e

mart-e Feb 12, 2019

Contributor

make the check of vals.get before the search. If we are writing on different fields, we don't even need the for loop

@api.multi
def write(self, vals):
for journal in self:
pos_session_ids = self.env['pos.session'].search([('state', '!=', 'closed'), '|', ('config_id.journal_id', '=', journal.id), ('config_id.journal_ids', 'in', journal.id)]).ids

This comment has been minimized.

@mart-e

mart-e Feb 12, 2019

Contributor

we potentially have

  • many session
  • a few pos config
    For performance reason, it is probably better to attack the issue on the pos.config side (if a config match the journal, then check if any session is still open)
    The code will be a bit longer and we will make several search but if in most cases, we don't trigger the error, we will actually make less search (avoid a slow config_id.journal_ids in ...),
result = super(PosConfig, self).write(vals)
if self.env['pos.session'].search_count([('config_id', '=', self.id), ('state', '!=', 'closed')]):
forbidden_fields = []
fields_to_check = self._get_fields_to_check()

This comment has been minimized.

@mart-e

mart-e Feb 12, 2019

Contributor

same as for the write, to check before actually making the slow search

@@ -370,8 +380,16 @@ def create(self, values):

@api.multi
def write(self, vals):
result = super(PosConfig, self).write(vals)
if self.env['pos.session'].search_count([('config_id', '=', self.id), ('state', '!=', 'closed')]):

This comment has been minimized.

@mart-e

mart-e Feb 12, 2019

Contributor

self can be more than one record, should be self.ids

@@ -26,6 +26,17 @@ def _onchange_sale_ok(self):
if not self.sale_ok:
self.available_in_pos = False

@api.multi
def write(self, vals):
if self.env['pos.session'].search_count([('state', '!=', 'closed')]):

This comment has been minimized.

@mart-e

mart-e Feb 12, 2019

Contributor

again, faster to check the content of vals that make a search on many records

if field in vals:
forbidden_fields.append(field)
if forbidden_fields:
raise UserError(_("You cannot change folowing fields while there is an active PoS session:") + "\n" + ", ".join(forbidden_fields))

This comment has been minimized.

@mart-e

mart-e Feb 12, 2019

Contributor

typo, "folowing"

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

@mart-e mart-e dismissed their stale review Feb 12, 2019

working on it

@@ -13,3 +13,5 @@
from . import product
from . import res_partner
from . import res_config_settings
from . import stock_picking_type

This comment has been minimized.

@mart-e

mart-e Feb 13, 2019

Contributor

res_users is not imported and try to keep the import in alphabetical order

addons/point_of_sale/models/res_users.py Outdated
def unlink(self):
for user in self:
if (self.env['pos.session'].search_count([('user_id', '=', user.id), ('state', '!=', 'closed')])
or user.has_group('point_of_sale.group_pos_user') or user.has_group('point_of_sale.group_pos_user')):

This comment has been minimized.

@mart-e

mart-e Feb 13, 2019

Contributor

what do you want to prevent execatly? you say "users that are in open session" but use an or clause. If all sessions are closed, I should be able to delete a user with pos group.

addons/point_of_sale/models/res_users.py Outdated
@api.multi
def unlink(self):
for user in self:
if (self.env['pos.session'].search_count([('user_id', '=', user.id), ('state', '!=', 'closed')])

This comment has been minimized.

@mart-e

mart-e Feb 13, 2019

Contributor

try to batch your search, you can use self.ids and make one global search instead of iterating on each record

addons/point_of_sale/models/stock_picking_type.py Outdated
@api.multi
def unlink(self):
for picking_type in self:
pos_session_ids = self.env['pos.session'].search([('config_id.picking_type_id', '=', picking_type.id), ('state', '!=', 'closed')]).ids

This comment has been minimized.

@mart-e

mart-e Feb 13, 2019

Contributor

same as above, doing a search with foo_id.bar_id = ... is a bit costly, especially if there is many records (like on a pos.session), better do it in the other way (picking -> config -> sessions)

addons/point_of_sale/models/stock_picking_type.py Outdated
@@ -0,0 +1,19 @@
# -*- coding: utf-8 -*-
# Part of Odoo. See LICENSE file for full copyright and licensing details.
# Copyright (C) 2004-2008 PC Solutions (<http://pcsol.be>). All Rights Reserved

This comment has been minimized.

@mart-e

mart-e Feb 13, 2019

Contributor

what is this copyright?

@mart-e mart-e force-pushed the odoo-dev:master-pos-robustness-gpe branch Feb 13, 2019

@mart-e
Copy link
Contributor

mart-e left a comment

@switch87 I added a couple of questions. Also please check what I did for the fields protection with a global value, it is easier and no need to define new method with hardcoding of fields

addons/point_of_sale/static/src/js/chrome.js Outdated
@@ -745,6 +745,36 @@ var Chrome = PosBaseWidget.extend(AbstractAction.prototype, {

popup.appendTo(this.$el);
},
json_error: function(err){
var self = this;
var body = "There are conflicts between the browser cache and the loaded data. " +

This comment has been minimized.

@mart-e

mart-e Feb 13, 2019

Contributor

translatable content

@@ -12,6 +12,7 @@

<script type="text/javascript" src="/point_of_sale/static/lib/backbone/backbone.js"></script>
<script type="text/javascript" src="/point_of_sale/static/lib/waitfont.js"></script>
<script type="text/javascript" src="/point_of_sale/static/lib/FileSaver.js"></script>

This comment has been minimized.

@mart-e

mart-e Feb 13, 2019

Contributor

@switch87 you no longer use it, right? We can remove it now?

@@ -0,0 +1,16 @@
# -*- coding: utf-8 -*-

This comment has been minimized.

@mart-e

mart-e Feb 13, 2019

Contributor

not imported file

addons/pos_hr/models/hr_employee.py Outdated
@api.multi
def unlink(self):
for employee in self:
pos_session_ids = self.env['pos.session'].search([('state','!=','closed'), ('module_pos_employee', '=', True), '|', ('employee_ids', 'in', employee.id), ('employee_ids', '=', False)]).ids

This comment has been minimized.

@mart-e

mart-e Feb 13, 2019

Contributor

this is probably untested as you are mixing fields of different modules. There is no field module_pos_employee.


@api.multi
def unlink(self):
for floor in self:

This comment has been minimized.

@mart-e

mart-e Feb 13, 2019

Contributor

is this still useful as we already have a protection on the pos.config on the field floor_ids?

@mart-e mart-e force-pushed the odoo-dev:master-pos-robustness-gpe branch Feb 13, 2019

@mart-e
Copy link
Contributor

mart-e left a comment

some issues on the translation in js

addons/point_of_sale/static/src/js/models.js Outdated
this.init_from_JSON(options.json);
return;
} catch(error) {
this.pos.chrome.json_error({title: _t("Payment journal ID ") + json.statement_id +_t( " is not available in the point of sale.")});

This comment has been minimized.

@mart-e

mart-e Feb 13, 2019

Contributor

do not make this kind of string construction, it makes it very hard to translate, in js, use _.str.sprintf
see https://slides.com/mart-e/i18n-code#/4

addons/point_of_sale/static/src/js/models.js Outdated
} else {
console.error('ERROR: trying to load a fiscal position not available in the pos');
} catch(error) {
this.pos.chrome.json_error({title: "Fiscal position ID " + json.fiscal_position_id + " is not available in the point of sale."});

This comment has been minimized.

@mart-e

mart-e Feb 13, 2019

Contributor

should be translatable

@mart-e mart-e force-pushed the odoo-dev:master-pos-robustness-gpe branch 4 times, most recently Feb 13, 2019

@switch87 switch87 force-pushed the odoo-dev:master-pos-robustness-gpe branch Feb 22, 2019

@switch87 switch87 force-pushed the odoo-dev:master-pos-robustness-gpe branch 2 times, most recently Feb 22, 2019

@mart-e mart-e force-pushed the odoo-dev:master-pos-robustness-gpe branch 3 times, most recently Feb 26, 2019

[IMP] account,point_of_sale: robustness
prevent editing in pos_config with active session:
    active
    module_pos_restaurant
    journal_ids
    sequence_ids
    pickint_type_id
    journal_id

prevent deletion and deactivation of models used
in pos session:
    account_account
    account_journal
    ir_sequence
    product_template
    res_partner
    res_users
    stock_picking_type

If locally stored session information does not match with the available
database information, show a popup windows explaining there is a
conflict in the data at hand. The popup gives the user the oportunity to
save the local storage data and clear the local storage so they can
continue selling.

Task id: 1879971

@switch87 switch87 force-pushed the odoo-dev:master-pos-robustness-gpe branch to 3601935 Mar 1, 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.