-
Notifications
You must be signed in to change notification settings - Fork 733
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
Fix deprovision cache issue #8213
Fix deprovision cache issue #8213
Conversation
Fix device settings cache persistence on deletion.
kolibri/core/device/models.py
Outdated
@@ -93,6 +100,10 @@ def save(self, *args, **kwargs): | |||
super(DeviceSettings, self).save(*args, **kwargs) | |||
cache.set(DEVICE_SETTINGS_CACHE_KEY, self, 600) | |||
|
|||
def delete(self, *args, **kwargs): | |||
super(DeviceSettings, self).delete(*args, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parsing the obfuscated Manager code, it seems that perhaps this method isn't necessary? It looks like it should be utilizing the delete
method on the QuerySet. Otherwise, it seems like this should return the result of super.delete
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha - can try removing this method and seeing if the tests still pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might add one more test. I see DeviceSettings.objects.all().delete()
which I think invokes the QuerySet method, but DeviceSettings.objects.delete()
would go through the manager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the manager test, it passed. Removed the model method, and it failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me!
Return output of super calls.
Expected checks are only in develop atm. Merging! |
Summary
References
Fixes issue after deprovisoining where device settings would persist in cache
Reviewer guidance
Setup device
Deprovision
Try to set up device again
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)