Create KnowledgeEquityResponse Model and Migration

[tarrow]

Bug: T419209

[outdooracorn]

This is looking good. I've left suggestions for two potential improvements.

[outdooracorn]

Should we also add cascadeOnDelete() so that the row in knowledge_equity_responses is automatically deleted if the wiki is deleted? I think the default is restrictOnDelete.

Suggested change

	$table->foreign('wiki_id')->references('id')->on('wikis');
	$table->foreign('wiki_id')->references('id')->on('wikis')->cascadeOnDelete();

[tarrow]

I looked at https://laravel.com/docs/10.x/migrations#foreign-key-constraints to try and figure out what we should do.

In our current setup for most of our tables there is no explicit constrained() and we only once added cascadeOnDelete() to a migration (specifically the tou_acceptances table).

From my understanding the difference here is probably that when we manually drop a user we might really also want to remove any ToU records although I'm not certain about how much we thought about it at the time.

In this case though I would imagine that when we (soft) delete a Wiki we actually will want to keep these records. That would allow us to be able to answer important questions in the future like: "How many Wiki's that were deleted were thought by their Manager to contribute to Knowledge Equity?"

I did attempt to test this and discovered that soft deletion seems to be fine with or without cascadeOnDelete(). However, hard (forceDelete()) on a Wiki with a KnowledgeEquityResponse does indeed fail. Adding cascadeOnDelete() does allow the Wiki to be permanently removed. I think you are right that the default must be restrictOnDelete() although I didn't see this in the docs.

I'm still unclear what impact an explicit constrained() has. I saw similar behaviour with and without it.

For consistency with our other models I think I'm leaning towards not setting cascadeOnDelete() and instead seeing attempts to Hard Delete Wikis that have other models referencing them as a violation of the data we want to keep in order to understand Wiki deletions.

My current thinking is that all references to Wikis as foreign keys should be cascadeOnDelete() (and perhaps cascadeOnUpdate()) or none of them should be.

What do you think?

[outdooracorn]

In our current setup for most of our tables there is no explicit constrained() and we only once added cascadeOnDelete() to a migration (specifically the tou_acceptances table).

Indeed, but I think that might be an oversight rather than a good pattern to follow.

From my understanding the difference here is probably that when we manually drop a user we might really also want to remove any ToU records although I'm not certain about how much we thought about it at the time.

Looking at the PR that introduced that migration, I suspect it wasn't an explicit decision by the team. However, I do think it was a good thing to include.

In this case though I would imagine that when we (soft) delete a Wiki we actually will want to keep these records. That would allow us to be able to answer important questions in the future like: "How many Wiki's that were deleted were thought by their Manager to contribute to Knowledge Equity?"

I did attempt to test this and discovered that soft deletion seems to be fine with or without cascadeOnDelete(). However, hard (forceDelete()) on a Wiki with a KnowledgeEquityResponse does indeed fail. Adding cascadeOnDelete() does allow the Wiki to be permanently removed. I think you are right that the default must be restrictOnDelete() although I didn't see this in the docs.

Soft deleting doesn't matter as it doesn't actually remove anything from the database, and these are database constraints.

My feeling is that we make a decision on what do we want to keep around when we delete a wiki when we start work on that topic. Until then, I think we should do the "sensible" thing (whatever that might be).

Given that we currently only do soft deletes, it probably doesn't matter too much.

I'm still unclear what impact an explicit constrained() has. I saw similar behaviour with and without it.

Looking at the source code, I believe constrained() is just an alternative syntax for ->references(...)->on(...).

For consistency with our other models I think I'm leaning towards not setting cascadeOnDelete() and instead seeing attempts to Hard Delete Wikis that have other models referencing them as a violation of the data we want to keep in order to understand Wiki deletions.

I do like to be consistent, but I also don't want us to perpetuate a previous decision, in the name of consistency, if we now decide there is a better implementation. However, given that we currently only do soft deletes, and if we want to keep information around for #metrics we will also keep around the entry in the wikis table so that we know the deletion reason, restricting rather than cascading is probably okay.

My current thinking is that all references to Wikis as foreign keys should be cascadeOnDelete() (and perhaps cascadeOnUpdate()) or none of them should be.

I would have to look at all the references to Wikis as foreign keys to make a call on that and that seems like a rabbit whole. I wonder if it would be useful for us to decide and document what we do by default for models / migrations and why.

What do you think?

TL;DR: I think I'm okay with omitting ->cascadeOnDelete() for now and leaving the default of restricted. Although, I do half wonder if we should be explicit rather than rely on the default.

[outdooracorn]

Should the wiki_id column also have a unique() constraint so that multiple knowledge entity responses can't accidentally be added?

Suggested change

	$table->unsignedInteger('wiki_id');
	$table->foreign('wiki_id')->references('id')->on('wikis');
	$table->unsignedInteger('wiki_id')->unique();
	$table->foreign('wiki_id')->references('id')->on('wikis');

[tarrow]

ah, that's a great question. I was assuming we'd explicitly want to collect multiple responses over time and was expecting us to eventually have many. What do you think about that?

[outdooracorn]

Interesting! I have mixed feelings about this. Some half-thought-through thoughts:

• whether or not there are multiple responses might impact how other parts of the code are implemented (e.g. do I need to do a sort on date created/updated to make sure I'm receiving the most recent response?)
• multiple responses is not in the current product requirements
• we should be strict now and open up the constraint if and when we want that feature in the future
• we currently have the created_at and updated_at columns (but I think that is best practice to include on all tables regardless?)
• is this really a product question?