Create tables for code review v2

[jamescodeorg]

Creating tables for code review v2 as described in the tech spec.

This is using the old pattern for deleted_at which we know to be flawed. I will migrate to the new pattern in a separate PR once we figure out what the new pattern should be.

Links

• spec: doc
• tech spec: doc
• jira tickets: LP-2298, LP-2299

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

• Tests provide adequate coverage
• Privacy and Security impacts have been assessed
• Code is well-commented
• New features are translatable or updates will not break translations
• Relevant documentation has been added or updated
• User impact is well-understood and desirable
• Pull Request is labeled appropriately
• Follow-up work items (including potential tech debt) are tracked and linked

[jamescodeorg]

Currently, closed_at will have the same flaw as deleted_at -- it is possible to have multiple rows with closed_at equal to null. Once we figure out what the correct pattern is for deleted_at, I will apply it to closed_at as well.

[bethanyaconnor]

Does closed_at need to be included in this index at all? I was expecting, based on the spec, a code review to be unique across a user, script, and level. It seems like deleted_at is there to be able to reset progress, so that also makes sense in the index. But I can't see how closed_at factors into this uniqueness

[jamescodeorg]

My understanding from the spec (which I'm now questioning 😄) is that, for a given script+level, we want to allow the author to open and close review requests multiple times. I think this means that we need to allow multiple code review requests for a given user, script, and level. We also probably want to enforce that there is only one open code review request for a given user, script, and level at any time. Right? (cc: @maureensturgeon)

[jamescodeorg]

I confirmed with Maureen that we want to enforce that there is at most one non-closed, non-deleted code_review_request row per user_id + script_id + level_id. Does this index look like it matches that intent?

[jamescodeorg]

I confirmed with Maureen that we want to enforce that there is at most one non-closed, non-deleted code_review_request row per user_id + script_id + level_id. Does this index look like it matches that intent?

[bethanyaconnor]

The spec says that code reviews cannot be reopened, so I assumed that we would want to enforce that there was one non-deleted code_review_request per user_id + script_id + level_id. But if the goal of this index is only to enforce that one non-deleted, non-closed row exists on these axis then this index makes sense.

[jamescodeorg]

Ohhhh. I took that to mean that we can't reopen a closed code review but we could open a new one against a given user_id, script_id, level_id. Thanks for pushing on this, let me check with Maureen.

[bethanyaconnor]

aha! that would make a lot of sense then and this index would look good