Skip to content

[17.0][IMP] tracking_manager: add domain condition for tracking fields - #3176

Closed
CRogos wants to merge 1 commit into
OCA:17.0from
c4a8-odoo:17.0-tracking_manager-domain
Closed

CRogos wants to merge 1 commit into
OCA:17.0from
c4a8-odoo:17.0-tracking_manager-domain

Conversation

@CRogos

@CRogos CRogos commented Jan 24, 2025

Copy link
Copy Markdown
Contributor

I would like to extend the functionality of tracking_manager by adding the possibility to enable tracking only on certain condition, by adding a tracking_domain field. The value is only tracked if the filter applies.

image

image

image

@OCA-git-bot

Copy link
Copy Markdown
Contributor

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

@fb-ife

fb-ife commented Jan 31, 2025

Copy link
Copy Markdown

@CRogos ,
can you please provide me with configuration'steps to test the functionality?

Thanks

@CRogos

CRogos commented Jan 31, 2025

Copy link
Copy Markdown
Contributor Author

@fb-ife have you had a look into the screenshots above and the usage documentation file? I've already highlighted the configuration steps. Not sure what else needs to be added?

@CRogos
CRogos force-pushed the 17.0-tracking_manager-domain branch from d7dfa81 to b94e4a1 Compare February 17, 2025 10:25
@CRogos

CRogos commented Feb 24, 2025

Copy link
Copy Markdown
Contributor Author

@sebastienbeau, @Kev-Roche I would really like to add this improvement to this module. Could you have a look at this?
thank you in advance.

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

Tested with the runboat. Functional & code review LGTM

@CRogos

CRogos commented Mar 4, 2025

Copy link
Copy Markdown
Contributor Author

@pedrobaeza what do you think about this change? I would be very pleased if you could do the second review and merge if it is OK.

@pedrobaeza pedrobaeza added this to the 17.0 milestone Mar 4, 2025

@pedrobaeza pedrobaeza left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The problem I see with this change is that affects performance even if you don't define any domain, so I'm not totally liking. You may do this in a extra module?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why changing the file name and putting drawio ?

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.

This picture is an valid png file, but also an valid draw.io file.
When you have the draw.io extension installed in vscode, you can edit the rectangles of the picture. That means you always have the unchanged screenshot and can add or remove additional highlighting easily.

image

image

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, didn't know. Thanks for telling me.

Comment thread tracking_manager/models/models.py Outdated
@CRogos
CRogos force-pushed the 17.0-tracking_manager-domain branch 3 times, most recently from 36c0a1b to 878aa2b Compare March 4, 2025 17:56

@pedrobaeza pedrobaeza left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Uhm, I'm not sure that this code fixes the performance thing and even if _mail_track is the correct hook. I still prefer to have that as a separate module, but let's see what others think.

@CRogos

CRogos commented Mar 17, 2025

Copy link
Copy Markdown
Contributor Author

@acsonefho @victoralmau what do you think about this change?

@acsonefho

Copy link
Copy Markdown
Contributor

@acsonefho @victoralmau what do you think about this change?

I have no real opinion on the matter. It could be a specific module or a new feature of this one, both are ok for me.
I also ask to the analyst who already use this module and same opinion as mine.

Thanks for the work and your contribution 💪

@CRogos
CRogos force-pushed the 17.0-tracking_manager-domain branch from 878aa2b to 1f99ec2 Compare March 24, 2025 16:45
@CRogos

CRogos commented Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

I did a quick performance check by calling _mail_track 100 times, and there is a significant performance decrease as @pedrobaeza assumed.

image

After inserting a cache to the query loading the tracking_domain value, this performance decrease is improved, but there is still a performance decrease by ~25% compared with before this change.
On the left graph, you can see that the change decreases the performance by ~100% in case the field has a tracking_domain.
image

I am not sure how significant this performance lost is. When we take into account the total save form process, I assume the 25% and also the 100% are not that relevant, but did not traced it with the performance tool.

@CRogos
CRogos force-pushed the 17.0-tracking_manager-domain branch from 1f99ec2 to 10426ab Compare March 24, 2025 16:51
@pedrobaeza

Copy link
Copy Markdown
Member

Thanks for doing the check. I think this enforces the argument of adding this feature in a separate module for those wanting it to assume that decrease.

@CRogos

CRogos commented Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

I think the failing tests is caused/solved by this: #3237
I also moved the change into an separate module if this is preferred my the majority: #3236

@CRogos
CRogos force-pushed the 17.0-tracking_manager-domain branch from 10426ab to a91f8c0 Compare March 25, 2025 07:29
@CRogos
CRogos marked this pull request as draft March 28, 2025 07:31
@CRogos

CRogos commented Apr 2, 2025

Copy link
Copy Markdown
Contributor Author

Moved to a separate module: #3236

@CRogos CRogos closed this Apr 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants