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

9.0 Port account_cutoff_base and account_cutoff_prepaid #37

Merged
merged 5 commits into from
Oct 17, 2016

Conversation

alexis-via
Copy link
Contributor

Extract a new module account_invoice_start_end_dates from account_cutoff_prepaid

Warning: it is not a port of the OCA/account-closing/8.0 version of the module, but a port of my LP branch https://code.launchpad.net/~akretion-team/account-closing/80-forecast-prepaid which is still the branch I use for Odoo v8 (which explains why I wasn't active on OCA/account-closing so far). This is due to the fact that my "forecast" branches were not merged on LP before the transition to github.

So the next step is to merge the few enhancements that took place in the OCA version of the module.

Extract a new module account_invoice_start_end_dates from account_cutoff_prepaid
@alexis-via
Copy link
Contributor Author

And of course, this PR is also a port to the new API.

@pedrobaeza pedrobaeza mentioned this pull request Sep 28, 2016
7 tasks
@alexis-via
Copy link
Contributor Author

@adrienpeiffer I don't see the pull request you told me about...

[ADD] Add some hooks that are in 8.0
Copy link
Contributor

@adrienpeiffer adrienpeiffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Code review and functional test

@adrienpeiffer
Copy link
Contributor

@chrisdec do you have a moment to test it on runbot ? 😄

Copy link
Member

@gurneyalex gurneyalex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code review, no test.

@gurneyalex gurneyalex added this to the 9.0 milestone Oct 13, 2016
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several change across full code


@api.model
def _inherit_default_cutoff_account_id(self):
'''Function designed to be inherited by other cutoff modules'''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstrings should use double triple-quota (applicable to all)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If docstring should really use double triple-quota, then why flake8/pylint says nothing about it ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good point. We need to add that check. But you can see here that what I say is correct: https://www.python.org/dev/peps/pep-0257/#id15 (For consistency, always use """triple double quotes""" around docstrings. Use r"""raw triple double quotes""" if you use any backslashes in your docstrings. For Unicode docstrings, use u"""Unicode triple-quoted strings""" .)

movelines_to_create.append((0, 0, vals))
amount_total += amount

# add contre-partie
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

counter-part

</data>
</openerp>
<odoo>
<data noupdate="1">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put directly <odoo noupdate="1">

-->

<openerp>
<odoo>
<data>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove <data> tag

<field name="cutoff_journal_id"/>
<field name="cutoff_account_id"/>
<field name="move_label"/>
<field name="cutoff_journal_id" required="1"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be put (required flag) at field level?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is set at view level because it is inherited in the module account_cutoff_prepaid and replaced by attrs = {'required': [('forecast', '=', False)]}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks for the explanation. A comment in the XML would save this question for future readers of the code.

@@ -6,8 +6,8 @@ msgid ""
msgstr ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove POT file

-->

<odoo>
<data>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove <data> tag

_inherit = 'account.invoice'

def inv_line_characteristic_hashcode(self, invoice_line):
'''Add start and end dates to hashcode used when the option "Group
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double quote

@chrisdec
Copy link

Hello,

Functional test :

Just one note : is it possible to show the message "Don't forget to Re-Generate Lines after entering or leaving forecast mode." only when user actives or disables the checkbox "forecast"? It's quite confusing to have this message when you open a new prepaid expense or a new prepaid revenue.

Except that, 👍

Use triple double quotes for docstring
Replace <openerp> by <odoo> in XML
Remove <data> tags in XML
Remove POT files
@alexis-via
Copy link
Contributor Author

@chrisdec Yes, we need to find a better way. I guess a better way would be to use a button to enter/leave forecast mode (like the "Active" button on partners for example) and delete all lines when the user clicks on this button. I'll try to find time to do that...

@pedrobaeza pedrobaeza merged commit 99ab00b into OCA:9.0 Oct 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants