[18.0][FIX] subscription_oca: prevent cron crash and enforce data consistency#1456
Open
rrebollo wants to merge 8 commits into
Conversation
…tion create When confirming a sale.order from a CRM opportunity, sale_crm injects default_tag_ids (containing crm.tag IDs) into the context. Without this fix, those IDs are applied by default_get() to sale.subscription.tag_ids, causing a FK constraint violation on sale_subscription_sale_subscription_tag_rel. Fix: use clean_context() from odoo.tools to strip all default_* keys from context before calling sale.subscription.create(), following the idiomatic Odoo core pattern used in res_bank, account_move, hr_expense_sheet, etc. Adds a regression test that reproduces the production scenario end-to-end.
Add stage_in_progress fixture with type in_progress and assign it to sub7, sub8, sub9 so their in_progress=True matches a proper stage type. Update test_subscription_oca_sub_cron_error to verify the cron catches and logs exceptions instead of propagating them.
Merge recurring_next_date, close_reason_id, and stage_id into a single write() call to eliminate the split-write window that could leave recurring_next_date=False with in_progress=True.
Enforce in_progress=True requires stage_id.type==in_progress and a valid recurring_next_date via @api.constrains.
Add per-record savepoint isolation so a single subscription failure does not kill the entire cron. Filter to pre/in_progress stages only. Add defensive guards on recurring_next_date and date comparisons.
Ensure the @api.constrains sees consistent state during write by including in_progress=False in the write dict, so the constraint fires before the write() override sets it.
Author
|
Maybe this is related to #1091. |
…ia write() Before super().write(), inject in_progress=False into values when the target stage type is not 'in_progress'. This ensures the @api.constrains validator sees consistent state during flush, preventing ValidationError for any write() that changes stage_id on an in-progress subscription.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
During a database upgrade, a subscription was found with
in_progress=Truebut no valid
recurring_next_date. When the cron jobcron_subscription_management()processed this record, the comparison
False <= date.today()raisedTypeError,crashing the entire cron and rolling back all invoices generated in that run.
Root Cause (Potentially)
close_subscription()wroterecurring_next_date = Falseas a standaloneassignment before the
write({'stage_id': ...})call, creating a split-writewindow that left the record in an inconsistent state.
What This Does
close_subscription()into a single atomicwrite()call@api.constrainsenforcingin_progress=Truerequires a consistentstage type and a valid
recurring_next_datenever kills the entire job
pre/in_progressstages onlyTypeErroronlegacy bad data
sub7/sub8/sub9) with correctin_progresssemantics (stage type
"in_progress")write()override timing: injectin_progress=Falsebeforesuper().write()when target stage is not"in_progress", so theconstraint sees consistent state during flush
Commits
[FIX] subscription_oca: fix test fixtures and cron error testAdd
stage_in_progresswith correct type, update error test for newcron behavior.
[FIX] subscription_oca: consolidate close_subscription atomic writeMerge all three fields into one
write()to prevent split-writeinconsistencies.
[FIX] subscription_oca: add constraint for in_progress consistencyEnforce
in_progress=Truerequiresstage_id.type="in_progress"and a valid
recurring_next_date.[FIX] subscription_oca: harden cron with savepoint and domain filterPer-record savepoint isolation, domain filter for
pre/in_progressstages, defensive guards on date comparisons.
[FIX] subscription_oca: set in_progress=False in close_subscriptionEnsure constraint sees consistent state during
write().[FIX] subscription_oca: prevent ValidationError when changing stage via write()Inject
in_progress=Falsebeforesuper().write()when the targetstage type is not
"in_progress", preventing constraint errors on anystage change for in-progress subscriptions.
Testing
All existing tests pass. Updated
test_subscription_oca_sub_cron_errorto verify the cron catches and logs exceptions instead of propagating them.
@BinhexTeam T18387