Tryton - Issues

 

Issue7407

Title add reconcile_date to account Reconciliation class
Priority feature Status chatting
Superseder Nosy List Timitos, ced, risto3
Type feature request Components account, account_fr
Assigned To Keywords
Reviews

Created on 2018-05-01.08:59:40 by risto3, last changed by ced.

Messages
msg44255 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2018-09-30.19:24:37
If you want to have more chance that your patches are included you should follow: http://www.tryton.org/how-to-contribute.html (no need to complaint about issue2178). But it is any way too late for 5.0.
msg44254 (view) Author: [hidden] (risto3) Date: 2018-09-30.17:45:24
Please consider pushing this for V5.
BTW, I use fields.DateTime now instead of TimeStamp as the granularity required sufficient.
msg40689 (view) Author: [hidden] (risto3) Date: 2018-05-14.23:43:36
https://bitbucket.org/risto3/account/commits/baad7a6e30b4e27af6fd159b9d62f62f568f8eeb?at=reconcile_date
simplified and ensured reports continuity.
Cast() no longer needed either.
msg40688 (view) Author: [hidden] (risto3) Date: 2018-05-14.18:47:47
I do not understand the issue with continuity.... it changes nothing in the report generation, where the correct date should always be used.

As for simplicity, if the __register__() code is too complex then using timestamp as the field type will simplify everything at the expense of 4 extra bytes per record.

For example, in our accounting, we have since ~6.5 fiscal years roughly 135K move lines, and roughly 15K reconciliation lines.

I can say that I don't loose too much sleep over 15K*8bytes => 120KB overhead!!

So then, I'm convinced, why don't we just use fields.Timestamp for reconcile_date and keep things simple for everybody by using the __register__() of the original patchset proposed.
msg40687 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2018-05-14.18:06:39
I really do not see what would be wrong by not doing it.
This field has strictly no meaning in accounting, it can be changed at any time by just canceling the reconciliation and redo it.
So there should be absolutely not conversion to ensure continuity in the report generation and simplicity of the code.
msg40686 (view) Author: [hidden] (risto3) Date: 2018-05-14.17:52:22
Given the potential nauseous propensity of the fiscal authorities to flag anything that they may consider suspicious, and given the single, one time hit of conversion, perhaps it is simply preferable to do it.

Alternatively, the field could possibly simply remain a timestamp , permitting the conversion automatically by python datetime.datetime... but there the expense is its doubled size.

Personally I prefer former, date with full conversion... considering, for example, those using
the French plan in the Antilles or Indian/Pacific Islands...
msg40682 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2018-05-14.15:14:34
I do not think it is required to make a migration using timezone. Having one day error is not really important.
For the AtTimeZone issue, I created https://python-sql.tryton.org/bug60
msg40676 (view) Author: [hidden] (risto3) Date: 2018-05-13.14:10:16
In persuing the latter, I'm testing this code:
        if not reconcile_date_exist:
            cursor.execute(
                *sql_table.update(
                    [sql_table.reconcile_date],
                    line.join(
                        move,
                        condition=move.id == line.move
                        ).join(
                        company,
                        condition=company.id == move.company
                        ).select(
                        Cast(
                            AtTimeZone(
                                AtTimeZone(sql_table.create_date, 'UTC'),
                                company.timezone),
                            'date'),
                        where=line.reconciliation == sql_table.id,
                        group_by=company.timezone)))

with the following added just prior:
        Company = pool.get('company.company')
        company = Company.__table__()

which generates the following:
'UPDATE "account_move_reconciliation" SET "reconcile_date" = (SELECT                                                                                                      
CAST("account_move_reconciliation"."create_date" AT TIME ZONE %s AT TIME ZONE %s AS date) FROM                                                                            
"account_move_line" AS "b" INNER JOIN "account_move" AS "c" ON ("c"."id" = "b"."move") INNER JOIN                                                                         
"company_company" AS "d" ON ("d"."id" = "c"."company") WHERE ("b"."reconciliation" =                                                                                      
"account_move_reconciliation"."id") GROUP BY "d"."timezone")'
where args:
>>> args                                                                                                                                                                  
('UTC', <sql.Column object at 0x7f1603483e80>)                                                                                                                            
>>> str(args[1])                                                                                                                                                          
'"timezone"'                                                                                                                                                              

I get the following exception:
...
File "/tmp/trytond-test/trytond/backend/postgresql/database.py", line 61, in execute 
  cursor.execute(self, sql, args)
