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

11.0 mig account_multicurrency_revaluation #64

Merged
merged 42 commits into from Jan 23, 2018
Merged

11.0 mig account_multicurrency_revaluation #64

merged 42 commits into from Jan 23, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jan 5, 2018

No description provided.

yvaucher and others added 30 commits January 5, 2018 12:21
- one for the business feature (account_multicurrency_revaluation)
- one for the report feature (account_multicurrency_revaluation_report)

I move the report feature to a dedicated module because else the whole module could not be upgraded because of this pull request: OCA/webkit-tools#10
Indeed, the module previously depended on the module base_headers_webkit. Now, only the module for the report (account_multicurrency_revaluation_report) will depend on it.

The reference to the currency type has been removed because it has been removed from the core addons in v8.

The code is not fully ported to the new API yet.
…folder and the views into a new 'views' folder
…PI consistency (mainly the use of the context). New README.rst file.
…E.rst files because this file is for individual
@fclementic2c
Copy link
Member

@OleksandrPaziuk Please ping me when travis is green so I can test on runbot. Thanks (and happy new year by the way)

Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

tests fail for missing dependency


.. image:: https://odoo-community.org/website/image/ir.attachment/5784_f2813bd/datas
:alt: Try me on Runbot
:target: https://runbot.odoo-community.org/runbot/89/10.0
Copy link
Contributor

Choose a reason for hiding this comment

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

/11.0



class AccountConfigSettings(models.TransientModel):
_inherit = 'account.config.settings'
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK this model does not exist anymore. res.config.settings holds them all

# Small class that avoid to override account account object
# only for pure perfomance reason.
# Browsing an account account object is not efficient
# beacause of function fields
Copy link
Contributor

Choose a reason for hiding this comment

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

*because

# only for pure perfomance reason.
# Browsing an account account object is not efficient
# beacause of function fields
# This object aim to be easly transpose to account account if needed
Copy link
Contributor

Choose a reason for hiding this comment

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

*aims ... *transposed

"""Get all line account move line that are need on report for current
account.
"""
sql = """Select res_partner.name,
Copy link
Contributor

Choose a reason for hiding this comment

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

*SELECT

revaluation_date = fields.Date(
string='Revaluation Date',
required=True,
default=_get_default_revaluation_date
Copy link
Contributor

Choose a reason for hiding this comment

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

use lambda to proxy the method

domain="[('type','=','general')]",
help="You can set the default journal in company settings.",
required=True,
default=_get_default_journal_id
Copy link
Contributor

Choose a reason for hiding this comment

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

same

help="This label will be inserted in entries description. "
"You can use %(account)s, %(currency)s and %(rate)s keywords.",
required=True,
default="%(currency)s %(account)s "
Copy link
Contributor

Choose a reason for hiding this comment

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

same, delegate to a method

@return: ids of created move_lines
"""

def create_move_and_lines(amount, debit_account_id, credit_account_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

move this to a static method

account_obj = self.env['account.account']

company = self.journal_id.company_id or self.env.user.company_id
if (not company.revaluation_loss_account_id and
Copy link
Contributor

Choose a reason for hiding this comment

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

split this to _check_company static method.

@ghost
Copy link
Author

ghost commented Jan 9, 2018

@fclementic2c Travis GREEN!

Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

please squash translation commits all together at the end

@simahawk
Copy link
Contributor

@OleksandrPaziuk I'd squash also some small commits among these:
image

/cc @yvaucher @grindtildeath reviews welcome 😉

@fclementic2c
Copy link
Member

fclementic2c commented Jan 11, 2018

I did a test on this PR runbot. I did not setup default accounts in the settings and I got a nasty message when I clicked on Validate button from the wizard :
image

Can you catch this error with a nicer message please?
(I know this bug was already there before but let's fix it)

Error:
Odoo Server Error

Traceback (most recent call last):
File "/.repo_requirements/odoo/odoo/http.py", line 646, in _handle_exception
return super(JsonRequest, self)._handle_exception(exception)
File "/.repo_requirements/odoo/odoo/http.py", line 307, in _handle_exception
raise pycompat.reraise(type(exception), exception, sys.exc_info()[2])
File "/.repo_requirements/odoo/odoo/tools/pycompat.py", line 87, in reraise
raise value
File "/.repo_requirements/odoo/odoo/http.py", line 683, in dispatch
result = self._call_function(**self.params)
File "/.repo_requirements/odoo/odoo/http.py", line 339, in _call_function
return checked_call(self.db, *args, **kwargs)
File "/.repo_requirements/odoo/odoo/service/model.py", line 97, in wrapper
return f(dbname, *args, **kwargs)
File "/.repo_requirements/odoo/odoo/http.py", line 332, in checked_call
result = self.endpoint(*a, **kw)
File "/.repo_requirements/odoo/odoo/http.py", line 927, in call
return self.method(*args, **kw)
File "/.repo_requirements/odoo/odoo/http.py", line 512, in response_wrap
response = f(*args, **kw)
File "/home/odoo/OCB-11.0/addons/web/controllers/main.py", line 928, in call_button
action = self._call_kw(model, method, args, {})
File "/home/odoo/OCB-11.0/addons/web/controllers/main.py", line 916, in _call_kw
return call_kw(request.env[model], method, args, kwargs)
File "/.repo_requirements/odoo/odoo/api.py", line 689, in call_kw
return call_kw_multi(method, model, args, kwargs)
File "/.repo_requirements/odoo/odoo/api.py", line 680, in call_kw_multi
result = method(recs, *args, **kwargs)
File "/home/odoo/build/OCA/account-closing/account_multicurrency_revaluation/wizard/wizard_currency_revaluation.py", line 278, in revaluate_currency
_("No revaluation or provision account are defined"
Warning: No revaluation or provision account are defined for your company.
You must specify at least one provision account or a couple of provision account.

"""
Get last date of previous fiscalyear
"""
return fields.date.today()

Choose a reason for hiding this comment

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

Is this working as intended? Otherwise docstring is wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

Docstring is wrong. I should have changed it while migrating to v9, but it went through.
As fiscal year don't exist anymore, the default should be today.

@ghost
Copy link
Author

ghost commented Jan 11, 2018

@fclementic2c I replace python Warning to odoo.exceptions Warning if it still not good let me know.

@fclementic2c
Copy link
Member

@OleksandrPaziuk Thanks the fix is ok.

I have another bug now: I have launched the unrealized gain/loss report (after I have successfully generated revaluation entries on the 31/12/2017) but I get this error:
image

Error:
Odoo Server Error

Traceback (most recent call last):
File "/home/odoo/OCB-11.0/addons/web/controllers/main.py", line 74, in wrap
return f(*args, **kwargs)
File "/home/odoo/OCB-11.0/addons/web/controllers/main.py", line 1521, in index
action["report_name"], report_ids, report_data, context])
File "/.repo_requirements/odoo/odoo/http.py", line 115, in dispatch_rpc
result = dispatch(method, params)
UnboundLocalError: local variable 'dispatch' referenced before assignment

@p-tombez
Copy link

@OleksandrPaziuk @fclementic2c Seems to be related to the fact that they removed the 'report' service : odoo/odoo@c23ef9a#diff-03542f2676ca18278ea62cb40f5a2261 and the issue is very similar to this one : odoo/odoo#21244 but I have no clue on how to fix the problem :(

data,
),
}
return self.env['report'].render(self._name[7:], docargs)
Copy link
Member

Choose a reason for hiding this comment

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

This won't work in Odoo 11.0

What is needed is to use the new report structure. report module has been splitted between web and base modules.
The good new is that it is now pretty simple to render a specific report. You need to use the xmlid:

self.env.ref('foo_report_xmild').render(recordset, data=data)

Thus in your case, try:

self.env.ref('account_multicurrency_revaluation.curr_unrealized').render(docs)

@@ -30,6 +30,31 @@ class AccountAccount(models.Model):
"balance",
}

def init(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

docstring pls

@api.onchange('user_type_id')
def _onchange_user_type_id(self):
for rec in self:
if rec.user_type_id.id in rec._get_revaluation_account_types():
Copy link
Contributor

Choose a reason for hiding this comment

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

call self._get_revaluation_account_types at the top, not for any records.

domain="[('currency_revaluation', '=', True)]"
string='Accounts',
domain="[('currency_revaluation', '=', True)]",
default=lambda rec: rec._default_account_ids(),
Copy link
Contributor

Choose a reason for hiding this comment

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

self: self.

return self.env.ref(
'account_multicurrency_revaluation.'
'action_report_currency_unrealized'
).report_action(docids, config=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

for your future self you might to put a comment about that config arg

"report/report.xml",
"report/unrealized_currency_gain_loss.xml",
],
"tests": [
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not needed

account_id, currency_id, partner_id = \
line['id'], line['currency_id'], line['partner_id']

accounts.setdefault(account_id, {})
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use collections.defaultdict 😉

Choose a reason for hiding this comment

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

I'm pretty sure nesting defaultdicts will make it less readable :\


class ShellAccount(object):

# Small class that avoid to override account account object
Copy link
Contributor

Choose a reason for hiding this comment

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

move this to docstring following PEP257

fields.date.today().strftime(DATE_FORMAT))
self.assertEqual(wizard.journal_id, self.reval_journal)

def setUp(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

better to:

  1. inherit from SavepointCase
  2. move setUp code to setUpClass
  3. move this method at the top

@mpanarin
Copy link

@yvaucher @grindtildeath @simahawk
Please update your reviews on the module and test it functionally if possible
(i did test it locally and it seems to be working properly)
Runbot is red for no apparent reason again.

@fclementic2c fclementic2c merged commit cfba3b7 into OCA:11.0 Jan 23, 2018
@grindtildeath
Copy link
Contributor

I'm not sure this should have been merged so, there are still a few things to improve IMO 😕

Copy link
Contributor

@grindtildeath grindtildeath left a comment

Choose a reason for hiding this comment

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

Not sure if I can still post the review....

<label string="Odoo will generate exchange rate difference entries for each account set as 'Allow Currency revaluation'.
If the account type is payable or receivable : 1 entry will be generated per account/currency/partner
for other account type : 1 entry will be generated per account/currency
You can check details of calculation thanks to the Print Currency unrealized report in the generic reports."/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add '.' to separate sentences.

"""
Get last date of previous fiscalyear
"""
return fields.date.today()
Copy link
Contributor

Choose a reason for hiding this comment

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

Docstring is wrong. I should have changed it while migrating to v9, but it went through.
As fiscal year don't exist anymore, the default should be today.

<field name="arch" type="xml">
<xpath expr="//div[@id='invoicing_settings']" position="after">
<h2>Multicurrency revaluation</h2>
<div class="row mt16 o_settings_container" id="multicurrency_revaluation_settings">
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this whole div should be reworked that it integrates nicely with the new settings wizard. (i.e. company dependent fields, grid for accounts, etc). Also I think we should hide the whole section if the user doesn't have the security group.
image

_("No revaluation or provision account are defined"
" for your company.\n"
"You must specify at least one provision account or"
" a couple of provision account.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add :
"You must specify at least one provision account or a couple of provision account in the accounting settings"

<field name="currency_id" ref="base.GBP"/>
</record>
<record model="res.currency.rate" id="currency_rate_gbp_2017_03">
<field name="name">2017-03-15</field>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change all the names so the year is automatically the actual one ? Otherwise it's a pain to test it manually.
Test case would have to be adapted as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet