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

[12.0][FIX] fiscal document return #960

Merged
merged 4 commits into from
Sep 25, 2020

Conversation

renatonlima
Copy link
Member

  • Não deve chamar o onchange do product_id na linha ao fazer as linhas de devolução (porque deve pegar os dados fiscais exatamente como da nota original);
  • O _create_return espera vários registros, mas é chamdo pelo método action_create_return tem um ensure_one();
  • Implementado uma validação caso a operação fiscal não tenha a operação de retorno definida
  • Não deveria gravar a operação fiscal do cabeçalho da NF-e em todas as linhas, pois cada linha pode ter operações fiscais diferentes e consequentemente operações fiscais de retorno diferentes.

@renatonlima renatonlima force-pushed the 12.0-fix-fiscal-document-return branch 2 times, most recently from 4b73f2c to edaface Compare September 17, 2020 20:29
Copy link
Member

@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.

Small English fixes.

fsc_op = record.fiscal_operation_id.return_fiscal_operation_id
if not fsc_op:
raise ValidationError(_(
"The fiscal operation {} there is not Return Fiscal "
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"The fiscal operation {} there is not Return Fiscal "
"The Fiscal Operation {} there has no Return Fiscal "

l10n_br_fiscal/models/document.py Outdated Show resolved Hide resolved
@rvalyi
Copy link
Member

rvalyi commented Sep 18, 2020

The Travis error was unrelated, it was due to a currency server connnection failure. I restarted the build to ensure it's green.

Copy link
Member

@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.

@renatonlima renatonlima force-pushed the 12.0-fix-fiscal-document-return branch 2 times, most recently from 0624d9c to 98926f4 Compare September 22, 2020 19:44
if not fsc_op:
raise ValidationError(_(
"The fiscal operation {} has no Return Fiscal "
"Operation definied".format(record.fiscal_operation_id)))
Copy link
Member

@rvalyi rvalyi Sep 23, 2020

Choose a reason for hiding this comment

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

definied -> defined
also be consistent if you use cap letters for Fiscal Operation or not in the same sentence (Odoo uses to do it but not mandatory).

rvalyi
rvalyi previously requested changes Sep 23, 2020
Copy link
Member

@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.

definied -> defined
also be consistent if you use cap letters for Fiscal Operation or not in the same sentence (Odoo uses to do it but not mandatory).

if not fsc_op_line:
raise ValidationError(_(
"The fiscal operation {} has no Return Fiscal "
"Operation definied".format(l.fiscal_operation_id)))
Copy link
Member

Choose a reason for hiding this comment

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

definied -> defined
also be consistent if you use cap letters for Fiscal Operation or not in the same sentence (Odoo uses to do it but not mandatory).

@rvalyi rvalyi dismissed their stale review September 23, 2020 06:05

changes attended

Copy link
Member

@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.

looks good!

Copy link
Member

@marcelsavegnago marcelsavegnago left a comment

Choose a reason for hiding this comment

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

LGTM

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@mileo
Copy link
Member

mileo commented Sep 25, 2020

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 12.0-ocabot-merge-pr-960-by-mileo-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 0b2476b into OCA:12.0 Sep 25, 2020
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 37aa56b. Thanks a lot for contributing to OCA. ❤️

mileo pushed a commit to kmee/l10n-brazil that referenced this pull request Mar 2, 2021
Signed-off-by mileo
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.

5 participants