-
-
Notifications
You must be signed in to change notification settings - Fork 305
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][ADD] l10n_it_central_journal_reportlab: aggiunto modulo libro giornale stampa reportlab #2927
base: 12.0
Are you sure you want to change the base?
[12.0][ADD] l10n_it_central_journal_reportlab: aggiunto modulo libro giornale stampa reportlab #2927
Conversation
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.
A parte il cambio di nome, è identico a quanto già approvato in #2754.
c8314c5
to
dbd1c52
Compare
dbd1c52
to
660861d
Compare
Ci sarebbe da trovare un modo per escludere l'installazione contemporanea con |
Per impedire l'installazione contemporanea dei due moduli, puoi usare la chiave Per quanto riguarda i test, ti riporto quello che ti ho suggerito settimana scorsa in Discord:
(rif. https://discord.com/channels/753902328494424064/753902328494424070/1015271990434729984) |
È la prima cosa che abbiamo provato ma spacca i test ovviamente. dbd1c52#diff-7ad9326d392049b42431ec6a794204e768e966ca715f05d2c1f204da31c4c99cR14
La dobbiamo risolvere pure su 14.0, per cui tanto vale cercare una soluzione diversa, non possiamo basarci sulla EXCLUDE di travis. Che comuque, se non ho capito male, ripeterebbe tutti i test due volte, una con un modulo escluso, una con l'altro escluso. Prima di andare in quella direzione, ci penserei un attimo. L'idea di rendere esclusivi i due moduli era per semplificare il lavoro, ma a questo punto forse conviene puntare a farli coesistere, anche a costo di trovarsi voci di menu doppie, e mettere l'esclusività solo come indicazione all'interno del README. |
Perché? Per le azioni github esiste una configurazione analoga:
Sì i test andrebbero eseguiti due volte: una senza l10n_it_central_journal_reportlab e una senza l10n_it_central_journal.
Sì certo se è possibile farli convivere tanto meglio, ma volevo solo chiarire che per poterli testare mantenendoli mutualmente esclusivi sono sufficienti due righe nella configurazione di Travis. |
https://discord.com/channels/753902328494424064/753902328494424070/1015271990434729984
Esatto, nemmeno io vedo alternativa, ma ciò comporterebbe il raddoppio dei tempi, il che per come la vedo io deve avere una giustificazione più forte della semplice comodità di non rendere i due moduli compatibili. Anche perché, comunque, se a livello di README è chiaro che la loro installazione contemporanea non è supportata, a noi basta che i test funzionino (cosa che apparentemente è). |
Sì per la |
Non lo so se è un problema per il merge, però è una rottura per chi deve fare i test funzionali, credo, se non funziona runboat. E poi comunque un 2x sul tempo dell'esecuzione dei test è già da solo eccessivo, anche se riuscissimo a sistemare completamente i test. |
non è più facile cambiare il nome alle funzioni e farlo diventare veramente indipendente? |
Sì, è esattamente quello che intendo: l'idea dell' |
ccae8f3
to
4ad0c7d
Compare
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.
Puoi aggiungere un test?
ok che il coverage non è obbligatorio, però 600 righe di Python senza nessuna copertura sono un po' tante.
Vedi tu se vuoi anche controllare il risultato della stampa
5d9db3f
to
b640640
Compare
@SirTakobi riesci a fare review? Grazie :) |
@SirTakobi quando puoi :) |
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.
Grazie della PR!
Gli aspetti secondo me più preoccupanti sono il comportamento nel multi-azienda, l'utilizzo delle query e il conflitto con l10n_it_central_journal.
Il resto sono consigli/osservazioni/note: nice to have ma forse non strettamente necessari per il merge.
Idealmente, la logica di estrazione dati (e volendo anche le informazioni di base per il layout) dovrebbe essere condivisa con l10n_it_central_journal (in un modulo a sé stante, dipendenza di entrambi), poi i due moduli dovrebbero occuparsi solo del rendering: l10n_it_central_journal con QWeb e questo modulo con reportlab.
'website': 'https://github.com/OCA/l10n-italy' | ||
'l10n_it_central_journal_reportlab', |
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.
'website': 'https://github.com/OCA/l10n-italy' | |
'l10n_it_central_journal_reportlab', | |
'website': 'https://github.com/OCA/l10n-italy' | |
'/tree/12.0/l10n_it_central_journal_reportlab', |
oppure togliere la seconda riga, altrimenti il link è sbagliato
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.
👍
class DateRangeInherit(models.Model): | ||
_inherit = "date.range" | ||
|
||
date_last_print = fields.Date('Last printed date') |
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.
In base alla regola multi-company dei date.range
, i record potrebbero essere condivisi tra diverse company:
<record id="date_range_comp_rule" model="ir.rule">
<field name="name">Date Range multi-company</field>
<field name="model_id" ref="model_date_range"/>
<field name="domain_force"> ['|',('company_id','child_of',[user.company_id.id]),('company_id','=',False)]</field>
</record>
quindi nel caso in cui company_id
sia vuoto, questi valori andrebbero in conflitto con valori scritti negli stessi campi, ma da altre company, è possibile correggere?
Si potrebbe rendere il campo date.range.company_id
obbligatorio, ma sarebbe un grosso vincolo per tutti i moduli che lo utilizzano.
Un'altra strada potrebbe essere creare un model apposito, diverso per azienda, dove salvare questi valori insieme a un Many2one
verso date.range
.
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.
Questi campi sono uguali a quelli di l10n_it_central_journal
, che finora non hanno dato problemi, soprattutto perchè su company_id
di date.range
c'è il default a self.env['res.company']._company_default_get('date.range')
.
Sulla convivenza dei 2 moduli, sono incompatibili? @Borruso avete previsto che possano restare installati entrambi?
Se sì, allora meglio mettere i campi condivisi in un modulo comune.
Se no, bisogna esplicitare l'incompatibilità e documentare la procedura per passare da uno all'altro (installare l10n_it_central_journal_reportlab
, disinstallare l10n_it_central_journal
...)
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.
@Borruso farei quindi così Borruso#15
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.
Ho modificato la Borruso#15 in base a come abbiamo discusso in chiamata
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.
nel caso in cui
company_id
sia vuoto, questi valori andrebbero in conflitto con valori scritti negli stessi campi, ma da altre company, è possibile correggere?
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.
❔
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.
Abbiamo ipotizzato di permettere di usare, nel wizard del libro giornale, solo date.range
che abbiano company_id
valorizzato
progressive_credit = fields.Float( | ||
'Progressive Credit', | ||
digits=dp.get_precision('Account'), | ||
default=lambda *a: float()) |
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.
Qui e sotto, questo è per evitare che il default sia False
?
Potrebbe andare bene anche un semplice 0.0
, così non c'è da preoccuparsi della firma del metodo in default
.
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.
👍
@@ -0,0 +1,2 @@ | |||
id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink | |||
access_date_range_group_account_manager,group_account_manager model_date_range,model_date_range,account.group_account_manager,1,1,0,0 |
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.
access_date_range_group_account_manager,group_account_manager model_date_range,model_date_range,account.group_account_manager,1,1,0,0 | |
access_date_range_group_account_manager,group_account_manager model_date_range,date_range.model_date_range,account.group_account_manager,1,1,0,0 |
Altrimenti viene creato silenziosamente un altro identificatore esterno per il model date.range
:
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.
L'identificatore esterno viene ancora creato, ma sembra sia un comportamento standard di Odoo: https://github.com/odoo/odoo/blob/b208570ce8399bc6d3e4a8ba02eef6558e0a6ccc/odoo/addons/base/models/ir_model.py#L266.
Quindi 👍
@@ -0,0 +1,48 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<odoo> | |||
<data> |
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.
Si può omettere
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.
👍
] | ||
) | ||
self.wizard_model = self.env["wizard.giornale.reportlab"] | ||
self.report_model = self.env["ir.actions.report"] |
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.
Dove vengono usati questi attributi? Se sono inutilizzati sarebbe meglio rimuoverli
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.
👍
wizard.print_giornale_reportlab() | ||
decode_giornale = base64.b64decode(wizard.report_giornale) | ||
self.minimal_reader_buffer = io.BytesIO(decode_giornale) | ||
self.minimal_pdf_reader = PdfFileReader(self.minimal_reader_buffer) |
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.
A cosa serve la chiamata a PdfFileReader
? Il suo risultato non viene usato nel test
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.
serve per vedere se il file pdf non sia danneggiato e leggibile
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.
👍
@@ -0,0 +1,27 @@ | |||
# Copyright 2018 Gianmarco Conte ([email protected]) |
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.
Questo file è identico a https://github.com/OCA/l10n-italy/blob/55f384251627cc5b2645876617c4e81dc8141d3f/l10n_it_central_journal/models/account.py, è possibile chiamare i campi in modo diverso?
Altrimenti le informazioni salvate da questo modulo e da l10n_it_central_journal rischiano di sovrascriversi a vicenda.
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.
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.
👍
tables.append(Table(line_data, colWidths=colwidths, style=style_table)) | ||
return tables, list_balance | ||
|
||
def get_balance_data_report_giornale(self, tot_debit, tot_credit, final=False): |
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.
Credo che questo final
sia solo per distinguere quando viene stampata l'ultima tabella, quindi non è lo stesso final
che distingue la stampa definitiva: è possibile rinominare uno dei due?
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.
❔
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.
❔
} | ||
|
||
def get_colwidths_report_giornale(self, width_available): | ||
colwidths = [32, 35, 130, 130, 130, 50, 50] |
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.
Se è troppo tardi per modificare il design pace, ma giusto una nota per il futuro: ci sono 7 colonne, ma questa informazione è sparsa in diversi punti: sarebbe meglio accentrarla in un unico punto.
Altrimenti aggiungere/modificare/rimuovere colonne diventa problematico.
b640640
to
b2bc504
Compare
c1d438d
to
35fcc04
Compare
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.
@Borruso ora i test falliscono con TypeError: 'frozendict' object is not callable
. Puoi verificare? Grazie
c1ae2ab
to
a490a7b
Compare
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.
Grazie delle modifiche!
I punti principali di #2927 (review) secondo me sono ancora da risolvere:
- comportamento nel multi-azienda ([12.0][ADD] l10n_it_central_journal_reportlab: aggiunto modulo libro giornale stampa reportlab #2927 (comment))
- mancato utilizzo delle regole di accesso/sicurezza ([12.0][ADD] l10n_it_central_journal_reportlab: aggiunto modulo libro giornale stampa reportlab #2927 (comment))
Il resto dei commenti fatti qui sotto non li vedo come bloccanti per il merge
class DateRangeInherit(models.Model): | ||
_inherit = "date.range" | ||
|
||
date_last_print = fields.Date('Last printed date') |
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.
❔
class AccountJournalInherit(models.Model): | ||
_inherit = "account.journal" | ||
|
||
central_journal_exclude = fields.Boolean('Exclude from General Journal') |
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.
👍🏻
gap = 1 * cm # gap between header/footer and page content | ||
gap_text = 0.5 * cm # gap between text | ||
margin_left = 0.5 * cm # layout margin left | ||
margin_bottom = 0.5 * cm # layout margin bottom | ||
footer_height = 2 * gap_text + 12 # layout footer height |
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.
È possibile aggiungerlo nella roadmap o qui in un commento?
date_move_line_from = fields.Date(required=True) | ||
date_move_line_from_view = fields.Date('From date') |
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.
👍🏻
</group> | ||
<separator string="PDF"/> | ||
<group> | ||
<field name="report_giornale" filename="report_giornale_name" readonly="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.
👍🏻
if wizard.target_move == 'all': | ||
target_type = ['posted', 'draft'] | ||
else: | ||
target_type = [wizard.target_move] |
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.
👍🏻
FROM | ||
account_move_line aml | ||
LEFT JOIN account_move am ON (am.id = aml.move_id) | ||
LEFT JOIN account_account aa ON (aa.id = aml.account_id) | ||
WHERE | ||
aml.date >= %(date_from)s | ||
AND aml.date <= %(date_to)s | ||
AND am.state in %(target_type)s | ||
AND aml.journal_id in %(journal_ids)s |
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.
👍🏻
) | ||
|
||
height_available -= gap | ||
i = 0 |
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.
👍🏻
tables.append(Table(line_data, colWidths=colwidths, style=style_table)) | ||
return tables, list_balance | ||
|
||
def get_balance_data_report_giornale(self, tot_debit, tot_credit, final=False): |
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.
❔
style_header = self.get_styles_report_giornale_line()["style_header"] | ||
style_header_number = \ | ||
self.get_styles_report_giornale_line()["style_header_number"] | ||
style_table = self.get_styles_report_giornale_line()["style_table"] | ||
style_name = self.get_styles_report_giornale_line()["style_name"] | ||
style_number = self.get_styles_report_giornale_line()["style_number"] | ||
style_table_line_above = \ | ||
self.get_styles_report_giornale_line()["style_table_line_above"] |
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.
❔
0e338e4
to
1c4b003
Compare
31bc0ee
to
2132e3c
Compare
2132e3c
to
13c7148
Compare
/ocabot rebase |
Congratulations, PR rebased to 12.0. |
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. |
siamo vicini al merge? |
@matteoopenf non mi pare proprio 😂 |
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. |
novità? |
@matteoopenf penso sia da verificare con @Borruso se tutti i commenti nelle review sono stati gestiti i test sono comunque rotti |
si può fare un rebase intanto? |
/ocabot rebase |
Congratulations, PR rebased to 12.0. |
13c7148
to
97157f6
Compare
i test intanto ora vanno |
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.
Test funzionale: OK
nuovo metodo libro giornale stampa reportlab
--
Confermo di aver firmato il CLA https://odoo-community.org/page/cla e di aver letto le linee guida su https://odoo-community.org/page/contributing