Skip to content

[14.0][FIX/IMP]l10n_it_account & l10n_it_fatturapa_out: Nicer XML Validation errors #4266

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 22, 2024
Merged

[14.0][FIX/IMP]l10n_it_account & l10n_it_fatturapa_out: Nicer XML Validation errors #4266

merged 2 commits into from
Jul 22, 2024

Conversation

aleuffre
Copy link
Contributor

@aleuffre aleuffre commented Jul 11, 2024 •
edited
Loading

Domani apro la issue, giuro 👼

Issue #4270

The strategy here is

  • do not fail during XML generation, even if e.g. a field is empty (False)
  • Return a simpler error message for the user during XML validation
  • Return a more detailed error message if in debug mode

@aleuffre
[FIX] l10n_it_account: do not fail conversion on empty field
63ec1f5 
We don't want XML generation to fail if a field is empty (i.e. False)
Once it's time to validate the XML, the system will produce a more
meaningful and human-readable error.
@aleuffre aleuffre mentioned this pull request Jul 12, 2024
Closed
2 tasks
SirAionTech
SirAionTech suggested changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@SirAionTech SirAionTech 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!
Ho provato in locale e a parte i commenti qui sotto funziona egregiamente.

Riusciresti ad aggiungere un test? Più che altro per le modifiche a l10n_it_fatturapa_out perché sono un po' più articolate, forse puoi semplicemente sfruttare uno di quelli esistenti e controllare il messaggio di errore.

@SirAionTech SirAionTech linked an issue Jul 12, 2024 that may be closed by this pull request
l10n_it_fatturapa_out: Better error messages on XML validation #4270
Closed
2 tasks
@aleuffre aleuffre force-pushed the 14.0-xml-validation-error branch from 0fed0c3 to e2684b6 Compare July 12, 2024 12:59
@aleuffre
Copy link
Contributor Author

Grazie della review @SirAionTech dovrei aver risolto tutti i commenti che mi hai lasciato.

@aleuffre aleuffre force-pushed the 14.0-xml-validation-error branch from e2684b6 to 2f897a3 Compare July 12, 2024 13:15
@SirAionTech
Copy link
Contributor

Grazie della review @SirAionTech dovrei aver risolto tutti i commenti che mi hai lasciato.

Ok, quando vuoi che ci riguardi ricorda per favore di segnare qui in github che devo aggiornare la revisione
image

(da https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/requesting-a-pull-request-review)

@aleuffre aleuffre requested a review from SirAionTech July 12, 2024 13:33
@aleuffre
Copy link
Contributor Author

Grazie della review @SirAionTech dovrei aver risolto tutti i commenti che mi hai lasciato.

Ok, quando vuoi che ci riguardi ricorda per favore di segnare qui in github che devo aggiornare la revisione

hai ragione, grazie

@aleuffre
Copy link
Contributor Author

Ma intanto devo capire perché ci sono questi errori che in locale io non ho...

@aleuffre aleuffre force-pushed the 14.0-xml-validation-error branch from 2f897a3 to b43b9c7 Compare July 12, 2024 13:47
SirAionTech
SirAionTech suggested changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@SirAionTech SirAionTech left a comment

Choose a reason for hiding this comment

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

Ma intanto devo capire perché ci sono questi errori che in locale io non ho...

Esatto, i test sono da correggere.
Se vuoi mettere le mani nelle request potrebbe tornarti utile guardare cosa fa MockRequest (https://github.com/odoo/odoo/blob/948235079f002794f9837d3cf91e2d20e3254e20/addons/website/tools.py#L17), purtroppo non lo puoi usare perché è in website.

Puoi provare ad eseguire i test come fa la CI, vedi i comandi https://github.com/OCA/l10n-italy/actions/runs/9908495939/job/27374393506?pr=4266#step:7:11 e https://github.com/OCA/l10n-italy/actions/runs/9908495939/job/27374393506?pr=4266#step:8:15.

Nel frattempo finché ci stai lavorando potresti mettere in bozza la PR.
Poi quando i test saranno a posto potrai impostare la PR come "Pronta per review" e chiedere di aggiornare eventuali revisioni.

@aleuffre aleuffre marked this pull request as draft July 12, 2024 14:01
@aleuffre aleuffre force-pushed the 14.0-xml-validation-error branch 2 times, most recently from 48f2e19 to 39cca25 Compare July 12, 2024 14:46
@aleuffre aleuffre marked this pull request as ready for review July 12, 2024 15:00
@aleuffre aleuffre requested a review from SirAionTech July 12, 2024 15:00
@aleuffre aleuffre force-pushed the 14.0-xml-validation-error branch from 39cca25 to cd0c490 Compare July 12, 2024 15:01
SirAionTech
SirAionTech approved these changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@SirAionTech SirAionTech left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Ho verificato in locale e mi pare essere tutto ok.
Ho fatto revisione del codice e verificato che il nuovo test giustamente fallisce senza queste modifiche.
Ho messo qualche commento qui sotto, ma secondo me niente di bloccante per il merge.

@aleuffre aleuffre force-pushed the 14.0-xml-validation-error branch from cd0c490 to 57596aa Compare July 15, 2024 07:01
francesco-ooops
francesco-ooops approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@francesco-ooops francesco-ooops left a comment

Choose a reason for hiding this comment

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

Funzionale ok!

@SirAionTech
Copy link
Contributor

@TheMule71 il commento che inizia con

XXX - da migliorare?

poco sopra le modifiche a l10n_it_fatturapa_out/wizard/efattura.py viene dal tuo commit c8bd4e3, secondo te ora si può togliere?

@francesco-ooops
Copy link
Contributor

@TheMule71 che dici?

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@francesco-ooops
Copy link
Contributor

@SirAionTech non mi sembra il caso di bloccarci su del codice commentato, mergiamo e se @TheMule71 risponderà facciamo volentieri una nuova PR :)

@SirAionTech
Copy link
Contributor

@SirAionTech non mi sembra il caso di bloccarci su del codice commentato, mergiamo e se @TheMule71 risponderà facciamo volentieri una nuova PR :)

Io gli darei ancora qualche giorno per rispondere, non è passata nemmeno una settimana; se vuoi mergiare però sai come si fa una patch 😉

@francesco-ooops
Copy link
Contributor

@OCA/local-italy-maintainers buondì, potreste sbloccarmi questa? grazie!

@francesco-ooops
Copy link
Contributor

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-4266-by-francesco-ooops-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 919ac3f into OCA:14.0 Jul 22, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 7b09e4c. 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
Reviewers

@francesco-ooops francesco-ooops francesco-ooops approved these changes

@SirAionTech SirAionTech SirAionTech approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

l10n_it_fatturapa_out: Better error messages on XML validation
4 participants
@aleuffre @SirAionTech @francesco-ooops @OCA-git-bot