[16.0][IMP] tracking_manager: allow tracking readonly fields

[CRogos]

@AnizR, @Kev-Roche, @sebastienbeau

What is the reason for the limitation that read_only fields cannot be tracked?
When there have a look at the Odoo native tracking, there are also read_only fields with tracking enabled:

In our case we what to track changes on an enterprise table account.asset, where most of the fields are read_only and can only be changed by a separate wizard. But through this wizard, user changes are possible and therefore tracking might be necessary.

Are there any doubt to remove the read_only filter?

[OCA-git-bot]

Hi @Kev-Roche, @sebastienbeau,
some modules you are maintaining are being modified, check this out!

[AnizR]

Why did you remove store=True?

I think that you may need to add something in the depends of your compute if the field isn't (re-)computed correctly.

[CRogos]

The trackable attribute is already computed, but with this PR we change the implementation how it is computed.
Changing the implementation does not change the stored value automatically.
I agree another options would be an migration update script to recalculate all values.

In think this is the only place where trackable is used. I am not sure if this syntax requires a stored field. But this could also be replaced by an lambda. Beside not changing it, I do not see the benefit of storing this simple value.

server-tools/tracking_manager/models/ir_model.py

Lines 128 to 135 in 485de9f

	def update_custom_tracking(self):
	for record in self:
	fields = record.field_id.filtered("trackable").filtered_domain(
	literal_eval(record.automatic_custom_tracking_domain)
	)
	fields.write({"custom_tracking": True})
	untrack_fields = record.field_id - fields
	untrack_fields.write({"custom_tracking": False})

[AnizR]

I think that a migration script might be needed like you suggested but I am not 100% sure about that..

[CRogos]

Maybe there is a 3rd opinion over the weekend. I am personally not so a big fan of store=True on computed values. But indeed, changing it has also its risks.

[Kev-Roche]

I ll prefer to keep it stored because in some cases, we want to save a custom choice of fields tracked, and this independently to its kind (read only, related, ...).

[CRogos]

I think "trackable" is only used as a filter when showing all trackable fields in the list. If this value is False, the field is not in the list and you cannot activating the track. There is also no possibility to change this value in the UI.

I do not see why this field needs to be persistent, but I agree changing it could also have unwanted side effects. But I did not have time yet to write the necessary upgrade script, to recalculate all values with the update. Without an update of the stored values, removing the readonly filter has no effect.

[Kev-Roche]

I m not sure about this.
In a way, the behavior of readonly field is known and not changed by users. In order to avoid some noise with too many tracked fields by default, I will prefer to not include the readonly fields as tracked.
IMHO, maybe just set as tracked the relevant fields to follow, as you mention in your specific case, will be enough.

[Kev-Roche]

I ll prefer to keep it stored because in some cases, we want to save a custom choice of fields tracked, and this independently to its kind (read only, related, ...).

[CRogos]

I m not sure about this. In a way, the behavior of readonly field is known and not changed by users. In order to avoid some noise with too many tracked fields by default, I will prefer to not include the readonly fields as tracked. IMHO, maybe just set as tracked the relevant fields to follow, as you mention in your specific case, will be enough.

I only give the user the option to track fields, and do not activate the tracking. (trackable vs. tracking_custom field)
Do you still have concerns?

[Kev-Roche]

I m not sure about this. In a way, the behavior of readonly field is known and not changed by users. In order to avoid some noise with too many tracked fields by default, I will prefer to not include the readonly fields as tracked. IMHO, maybe just set as tracked the relevant fields to follow, as you mention in your specific case, will be enough.

I only give the user the option to track fields, and do not activate the tracking. (trackable vs. tracking_custom field) Do you still have concerns?

But if you set tracking on a new model or just update it with update_custom_tracking button, it will add all the readonly fields as tracked and we don't want too many automatic and unwanted tracked fields. Maybe just trackable as readonly=False and store will be enough to deal with specific cases, or making optional the readonly fields as trackable ?

[CRogos]

I've tested it on res.partner and there are 74 fields tracked instead of 59. Yes these are 15 fields more.
How about initializing the default domain this way to prevent readonly fields from automatic configuration?

[CRogos]

I've now re-added the store flag and added an upgrade script to recompute the existing value.

[Kev-Roche]

as you said "to prevent readonly fields from automatic configuration", my main point is to keep the configuration and reading of tracking fields simple.
From my opinion, the tracking of readonly fields is marginal and can not be the main rule of this module.

cc @sebastienbeau

[CRogos]

@Kev-Roche I think everything is done now. Could you update your review to approved?

[CRogos]

I think everything is final now. Could you review again?

[CRogos]

@Kev-Roche @AnizR @legalsylvain could update your reviews?

[CRogos]

@Kev-Roche

From my opinion, the tracking of readonly fields is marginal and can not be the main rule of this module.

Unfortunately the account_asset module of odoo enterprise is one, were the fields are readonly and the user changes the fields via a wizard. So technically they are readonly, but actually they are not and they are very important to track.

If you really see a risk, I could also add a global setting that needs to be enabled first before allowing to track readonly attributes. But "Keep the configuration simple" could also mean not to add this global setting ;-) .

@pedrobaeza do you have an opinion on this?

[Kev-Roche]

@Kev-Roche

From my opinion, the tracking of readonly fields is marginal and can not be the main rule of this module.

Unfortunately the account_asset module of odoo enterprise is one, were the fields are readonly and the user changes the fields via a wizard. So technically they are readonly, but actually they are not and they are very important to track.

If you really see a risk, I could also add a global setting that needs to be enabled first before allowing to track readonly attributes. But "Keep the configuration simple" could also mean not to add this global setting ;-) .

@pedrobaeza do you have an opinion on this?

I m good with the current way

[OCA-git-bot]

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). 🤖

[pedrobaeza]

I think it's usual that some fields are readonly, but unlocked on certain conditions (like state and having force_save="1" parameter) or modified by code, so they should be tracked as well.

[CRogos]

@sebastienbeau could you merge?

[Kev-Roche]

/ocabot merge minor

[OCA-git-bot]

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-2813-by-Kev-Roche-bump-minor, awaiting test results.

[OCA-git-bot]

@Kev-Roche your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-2813-by-Kev-Roche-bump-minor.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

[CRogos]

@Kev-Roche do you have an idea what's the issue with the merge? Error does not look related to this change? Shall I rebase or could you give it another try?

[Kev-Roche]

@CRogos I have no Idea, don't see any related reason... can you rebase please, then I ll try again.

[CRogos]

This themes to be the problem: #2578

[CRogos]

@Kev-Roche the problem with the repo has been solved. Could you retry the merge?

[Kev-Roche]

/ocabot merge minor

[OCA-git-bot]

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-2813-by-Kev-Roche-bump-minor, awaiting test results.

[OCA-git-bot]

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