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

[FIX] product_configurator (Sessions being removed) #65

Merged
merged 2 commits into from
Jul 4, 2022
Merged

[FIX] product_configurator (Sessions being removed) #65

merged 2 commits into from
Jul 4, 2022

Conversation

patrickrwilson
Copy link
Contributor

The product.config.session model is a regular model however the wizards that inherit are set a transient models. This is causing sessions to be deleted when the vacuum cleaner runs. Since sessions can contain important information like custom values, they shouldn't be deleted by the vacuum so this changes the transient models to regular models.

If it's desired to 'clean' sessions periodically a scheduled action is suggested as it can provide more control over cleanup depending on individual needs.

@OCA-git-bot
Copy link
Contributor

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

@patrickrwilson
Copy link
Contributor Author

@dreispt Here is the fix PR for the session deleting issue and it's ready for your review. Also, Travis was failing due to a compute method singleton error so this also includes the small fix to make this green.

@bizzappdev
Copy link
Contributor

@patrickrwilson @dreispt I doubt that the vacuum method of the TransientModel is deleting the session.
In the website module, we have the cron which is deleting the unused/inactive sessions. might be that is the culprit. as I can not find any relation m20 in the product.config.session to product.configurator which has delete oncascade?
For your reference.
https://github.com/OCA/product-configurator/blob/14.0/website_product_configurator/data/cron.xml

@patrickrwilson
Copy link
Contributor Author

@bizzappdev thanks for your feedback, we did originally look at the website module as the culprit however it wasn't installed which lead us down the path to discover the vacuum being the cause. I worked with @dreispt to troubleshoot and sure enough after a bit of time when the vacuum would run the sessions would be 'cleaned' out, which causes problems since sessions can contain pertanent information like custom values.

@bizzappdev
Copy link
Contributor

@patrickrwilson thank you for your answer. if it was not that cron. then something else.
IMO you should find the real reason for records getting deleted.

  1. we do not have any m2o in session to the configuration wizard with delete oncascade. so I can not see the real reason for the same.
  2. also making product.configurator and other objects as the regular model will create data redundancy which is not at all needed.

@dreispt
Copy link
Member

dreispt commented Apr 28, 2022

Hello @bizzappdev, we are pretty sure the Autovacuum is deleting the records.
And this makes sense, because at some point the Model is marked as Transient.
It is not related to m2o cascading because the instance had no one connected nor other actions running when seeing the unlink happening.

The objects are already regular Models, and then we see code inheriting them as TransientModel, which is not something that should be done, and should be fixed anyway.

It is correct you want to clean up old sessions.
That can be achieved with the existing cron, it is just it should be moved from the website to the base module.
@patrickrwilson Can you do this? It can be a separate PR.

@dreispt
Copy link
Member

dreispt commented Apr 29, 2022

@patrickrwilson I'm thinking, we need to leave the workbench clean when we leave: should these files be moved out of the Wizards subdiretory?

@bizzappdev
Copy link
Contributor

Hello @bizzappdev, we are pretty sure the Autovacuum is deleting the records. And this makes sense, because at some point the Model is marked as Transient. It is not related to m2o cascading because the instance had no one connected nor other actions running when seeing the unlink happening.

The objects are already regular Models, and then we see code inheriting them as TransientModel, which is not something that should be done, and should be fixed anyway.

It is correct you want to clean up old sessions. That can be achieved with the existing cron, it is just it should be moved from the website to the base module. @patrickrwilson Can you do this? It can be a separate PR.

I will also try to investigate why it is marking the Session as a transient model.

@bizzappdev
Copy link
Contributor

@dreispt @patrickrwilson
https://github.com/OCA/product-configurator/blob/14.0/product_configurator/wizard/product_configurator.py#L894 Please check this before proceeding. this is your culprit.

@patrickrwilson
Copy link
Contributor Author

@bizzappdev thank you for your input, I was able to test commenting out the unlink method and the sessions do stay now and do not get deleted when the vacuum runs.
@dreispt do you have any objections to me going ahead and updating this to just comment out the unlink method Bizzappdev pointed out instead of changing the models?

@dreispt
Copy link
Member

dreispt commented May 6, 2022

@patrickrwilson I like better to just comment out the unlink method Bizzappdev pointed out.

@patrickrwilson
[IMP] Change from Transient to regular Model
a843569 
Revert "[IMP] Change from Transient to regular Model"

This reverts commit 88cc8fe.

[FIX] commented out unlink method
@patrickrwilson
[FIX] product_configurator_mrp_component (Singleton Error)
ee3b1bf 
Travis failed due to singleton error while
@patrickrwilson patrickrwilson force-pushed the product_configurator_sessions_getting_removed branch from e345b58 to ee3b1bf Compare May 6, 2022 13:07
@patrickrwilson patrickrwilson changed the title [FIX] Change from Transient to regular Model [FIX] product_configurator (Sessions being removed) May 6, 2022
@dreispt dreispt requested a review from bizzappdev May 11, 2022 12:51
@dreispt
Copy link
Member

dreispt commented May 11, 2022

@bizzappdev can you review please?

@bizzappdev
Copy link
Contributor

LGTM

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

@bizzappdev
Copy link
Contributor

@dreispt proceed further for the merge. it has been approved. and it is a critical bug fix.

@PCatinean
Copy link
Contributor

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-65-by-PCatinean-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 5d8bda0 into OCA:14.0 Jul 4, 2022
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 3654af6. 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
Reviewers

@bizzappdev bizzappdev bizzappdev approved these changes

@PCatinean PCatinean PCatinean approved these changes

@dreispt dreispt dreispt approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@patrickrwilson @OCA-git-bot @bizzappdev @dreispt @PCatinean