Tryton - Issues

 

Issue8737

Title Point of Sale
Priority feature Status in-progress
Superseder Set parent field in One2Many records
View: 8742
Nosy List albertca, ced, dave, frflores11, lukio, pokoli, reviewbot, semarie, yangoon
Type feature request Components
Assigned To ced Keywords review
Reviews 21391002
View: 21391002

Created on 2019-10-17.16:10:02 by ced, last changed by reviewbot.

Messages
New review21391002 at https://codereview.tryton.org/21391002/#ps323171003
msg56764 (view) Author: [hidden] (frflores11) Date: 2020-03-28.21:15:26
Hi guys!,
While working with the module, I'm experience an error that I assume is related to the new implementation of the company module (sorry that I'm including this here but I didn't found the right place!...your advice is welcomed :)

This is the stack trace that I get:

Traceback (most recent call last):
  File "/Users/fflores/python_prjs/tryton-env/trytond/trytond/protocols/dispatcher.py", line 181, in _dispatch
    result = rpc.result(meth(*c_args, **c_kwargs))
  File "/Users/fflores/python_prjs/tryton-env/trytond/trytond/model/workflow.py", line 37, in wrapper
    result = func(cls, filtered, *args, **kwargs)
  File "/Users/fflores/python_prjs/tryton-env/trytond/trytond/modules/sale_point/sale.py", line 272, in post
    move = line.get_stock_move()
  File "/Users/fflores/python_prjs/tryton-env/trytond/trytond/modules/sale_point/sale.py", line 569, in get_stock_move
    if self.product.type == 'service':
  File "/Users/fflores/python_prjs/tryton-env/trytond/trytond/model/fields/field.py", line 336, in __get__
    return inst.__getattr__(self.name)
  File "/Users/fflores/python_prjs/tryton-env/trytond/trytond/model/modelstorage.py", line 1569, in __getattr__
    read_data = self.read(list(ids), list(ffields.keys()))
  File "/Users/fflores/python_prjs/tryton-env/trytond/trytond/model/modelsql.py", line 692, in read
    domain = Rule.domain_get(cls.__name__, mode='read')
  File "/Users/fflores/python_prjs/tryton-env/trytond/trytond/ir/rule.py", line 248, in domain_get
    clause, clause_global = cls.get(model_name, mode=mode)
  File "/Users/fflores/python_prjs/tryton-env/trytond/trytond/ir/rule.py", line 213, in get
    decoder = PYSONDecoder(cls._get_context())
  File "/Users/fflores/python_prjs/tryton-env/trytond/trytond/modules/company/ir.py", line 77, in _get_context
    if user.employee:
  File "/Users/fflores/python_prjs/tryton-env/trytond/trytond/model/fields/field.py", line 336, in __get__
    return inst.__getattr__(self.name)
  File "/Users/fflores/python_prjs/tryton-env/trytond/trytond/model/modelstorage.py", line 1576, in __getattr__
    fvalue = instantiate(field, data[fname], data)
  File "/Users/fflores/python_prjs/tryton-env/trytond/trytond/model/modelstorage.py", line 1544, in instantiate
    value = int(value)
ValueError: invalid literal for int() with base 10: ''


I did some tracing and I found that the problem is because tryton is trying to instantiate the company linked to this employee, and to do so it reads a series of parameters from cache (this in the file modelstorage.py ~ line 1550:

        # Read the data
        with Transaction().set_current_transaction(self._transaction), \
                self._transaction.set_user(self._user), \
                self._transaction.reset_context(), \
                self._transaction.set_context(self._context):
            if self.id in self._cache and name in self._cache[self.id]:
                logger.info('before reading data, model is: %s and user is: %s' % (self.__name__, self._user))
                # Use values from cache
                ids = islice(chain(islice(self._ids, index, None),
                        islice(self._ids, 0, max(index - 1, 0))),
                    self._transaction.database.IN_MAX)
                ffields = {name: ffields[name]}
                read_data = [{'id': i, name: self._cache[i][name]}
                    for i in ids
                    if i in self._cache and name in self._cache[i]]
            else:
                read_data = self.read(list(ids), list(ffields.keys()))


However in the _get_context method (in file module/company/ir.py) the user is changed to Root therefore there is no company (nor employee) linked to the Root user(I understand this is new in version 5.5):

 @classmethod
    def _get_context(cls):
        pool = Pool()
        User = pool.get('res.user')
        Employee = pool.get('company.employee')
        context = super()._get_context()
        # Use root to avoid infinite loop when accessing user attributes
        with Transaction().set_user(0):
            user = User(Transaction().user)
        if user.employee:
            with Transaction().set_context(
                    _check_access=False, _datetime=None):
                context['employee'] = EvalEnvironment(
                    Employee(user.employee.id), Employee)
        return context


Sorry for the long series of posts... but this is my first project here and I'm not familiar with the process (once again... advice is welcomed :),

BR,
msg56763 (view) Author: [hidden] (frflores11) Date: 2020-03-28.21:01:59
Additional Features

While reviewing the module I came up with a few features that I think would be great for the implementation:

1.- Identify customer in the sale: some countries require the customer id to appear in the invoice, so this should be a matter of adding a field in the sale.point.sale model
2.- Print the ticket: preferably at the time of clicking the process button
3.- Restaurant mode, with options to:
    - manage several floors, and tables within those floors in the restaurant
    - make reservations
    - split bills between the customers that are in a given table
    - change table (when a customer wants to move to somewhere else in the restaurant)
    - print (non repeating) kitchen orders
4.- discounts (per customer category, per line in the sale, per whole sale, vouchers, etc...)
5.- barcodes
6.- sales in account (or credit sales)... probably using the module account_credit_limit
7.- low (or no) inventory alert
8.- default coin of sale for employee (probably storing the value on the cache after the first sale produced in the session)
9.- default quantity of 1 for every new line
10.- default payment method
11.- create a "framework" for e-invoice, probably this would mean create a different module for every country given that the specs are not the same across the different countries... this is requirement for most of LATAM countries and I understand that also in France & Spain

Please your comments,
msg56762 (view) Author: [hidden] (frflores11) Date: 2020-03-28.20:46:18
Hi guys!..
I've been working with the module (mostly understanding how it works)... I found 5 TODO's in the code that I'll like to clarify:

1- TODO: "number on close": I understand this as "obtain the next number from sequence and apply to the number field after the user clicks on the “process” button,... however currently the record is created when the process button is clicked (in the create method)... so I don't see the point)

2- TODO: "prevent delete when numbering": I understand this as temporary disable the delete button from interface while getting the number from sequence... however in my understanding the number is retrieved from sequence in one single transaction (it's an atomic operation), so I don't see the change that something else could happen while getting the number from sequence... I'm right?

3- TODO: "allow reopen": I understand this as include a couple of new transitions (‘done’, ‘open’), (‘posted’, ‘open’) and the corresponding handling methods, is that correct?

4- TODO: "wizard to create an invoice from sale": should this be a wizard or a process that runs on “process” or “post” clicks?… (I mean, something that happens without user interaction <as opposed to what a wizard does AFAIK that requires to be triggered>)

5- TODO: "packaging UOM': is that fix the error that triggers for the fact that product brings only the id instead of the whole object? (I'm still trying to understand why the product doesn't brings all the other attributes (only the id)... but I solved this getting the object from db like this:

@fields.depends('product')
    def on_change_with_uom(self, name=None):
        # TODO: packaging UOM
        if self.product:
            pool = Pool()
            Product = pool.get('product.product')
            product = Product.search([
                    ('id',  '=',  self.product.id)
                    ]
            )[0]
#            logger.info('product sale_uom id is %s' % str(product.sale_uom.id))
            return product.sale_uom.id
review21391002 updated at https://codereview.tryton.org/21391002/#ps289561002
review21391002 updated at https://codereview.tryton.org/21391002/#ps317071002
msg52606 (view) Author: [hidden] (dave) (Tryton committer) Date: 2019-10-22.11:42:40
I have heard of it referred to as the "Gross Price" when talking about prices that that may, or may not, include VAT.

I have never heard it called the Public List Price.

I think the problem is that terms Gross / Net are more general than just about taxes.  They are used in a range of different places to mean the whole thing including everything (gross), or just the (well defined) part of something (net).  The gross price may include tax, or be before discounts are deducted, etc. the key thing is that it depends on the context.
msg52582 (view) Author: [hidden] (yangoon) (Tryton translator) Date: 2019-10-19.20:01:09
> We have a discussion about the correct name for the unit price
> on the product which include the tax [1].

AFAIK this is usually the Gross Price or Price including taxes.
msg52577 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2019-10-19.15:35:18
> We have a discussion about the correct name for the unit price on the product which include the tax [1].
> Suggestions?

In spain we normally use: "Public List Price", which means the price for general public (not people subjected to VAT) which implies that it includes the taxes.
msg52576 (view) Author: [hidden] (dave) (Tryton committer) Date: 2019-10-19.12:21:19
I'm not sure what is best, I originally suggested retail_price because, even though it is another name for list price, I thought it was quite descriptive because prices shown in shops (at retail) often include tax.

Having said that, I now think list_price_including_tax is probably a better option, as it is clear what it means, even though it is quite long.
msg52574 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2019-10-19.11:37:30
We have a discussion about the correct name for the unit price on the product which include the tax [1].
Suggestions?


[1] https://codereview.tryton.org/21391002/diff/262291002/modules/sale_point/product.py#newcode23
review21391002 updated at https://codereview.tryton.org/21391002/#ps262291002
review21391002 updated at https://codereview.tryton.org/21391002/#ps280271002
msg52547 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2019-10-17.16:10:02
review21391002 is a long standing module that I update. I think it is good to have an issue linked.
For now, it misses the creation of invoice and a return wizard.
History
Date User Action Args
2020-04-26 00:19:09reviewbotsetmessages: + msg57636
2020-04-26 00:19:08reviewbotsetreviews: 21391002
2020-03-28 21:15:27frflores11setmessages: + msg56764
2020-03-28 21:01:59frflores11setmessages: + msg56763
2020-03-28 20:46:19frflores11setreviews: 21391002 -> (no value)
nosy: + frflores11
messages: + msg56762
2020-03-25 01:08:42reviewbotsetmessages: + msg56627
2020-03-25 00:29:14reviewbotsetmessages: + msg56626
2020-02-10 01:27:40lukiosetnosy: + lukio
2019-10-22 11:42:41davesetmessages: + msg52606
2019-10-20 10:12:08semariesetnosy: + semarie

Showing 10 items. Show all history (warning: this could be VERY long)