Convert ReusedGUID to AddonGUID, containing fks for all Addons #14406
Conversation
eviljeff
requested review from
a team and
bobsilverberg
and removed request for
a team
| addonguids = [ | ||
| AddonGUID( | ||
| addon_id=addon['id'], guid=addon['guid']) for addon in addons] | ||
| AddonGUID.objects.bulk_create(addonguids, batch_size) |
There was a problem hiding this comment.
@diox for second opinion on this migration
There was a problem hiding this comment.
This might be a little too much for a migration... I guess we can test that on dev/stage and see if it takes too long...
In prod, we'll likely want the site in read only to ensure that no new add-ons are created before the new code is up - Otherwise there is a window between the guids being fetched and new code being deployed where add-ons would not have a corresponding AddonGUID.
There was a problem hiding this comment.
This looks good to me, nice work @eviljeff. I added a couple of questions.
r+wc
| ReusedGUID.objects.create(addon=old_two, guid='reuse@') | ||
| ReusedGUID.objects.create(addon=old_other, guid='other@') | ||
| ReusedGUID.objects.create(addon=old_noguid, guid='') | ||
| old_one = addon_factory(status=amo.STATUS_DELETED, guid=None) |
There was a problem hiding this comment.
Why is it necessary to set guid to None now, when it wasn't before? Is this just making the test more explicit?
There was a problem hiding this comment.
It's to match the states the addon instances would be in reality. If they're supposed to be deleted and re-uploaded then the guid would have been reset on the old instance after the new upload.
(If we don't set the guid to None then we'd need to find and update the AddonGUID instance for them to point to guid='reuse@' instead - it'd be AddonGUID.objects.get(addon=old_one).update(guid='reuse@'))
There was a problem hiding this comment.
... and the guid hasn't been set to None since 2016, when someone changed it in #1471.
There was a problem hiding this comment.
Now it's back to not setting guid to None.
| ReusedGUID.objects.filter(guid=addon.guid).values_list( | ||
| 'addon_id', flat=True) if addon.guid else []) | ||
| AddonGUID.objects.filter(guid=addon.guid) | ||
| .exclude(addon=addon) |
There was a problem hiding this comment.
Do we need this exclude now because we don't want the id of the current addon included in this list? Is there any test coverage for this change? I.e., should there be a test where the id for the current addon is in AddonGUID? Maybe that's already covered in the changes to the tests?
There was a problem hiding this comment.
Do we need this
excludenow because we don't want theidof the currentaddonincluded in this list?
Yes. The current addon wouldn't have been in ReusedGUID before, but is in AddonGUID now.
Is there any test coverage for this change?
the existing test lines below would fail if we don't exclude the current add-on
expected = [
(f'{old_one.id}', reverse('reviewers.review', args=[old_one.id])),
(f'{old_two.id}', reverse('reviewers.review', args=[old_two.id])),
]
6736be3
to
aa63a0a
Compare
|
rebased to fix the conflicts and to adjust things a little after #14423 |
|
going to merge. If -dev fails to deploy we can reconsider the migration - though if we don't have it in the migration we'll need to disable bloomfilter generation until it's been executed in some other way. |
fixes mozilla/addons#7606