search

OCA / product-configurator

Advanced Product Configurator (from the original Pledra project https://github.com/pledra/odoo-product-configurator)
GNU Affero General Public License v3.0
33 stars 78 forks source link

[FIX] product_configurator (Sessions being removed) #65

Closed patrickrwilson closed 2 years ago

patrickrwilson commented 2 years ago

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 commented 2 years ago

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

patrickrwilson commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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

dreispt commented 2 years ago

@bizzappdev can you review please?

bizzappdev commented 2 years ago

LGTM

OCA-git-bot commented 2 years ago

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 commented 2 years ago

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

PCatinean commented 2 years ago

/ocabot merge patch

OCA-git-bot commented 2 years ago

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 commented 2 years ago

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