[9.0][MIG] account_reversal #405

antespi · 2016-09-30T09:58:37Z

Migration of addon account_reversal

pedrobaeza · 2016-09-30T10:04:53Z

Please squash all the commits from "OCA Transbot" together

antespi · 2016-09-30T10:19:27Z

Squash done, please mark this PR as needsreview

pedrobaeza · 2016-09-30T10:38:00Z

You can still join more the translations commits although they are not together. You can also join the one with the en.po removal

antespi · 2016-09-30T11:35:21Z

There is something weird in coveralls because only 10 files are evaluated and all come from currency_rate_update addon

antespi · 2016-10-17T10:51:33Z

Codecov says, that account_reversal addon files ar not tracked:

Where can I change this, in order to include them?

pedrobaeza · 2016-10-17T12:27:24Z

account_reversal/README.rst

- * a checkbox and filter "to be reversed"
- * a link between an entry and its reversal entry
+* a checkbox and filter "to be reversed"
+* a link between an entry and its reversal entry


Explain the limitations of standard reversal system as the reason for this module.

pedrobaeza · 2016-10-17T12:27:32Z

account_reversal/README.rst

+
+Usage
+=====
+


Include usage instructions.

pedrobaeza · 2016-10-17T12:28:07Z

account_reversal/README.rst

+
+
+Credits
+=======

 Module originally developped by Alexis de Lattre <alexis.delattre@akretion.com>


Remove this text, as it's not the template. He is already in the contributors part and the git history.

pedrobaeza · 2016-10-17T12:35:34Z

account_reversal/models/account_move.py

+
+    def _move_lines_reverse_prepare(self, lines, date=False, journal=False,
+                                    line_prefix=False):
+        for line in lines:


With new API, lines should be a recordset instead of a dictionary to ease developments. Remember methods _convert_to_write and _convert_to_read and the possibility to have a in-memory recordset with env['model'].new({})

This is a dict because _move_reverse_prepare (copy_data method use inside) also return a dict with (0,0,{}) tuples as move lines.
So, I don't convert it again to other data structure.

Well, there are several problems in this prepare design, like the fact that you don't know here the source account.move.line, so in practice, there's no use for this method. Remove it or refactor it discarding copy_data dictionary and iterating over source account.move.line for getting the sub-records.

What source account.move.line do you want to know?
Why do you say that "in practice, there's no use for this method"?, if you want to include/modify another field in other addon you can do it easily inheriting this method

The source account.move.line. You have the copied move lines, but not from each journal item they come. How do you use this method then?

Ok, now _move_lines_reverse_prepare receive move, instead of lines, in order to have journal item info.

pedrobaeza · 2016-10-17T12:37:25Z

account_reversal/models/account_move.py

+                data.get('line_ids', []), date=date, journal=journal,
+                line_prefix=line_prefix)
+            moves |= self.create(data)
+        if moves and post:


Post option shouldn't be taken for journal flag?

This option comes from the reversal wizard, we can default it to journal configuration

I let you decide to do what you consider more consistent

Journal has not this flag

pedrobaeza · 2016-10-17T12:48:58Z

account_reversal/models/account_move.py

+        if moves and post:
+            moves.post()
+            if reconcile:
+                for move in moves:


This is @api.multi, so use it: moves.move_reverse_reconcile()

pedrobaeza · 2016-10-17T13:11:43Z

No clue about codecov.

pedrobaeza · 2016-10-18T17:26:13Z

account_reversal/README.rst

@@ -12,9 +12,30 @@ Also add on account entries:
 * a checkbox and filter "to be reversed"
 * a link between an entry and its reversal entry

+Odoo v9c include an action similar (overwritten by this addon), but with less


s/an action similar/a similar action

pedrobaeza · 2016-10-18T17:26:36Z

account_reversal/README.rst

+features, for instance:
+
+* Allowing inheritance
+* Options like prefix (for entry and entry line), post and reconcile.


entry: journal entry
entry line: journal item

pedrobaeza · 2016-10-18T17:27:10Z

account_reversal/README.rst

+
+* If no Reversal Journal is selected, then the same journal is used
+* If Post is True, then reversal entry will be posted else it will be leaved
+as a draft entry


bad indentation (it misses 2 spaces).

I'm not following you. What spaces are missed?

2 spaces at the beginning to align with the list.

* If Post is True, then the reversal entry will be posted else it will be left as a draft entry

elicoidal · 2016-10-19T13:11:46Z

account_reversal/README.rst

+* If Post and Reconcile are True, then all entry lines with reconciled accounts
+of the entry will be reconciled with the reserval entry ones.
+
+Also, there is new menu Accounting > Adviser > Journal Entries to be Reversed


There is also a new menu Accounting > Adviser > Journal Entries to be Reversed
in order to allow tracking entries that must be reserved for any reason.

Done, thanks

sbidoul

Hi there,

I just did a quick test of runbot. Runs fine mostly. Two remarks:

the to_be_reversed flag is not set to False after reversing
the default prefix for items is blank, IMO it should be REV: as for the move reference (in v8 it was the contrary and I think the prefix must be set on both)

sbidoul · 2016-10-30T15:08:15Z

account_reversal/models/account_move.py

+            'date': date,
+            'ref': ref,
+            'to_be_reversed': False,
+            'reversal_id': self.id,


I think the problem is here: to_be_reversed and reversal_id must be set on the original move, not the reversal move.

I set in both, thanks

@antespi I would remove 'reversal_id': self.id

Buth this field is needed to known the move who was reversed. It is the same concept like origin in refund invoices

It's debatable The field is named "Reversal move" so the relation is in the other direction. So it's important that you add it on the other side. It might be ok to leave it here although it's slightly confusing.

I've just understand it, viewing original addon in v8. Sorry, you're right

antespi · 2016-11-03T09:53:30Z

@sbidoul Thanks for your suggestions. I've done all of them

sbidoul · 2016-11-03T13:56:34Z

account_reversal/models/account_move.py

+                data, date=date, journal=journal, line_prefix=line_prefix)
+            moves |= self.create(data)
+            if orig.to_be_reversed:
+                orig.to_be_reversed = False


@antespi orig.reversal_id must be set to the reversal move here.

sbidoul · 2016-11-03T13:58:14Z

account_reversal/models/account_move.py

+            'date': date,
+            'ref': ref,
+            'to_be_reversed': False,
+            'reversal_id': self.id,


@antespi I would remove 'reversal_id': self.id

antespi · 2016-11-04T14:43:17Z

@sbidoul Changes done, thanks

rafaelbn

Please @sbidoul @pedrobaeza could you review again? Comments are attended @Tecnativa

sbidoul · 2016-11-22T14:04:05Z

Looks good. I did a test on runbot, works fine. Thanks!

@alexis-via @adrienpeiffer it would be good to depend on this module in OCA/account-closing 9.0+ to generate cutoff and accrual moves with the to be reversed flag set.

…#387) [IMP] account_reversal: Several improvements * Default date for reverse move is not start_date of the period following the period of the original move (and not the period following today's period) * Return form view of account.move if only 1 move is reversed * Add option to reconcile with the reversal entry.

antespi force-pushed the 9.0-account_reversal branch from 5e2c59e to 264d49e Compare September 30, 2016 10:18

pedrobaeza added the needs review label Sep 30, 2016

antespi force-pushed the 9.0-account_reversal branch from 264d49e to 950a9e8 Compare September 30, 2016 10:51

antespi force-pushed the 9.0-account_reversal branch 2 times, most recently from b5f32fa to 0cc6f47 Compare September 30, 2016 16:46

antespi mentioned this pull request Oct 5, 2016

Fiscal year closing unified for Italy and Spain OCA/l10n-italy#245

Closed

pedrobaeza requested changes Oct 17, 2016

View reviewed changes

pedrobaeza reviewed Oct 18, 2016

View reviewed changes

elicoidal reviewed Oct 19, 2016

View reviewed changes

sbidoul requested changes Oct 30, 2016

View reviewed changes

sbidoul reviewed Oct 30, 2016

View reviewed changes

sbidoul requested changes Nov 3, 2016

View reviewed changes

rafaelbn reviewed Nov 17, 2016

View reviewed changes

sbidoul approved these changes Nov 22, 2016

View reviewed changes

pedrobaeza approved these changes Nov 22, 2016

View reviewed changes

pedrobaeza and others added 4 commits November 22, 2016 15:28

OCA Transbot updated translations from Transifex

751d234

account_reversal: bump version

5a60bea

[MIG] account_reversal

4ff1b53

pedrobaeza force-pushed the 9.0-account_reversal branch from d1dd25c to 4ff1b53 Compare November 22, 2016 14:28

pedrobaeza merged commit a771042 into OCA:9.0 Nov 22, 2016

pedrobaeza deleted the 9.0-account_reversal branch November 22, 2016 14:53

pedrobaeza mentioned this pull request Nov 22, 2016

Migration to version 9.0 #311

Closed

38 tasks

pedrobaeza mentioned this pull request Jan 23, 2017

[10.0]Method refactoring to allow override when reversing an accounting entry odoo/odoo#15179

Merged


		Usage
		=====

[9.0][MIG] account_reversal #405

[9.0][MIG] account_reversal #405

Conversation

antespi commented Sep 30, 2016

pedrobaeza commented Sep 30, 2016

antespi commented Sep 30, 2016

pedrobaeza commented Sep 30, 2016

antespi commented Sep 30, 2016

antespi commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antespi Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elicoidal Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbidoul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antespi commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antespi commented Nov 4, 2016

rafaelbn left a comment

Choose a reason for hiding this comment

sbidoul commented Nov 22, 2016

antespi Oct 18, 2016 •

edited

Loading

elicoidal Oct 19, 2016 •

edited

Loading