-
-
Notifications
You must be signed in to change notification settings - Fork 246
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
[14.0][IMP] l10n_br_fiscal: Melhoria no funcionamento dos impostos estimados #3373
base: 14.0
Are you sure you want to change the base?
Conversation
310b6fe
to
2ee3857
Compare
Hi @renatonlima, |
@corredato se possível fecha então a PR #3152 |
2ee3857
to
6fd0ec9
Compare
@marcelsavegnago @antoniospneto @rvalyi @renatonlima podem revisar plz. |
Pessoal, eu pretendo revisar essa PR com mais calma nos proximos dias. |
Tem um system parameter que foi alterado uns meses atrás e não esta setado por padrão. @corredato pode incluir isso no seu PR como uma melhoria? ibpt_request_timeout |
Sim, pode deixar |
active = fields.Boolean(string="Ativo", default=True) | ||
|
||
def _deactivate_old_estimates(self): | ||
for estimate in self: | ||
domain = [ | ||
("id", "!=", estimate.id), | ||
("state_id", "=", estimate.state_id.id), | ||
"|", | ||
("ncm_id", "=", estimate.ncm_id.id), | ||
("nbs_id", "=", estimate.nbs_id.id), | ||
] | ||
old_estimates = self.search(domain) | ||
old_estimates.write({"active": False}) | ||
|
||
@api.model_create_multi | ||
def create(self, vals_list): | ||
estimates = super().create(vals_list) | ||
estimates._deactivate_old_estimates() | ||
return estimates |
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.
O que motivou a criar esse campo de ativo? não vejo muita necessidade nisso.
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.
Vi que isso foi proposto pelo @renatonlima aqui:
#3152 (comment)
Mas tem que ver melhor, da forma que tá sendo feito não tá fazendo diferença..
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, o que eu acho que dá pra fazer, é manter apenas uma linha de imposto estimado para cada estado no NCM/NBS. não acho necessário fazer um controle de ativo ou desativo. Nem é interessante manter um histórico das taxas antigas, isso só geraria muito lixo no banco de dados, só de NCM temos 40 mil linhas.. pensa no quanto isso pode crescer.
Poderia até fazer uma validação para não permitir dois impostos aproximados para o mesmo Estado, mesmo NCM.
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.
talvez já daria para manter o histórico das mudanças no chatter apenas?
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.
acho que por a data de sincronização na linha do imposto estimado já seria o bastante, se não o chatter vai ficar monstruoso tbm, a configuração padrão é atualizar a cada 15 dias, acho que vai ser muito spam, opnião minha.
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.
concordo no módulo l10n_br_fiscal é melhor assim mesmo. Tem como logar isso num módulo extra se alguém quiser. Num projeto eu sobrecarreguei a logica do chatter para conservar apenas os ultimos N mensagens. Mas melhor guardar isso para um modulo extra do que acumular muito mais coisas nesse módulo.
def get_estimated_taxes(self): | ||
self.ensure_one() | ||
object_field = OBJECT_FIELDS.get(self._name) | ||
last_estimated = self.env["l10n_br_fiscal.tax.estimate"].search( | ||
[ | ||
(object_field, "=", self.id), | ||
("state_id", "=", self.env.company.state_id.id), | ||
], | ||
order="create_date DESC", | ||
limit=1, | ||
) |
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.
Aqui eu ainda não consegui compreender qual vantagem de trocar compute por esse método..
Tem que ver se isso é o que o @renatonlima queria, talvez não era bem isso..
Eu penso que talvez não é o mais seguro buscar o estado de dentro da empresa ativa.
Acho que seria melhor o local que precisar dessa informação informar o estado, por exemplo na Fatura, seria pego o estado de dentro do company_id definido na fatura.
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.
Concordo na parte de remover o company_id.
Concordo em por o timeout padrão.
Quanto aos campos computados estimate_tax_national, estimate_tax_imported que foram substituidos por um método, precisa ser descutido melhor.
Quanto ao controle de estimativas ativas, tbm penso que tem que ser discutido melhor, não tá claro isso.
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.
ValueError: Invalid field l10n_br_fiscal.tax.estimate.company_id in leaf ('company_id', 'in', [1])
Ta dando um erro estranho ao inserir um item, pode verificar?
840d599
to
e9712a0
Compare
Esta PR substitui a PR #3152
@renatonlima Assim que puder, pode fazer a revisão? Fiz a melhoria com base na segunda solução que você propôs no PR #3152
Resumindo: