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] [FIX] XML "show preview" not translatable #3004

Merged
merged 2 commits into from
Nov 21, 2022

Conversation

primes2h
Copy link
Contributor

@primes2h primes2h commented Nov 4, 2022

immagine

immagine

@primes2h primes2h marked this pull request as draft November 4, 2022 23:03
@primes2h primes2h force-pushed the 14.0-fix-show_preview branch 3 times, most recently from 7dc5359 to 5f2768e Compare November 6, 2022 20:18
@primes2h primes2h marked this pull request as ready for review November 6, 2022 20:30
@primes2h
Copy link
Contributor Author

primes2h commented Nov 6, 2022

Il pre-commit 🔴 non dipende da questa PR.

@primes2h primes2h force-pushed the 14.0-fix-show_preview branch 3 times, most recently from 45be980 to 588b722 Compare November 7, 2022 13:30
@primes2h
Copy link
Contributor Author

primes2h commented Nov 7, 2022

Pre-commit ora è 🟢

Copy link
Member

@tafaRU tafaRU left a comment

Choose a reason for hiding this comment

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

Grazie della PR!
Per evitare duplicazioni di codice potresti definire il metodo ftpa_preview nel modulo l10n_it_fatturapa analogamente a quanto fatto per ftpa_preview_link?

ftpa_preview_link = fields.Char(

@primes2h
Copy link
Contributor Author

Grazie della PR! Per evitare duplicazioni di codice potresti definire il metodo ftpa_preview nel modulo l10n_it_fatturapa analogamente a quanto fatto per ftpa_preview_link?

Fatto.

@primes2h primes2h requested a review from tafaRU November 21, 2022 08:11
@tafaRU
Copy link
Member

tafaRU commented Nov 21, 2022

Grazie @primes2h. Un'ultima cosa: puoi splittare i commit in due? Uno per ogni modulo coinvolto. Grazie.

@primes2h
Copy link
Contributor Author

@tafaRU

Fatto!

Copy link
Member

@tafaRU tafaRU left a comment

Choose a reason for hiding this comment

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

Ti avevo erroneamente indicato:

Uno per ogni modulo coinvolto

In questo caso specifico va bene come hai fatto ora tu:

image

infatti il primo dei due è corretto che contenga anche le modifiche a l10n_it_fatturapa , spezzarli ulteriormente avrebbe infatti comportato l'aggiunta di un commit in cui il modulo non era funzionante.

L'ho anche testata su runboat 👍

@primes2h
Copy link
Contributor Author

Ti avevo erroneamente indicato:

Uno per ogni modulo coinvolto

In questo caso specifico va bene come hai fatto ora tu:

image

infatti il primo dei due è corretto che contenga anche le modifiche a l10n_it_fatturapa , spezzarli ulteriormente avrebbe infatti comportato l'aggiunta di un commit in cui il modulo non era funzionante.

Infatti mi sembrava l'unico modo per gestire correttamente la cosa.
Grazie!

@primes2h
Copy link
Contributor Author

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-3004-by-primes2h-bump-patch, awaiting test results.

@TheMule71
Copy link
Contributor

Allora, la butto lì...
visto che il metodo è statico, non so se chiamerei il parametro self, piuttosto record (esattamente come per i metodi di classe si preferisce cls invece di self). E una mera questione di leggibilità.

E di conseguenza, i miei sensi di ragno mi dicono che forse un ensure_one() da qualche parte ci va, perché credo che sia un recordset e da qualche parte (magari prima di chiamare il metodo statico) bisogna assicurarsi che la lunghezza sia 1 (altrimenti chiamerei la variabile records o rset).

@TheMule71
Copy link
Contributor

Too late... :)

@OCA-git-bot OCA-git-bot merged commit 13b477b into OCA:14.0 Nov 21, 2022
@OCA-git-bot
Copy link
Contributor

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

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