Skip to content

[14.0][IMP] assets_management multicompany#3614

Merged
OCA-git-bot merged 1 commit into
OCA:14.0from
odooNextev:14.0-imp-assets_management-multicompany
Feb 23, 2024
Merged

[14.0][IMP] assets_management multicompany#3614
OCA-git-bot merged 1 commit into
OCA:14.0from
odooNextev:14.0-imp-assets_management-multicompany

Conversation

@odooNextev

@odooNextev odooNextev commented Sep 28, 2023

Copy link
Copy Markdown
Contributor

Questa PR serve per mostrare sia in lettura che in scrittura i record relativi alle company consentite all'utente (solo quelle selezionate nel menu in alto a destra).

Sono state quindi modificate tutte le regole di accesso dei modelli dei cespiti passando da:
['|',('company_id','=',False),('company_id','child_of',[user.company_id.id])]
|
V
['|',('company_id','=',False),('company_id','in',company_ids)]

Ho aggiunto il reset dei campi legati alle aziende alla modifica del campo company_id.

Per completare ho aggiunto il seguente dominio nei campi legati alle aziende nelle viste:
domain="[('company_id', '=', company_id)]"

rif #3651

@francesco-ooops

Copy link
Copy Markdown
Contributor

@sergiocorato al di là della forma, ti sembra un'implementazione corretta?

Comment thread assets_management/views/asset_category.xml Outdated
Comment thread assets_management/views/asset_depreciation_line.xml Outdated
Comment thread assets_management/views/asset.xml
@odooNextev odooNextev force-pushed the 14.0-imp-assets_management-multicompany branch from 708de86 to 0412e30 Compare September 28, 2023 15:59
@francesco-ooops

Copy link
Copy Markdown
Contributor

@sergiocorato dovremmo esserci, che ne pensi?

@odooNextev odooNextev force-pushed the 14.0-imp-assets_management-multicompany branch from 9b40439 to 59c7113 Compare October 2, 2023 06:59
@odooNextev odooNextev requested a review from GSLabIt October 2, 2023 07:14
@francesco-ooops

Copy link
Copy Markdown
Contributor

@matteoopenf che ne pensi?

@francesco-ooops francesco-ooops 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.

Funzionale ok!

@OCA-git-bot

Copy link
Copy Markdown
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). 🤖

@odooNextev odooNextev force-pushed the 14.0-imp-assets_management-multicompany branch 2 times, most recently from 388e036 to 9f6036f Compare October 13, 2023 12:27
@odooNextev

Copy link
Copy Markdown
Contributor Author

@SirAionTech @TheMule71 @eLBati ho rivisto i commit come dicevamo stamattina

@matteoopenf matteoopenf 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.

review del codice ok

@matteoopenf

Copy link
Copy Markdown
Contributor

@matteoopenf che ne pensi?

per me ok, se non c'e' altro potremmo procedere

@francesco-ooops

Copy link
Copy Markdown
Contributor

@sergiocorato buona per te?

@francesco-ooops

Copy link
Copy Markdown
Contributor

@sergiocorato gentile reminder :)

Comment thread assets_management/tests/test_assets_common.py Outdated
Comment thread assets_management/models/asset_category.py Outdated
@odooNextev

Copy link
Copy Markdown
Contributor Author

@tafaRU come faccio a far ripartire i test dopo le modifiche di pre-commit?
Ho fatto un rebase e push, ma mi dice (ovviamente) che non ci sono modifiche dal mio lato e quindi non posso far assimilare la modifica.

@OpenCode

Copy link
Copy Markdown
Contributor

@tafaRU come faccio a far ripartire i test dopo le modifiche di pre-commit? Ho fatto un rebase e push, ma mi dice (ovviamente) che non ci sono modifiche dal mio lato e quindi non posso far assimilare la modifica.

Ho rilanciato il job

@HekkiMelody

HekkiMelody commented Oct 31, 2023

Copy link
Copy Markdown
Contributor

@tafaRU come faccio a far ripartire i test dopo le modifiche di pre-commit? Ho fatto un rebase e push, ma mi dice (ovviamente) che non ci sono modifiche dal mio lato e quindi non posso far assimilare la modifica.

Se ti dice che non ci sono modifiche, vuol dire che non hai fatto rebase correttamente. Altrimenti, dopo il rebase, il tuo git status dovrebbe farti vedere qualcosa tipo:

Your branch and '<remote branch name>' have diverged,
and have 34 and 1 different commits each, respectively.

Hai fatto un fetch prima?

@francesco-ooops

Copy link
Copy Markdown
Contributor

@sergiocorato

Comment on lines +145 to +146
@api.onchange("company_id")
def onchange_company_id(self):

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.

nitpick:

Suggested change
@api.onchange("company_id")
def onchange_company_id(self):
@api.onchange("company_id")
def _onchange_company_id(self):

https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#L901

Comment thread assets_management/security/rules.xml Outdated
Comment on lines +4 to +11
<record id="asset_multicompany_rule" model="ir.rule">
<field name="name">Asset multi company rule</field>
<field name="model_id" ref="model_asset_asset" />
<field name="global" eval="True" />
<field name="domain_force">
['|',('company_id','=',False),('company_id','in',company_ids)]
</field>
</record>

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.

