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

[16.0] [MIG] l10n_it_fatturapa_in_rc #3424

Closed

Conversation

odooNextev
Copy link
Contributor

Dipende da #3300

@odooNextev odooNextev mentioned this pull request Jul 6, 2023
79 tasks
@odooNextev odooNextev force-pushed the 16.0-mig-l10n_it_fatturapa_in_rc branch 2 times, most recently from 1a45661 to 3e96d3a Compare July 6, 2023 10:11
Copy link
Contributor

@SirTakobi SirTakobi 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!
Vedi i commenti qui sotto

@@ -57,6 +57,11 @@ class FatturaPAAttachmentIn(models.Model):

Copy link
Contributor

Choose a reason for hiding this comment

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

Come mai questa PR di migrazione per l10n_it_fatturapa_in_rc modifica anche l10n_it_fatturapa_in?

Copy link
Contributor Author

@odooNextev odooNextev Jul 25, 2023

Choose a reason for hiding this comment

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

sono modifiche introdotte da questo commit non nostro: 647af23
ho fatto un revert

Copy link
Contributor

Choose a reason for hiding this comment

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

Se è un commit che non ha a che fare con la migrazione, va rimosso dalla PR di migrazione

Copy link
Contributor Author

@odooNextev odooNextev Sep 19, 2023

Choose a reason for hiding this comment

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

Senza dover rifare la PR, come potrei fare per aggiornare il mio branch con le ultime modifiche introdotte dalla PR #3577?
Basta un merge da 16.0 al mio branch e poi fare un rebase per nascondere il commit di merge?

Copy link
Contributor

Choose a reason for hiding this comment

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

git checkout 16.0
git pull
git checkout 16.0-mig-l10n_it_fatturapa_in_rc
git rebase 16.0
# Controlli che sia tutto ok
git push -f

Copy link
Contributor

@aleuffre aleuffre Sep 19, 2023

Choose a reason for hiding this comment

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

senza git checkout, credo basterebbe:

git fetch # o git fetch <remote name> 16.0 se lavori con più remote
git rebase <remote name>/16.0
git push -f 

Copy link
Contributor

Choose a reason for hiding this comment

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

senza git checkout, credo basterebbe:

git fetch # o git fetch <remote name> 16.0 se lavori con più remote
git rebase 16.0
git push -f 

A me 16.0 non si aggiorna semplicemente con un fetch, ma rimane indietro rispetto a origin/16.0; non dovrebbe essere invece git rebase origin/16.0? Supponendo che origin sia il remote di OCA

Copy link
Contributor

Choose a reason for hiding this comment

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

Hai ragione sul rebase, avevo dimenticato un pezzo nel comando 😓

Ma se specifichi git rebase origin/16.0 (invece che solo git rebase 16.0) fa il rebase in base all'ultimo stato noto del branch sul remote (che non è detto sia lo stesso stato del tuo branch locale 16.0). git pull fa semplicemente git fetch + git merge del branch remoto sul branch corrente.

Scusa, adesso mi zittisco. Non era importante, ma git mi piace molto haha.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ma se specifichi git rebase origin/16.0 (invece che solo git rebase 16.0) fa il rebase in base all'ultimo stato noto del branch sul remote (che non è detto sia lo stesso stato del tuo branch locale 16.0). git pull fa semplicemente git fetch + git merge del branch remoto sul branch corrente.

Sì, infatti proponevo la modifica ai tuoi step dove avevi fatto subito prima git fetch; di solito uso la procedura con checkout così colgo l'occasione per aggiornare anche 16.0 in locale.

Scusa, adesso mi zittisco. Non era importante, ma git mi piace molto haha.

Ma va, è sempre utile un po' di confronto 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SirAionTech @aleuffre grazie per i suggerimenti come sempre
Purtroppo però faccio prima a rifare siccome il rebase mi da dei conflitti...
Aggiorno subito

Copy link
Contributor

Choose a reason for hiding this comment

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

Diversi commit vanno schiacciati seguendo https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests, ad esempio:

Come mai il commit di migrazione è contenuto in doppi apici (")? Li potresti mica rimuovere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Faccio lo squash.
Quella degli apici è stata una svista nel lancio del comando da shell...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ho modificato il commit di migrazione, mentre per lo squash aspetto di capire cosa fare con questo prima: 647af23

Copy link
Contributor

Choose a reason for hiding this comment

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

Va bene, mentre stai lavorando alla PR potresti metterla in bozza
image
e poi quando hai finito la riapri con
image
e mi chiedi di aggiornare la review
image
(sì mi piace fare gli screenshot 😄)

Chiaramente se hai bisogno mi puoi scrivere qui o su Discord

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Grazie per il consiglio 👍
Mi potresti dire cosa fare con il commit che modifica l10n_it_fatturapa_in che citavo prima?
E' corretto che faccia il revert ed annulli quella modifica?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gabriele-baldessari stiamo migrando il modulo l10n_it_fatturapa_in_rc ed abbiamo trovato un tuo commit 647af23 che aggiunge codice al modulo l10n_it_fatturapa_in.
Ci sapresti spiegare a cosa serve e se devo portarlo avanti o posso escluderlo dalla migrazione alla v16?

Copy link
Contributor

Choose a reason for hiding this comment

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

Non dovrebbe essere in questa PR, quindi sarebbe più corretto dropparlo invece di fare revert.
Droppandolo andrebbe perso, quindi prima di farlo è meglio che cerchiate di capire come mai era stato incluso qui per evitare di perdere del lavoro.

Non sono sicuro al 100% ma mi sembra di ricordare che, usando odoo-module-migrator, se un commit tocca più moduli (raro ma accade), venga preso il commit, ma del commit vengano poi prese solo le modifiche che toccano il modulo che si sta migrando e non le altre.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Non dovrebbe essere in questa PR, quindi sarebbe più corretto dropparlo invece di fare revert.
Droppandolo andrebbe perso, quindi prima di farlo è meglio che cerchiate di capire come mai era stato incluso qui per evitare di perdere del lavoro.

Non sono sicuro al 100% ma mi sembra di ricordare che, usando odoo-module-migrator, se un commit tocca più moduli (raro ma accade), venga preso il commit, ma del commit vengano poi prese solo le modifiche che toccano il modulo che si sta migrando e non le altre.

In questo caso allora, siccome il commit tocca solo il modulo "esterno", se seguo il comportamento di odoo-module-migrator posso dropparlo interamente

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, scusate, avevo capito male allora. Se non tocca nemmeno in parte il modulo che si sta migrando, sicuramente si, da droppare.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ci sono ancora commit di oca-ci e https://github.com/weblate da schiacciare

Copy link
Contributor

Choose a reason for hiding this comment

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

I test stanno fallendo perché non trovano l10n_it_reverse_charge, lo potresti includere seguendo https://github.com/OCA/maintainer-tools/wiki/Use-temporary-reference(s)-to-another-pull-request(s)?
Così possiamo vedere se passano

Copy link
Contributor Author

@odooNextev odooNextev Jul 25, 2023

Choose a reason for hiding this comment

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

Fatto, ma fallisce
Riesci a dirmi dove sbaglio?

Copy link
Contributor

Choose a reason for hiding this comment

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

Riesci a dirmi dove sbaglio?

Hai messo l10n_it_fatturapa_in_rc, mentre dovresti mettere il modulo l10n_it_reverse_charge contenuto nella PR che non è ancora stata mergiata

@@ -3,7 +3,7 @@
{
"name": "ITA - Fattura elettronica - Inversione contabile",
"summary": "Modulo ponte tra e-fattura in acquisto e inversione" " contabile",
"version": "14.0.1.1.1",
"version": "16.0.1.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Questa modifica dovrebbe essere nel commit di migrazione, non in quello di pre-commit.
Per aiutarti con il processo di migrazione puoi usare https://github.com/OCA/odoo-module-migrator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fatto

Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

@odooNextev odooNextev marked this pull request as draft July 25, 2023 14:43
@SirTakobi
Copy link
Contributor

@odooNextev vedo che è in draft da settimana scorsa, posso aggiornare la review o ci stai ancora lavorando?

@odooNextev odooNextev marked this pull request as ready for review August 2, 2023 13:04
@odooNextev
Copy link
Contributor Author

@odooNextev vedo che è in draft da settimana scorsa, posso aggiornare la review o ci stai ancora lavorando?

L'ho riaperta
C'è modo di rilanciare i test che non so perchè erano terminati per timeout?

@SirTakobi
Copy link
Contributor

C'è modo di rilanciare i test

L'unico modo che conosco è fare un push nella PR.
Puoi rifare l'ultimo commit senza modificarlo con

git commit --amend
git push -f

@odooNextev odooNextev force-pushed the 16.0-mig-l10n_it_fatturapa_in_rc branch from a253978 to 2655ad8 Compare August 2, 2023 13:29
@odooNextev
Copy link
Contributor Author

C'è modo di rilanciare i test

L'unico modo che conosco è fare un push nella PR. Puoi rifare l'ultimo commit senza modificarlo con

git commit --amend
git push -f

C'è qualcosa che non va nell'esecuzione dei test perchè è bloccato anche stavolta, ma non si riesce a vedere il log

@OpenCode
Copy link
Contributor

OpenCode commented Aug 2, 2023

C'è modo di rilanciare i test

L'unico modo che conosco è fare un push nella PR. Puoi rifare l'ultimo commit senza modificarlo con

git commit --amend
git push -f

C'è qualcosa che non va nell'esecuzione dei test perchè è bloccato anche stavolta, ma non si riesce a vedere il log

Quello che leggi è già il log con l'errore

odoo-addon-l10n_it_reverse_charge @ git+https://github.com/OCA/l10n-italy.git@refs/pull/3300/head#subdirectory=setup/l10n_it_reverse_charge
Unreleased dependencies found in test-requirements.txt.

@@ -57,6 +57,11 @@ class FatturaPAAttachmentIn(models.Model):

Copy link
Contributor

Choose a reason for hiding this comment

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

Se è un commit che non ha a che fare con la migrazione, va rimosso dalla PR di migrazione

Copy link
Contributor

Choose a reason for hiding this comment

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

Ci sono ancora commit di oca-ci e https://github.com/weblate da schiacciare

@@ -3,7 +3,7 @@
{
"name": "ITA - Fattura elettronica - Inversione contabile",
"summary": "Modulo ponte tra e-fattura in acquisto e inversione" " contabile",
"version": "14.0.1.1.1",
"version": "16.0.1.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

C'è qualcosa che non va nell'esecuzione dei test perchè è bloccato anche stavolta, ma non si riesce a vedere il log

I log che vedi nella CI già dicono quasi tutto, per sapere qualcosa di più puoi cliccare su
image
e lì puoi vedere che ha aspettato 6 ore prima di desistere:

2023-08-02T13:33:30.9834789Z 2023-08-02 13:33:30,982 355 �[1;32m�[1;49mINFO�[0m odoo odoo.addons.l10n_it_fatturapa_in_rc.tests.test_fatturapa_in_rc: Starting TestInvoiceRC.test_00_xml_import ...
2023-08-02T19:29:26.3506489Z ##[error]The operation was canceled.

Hai provato ad eseguirli in locale? Perché a me in locale si bloccano.

Ho indagato un po' e sono arrivato a trovare che la query per la creazione dell'utente per i test è bloccata da un'altra query:

 blocked_pid | blocked_user | blocking_pid | blocking_user |                                                                                                                                     blocked_statement                                                                                                                                     |                                                                                                   current_statement_in_blocking_process                                                                                                    
-------------+--------------+--------------+---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
      190247 | odoo         |       190121 | odoo          | INSERT INTO "res_users" ("active", "company_id", "create_date", "create_uid", "login", "notification_type", "partner_id", "write_date", "write_uid") VALUES (true, 1, '2023-08-03 12:32:13.939296', 1, 'it_account_manager', 'email', 83, '2023-08-03 12:32:13.939296', 1) RETURNING "id" | INSERT INTO "withholding_tax_rate" ("base", "create_date", "create_uid", "tax", "withholding_tax_id", "write_date", "write_uid") VALUES (0.2, '2023-08-03 12:32:03.229085', 30, 26.0, 48, '2023-08-03 12:32:03.229085', 30) RETURNING "id"
(1 row)

puoi trovare la query in https://wiki.postgresql.org/wiki/Lock_Monitoring.

Prima di spaccarci la testa a capire perché ci sono query bloccate, ti consiglio di ripulire il setup dei test del modulo perché vedo che ci sono diverse cose duplicate di altri test, ad esempio il conto 295000 viene già creato durante il setup dei test di l10n_it_reverse_charge.
A proposito di l10n_it_reverse_charge, ho rifatto la migrazione in #3514.

@odooNextev odooNextev force-pushed the 16.0-mig-l10n_it_fatturapa_in_rc branch from 171e2b6 to 1210aee Compare September 12, 2023 13:31
@eLBati
Copy link
Member

eLBati commented Sep 15, 2023

Quindi ci sarebbe da fare una PR per portare le modifiche relative a l10n_it_fatturapa_in (e solo a lui) di #3084

Poi questa PR (#3424) dipenderà dalla PR per l10n_it_fatturapa_in

@odooNextev
Copy link
Contributor Author

Quindi ci sarebbe da fare una PR per portare le modifiche relative a l10n_it_fatturapa_in (e solo a lui) di #3084

Poi questa PR (#3424) dipenderà dalla PR per l10n_it_fatturapa_in

Esatto.

@TheMule71
Copy link
Contributor

#3577

@TheMule71
Copy link
Contributor

#3577 mergiata

@odooNextev odooNextev force-pushed the 16.0-mig-l10n_it_fatturapa_in_rc branch from 1210aee to ddf4d4e Compare September 19, 2023 14:38
sergiocorato and others added 10 commits September 20, 2023 14:24
[10.0] refactor in a separate module

[FIX] flake
ADD tests
FIX RC flag and inconsistency message about totals
…RC ITA" to be taken instead of "22% e-bill"

FIX

2019-10-28 16:27:59,292 5898 WARNING openerp_test odoo.addons.l10n_it_fatturapa_in.wizard.wizard_import_fatturapa: Too many taxes with percentage equals to '22.00'.
Fix it if required
2019-10-28 16:28:00,156 5898 ERROR openerp_test odoo.addons.l10n_it_fatturapa_in_rc.tests.test_fatturapa_in_rc: FAIL
2019-10-28 16:28:00,160 5898 INFO openerp_test odoo.addons.l10n_it_fatturapa_in_rc.tests.test_fatturapa_in_rc: ======================================================================
2019-10-28 16:28:00,160 5898 ERROR openerp_test odoo.addons.l10n_it_fatturapa_in_rc.tests.test_fatturapa_in_rc: FAIL: test_00_xml_import (odoo.addons.l10n_it_fatturapa_in_rc.tests.test_fatturapa_in_rc.TestInvoiceRC)
2019-10-28 16:28:00,160 5898 ERROR openerp_test odoo.addons.l10n_it_fatturapa_in_rc.tests.test_fatturapa_in_rc: Traceback (most recent call last):
2019-10-28 16:28:00,160 5898 ERROR openerp_test odoo.addons.l10n_it_fatturapa_in_rc.tests.test_fatturapa_in_rc: `   File "/home/travis/build/OCA/l10n-italy/l10n_it_fatturapa_in_rc/tests/test_fatturapa_in_rc.py", line 112, in test_00_xml_import
2019-10-28 16:28:00,160 5898 ERROR openerp_test odoo.addons.l10n_it_fatturapa_in_rc.tests.test_fatturapa_in_rc: `     '22% e-bill')
2019-10-28 16:28:00,160 5898 ERROR openerp_test odoo.addons.l10n_it_fatturapa_in_rc.tests.test_fatturapa_in_rc: ` AssertionError: 'Tax 22% Purchase RC ITA' != '22% e-bill'
2019-10-28 16:28:00,160 5898 ERROR openerp_test odoo.addons.l10n_it_fatturapa_in_rc.tests.test_fatturapa_in_rc: ` - Tax 22% Purchase RC ITA
2019-10-28 16:28:00,161 5898 ERROR openerp_test odoo.addons.l10n_it_fatturapa_in_rc.tests.test_fatturapa_in_rc: ` + 22% e-bill
2019-10-28 16:28:00,161 5898 ERROR openerp_test odoo.addons.l10n_it_fatturapa_in_rc.tests.test_fatturapa_in_rc:
@odooNextev odooNextev force-pushed the 16.0-mig-l10n_it_fatturapa_in_rc branch from ddf4d4e to b60b2e8 Compare September 20, 2023 12:30
@odooNextev
Copy link
Contributor Author

Non capisco perchè fallisce il test: https://github.com/OCA/l10n-italy/actions/runs/6248556593/job/16963418805?pr=3424#step:8:455
Passando in debug questa assegnazione sulla riga della fattura è corretta, la prima non è RC, la seconda sì
https://github.com/OCA/l10n-italy/pull/3424/files#diff-b6bca1f8af561cbe24144f0a4fc1f5e0699eeac18bdb207987e9791240cb92cdR10
Però quando arriva in questo punto sono tutte 2 True: https://github.com/OCA/l10n-italy/pull/3424/files#diff-ac116915836ab040d4dbee8b21bf4599690f396f8060dd178fbe36e6c1f40252R113

La cosa ancora più strana è che se faccio il debug proprio step-by-step e valuto ogni tanto invoice.invoice_line_ids[0].rc nella debug console per capire chi e quando lo mette a False, resta misteriosamente sempre falso e passa il test....

@odooNextev
Copy link
Contributor Author

Ho notato che non viene invocato _compute_rc_flag di l10n_it_reverse_charge all'importazione della fattura, ma nei depends non è cambiato nulla da v14 a v16, perciò non dovrebbe essere quello il problema.

@SirAionTech
Copy link
Contributor

In accordo con @odooNextev (https://discordapp.com/channels/753902328494424064/753902328494424070/1156563073407139912) prendo in carico la PR, ne farò un'altra a breve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.