Skip to content

[16.0] [MIG] l10n_it_website_portal_fiscal_code#3338

Merged
OCA-git-bot merged 10 commits into
OCA:16.0from
PyTech-SRL:16.0-mig-l10n-it-portal-fiscal-code
Oct 6, 2023
Merged

[16.0] [MIG] l10n_it_website_portal_fiscal_code#3338
OCA-git-bot merged 10 commits into
OCA:16.0from
PyTech-SRL:16.0-mig-l10n-it-portal-fiscal-code

Conversation

@HekkiMelody

Copy link
Copy Markdown
Contributor

Superseeds #3025

@tafaRU

tafaRU commented Jul 6, 2023

Copy link
Copy Markdown
Member

/ocabot rebase

@OCA-git-bot

Copy link
Copy Markdown
Contributor

@tafaRU The rebase process failed, because command git push --force PyTech-SRL tmp-pr-3338:16.0-mig-l10n-it-portal-fiscal-code failed with output:

remote: Permission to PyTech-SRL/l10n-italy.git denied to OCA-git-bot.
fatal: unable to access 'https://github.com/PyTech-SRL/l10n-italy/': The requested URL returned error: 403

@HekkiMelody HekkiMelody force-pushed the 16.0-mig-l10n-it-portal-fiscal-code branch from ca57bde to c2df466 Compare July 6, 2023 08:43
@HekkiMelody

Copy link
Copy Markdown
Contributor Author

Fatto il rebase a mano

@SirAionTech SirAionTech left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Grazie della PR!
Potresti modificare la storia dei commit come indicato in https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests?

error, error_message = super(
WebsitePortalFiscalCode, self
).details_form_validate(data)
).details_form_validate(data, partner_creation)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

È possibile passare l'argomento con partner_creation=partner_creation?
Altrimenti, se qualche altro modulo dovesse modificare l'ordine dei kwarg non verrebbero assegnati i parametri corretti.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

?

for="fiscalcode"
>Fiscal Code</label>
<input
type="text"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Più sotto negli attributi t-att* si usano logiche copiate da come era fatto il template in 14.0, queste sono però state corrette in 16.0 (vedi odoo/odoo@beb50e8); è possibile riportare le correzioni anche in questa migrazione?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@guidgaro guidgaro left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ho testato il modulo e funziona correttamente.

@eLBati

eLBati commented Sep 25, 2023

Copy link
Copy Markdown
Member

/ocabot rebase

@OCA-git-bot

Copy link
Copy Markdown
Contributor

@eLBati The rebase process failed, because command git push --force PyTech-SRL tmp-pr-3338:16.0-mig-l10n-it-portal-fiscal-code failed with output:

remote: Permission to PyTech-SRL/l10n-italy.git denied to OCA-git-bot.
fatal: unable to access 'https://github.com/PyTech-SRL/l10n-italy/': The requested URL returned error: 403

@francesco-ooops

Copy link
Copy Markdown
Contributor

@renda-dev o @PicchiSeba riuscite voi?

@PicchiSeba PicchiSeba force-pushed the 16.0-mig-l10n-it-portal-fiscal-code branch from c2df466 to 1f60375 Compare September 25, 2023 14:30
@PicchiSeba

Copy link
Copy Markdown
Contributor

@francesco-ooops proviamo adesso

@TheMule71

Copy link
Copy Markdown
Contributor

/ocabot rebase

@OCA-git-bot

Copy link
Copy Markdown
Contributor

@TheMule71 The rebase process failed, because command git push --force PyTech-SRL tmp-pr-3338:16.0-mig-l10n-it-portal-fiscal-code failed with output:

remote: Permission to PyTech-SRL/l10n-italy.git denied to OCA-git-bot.
fatal: unable to access 'https://github.com/PyTech-SRL/l10n-italy/': The requested URL returned error: 403

@PicchiSeba

Copy link
Copy Markdown
Contributor

Devo sentire l'amministratore del nostro fork. Vi taggo non appena sblocchiamo la situazione

@FrancescoBallerini FrancescoBallerini left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Functional : LGTM

>
<label
class="col-form-label label-optional"
for="vat"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

riga 29

t-att-disabled="None if partner_can_edit_vat else '1'"

riga 30

t-att-title="None if partner_can_edit_vat else 'Changing Fiscal Code is not allowed once document(s) have been issued for your account. Please contact us directly for this operation.'"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/odoo/odoo/blob/16.0/addons/website_sale/views/templates.xml#L1843

Per consistenza con gli altri campi, sarei per mantenere readonly invece che disabled

@Borruso Borruso Oct 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ti chiedevo di modificarla perché coerente con la vista
https://github.com/odoo/odoo/blob/16.0/addons/portal/views/portal_templates.xml#L404
che stai facendo modificando

è solo un osservazione per me va bene anche così

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

done!

puoi controllare vedo ancora if partner.partner.can_edit_vat() else invece che if partner_can_edit_vat else

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Lo sapevate che se non salvi il file, poi sul file non vengono scritte le modifiche? Sapevatelo.

@HekkiMelody HekkiMelody force-pushed the 16.0-mig-l10n-it-portal-fiscal-code branch from 1f60375 to bb7fd1b Compare October 5, 2023 12:17

