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] Edoc Send Email #3183

Draft
wants to merge 7 commits into
base: 14.0
Choose a base branch
from
Draft

[14.0][NEW] Edoc Send Email #3183

wants to merge 7 commits into from

Conversation

mileo
Copy link
Member

@mileo mileo commented Jul 17, 2024

Uppport: #1406

@OCA-git-bot
Copy link
Contributor

Hi @renatonlima,
some modules you are maintaining are being modified, check this out!

@antoniospneto
Copy link
Contributor

Sei que a PR ainda está em rascunho, mas gostaria de deixar minha opinião sobre o gatilho do método de envio de e-mail. Atualmente, esse método é acionado ao mudar o estado do documento, o que faz com que a lógica de envio seja processada na mesma requisição de comunicação com os webservices. Isso inclui a criação dos PDFs necessários para o anexo do e-mail, tornando a preparação do e-mail bastante pesada, mesmo que o envio em si seja programado para ocorrer em seguida.

Acredito que seria interessante remover esse gatilho para que o envio do e-mail não seja acionado na mesma requisição. Uma solução seria marcar uma flag, como "envio de e-mail pendente", e utilizar um cron para verificar a cada 1 ou 5 minutos os e-mails que precisam ser enviados, acionando então o método send_email.

O que acham?

@mileo @marcelsavegnago @rvalyi @renatonlima @felipemotter

@mileo
Copy link
Member Author

mileo commented Jul 17, 2024

Sei que a PR ainda está em rascunho, mas gostaria de deixar minha opinião sobre o gatilho do método de envio de e-mail. Atualmente, esse método é acionado ao mudar o estado do documento, o que faz com que a lógica de envio seja processada na mesma requisição de comunicação com os webservices. Isso inclui a criação dos PDFs necessários para o anexo do e-mail, tornando a preparação do e-mail bastante pesada, mesmo que o envio em si seja programado para ocorrer em seguida.

Acredito que seria interessante remover esse gatilho para que o envio do e-mail não seja acionado na mesma requisição. Uma solução seria marcar uma flag, como "envio de e-mail pendente", e utilizar um cron para verificar a cada 1 ou 5 minutos os e-mails que precisam ser enviados, acionando então o método send_email.

O que acham?

@mileo @marcelsavegnago @rvalyi @renatonlima @felipemotter

Vou considerar isso na implementação.

@rvalyi
Copy link
Member

rvalyi commented Jul 17, 2024

alem disso, essas coisas de notificação por email é mais um examplo de código que poderia ser estraido num módulo pequeno. Se o l10n_br_fiscal pesasse apenas 5000 linhas, seria de boa ter isso dentro. Mas com mais de 20k linhas definitivamente faz parte das coisas que poderiam ir num novo modulo...

O foco do l10n_br_fiscal realmente tem que ser o conceito basico de documento fiscal e o calculo dos impostos que até empresas do simples podem ter ou que são comuns numa NFe ou NFSe, o restante da para botar em outros módulos geralmente. Apenas com esse escopo que eu mentionei, já é difícil fazer menos de 10k linhas... Mas hoje 80% ou mais dos módulos que dependem do l10_b_fiscal não dependem dessas coisas de email, logo não ha motivo algum de ficar junto no mesmo modulo...

@rvalyi
Copy link
Member

rvalyi commented Jul 17, 2024

sendo que se for um outro módulo e vc nao gostar do desempenho, ai vc poderia nao usa esse módulo ou usa um modulo alternativo.

Tb hoje por exemplo o l10_br_fiscal é relativamente facil de migrar, mas por exemplo para migrar ele para a v16, eu tive que gastar tempo migrando os templates de e-mail de mako para qweb. Algo que não era tão necessário para destravar a migração de outros módulos; enfim apenas um exemplo de porque essa falta de modularidade do módulo l10n_br_fiscal é ruim.

@mileo
Copy link
Member Author

mileo commented Jul 17, 2024

alem disso, essas coisas de notificação por email é mais um examplo de código que poderia ser estraido num módulo pequeno. Se o l10n_br_fiscal pessasse apenas 5000 linhas, seria de boa ter isso dentro. Mas com mais de 20k linhas definitivamente faz parte das coisas que poderiam ir num novo modulo...

O foco do l10n_br_fiscal realmente tem que ser o conceito basico de documento fiscal e o calculo dos impostos que até empresas do simples podem ter ou que são comuns numa NFe ou NFSe, o restante da para botar em outros módulos geralmente. Apenas com esse escopo que eu mentionei, já é difícil fazer menos de 10k linhas...

Isso eu concordo que pode ser extraído em outro módulo sem problemas, pois tem as mesma sistemática do fiscal_close e não impacta vendas/compras/estoque.

Ou seja não faz parte do motor de impostos.

@antoniospneto
Copy link
Contributor

antoniospneto commented Jul 17, 2024

acho interessante por em outro módulo tbm, até pq realmente podem ser usado estrategias diferentes para o envio dos e-mails. nós por exemplo estamos usando muito o módulo nativo de ações automatizadas para esse fim.

@rvalyi
Copy link
Member

rvalyi commented Jul 17, 2024

entao sobre extrair, quando mais cedo melhor... Alem disso isso sim eh algo que o cara nao precisa ser pica das galaxias para conseguir fazer.

@mileo
Copy link
Member Author

mileo commented Jul 17, 2024

Posso fazer isso nesse PR, primeiro passo é fazer isso pegar direito, ontem eu só fiz o cherry-pick dos commits e por incrível que pareça os teses passaram. Vamos revisar funcionalmente se continuar ok e planejar a extração.

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 Jan 26, 2025
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