Tryton Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(365)

Issue 28811002: account_statement_import: New module (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 1 week ago by pokoli
Modified:
4 months, 1 week ago
Reviewers:
ced, reviewbot
Visibility:
Public.

Description

Patch Set 1 #

Total comments: 12
Unified diffs Side-by-side diffs Delta from patch set Stats (+1692 lines, -0 lines) Patch
A .drone.yml View 1 chunk +9 lines, -0 lines 0 comments Download
A COPYRIGHT View 1 chunk +14 lines, -0 lines 0 comments Download
A INSTALL View 1 chunk +29 lines, -0 lines 0 comments Download
A LICENSE View 1 chunk +674 lines, -0 lines 0 comments Download
A MANIFEST.in View 1 chunk +13 lines, -0 lines 0 comments Download
A README View 1 chunk +35 lines, -0 lines 0 comments Download
A __init__.py View 1 chunk +17 lines, -0 lines 0 comments Download
A doc/index.rst View 1 chunk +9 lines, -0 lines 0 comments Download
A setup.py View 1 chunk +126 lines, -0 lines 0 comments Download
A statement.py View 1 chunk +314 lines, -0 lines 12 comments Download
A statement.xml View 1 chunk +52 lines, -0 lines 0 comments Download
A tests/__init__.py View 1 chunk +9 lines, -0 lines 0 comments Download
A tests/scenario_account_statement_import.rst View 1 chunk +259 lines, -0 lines 0 comments Download
A tests/test_account_statement_import.py View 1 chunk +28 lines, -0 lines 0 comments Download
A tox.ini View 1 chunk +18 lines, -0 lines 0 comments Download
A tryton.cfg View 1 chunk +8 lines, -0 lines 0 comments Download
A view/import_statement_start_view_form.xml View 1 chunk +19 lines, -0 lines 0 comments Download
A view/statement_form.xml View 1 chunk +10 lines, -0 lines 0 comments Download
A view/statement_line_form.xml View 1 chunk +9 lines, -0 lines 0 comments Download
A view/statement_origin_form.xml View 1 chunk +26 lines, -0 lines 0 comments Download
A view/statement_origin_tree.xml View 1 chunk +14 lines, -0 lines 0 comments Download

Messages

Total messages: 3
pokoli
5 months, 1 week ago (2016-09-14 14:53:28 UTC) #1
reviewbot
https://codereview.tryton.org/28811002/diff/1/tests/__init__.py#newcode5 tests/__init__.py:5: E501 line too long (98 > 79 characters) URL: https://codereview.tryton.org/28811002
5 months, 1 week ago (2016-09-14 15:02:49 UTC) #2
ced
4 months, 2 weeks ago (2016-10-04 21:20:20 UTC) #3
https://tryton-rietveld.appspot.com/28811002/diff/1/statement.py
File statement.py (right):

https://tryton-rietveld.appspot.com/28811002/diff/1/statement.py#newcode32
statement.py:32: fields.Selection(STATES, 'Statement State'),
You should use method to get selections from Statement

https://tryton-rietveld.appspot.com/28811002/diff/1/statement.py#newcode35
statement.py:35: date = fields.Date('Date', states=_states, depends=_depends)
For me date should be required.

https://tryton-rietveld.appspot.com/28811002/diff/1/statement.py#newcode38
statement.py:38: states=_states, depends=_depends)
Missing number field

https://tryton-rietveld.appspot.com/28811002/diff/1/statement.py#newcode46
statement.py:46: ('statement', '=', Eval('statement')),
The date should be the same

https://tryton-rietveld.appspot.com/28811002/diff/1/statement.py#newcode49
statement.py:49: state = fields.Selection(ORIGIN_STATES, 'State', readonly=True,
I do not think it needs to have a state.
The Statement state and pending amount are enough.

https://tryton-rietveld.appspot.com/28811002/diff/1/statement.py#newcode65
statement.py:65: cls._transitions |= set((
I do not see the point of a workflow if all transitions are allowed.

https://tryton-rietveld.appspot.com/28811002/diff/1/statement.py#newcode96
statement.py:96: lines_amount = sum(l.amount or Decimal(0) for l in self.lines)
l.amount could fail, you must use getattr with default value.
But maybe it is not needed to have a on_change_with.

https://tryton-rietveld.appspot.com/28811002/diff/1/statement.py#newcode100
statement.py:100: return self.name or self.statement.rec_name
I do not think that the name is rec_name. At best it should be number.
I do not think neither it should be statement rec_name.

https://tryton-rietveld.appspot.com/28811002/diff/1/statement.py#newcode134
statement.py:134: cls.create_move(origins)
I do not think it should be in standard module.
For me, it is not a good practice to post line in advance.

https://tryton-rietveld.appspot.com/28811002/diff/1/statement.py#newcode145
statement.py:145: return Statement.create_move(origins)
For me, it is wrong to create move base on origin. The real information should
always be the lines.

https://tryton-rietveld.appspot.com/28811002/diff/1/statement.py#newcode207
statement.py:207: fields.Selection(ORIGIN_STATES, 'Origin State'),
It is better to use a method that will return the selections of the origin.

https://tryton-rietveld.appspot.com/28811002/diff/1/statement.py#newcode282
statement.py:282: class ImportStatement(Wizard):
Why not run on top of an existing empty statement instead of asking all the
fields in a view.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cd18842