-
-
Notifications
You must be signed in to change notification settings - Fork 245
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
[12.0][l10n_br_fiscal] fiscal usability #1610
Conversation
Hi @rvalyi! Thank you very much for this contribution. As the addon you are improving does not have a declared maintainer, I take the opportunity to mention that you can consider adopting it. To do so, please read the maintainer role description, and, if interested, create a pull request to add your GitHub login to the |
Não vai rolar de fazer apenas cherry-pick porque esses commits não passam o pre-commit... Pessoal, o hook se chama pre-commit para rodar antes de fazer commit, não é a toa... Gosta de resolver conflitos depois? problema seu... |
6087379
to
550932c
Compare
infelizmente botando o teste agora de encontrar apenas uma linha de operação fiscal valida, parece que encontramos varias em alguns testes. Vamos ter que solucionar isso... No PR #1526 não tem esse problema (mas tem muitos outros). Porem eu não pude identificar porque ainda. Ajuda bem vinda. |
@mbcosta seu ultimo commit fez o Travis passar com os dados de demo/teste, porem o modulo purchase ainda nao carrega os dados de demo (deve ser o mesmo tipod e problema) e isso faz o Runbot dar esse warning:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@mbcosta continua com o mesmo problema com os dados de demo do l10n_br_purchase no Runbot:
logs completos aquihttps://runbot2.odoo-community.org/runbot/static/build/3504859-1610-4b70eb/logs/job_20_test_all.txt |
4b70eb0
to
02770a6
Compare
@rvalyi procurei resolver o problema nos dois últimos "commits" preenchendo o campo Tipo Fiscal do Produto na Linha de Operação Fiscal porque assim na hora de mapear o método retorna apenas um registro evitando o erro de "Mais de uma linha de operação selecionada" porém isso torna esse mapeamento especifico para um Tipo Fiscal do Produto para os outros casos se torna necessário incluir uma nova Linha de Operação Fiscal, praticamente igual a primeira apenas com Tipo Fiscal do Produto diferente, só que isso gera outro erro nos testes do l10n_br_purchase que buscam os Impostos a serem usados, porque o Nome da Linha de Operação Fiscal precisa ter o mesmo nome do CFOP https://github.com/OCA/l10n-brazil/blob/12.0/l10n_br_purchase/tests/test_l10n_br_purchase.py#L97 e os nomes das Linhas de Operação Fiscal não podem ser iguais porque existe uma restrição/constraint para não permitir nomes duplicados. Poderíamos considerar retirar esse "raise" quando o mapeamento traz mais de uma Linha de Operação Fiscal, mas acredito que isso foi incluído pensando na situação onde o Responsável pela Parametrização Fiscal é uma pessoa e o Operador/usuário e outra que não vai saber selecionar qual a linha correta deve ser usada quando vir mais de uma, provavelmente uma forma de forçar a parametrização e evitar erros. Também poderíamos considerar alterar o teste do l10n_br_purchase para não ter o problema, mas pelo o que entendi a principal questão a ser considerada é que em uma Linha de Operação Fiscal teria que ser possível associar mais de um Tipo Fiscal do Produto, só que para isso o campo teria que ser um "Many2many" e nesse sentido teríamos que transformar esse campo que hoje é um "Selection" em um novo Objeto, é preciso confirmar se realmente essa alteração precisa ser feita e se é possível fazer via "openupgrade" ou SQL ou se esse problema deve ser visto apenas a partir da v14 ou se o mapeamento de impostos precisa ser entendido e feito de outra forma. cc @renatonlima |
l10n_br_fiscal/models/operation.py
Outdated
line = self.line_ids.search( | ||
self._line_domain(company, partner, product), limit=1 | ||
) | ||
line = self.line_ids.search(self._line_domain(company, partner, product)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pessoal não faz muito sentido essa alteração, pode existir mais de uma linha de operação fiscal que coincida com a operação do documento fiscal, o importante é trazer sempre a regra mais especifica para a mais genérica, podemos fazer melhorias futuras mas neste PR podemos remover essa validação.
02770a6
to
5fd5527
Compare
cf469d4
to
12c9009
Compare
… Fiscal Operations.
12c9009
to
350301e
Compare
O travis esta verde, |
Eu não posso aprovar meu proprio PR, porem os commits não sao meus, foi um cherry-pick de um PR do @mileo . Enfim agora que ta passando os testes eu aprovo, se mais alguém aprovar, bora fazer o merge... |
/ocabot merge minor |
Hey, thanks for contributing! Proceeding to merge this for you. |
It looks like something changed on |
Congratulations, your PR was merged at 5fb2008. Thanks a lot for contributing to OCA. ❤️ |
extração (cherry-pick) de 5 commits dentro dos 24 (com 500 linhas de diff!) de #1526:
Isso não quer dizer que temos que nos limitar apenas a esses 5 commits, longe disso. Mas o que eu vi é que era facil ter consenso dos maintainers do l10n_br_fiscal, então já que copo meio cheio é melhor do que copo vazio...
@mileo eu posso entender a pressa de resolver uma situação num projeto, mas tb não deveria ser novidade para vc que ninguém na OCA inteira faz um PR de 500 linhas e 24 commits a não ser talvez que seja o proprio autor do modulo e olha la. Isso não anda em repo da OCA nenhum, não é frescura nossa não... Se vc quer velocidade, vc tem que fazer como todo mundo, mandar PRs de melhorias de umas 100-200 linhas de diff no maximo por ai.
E confiar de olho fechado no "pacote" só porque te aceitamos como committer quando no mesmo periodo vc manda umas dessas #1562 #820 #806 #1567 ou #1311 sem pena tb não da viu... Serio mesmo. Nesse PR #1526 tem muita coisa que falar ainda ta. Só que como eu falei revisar pacotão de 500 linhas ninguém é obrigado não.
Ai @mileo depois desse merge, a ideia seria que vc faz um rebase da sua branch #1526 e detona esses 5 commits de la (mas nem deveria precisar se vc apenas quiser fazer rebase depois do merge). Mas o melhor mesmo seria vc quebrar um PR desse em 4 ou 5 PRs mesmo. Assim como seu tempo, nosso tempo como revisor e maintainer é precioso...
Nota: fiz cherry-pick dos 5 commits e resolvi 2 conflitos devidos ao avanço no HEAD da 12.0 mesmo (e não a ordem desses 5 commits dentro dos 24). Porem alguns commits não tavam passando o pre-commit e optei para fazer um sexto commit manual para isso, pensando que facilitaria o rebase da branch #1526