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.

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

refresh buffer during cron action #59

Closed

Conversation

JordiBForgeFlow
Copy link
Sponsor Member

No description provided.

@OCA-git-bot
Copy link
Contributor

Hi @LoisRForgeFlow,
some modules you are maintaining are being modified, check this out!

Copy link

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

👍 Makes sense

@JordiBForgeFlow
Copy link
Sponsor Member Author

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 13.0-ocabot-merge-pr-59-by-JordiBForgeFlow-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jul 27, 2020
Signed-off-by JordiBForgeFlow
@OCA-git-bot
Copy link
Contributor

@JordiBForgeFlow your merge command was aborted due to failed check(s), which you can inspect on this commit of 13.0-ocabot-merge-pr-59-by-JordiBForgeFlow-bump-patch.

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.

Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

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

Why is it needed? is this solving any bug?

Comment on lines +1221 to 1223
self.refresh()
if not only_nfp or only_nfp == "out":
self._calc_qualified_demand()
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is also used for the nfp calculation, with the refresh in that position it will always be executed, even when you only want to update NFP.

@@ -1218,6 +1218,7 @@ def cron_actions(self, only_nfp=False):
"""This method is meant to be inherited by other modules in order to
enhance extensibility."""
self.ensure_one()
self.refresh()
Copy link
Member

Choose a reason for hiding this comment

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

    @api.model
    def refresh(self):
        """ Clear the records cache.

            .. deprecated:: 8.0
                The record cache is automatically invalidated.
        """
        self.invalidate_cache()

The correct method is: self.invalidate_cache()

Copy link
Member

Choose a reason for hiding this comment

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

And actually it looks like a dangerous call, because it wipes all the cache for the stock buffers, I suppose it should be at least self.invalidate_cache(ids=self.ids), but probably fnames should be set as well so only the desired computed fields are invalidated.

Copy link
Member

Choose a reason for hiding this comment

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

I tried something like this:

    def cron_actions(self, only_nfp=False):
        """This method is meant to be inherited by other modules in order to
        enhance extensibility."""
        self.ensure_one()
        # invalidate all non-stored computed values
        self.invalidate_cache(
            fnames=[
                fieldname
                for fieldname, props in self._fields.items()
                if props.compute
                and not props.store
                and not props.relational
                and not props.related
            ],
            ids=self.ids,
        )
        if not only_nfp or only_nfp == "out":

But even then, it recomputes probably too much, for each buffer of the chunks.

@@ -1218,6 +1218,7 @@ def cron_actions(self, only_nfp=False):
"""This method is meant to be inherited by other modules in order to
enhance extensibility."""
self.ensure_one()
self.refresh()
Copy link
Member

Choose a reason for hiding this comment

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

And actually it looks like a dangerous call, because it wipes all the cache for the stock buffers, I suppose it should be at least self.invalidate_cache(ids=self.ids), but probably fnames should be set as well so only the desired computed fields are invalidated.

@guewen
Copy link
Member

guewen commented Feb 3, 2021

Why is it needed? is this solving any bug?

Same question :)

guewen added a commit to camptocamp/ddmrp that referenced this pull request Feb 3, 2021
Fix tests:

* test_22_procure_recommended
* test_23_buffer_zones_all

It is possible that the invalidation is only required because of the
tests and would not be required at runtime. But this has been proposed
in the past: OCA#59

However, this commit tries to invalidate the less possible fields to
limit the computations and data to fetch again.
guewen added a commit to camptocamp/ddmrp that referenced this pull request Feb 3, 2021
Fix tests:

* test_22_procure_recommended
* test_23_buffer_zones_all

It is possible that the invalidation is only required because of the
tests and would not be required at runtime. But this has been proposed
in the past: OCA#59

However, this commit tries to invalidate the less possible fields to
limit the computations and data to fetch again.
@LoisRForgeFlow
Copy link
Contributor

I think we can close this in favor of the fixes included in #81. @JordiBForgeFlow If we miss something please reopen it.

DavidBForgeFlow pushed a commit to ForgeFlow/ddmrp that referenced this pull request Mar 17, 2021
Fix tests:

* test_22_procure_recommended
* test_23_buffer_zones_all

It is possible that the invalidation is only required because of the
tests and would not be required at runtime. But this has been proposed
in the past: OCA/ddmrp#59

However, this commit tries to invalidate the less possible fields to
limit the computations and data to fetch again.
DavidBForgeFlow pushed a commit to ForgeFlow/ddmrp that referenced this pull request Mar 19, 2021
Fix tests:

* test_22_procure_recommended
* test_23_buffer_zones_all

It is possible that the invalidation is only required because of the
tests and would not be required at runtime. But this has been proposed
in the past: OCA/ddmrp#59

However, this commit tries to invalidate the less possible fields to
limit the computations and data to fetch again.
DavidBForgeFlow pushed a commit to ForgeFlow/ddmrp-1 that referenced this pull request Mar 19, 2021
Fix tests:

* test_22_procure_recommended
* test_23_buffer_zones_all

It is possible that the invalidation is only required because of the
tests and would not be required at runtime. But this has been proposed
in the past: OCA#59

However, this commit tries to invalidate the less possible fields to
limit the computations and data to fetch again.
ChrisOForgeFlow pushed a commit to ForgeFlow/ddmrp that referenced this pull request Dec 16, 2021
Fix tests:

* test_22_procure_recommended
* test_23_buffer_zones_all

It is possible that the invalidation is only required because of the
tests and would not be required at runtime. But this has been proposed
in the past: OCA/ddmrp#59

However, this commit tries to invalidate the less possible fields to
limit the computations and data to fetch again.
BernatPForgeFlow pushed a commit to ForgeFlow/ddmrp that referenced this pull request Jun 6, 2023
Fix tests:

* test_22_procure_recommended
* test_23_buffer_zones_all

It is possible that the invalidation is only required because of the
tests and would not be required at runtime. But this has been proposed
in the past: OCA/ddmrp#59

However, this commit tries to invalidate the less possible fields to
limit the computations and data to fetch again.
patrickrwilson pushed a commit to ursais/ddmrp that referenced this pull request Apr 18, 2024
Fix tests:

* test_22_procure_recommended
* test_23_buffer_zones_all

It is possible that the invalidation is only required because of the
tests and would not be required at runtime. But this has been proposed
in the past: OCA#59

However, this commit tries to invalidate the less possible fields to
limit the computations and data to fetch again.
Vandan-OSI pushed a commit to ursais/ddmrp that referenced this pull request May 6, 2024
Fix tests:

* test_22_procure_recommended
* test_23_buffer_zones_all

It is possible that the invalidation is only required because of the
tests and would not be required at runtime. But this has been proposed
in the past: OCA#59

However, this commit tries to invalidate the less possible fields to
limit the computations and data to fetch again.
DavidJForgeFlow pushed a commit to ForgeFlow/ddmrp-1 that referenced this pull request Jun 13, 2024
Fix tests:

* test_22_procure_recommended
* test_23_buffer_zones_all

It is possible that the invalidation is only required because of the
tests and would not be required at runtime. But this has been proposed
in the past: OCA#59

However, this commit tries to invalidate the less possible fields to
limit the computations and data to fetch again.
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.

None yet

6 participants