@andreampiovesana andreampiovesana left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM - functional review

@marcelofrare marcelofrare left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@MaurizioPellegrinet MaurizioPellegrinet left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@HekkiMelody HekkiMelody marked this pull request as draft October 6, 2023 07:59
@HekkiMelody

Copy link
Copy Markdown
Contributor Author

Metto in draft per non avere merge preventivi, visto che ho ricevuto un sacco di approvazioni, ma vorrei guardare quello che hanno suggerito @Borruso e @SirAionTech . Spero di avere tempo oggi.

@HekkiMelody HekkiMelody force-pushed the 16.0-mig-l10n-it-portal-fiscal-code branch from bb7fd1b to eda96a9 Compare October 6, 2023 09:45
@HekkiMelody HekkiMelody marked this pull request as ready for review October 6, 2023 09:47
monen17 and others added 7 commits October 6, 2023 12:06
Steps:

 - Create a partner (type company) and give them portal access
 - With the new user, access to portal
 - Edit partner details setting fiscal code with 11 digits
 - Using admin, create an invoice for that partner and validate
 - Using the new user, access to portal, open partner details and save

Get "The fiscal code doesn't seem to be correct"
Currently translated at 100.0% (2 of 2 strings)

Translation: l10n-italy-12.0/l10n-italy-12.0-l10n_it_website_portal_fiscalcode
Translate-URL: https://translation.odoo-community.org/projects/l10n-italy-12-0/l10n-italy-12-0-l10n_it_website_portal_fiscalcode/it/
@HekkiMelody HekkiMelody force-pushed the 16.0-mig-l10n-it-portal-fiscal-code branch from eda96a9 to 22f3f7f Compare October 6, 2023 10:06
@HekkiMelody

Copy link
Copy Markdown
Contributor Author

Done

@SirAionTech SirAionTech left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Vedo che hai incluso 9a879d9 nel commit di migrazione, sarebbe stato da mantenere nella history come gli altri commit di 14.0, riesci a correggere?
Altrimenti ci sarebbe almeno da chiedere all'autore @FrancescoBallerini se va bene così

def details_form_validate(self, data, partner_creation=False):
error, error_message = super(
WebsitePortalFiscalCode, self
).details_form_validate(data, partner_creattion=partner_creation)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
).details_form_validate(data, partner_creattion=partner_creation)
).details_form_validate(data, partner_creation=partner_creation)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

È preoccupante che i test passassero anche con questo typo

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

concordo

@FrancescoBallerini

Copy link
Copy Markdown
Contributor

Vedo che hai incluso 9a879d9 nel commit di migrazione, sarebbe stato da mantenere nella history come gli altri commit di 14.0, riesci a correggere? Altrimenti ci sarebbe almeno da chiedere all'autore @FrancescoBallerini se va bene così

Non sono certo di come funzioni il mantenimento della commit history per le migrazioni, tuttavia sono abbastanza sicuro che non servirà un "rollback" del commit vista l'entità della modifica, al di la di questo non so se possa creare altre problematiche.. ma per me non è un gran problema se decidete di includerla direttamente perdendo eventualmente il commit

@HekkiMelody

HekkiMelody commented Oct 6, 2023

Copy link
Copy Markdown
Contributor Author

Vedo che hai incluso 9a879d9 nel commit di migrazione, sarebbe stato da mantenere nella history come gli altri commit di 14.0, riesci a correggere? Altrimenti ci sarebbe almeno da chiedere all'autore @FrancescoBallerini se va bene così

Non credo di averlo incluso, era una correzione che avevo rifatto io, non sapevo ci fosse la PR per la 14. Io sono partito prendendo #3025 così com'era. Comunque adesso per pulizia faccio cherry-pick

@HekkiMelody HekkiMelody force-pushed the 16.0-mig-l10n-it-portal-fiscal-code branch from 22f3f7f to 263251c Compare October 6, 2023 10:42
@FrancescoBallerini

Copy link
Copy Markdown
Contributor

Vedo che hai incluso 9a879d9 nel commit di migrazione, sarebbe stato da mantenere nella history come gli altri commit di 14.0, riesci a correggere? Altrimenti ci sarebbe almeno da chiedere all'autore @FrancescoBallerini se va bene così

Non credo di averlo incluso, era una correzione che avevo rifatto io, non sapevo ci fosse la PR per la 14. Io sono partito prendendo #3025 così com'era. Comunque adesso per pulizia faccio cherry-pick

In tal caso, se dovesse tornarti utile questa è la PR con il commit #3521

@OpenCode

OpenCode commented Oct 6, 2023

Copy link
Copy Markdown
Contributor

/ocabot merge nobump

@OCA-git-bot

Copy link
Copy Markdown
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-3338-by-OpenCode-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Oct 6, 2023
Signed-off-by OpenCode
@OCA-git-bot

Copy link
Copy Markdown
Contributor

It looks like something changed on 16.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 16.0-ocabot-merge-pr-3338-by-OpenCode-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 0442c9c into OCA:16.0 Oct 6, 2023
@OCA-git-bot

Copy link
Copy Markdown
Contributor

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

@HekkiMelody HekkiMelody deleted the 16.0-mig-l10n-it-portal-fiscal-code branch October 31, 2023 08:38
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.