psycopg2.ProgrammingError: can't adapt type 'Column'                                                                          │                     
But I if I execute the generated SQL directly in psql after creating the column and manually
replacing the parameters, it seems to work just fine.

Is there something esoteric I'm missing or is this perhaps an anomalie in the database layers?
msg40675 (view) Author: [hidden] (risto3) Date: 2018-05-13.10:50:41
I'm dubious of trying Transaction().context.get('company'), is it possible that there are reconciled lines of multiple companies in potentially different timezones existing in the table account_move_reconciliation? 

That is, do I presume correctly perhaps what is to be used is the distinct line.move.company.timezone for the lines involved in each reconciliation entry?
msg40674 (view) Author: [hidden] (risto3) Date: 2018-05-13.09:32:25
For migration, I'd like to try something like:
        # Migration from 4.8: new reconcile_date field, reuse create_date
        if not reconcile_date_exist:
            cursor.execute(
                *sql_table.update(
                    [sql_table.reconcile_date],
                    [Cast(
                        AtTimeZone(
                            AtTimeZone(sql_table.create_date, 'UTC'),
                            company.timezone.zone),
                        'date')]))

But how to get a valid company handle in account/move.py's Reconciliation::__register__() ?
msg40673 (view) Author: [hidden] (risto3) Date: 2018-05-13.08:26:06
re: msg40671   excellent observation!
msg40672 (view) Author: [hidden] (risto3) Date: 2018-05-13.08:20:54
4 bytes is not too costly considering the FEC is also something certain [French] professionals *import*.
(It is possible there is work planned or underway to do that with Tryton, which would be a great way to kickstart an accounting)

But as I already mentioned, it is possible to have only account_fr support this, at least initially. 
The minute there is a second or a third plan needing this for any reason, it should preferably be in core account as it becomes a generic requirement and not only a national one.
Since the model is cleaner and more coherent, I migrated my initial internal patch to be the generic one proposed here.
msg40671 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2018-05-13.08:04:11
Indeed I see one reason why adding a field will be better, it is because 'create_date' is a datetime in UTC so depending of the timezone, the extracted date could be wrong by one day.
msg40670 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2018-05-13.00:32:45
I do not think it worth the cost to add a field because the create_date contains already the right value.
For migration, I would say it is a matter to set the right date for the create_date via SQL query.
msg40492 (view) Author: [hidden] (risto3) Date: 2018-05-01.16:37:50
looks as if there are as of today new Field class methods for Cast() called sql_cast()... 
line 496 in account/move.py can probably be updated to use it prior to final commit.
msg40491 (view) Author: [hidden] (risto3) Date: 2018-05-01.08:59:40
As discussed in the list, the FEC generated for the French accounting plan expects a date of reconcilation and not its effective date.

This is not a show stopper for new accounting but migrating from other systems need a way to import this date which is currently not possible without trickery.

The following patch at https://bitbucket.org/risto3/account/commits/d0227484b8c5aa8430b713b8c1e626b651131358?at=reconcile_date
adds reconcile_date to Reconciliation as well as migrates an existing accounting, permitting the following simple fix in the account_fr module to use the new field:

diff -r 0a79f2a6089d account.py
--- a/account.py        Mon Apr 23 17:43:24 2018 +0200
+++ b/account.py        Tue May 01 08:54:16 2018 +0200
@@ -297,7 +297,7 @@
             format_number(line.debit or 0),
             format_number(line.credit or 0),
             reconciliation.rec_name if reconciliation else '',
-            format_date(reconciliation.create_date) if reconciliation else '',
+            format_date(reconciliation.reconcile_date) if reconciliation else '',
             format_date(line.move.post_date),
             format_number(line.amount_second_currency)
             if line.amount_second_currency else '',

It may be possible, worse case, to only add this only in account_fr, but it seems cleaner
to make it in base account module.
History
Date User Action Args
2018-09-30 19:24:37cedsetmessages: + msg44255
2018-09-30 17:45:25risto3setmessages: + msg44254
2018-05-14 23:43:36risto3setmessages: + msg40689
2018-05-14 18:47:47risto3setmessages: + msg40688
2018-05-14 18:06:39cedsetmessages: + msg40687
2018-05-14 17:52:23risto3setmessages: + msg40686
2018-05-14 15:14:34cedsetmessages: + msg40682
2018-05-13 14:10:17risto3setmessages: + msg40676
2018-05-13 10:50:41risto3setmessages: + msg40675
2018-05-13 09:32:26risto3setmessages: + msg40674

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