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.

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

[14.0][FIX]l10n_it_fatturapa_in: remove unnecessary write #4240

Merged
merged 1 commit into from
Aug 30, 2024
Merged

[14.0][FIX]l10n_it_fatturapa_in: remove unnecessary write #4240

merged 1 commit into from
Aug 30, 2024

Conversation

PicchiSeba
Copy link
Contributor

@PicchiSeba PicchiSeba commented Jun 28, 2024 •
edited by francesco-ooops
Loading

There's no need to overwrite values when they don't change. Moreover a user might not have permissions to write certain fields (in my case it was stock_valuation_layer_ids, which requires the Stock / Administrator group)

Solves: #4242

@PicchiSeba PicchiSeba force-pushed the 14.0-l10n_it_fattura_pi_in-write-changing-values branch from fc93349 to 4b2c86c Compare June 28, 2024 10:50
@PicchiSeba PicchiSeba marked this pull request as ready for review June 28, 2024 11:04
@PicchiSeba PicchiSeba force-pushed the 14.0-l10n_it_fattura_pi_in-write-changing-values branch from 4b2c86c to 3b311cd Compare June 28, 2024 11:48
@PicchiSeba PicchiSeba mentioned this pull request Jun 28, 2024
Closed
2 tasks
@francesco-ooops francesco-ooops linked an issue Jun 28, 2024 that may be closed by this pull request
Permission error while importing an incoming e-invoice #4242
Closed
2 tasks
aleuffre
aleuffre suggested changes Jun 28, 2024
View reviewed changes
Copy link
Contributor

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

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

Code review

l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py Outdated Show resolved Hide resolved
l10n_it_fatturapa_in/wizard/wizard_import_fatturapa.py Outdated Show resolved Hide resolved
@PicchiSeba PicchiSeba force-pushed the 14.0-l10n_it_fattura_pi_in-write-changing-values branch 3 times, most recently from c57f2bc to 51a083b Compare July 1, 2024 13:54
@PicchiSeba PicchiSeba requested a review from aleuffre July 1, 2024 13:54
aleuffre
aleuffre approved these changes Jul 1, 2024
View reviewed changes
Copy link
Contributor

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

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

Code review, LGTM

francesco-ooops
francesco-ooops approved these changes Jul 2, 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!

@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

@OCA/local-italy-maintainers si può mergiare?

@francesco-ooops
Copy link
Contributor

@SirAionTech buona per te?

@SirAionTech
Copy link
Contributor

@SirAionTech buona per te?

A me già non convinceva questa parte di codice che prende tutto quello che c'è nella cache e lo scrive.
Questa PR non mi sembra migliorare questo aspetto, ma già non mi spiegavo il codice di prima quindi non saprei cosa dire di questa modifica.
Forse sarebbe meglio lasciar stare la cache.

@francesco-ooops
Copy link
Contributor

@SirAionTech riusciamo a risalire a chi/quando era stata aggiunta questa funzione?

@SirAionTech
Copy link
Contributor

SirAionTech commented Jul 11, 2024 •
edited
Loading

@SirAionTech riusciamo a risalire a chi/quando era stata aggiunta questa funzione?

Fu aggiunta in 773e1c3 (il commit originale è cb063e9) da @eLBati

@francesco-ooops
Copy link
Contributor

Ok, dunque è lì da un bel po'

@eLBati hai idea dell'utilità di questa funzione?

@eLBati
Copy link
Member

eLBati commented Jul 12, 2024

_convert_to_write era usato per replicare nell'importazione quello che avviene nell'interfaccia all'esecuzione dell'onchange e scriverlo sul record (altrimenti le modifiche dell'onchange non venivano salvate). Quindi in questo caso serviva a salvare le modifiche fatte da _onchange_invoice_line_wt_ids.

@francesco-ooops
Copy link
Contributor

@eLBati grazie, secondo te come possiamo intervenire per risolvere il problema indicato nella issue?

@francesco-ooops
Copy link
Contributor

@eLBati ping! Che ne pensi di questa PR? Andiamo a fare danni?

@francesco-ooops
Copy link
Contributor

francesco-ooops commented Jul 25, 2024 •
edited
Loading

@eLBati un feedback sarebbe molto apprezzato dato che per far validare una fattura dobbiamo dare accesso admin al magazzino🙏 grazie!

@PicchiSeba PicchiSeba force-pushed the 14.0-l10n_it_fattura_pi_in-write-changing-values branch from 51a083b to 14482bc Compare August 6, 2024 07:18
@PicchiSeba PicchiSeba changed the title [14.0][IMP]l10n_it_fatturapa_in: copy changing values only [14.0][FIX]l10n_it_fatturapa_in: remove unnecessary write Aug 6, 2024
francesco-ooops
francesco-ooops approved these changes Aug 6, 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 sempre ok

@eLBati puoi approvare?

@francesco-ooops
Copy link
Contributor

@eLBati quando puoi, grazie!

@francesco-ooops
Copy link
Contributor

@eLBati che dici?

@eLBati
Copy link
Member

eLBati commented Aug 30, 2024

Grazie

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-4240-by-eLBati-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit f8b54cc into OCA:14.0 Aug 30, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@PicchiSeba PicchiSeba deleted the 14.0-l10n_it_fattura_pi_in-write-changing-values branch September 4, 2024 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aleuffre aleuffre aleuffre approved these changes

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

@eLBati eLBati Awaiting requested review from eLBati

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

Successfully merging this pull request may close these issues.

Permission error while importing an incoming e-invoice
6 participants
@PicchiSeba @OCA-git-bot @francesco-ooops @SirAionTech @eLBati @aleuffre