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

8.0 l10n br account payment mode #580

Merged
merged 46 commits into from
Aug 17, 2018

Conversation

rvalyi
Copy link
Member

@rvalyi rvalyi commented Oct 17, 2017

No description provided.

Copy link
Member Author

@rvalyi rvalyi left a comment

Choose a reason for hiding this comment

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

as for tests, there is no much logic here, so just having the module installing is probably okay. Installation depends on the l10n_br_account_banking_payment merge though. Meanwhile minor changes are required. cc @mbcosta @mileo @renatonlima

* l10n_br_account
* l10n_br_data_base
* account_due_list_payment_mode
* account_banking_payment_export
Copy link
Member Author

Choose a reason for hiding this comment

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

account_banking_payment_export dependency is listed twice

'account_due_list_payment_mode',
'account_banking_payment_export',
'l10n_br_account_banking_payment',
],
Copy link
Member Author

Choose a reason for hiding this comment

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

these dependencies look fine but require OCA/bank-payment and OCA/account-payment to be added in the oca_dependencies.txt file of the repo.
Also this PR depends on the merge of l10n_br_account_banking_payment in this repo. I tried to extract it with its relative commits but didn't succeed yet. I'll try it again soon.

# You should have received a copy of the GNU Affero General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
#
##############################################################################
Copy link
Member Author

Choose a reason for hiding this comment

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

prefer shorter license headers, specially for the init.py files

@api.constrains('bank_bic')
def check_bic_length(self):
'''
sobrescrever constrains do core que não leva em consideração bancos
Copy link
Member Author

Choose a reason for hiding this comment

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

nao sou eu o brasileiro mas o portugues parece zoado. Nao seria melhor
"sobreescreve constraints do core..." ?

sobrescrever constrains do core que nao leva em consideração bancos
que nao são intenacionais.
'''
return True
Copy link
Member Author

Choose a reason for hiding this comment

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

nao sou eu o brasileiro mas o portugues parece zoado. Nao seria melhor
"sobreescreve constraints do core..." ?

# You should have received a copy of the GNU Affero General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
#
##############################################################################
Copy link
Member Author

Choose a reason for hiding this comment

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

short style license header please

# You should have received a copy of the GNU Affero General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
#
##############################################################################
Copy link
Member Author

Choose a reason for hiding this comment

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

short style license headers please

@mbcosta
Copy link
Contributor

mbcosta commented Jan 30, 2018

Hi @rvalyi
Requested changes done , now just waiting review and merge of PR #586

@mbcosta mbcosta force-pushed the 8.0-l10n_br_account_payment_mode branch from cf58b58 to 03cb284 Compare March 19, 2018 17:34
Copy link
Member Author

@rvalyi rvalyi left a comment

Choose a reason for hiding this comment

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

minor changes requested

Brazil Account Banking - Payments Mode
======================================

This module provide an mode payment for payment orders.
Copy link
Member Author

Choose a reason for hiding this comment

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

s/provide/provides
s/an mode for payment orders/a mode for payment orders in Brazil

# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).

from . import models

Copy link
Member Author

Choose a reason for hiding this comment

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

remove last line


from . import payment_mode
from . import account_invoice

Copy link
Member Author

Choose a reason for hiding this comment

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

remove last line

@mbcosta
Copy link
Contributor

mbcosta commented Mar 22, 2018

Changes done @rvalyi

@mileo mileo requested review from hendrixcosta and mileo March 23, 2018 18:27
@mbcosta
Copy link
Contributor

mbcosta commented Apr 20, 2018

Error on travis seems not be related with this PR

" no such option: --allow-external

The command "pip install https://github.com/aricaldeira/PySPED/archive/master.zip#egg=PySPED-1.1.0 --allow-external PyXMLSec --allow-insecure PyXMLSec" failed and exited with 2 during . "

@mbcosta
Copy link
Contributor

mbcosta commented Jul 11, 2018

Waiting merge #606

# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).

from . import payment_mode
from . import account_invoice
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no account_invoice file

@mileo mileo merged commit 081d197 into OCA:8.0 Aug 17, 2018
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.

8 participants