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

[14.0][NEW] l10n_br_mdfe #2603

Closed
wants to merge 61 commits into from
Closed

Conversation

ygcarvalh
Copy link
Contributor

@ygcarvalh ygcarvalh commented Jul 12, 2023

Assim como está sendo feito para a CT-e, e como a MDF-e é um documento que tem relação, a objetivo é pegar a mesma ideia através do PR #2485 e continuar o que estava sendo feito em #851.

Esses modelos iniciais são os que consegui pensar que são os básicos para esse tipo de documento e posteriormente devem ser adicionados mais para abranger todos os casos disponíveis.

Depende dos PR's:

@felipezago felipezago force-pushed the 14.0-add-l10n_br_mdfe branch 3 times, most recently from c6def6b to 7facfa8 Compare September 15, 2023 12:47
@rvalyi
Copy link
Member

rvalyi commented Sep 22, 2023

@ygcarvalh @mileo

Mesma pergunta do que para a CTe ( #2586 ) : não seria ideal ja proceder com o merge de #2485 se vcs acham tudo certo com as classes geridas para a MDFe?

cc @renatonlima @mbcosta @marcelsavegnago @antoniospneto @felipemotter @crsilveira

@mileo
Copy link
Member

mileo commented Sep 27, 2023

Pode fazer um rebase?

@mileo
Copy link
Member

mileo commented Sep 28, 2023

@felipezago acho q vc misturou os PRs ai, tem coisa da nf-e no meio, esta correto?

@felipezago
Copy link
Contributor

@felipezago acho q vc misturou os PRs ai, tem coisa da nf-e no meio, esta correto?

Está certo, é que eu precisei pegar algumas alterações desse PR: #2552

@felipezago felipezago force-pushed the 14.0-add-l10n_br_mdfe branch 4 times, most recently from 705e082 to ba0abaa Compare September 29, 2023 19:25
@felipezago
Copy link
Contributor

felipezago commented Sep 29, 2023

A implementação inicial do MDFe está finalizada, porém para o funcionamento os seguintes PRs são necessários:

Podem verificar por favor?

@mileo @rvalyi

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.

Eu diria que seria melhor deixar a questão da transmissão para um segundo PR. Primeiro seria bom encher de teste de serializaçao e importação de mdfe (algo semelhante ao que foi feito no modulo l10n_br_nfe), depois que tiver bem robusto da para fazer o merge e ai depois ver um PR com a transmissão, pois ai tem muitas possibilidades de melhoria nessa questão de transmissão e é bom não criar divida tecnologica no projeto e nem nas dependencias como erpbrasil.edoc... Pois infelizmente, ja tivemos muitos casos da KMEE enfia tais dividas no projeto e depois de 2 ou 3 anos as dividas continuar la poluindo o code base sem nenhuma correção. Podemos citar: gateway de pagamento que ficou uns 2 anos logando todos dados dos cartões de credito, aquele coisa da DFe que ficou uns 3 anos de codigo morto no l10n_br_fiscal e que vc corrigiu recentemente, mas tem muitos outras coisas nas contribuiçoes da KMEE... Por isso vamos com calma antes de aumentar as contribuições da KMEE, um passo de cada vez e muitos testes por favor.

@rvalyi
Copy link
Member

rvalyi commented Oct 2, 2023

@felipezago veja que cada vez que vc manda um commit isso inicia 20 minutos de testes de CI, alguns rodando no servidores da OCA, como o Runboat... Nisso é bom de testar um pouco localmente, até o pre-commit em vez de fazer 30 pushes por dia...

@felipezago felipezago force-pushed the 14.0-add-l10n_br_mdfe branch 2 times, most recently from 9afee96 to 3f90f00 Compare October 4, 2023 15:10
@rvalyi
Copy link
Member

rvalyi commented Oct 5, 2023

@ygcarvalh @felipezago @mileo pessoal, toda parte de fazer funcionar os _stacked e outros parametros com varios schemas eu vou propor uma implementação alternativa em breve. Eu tava com essa melhoria na cabeça desde a inclusão inicial do modulo spec_driven_model, mas tinham outras prioridades. Eu proponho se segurar o PR até isso, pois não acho limpo a forma de resolver que foi proposta (ficara claro com meu PR alternativo). Nesse meio tempo se isso funciona no projeto de vcs perfeito. Não ira mudar muita coisa depois da nova solução, principalemente vai tirar gambiarras que foram feitas aqui no l10n_br_nfe por examplo. Fora essa questão não vejo grande problema com esse PR hoje, mas é importante a gente não aumentar o trabalho no core fazendo o merge antes dessa solução limpa para lidar com varios esquemas (na vdd é a mesma solução que vai permitir lidar bem com varias versões de um esquema tb).

cc @renatonlima @marcelsavegnago

@ygcarvalh ygcarvalh marked this pull request as ready for review October 6, 2023 11:02
@ygcarvalh ygcarvalh changed the title [14.0][WIP][NEW] l10n_br_mdfe [14.0][NEW] l10n_br_mdfe Oct 6, 2023
@ygcarvalh
Copy link
Contributor Author

@rvalyi você chegou a comentar um tempo atrás que iria propor uma alternativa para o _stacked. Você chegou a fazer isso? Se sim, pode me mostrar o PR para eu ter uma ideia de como atualizar esse módulo para essas questões? Desde já, obrigado.

@marcelsavegnago
Copy link
Member

@ygcarvalh @felipezago @mileo podem fazer um rebase por favor ?

1 similar comment
@marcelsavegnago
Copy link
Member

@ygcarvalh @felipezago @mileo podem fazer um rebase por favor ?

@marcelsavegnago
Copy link
Member

@mileo consegue fazer um rebase desta PR e a do CT-e ?

@rvalyi
Copy link
Member

rvalyi commented Jun 14, 2024

sendo que como recentemente eu trabalhei na migração do l10n_br_nfe para as v15 (commentei la) e a v16 eu consigo logo propor as pequenas mudanças nos spec_driven_model para superar os detalhes que impedia os merges dos módulos de mdfe e cte... Depois os cherry picks disso entre as v14 e v16+ vai ser mole tb (na real so não é mole com o módulo l10n_br_account).

@marcelsavegnago
Copy link
Member

Um squash dos commits também ajudaria.. percebi alterações no mesmo local em commits distintos

@rvalyi
Copy link
Member

rvalyi commented Jun 14, 2024

sei la as vezes para módulos complexos pode ser justificado um pequeno histórico. Agora sim teve muitas vezes que os módulos entraram com muitos commits simplesmente porque quem fazia os commits não dominava o git...

Por exemplo nos módulos de SPED dexei algumas dezenas de commits que é relativamente pouco para adicionar 70k linhas de codigo. Mas eu consegui deixar "apenas" essas pequenas dezenas de commits porque dei dezenas de rebases e squashes para limpar qdo eu tava prototipando... Eh importante a gente se esforçar sim para fazer essas coisas com qualidade. Pois senão vc nunca vai poder abrir um git blame e entender porque fulano que não mexe mais com Odoo faz 3 anos fez aquele código daquele jeito estranho, coisa que vc geralmente consegue a explicação nos módulos da OCA...

@marcelsavegnago
Copy link
Member

Para revisar fica ruim também. Mas concordo com um pequeno histórico.

@marcelsavegnago
Copy link
Member

@mileo consegue fazer um rebase por favor?

Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 20, 2024
@rvalyi
Copy link
Member

rvalyi commented Oct 20, 2024

substituído por #3445

@rvalyi rvalyi closed this Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants