-
Notifications
You must be signed in to change notification settings - Fork 18
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
'was_renewed' field set incorrectly in expired reservations. #674
Comments
Needs a test writing, I guess.Have you looked for "was_renewed" in the test suite? |
Yeah, this is fairly obviously problematic: Lines 167 to 171 in 59720b8
IIRC we use this function both in expiry and in renewal. We only need to do this bit in the latter. |
Thanks @lentinj - Those lines are indeed part of the problem. I've been chatting about this with James. We think there are 2 things wrong:
|
Yeah (1) is pretty obvious how to sort, and wouldn't take very long. And yes, I'd not realised about (2). Do we know what fields should be preserved, besides |
Great
|
Anyway, panic over. We thought these had all gone missing completely! But it's all recoverable, phew. |
Is |
Gah, somehow I managed to not notice the convenient Lines 138 to 144 in 59720b8
(even more annoyingly, I was the one that moved it there in the first place. Ah well.) But, it doesn't preserve |
was_renewed should only be for expired entries that were either renewed or expired and later repurchased. Set it during the renewal process insted of at expiry.
Instead of removing rows on expiry, use clear_reservation() to remove everything bar the view counts. Make sure these are preserved through the purchase & expiry process.
Either way, the code changes are above. Note the latter also changes |
It's not used, so remove mention of it from queries and force it to always be empty.
@hyanwong The above commit removes as much of the deactivated field as possible without restructuring the database. It's not part of the pull request above but can add it if you still think it's a good plan? |
Everything that's moved across to expired reservations seems to have 'was_renewed' = 1 regardless of whether it was really renewed or not. e.g. OTTID 50727. I think we need to check this as it's probably a bug.
The text was updated successfully, but these errors were encountered: