DEV: Change upload verified column to be integer (PR #10643)

Per review https://review.discourse.org/t/dev-add-verified-to-uploads-and-fill-in-s3-inventory-10406/14180

Change the verified column for Upload to a verified_status integer column, to avoid having NULL as a weird implicit status.

GitHub

:heart:

Technically we don’t need this since the default is already 1.

This seems alittle odd because I don’t think it is possible to rollback this migration without the migration already being called.

We should mark this column as readonly first to prevent any writes going to it.

Just curious, what is the difference between not_checked and not_verified? Those two keys seem to have the similar meaning to me :grinning:

Since we’re doing a mass update to the column, we should leave out the index first so that the UPDATE below will be faster. Some sites may have a ton of uploads so the UPDATE may be potentially slow.

…foiled again! I think you are right. Maybe unchecked? The first value means we have not yet gone and checked the S3 inventory for that upload. Not verified means the upload etag does not match after we have checked.

Great point, will add the index after update.

For this you mean Migration::ColumnDropper.mark_readonly(:uploads, :verified) yes?

Hmm yeah good point

@tgxworld

Maybe unverified, verified and invalid? Does that seem to be clearer while still holding the original meaning?

Yup :slight_smile:

unverified reads as the opposite of verified for me. I think i will do unchecked, verified, and invalid_etag to be more specific. (maybe in future then we can have more than one invalid state)

That is better :+1: Anyway, the keys are pretty flexible since it only affects the ruby side code.

I am curious if here wouldn’t be beneficial to use rails enums.

That would be default define scopes so for example later instead of Upload.where(verification_status: Upload.verification_statuses[:invalid_etag]) We would be able to simplify it to Upload.invalid_etag

I grepped and we are not using enums too often so maybe there is a reason for that. I only found one in ApplicationRequest model

I am not sure, we have lib/enum.rb so I am just being consistent. IMO we could easily make the API easier so at least we could do Upload.verification_statuses.invalid_etag