nitpick: magari mi sbaglio, ma questo file mi risulta un tab troppo avanti

readonly="1"
groups="base.group_multi_company"
options="{'no_open':1, 'no_create_edit': True}"
options="{'no_open':1, 'no_create': True, 'no_create_edit': True}"

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.

question: non basta già no_create_edit a non permettere entrambi?

Oltretutto, se il campo è impostato come readonly non ci dovrebbere essere bisogno di nessuno dei due, giusto?

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.

esatto, il readonly è stato inserito dopo e le opzioni non servono più

{"company_id": cls.company2.id}
)
# Asset categories
cls.asset_category_1_company1 = cls.env["asset.category"].create(

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.

nitpick: Preferisco personalmente avere i nomi delle variabili più consistenti tra di loro, potresti mettere gli underscore anche per i numeri?

https://peps.python.org/pep-0008/#function-and-variable-names

@stenext stenext force-pushed the 14.0-imp-assets_management-multicompany branch from 34b8341 to 78caca0 Compare February 1, 2024 09:32
@odooNextev odooNextev requested a review from renda-dev February 1, 2024 10:02

@renda-dev renda-dev 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.

Code Review, LGTM

@francesco-ooops

Copy link
Copy Markdown
Contributor

@sergiocorato fatta ulteriore review, se gentilmente puoi verificare anche te arriviamo alla chiusura. Grazie!

@HekkiMelody HekkiMelody 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.

Code review: ok

quibble (non-blocking): c'è parecchia duplicazione, ad esempio tutti i campi company_id sono impostati come readonly sia lato python che lato .xml. Per pulizia e chiarezza si potrebbe togliere il readonly lato .xml e lasciare quello impostato sul model.

Comment on lines +145 to +159
@api.onchange("company_id")
def _onchange_company_id(self):
if self.company_id:
self.update(
{
"type_ids": False,
"journal_id": False,
"asset_account_id": False,
"fund_account_id": False,
"depreciation_account_id": False,
"gain_account_id": False,
"loss_account_id": False,
"tag_ids": False,
}
)

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.

question (non-blocking): non mi è chiaro quando questa onchange si attivi, visto che company_id è readonly

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.

sono stati fatti molti cambiamenti da quando è stata creata questa PR e probabilmente serviva all'inizio, poi è stato messo readonly nel modello ed ora è inutile 👍

@stenext stenext force-pushed the 14.0-imp-assets_management-multicompany branch from 78caca0 to e9d0a5e Compare February 13, 2024 14:53
@francesco-ooops

Copy link
Copy Markdown
Contributor

@stenext si son rotti i test

@stenext stenext force-pushed the 14.0-imp-assets_management-multicompany branch from e9d0a5e to 5ad335f Compare February 13, 2024 15:13
@odooNextev

Copy link
Copy Markdown
Contributor Author

@stenext si son rotti i test

ora c'è qualcosa con l10n_it_bill_of_entry

@stenext stenext force-pushed the 14.0-imp-assets_management-multicompany branch 2 times, most recently from c205415 to ac4c41a Compare February 15, 2024 10:51
name="company_id"
options="{'no_open':1, 'no_create_edit': True}"
invisible="1"
readonly="1"

@HekkiMelody HekkiMelody Feb 20, 2024

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.

Qui, a differenza di tutti gli altri, è stato lasciato il readonly e tolto l'invisible, è voluto?

@odooNextev odooNextev Feb 20, 2024

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.

Probabilmente era per testarlo perchè lasciandolo invisible non avrei avuto la prova immediata.
Per le move lines sono tutti campi visibili, ma readonly, per me è uguale seguire questa linea anche per l'azienda o lasciarlo invisibile, ditemi voi cosa è meglio.

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.

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.

alla fine mi sono reso conto che sia currency_id che company_id sono già in sola lettura da modello perciò l'ho tolto da XML ed ho lasciato invisible
cosa ne pensi?

@HekkiMelody HekkiMelody Feb 20, 2024

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.

Mi sembra ok!

@HekkiMelody

Copy link
Copy Markdown
Contributor

A parte, quella piccola cosa, LGTM.

@HekkiMelody HekkiMelody 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.

Code review

@stenext stenext force-pushed the 14.0-imp-assets_management-multicompany branch from ac4c41a to 73721ae Compare February 20, 2024 14:52
@francesco-ooops

Copy link
Copy Markdown
Contributor

@OCA/local-italy-maintainers buongiorno, abbiamo 5 approvazioni, si può mergiare?

@eLBati

eLBati commented Feb 23, 2024

Copy link
Copy Markdown
Member

@eLBati

eLBati commented Feb 23, 2024

Copy link
Copy Markdown
Member

/ocabot merge minor

@OCA-git-bot

Copy link
Copy Markdown
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-3614-by-eLBati-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 329796d into OCA:14.0 Feb 23, 2024
@OCA-git-bot

Copy link
Copy Markdown
Contributor

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[assets_management] creazione e configurazione cespiti con utente multicompany

10 participants