FIX: Properly associate user_profiles background urls via upload id. (PR #7454)

GitHub

You’ve signed the CLA, tgxworld. Thank you! This pull request is ready for review.

1 Warning
:warning: This pull request is big! We prefer smaller PRs whenever possible, as they are easier to review. Can this be split into a few smaller PRs?

Generated by :no_entry_sign: Danger

@gschlager @davidtaylorhq I would like to extra pairs of eyes on this PR since it involves the migration of data from one column to another. It’ll be great if I can get your reviews on this PR. Thank you in advance.

The default behavior of add_foreign_key is to use on_delete: :restrict. That makes it impossible to delete the upload row until the foreign key is removed. I think maybe we would prefer on_delete: :nullify. What do you think?

It all looks good to me, and I tested running the migration locally. I just have one question/comment about the foreign keys

Looks good! I had only minor comments on the specs. And I love that you added specs for the migrations! :heart_eyes: Some of the specs are failing though and on first glance the failures seem legit.

It might be worth using two different uploads in order to make sure that the URLs aren’t mixed up.

I guess it depends. The default behavior is perfectly fine if you want to make sure that you don’t accidentally delete the upload. I think it’s fine as it is.

Same thing here. I’d use 2 different uploads to prevent mixups.

This index is going no? and the column? might as well drop the annotation

As long as you take maintaining any weird with the new foreign key constraint I guess I am ok, go ahead and merge.

I don’t feel too strongly either way, would be happy for it to stay as restrict. However, it might be annoying if you can’t Upload.find(123).destroy any more :thinking:

Yea it should be restrict here. We shouldn’t be able to delete the upload if it is still tied to a profile or card background.

@SamSaffron We need to drop the columns in a post migration but I’m playing it safe first by not dropping the columns since we’re migrating data.

And I love that you added specs for the migrations!

@gschlager I actually wrote specs because I wasn’t sure if the queries I wrote for my migration was going to be correct or not :grin: I’m glad I did actually because it helped me to catch a few edge cases I would have missed have I not written the specs.

Thanks for reviewing this @gschlager @davidtaylorhq and @SamSaffron :+1: