Foreign Key Usage Design Spec #12

ghislaineguerin · 2021-11-11T21:34:41Z

This PR contains the specs needed to implement the design for Usage of Foreign Key Contraints

…c-foreign-key-usage # Conflicts: # design/specs/foreign-key-constraints.md

kgodey

I think this looks good overall. Added some comments with further thoughts.

design/specs/foreign-key-constraints.md

dmos62 · 2021-11-16T14:06:14Z

Nice work! I've a few quick notes. These may have been addressed already.

Additional options and defaults

Postgres provides some options for FKs, like what to do when a referenced column is deleted or updated, or whether to even allow that (ON UPDATE, ON DELETE). Also, there's an option for whether or not a multi-column FK can have referencing cells that are undefined (MATCH FULL, MATCH SIMPLE).

For the sake of development speed we might just want to use a sensible default at the moment. If so, it might be nice to tell the user what that default is. I think defaults were discussed at some point and we arrived at ON UPDATE CASCADE, ON DELETE RESTRICT, and MATCH SIMPLE.

Terminology

SQL community's terms around FK are pretty confusing and not well established (I've seen referencing, referred, source, target, columns, etc.). When you're looking at a different SQL library or a different documentation, chances are that it will be calling these properties something new.

Concern here is that when the user will be looking at the create/edit FK constraint dialog it might be taxing for him to understand what do the terms referenced table, reference column, column (no prefix) mean. I'd say there isn't an easy solution for this: we either make it verbose or short but confusing. I'm leaning towards verbosity: referred table/column and referring table/column. Referred/referring being popular abbreviations for referenced/referencing.

Also, in the context of FKs, saying column or table without specifying whether we're talking about the referred or referring column/table is somewhat ambiguous. There's a colloquial convention that when you're ambiguous you mean the referring object, but it might take a user some extra time to realise that. Something to think about.

pavish

@ghislaineguerin This looks really good and detailed. I had a couple comments.

design/specs/foreign-key-constraints.md

kgodey · 2021-12-03T21:43:53Z

@ghislaineguerin I updated the status of this PR to review and added everyone to the assignee list.

@dmos62 @mathemancer @pavish @seancolsen @silentninja Please unassign yourselves once you've reviewed.

seancolsen

Aside from one clarifying question, this looks great!

seancolsen · 2021-12-03T22:04:25Z

design/specs/foreign-key-constraints.md

+  - If the table contains no records, the user sees an option to add a record.
+  - If there aren't any matches, the user sees an option to add a record for the newly entered value or try changing the 'Search Columns' table preference.


What happens after the user clicks "Add Record" here?

@seancolsen we haven't defined that part of the flow yet. I assume we'll know more when we work on views as it also involves the creation of new records in other tables. I'll add a note for this.

I think we should remove the "Add Record" option from the spec, we can add it in once we have it defined. It'll confuse implementation.

mathemancer

Looks great overall.

Small comment: I noticed in the video for Scenario 2 that we "remove" the foreign key constraint from some parts of the UI, but we "delete" the foreign key constraint from others. I think these should be unified to one specific word/concept to avoid confusion, and that word/concept should be chosen carefully. We want to make sure the User understands they're not deleting any data when they take the action, but only the attachment between the tables.

seancolsen · 2021-12-08T15:35:38Z

@mathemancer @ghislaineguerin I added a discussion about "delete" vs "remove"

kgodey

@ghislaineguerin This looks great overall. Some (hopefully) minor comments.

kgodey · 2021-12-08T19:42:53Z

design/specs/foreign-key-constraints.md

-#### 2. The user selects a table from the list of available tables
- The user selects the table on the 'one side of the 'one-to-many' relationship.
+- The user is not familiar with the concept of foreign keys.
+- The user has created tables within Mathesar, where the ID column is automatically generated and sent as the primary key.


I don't think this is a good assumption to make. We should handle cases for all tables, even without primary keys.

We can solve this for the first version by not supporting creating links through scenario 1a. Perhaps we could have a little help message that says "Why don't I see all my tables in this list?" and explain that we only show tables with primary keys.

I think it would be better if we show all the tables, but disable the table without a primary key with a text hint that conveys the message that the primary key is missing, this could be extended to directing them to create a primary key

I agree with @silentninja. I think it would be best to show the tables and prevent selection. I will add details for this in the spec.

kgodey · 2021-12-08T19:44:04Z

design/specs/foreign-key-constraints.md

+  - If the table contains no records, the user sees an option to add a record.
+  - If there aren't any matches, the user sees an option to add a record for the newly entered value or try changing the 'Search Columns' table preference.


I think we should remove the "Add Record" option from the spec, we can add it in once we have it defined. It'll confuse implementation.

kgodey · 2021-12-08T19:44:59Z

design/specs/foreign-key-constraints.md

+
+### Linked Records for Multi-Column Foreign Key Constraints
+
+- The current UI does not consider multi-column foreign key constraints and how those would be retrieved and selected.


I think we should define this a bit, even if only to say that we treat it like a text column for now.

@kgodey I've added additional information.

kgodey · 2021-12-08T19:45:16Z

design/specs/foreign-key-constraints.md

+
+### Usage of Color
+
+- A suggestion to use color as a means to differentiate table references has been included in this spec. However, there are implementation details that need to be discussed before the team can make a decision.


Decision about what?

@kgodey I've updated to add more detail.

kgodey · 2021-12-08T19:45:48Z

design/specs/foreign-key-constraints.md

+- The user opens the column menu for a column that has a foreign key constraint applied
+- The user clicks on the preferences link
+
+## Components


What about the constraints modal component?

kgodey · 2021-12-09T21:05:50Z

@ghislaineguerin Please re-assign me when the spec is ready for re-review.

kgodey

@ghislaineguerin Looks good overall, a couple of minor changes requested.

kgodey · 2021-12-16T13:37:19Z

design/specs/foreign-key-constraints.md

+
+### Terminology
+
+- `Remove` and `Delete` are used throughout this spec and should be applied consistently across the entire user interface as they are different affordances. `Remove` will be used for data that can be added back, while `Delete` will indicate data destruction.


We actually decided to use Delete for everything in mathesar-foundation/mathesar#872

kgodey · 2021-12-16T13:40:40Z

design/specs/foreign-key-constraints.md

-#### 2. The user selects a table from the list of available tables
- The user selects the table on the 'one side of the 'one-to-many' relationship.
+- The user is not familiar with the concept of foreign keys.
+- The user has at least two tables with primary key constraints set.


I think we just need one table - the one that's being linked to. And it should be a single primary key and not a composite primary key.

I agree. I've updated the assumptions.

kgodey

@ghislaineguerin I noticed a typo, made a suggested fix along with a clarification.

design/specs/foreign-key-constraints.md

kgodey

Looks good other than my one suggested change.

Co-authored-by: Kriti Godey <kriti@kritigodey.com>

ghislaineguerin added 8 commits November 11, 2021 22:30

add first spec draft

5aaf0de

update text

83014fa

Merge branch 'master' of github.com:centerofci/mathesar-wiki into spe…

64287f9

…c-foreign-key-usage # Conflicts: # design/specs/foreign-key-constraints.md

add foreign key edit and preview sections

6a548ca

update sections

dd32018

add scenario descriptions

485948a

add related discussions and update list of components

738ac4e

add video links

62582ee

ghislaineguerin marked this pull request as ready for review November 14, 2021 09:20

ghislaineguerin requested a review from a team as a code owner November 14, 2021 09:20

github-actions bot requested review from eito-fis, kgodey, mathemancer, pavish, seancolsen and silentninja and removed request for a team November 14, 2021 09:20

ghislaineguerin assigned seancolsen, kgodey, silentninja, mathemancer and pavish Nov 14, 2021

kgodey requested a review from dmos62 November 15, 2021 22:57

kgodey reviewed Nov 15, 2021

View reviewed changes

design/specs/foreign-key-constraints.md Show resolved Hide resolved

design/specs/foreign-key-constraints.md Outdated Show resolved Hide resolved

seancolsen reviewed Nov 16, 2021

View reviewed changes

seancolsen removed their assignment Nov 16, 2021

pavish reviewed Nov 16, 2021

View reviewed changes

kgodey removed their assignment Nov 16, 2021

kgodey added the status: review In review label Nov 16, 2021

seancolsen approved these changes Dec 3, 2021

View reviewed changes

seancolsen removed their assignment Dec 3, 2021

mathemancer reviewed Dec 8, 2021

View reviewed changes

mathemancer removed their assignment Dec 8, 2021

kgodey requested changes Dec 8, 2021

View reviewed changes

kgodey removed their assignment Dec 8, 2021

silentninja removed their assignment Dec 9, 2021

ghislaineguerin added 2 commits December 9, 2021 13:58

clarify details

65e5385

add table selection details

4c26ec8

add additional considerations

c8c5e2e

ghislaineguerin requested a review from kgodey December 13, 2021 20:32

ghislaineguerin assigned kgodey Dec 13, 2021

ghislaineguerin added 2 commits December 15, 2021 18:24

remove table link preferences

207fa44

update dropdown description

f91aeb5

kgodey requested changes Dec 16, 2021

View reviewed changes

kgodey removed their assignment Dec 16, 2021

update terminology and assumptions

65be4a8

ghislaineguerin assigned kgodey Dec 17, 2021

kgodey reviewed Dec 17, 2021

View reviewed changes

design/specs/foreign-key-constraints.md Outdated Show resolved Hide resolved

kgodey approved these changes Dec 17, 2021

View reviewed changes

kgodey removed their assignment Dec 17, 2021

Update design/specs/foreign-key-constraints.md

51e9233

Co-authored-by: Kriti Godey <kriti@kritigodey.com>

pavish removed their assignment Dec 17, 2021

dmos62 removed their request for review December 21, 2021 16:21

ghislaineguerin unassigned dmos62 Dec 21, 2021

ghislaineguerin merged commit 6652f36 into master Dec 21, 2021

ghislaineguerin deleted the spec-foreign-key-usage branch December 21, 2021 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Foreign Key Usage Design Spec #12

Foreign Key Usage Design Spec #12

ghislaineguerin commented Nov 11, 2021 •

edited

Loading

kgodey left a comment

dmos62 commented Nov 16, 2021

pavish left a comment

kgodey commented Dec 3, 2021 •

edited

Loading

seancolsen left a comment

seancolsen Dec 3, 2021

ghislaineguerin Dec 7, 2021

kgodey Dec 8, 2021 •

edited

Loading

mathemancer left a comment

seancolsen commented Dec 8, 2021

kgodey left a comment

kgodey Dec 8, 2021

silentninja Dec 9, 2021 •

edited

Loading

ghislaineguerin Dec 9, 2021

kgodey Dec 8, 2021 •

edited

Loading

kgodey Dec 8, 2021

ghislaineguerin Dec 9, 2021

kgodey Dec 8, 2021

ghislaineguerin Dec 9, 2021

kgodey Dec 8, 2021

kgodey commented Dec 9, 2021

kgodey left a comment

kgodey Dec 16, 2021

kgodey Dec 16, 2021

ghislaineguerin Dec 17, 2021

kgodey left a comment •

edited

Loading

kgodey left a comment

		- If the table contains no records, the user sees an option to add a record.
		- If there aren't any matches, the user sees an option to add a record for the newly entered value or try changing the 'Search Columns' table preference.


		### Linked Records for Multi-Column Foreign Key Constraints

		- The current UI does not consider multi-column foreign key constraints and how those would be retrieved and selected.


		### Usage of Color

		- A suggestion to use color as a means to differentiate table references has been included in this spec. However, there are implementation details that need to be discussed before the team can make a decision.


		### Terminology

		- `Remove` and `Delete` are used throughout this spec and should be applied consistently across the entire user interface as they are different affordances. `Remove` will be used for data that can be added back, while `Delete` will indicate data destruction.

Foreign Key Usage Design Spec #12

Foreign Key Usage Design Spec #12

Conversation

ghislaineguerin commented Nov 11, 2021 • edited Loading

kgodey left a comment

Choose a reason for hiding this comment

dmos62 commented Nov 16, 2021

Additional options and defaults

Terminology

pavish left a comment

Choose a reason for hiding this comment

kgodey commented Dec 3, 2021 • edited Loading

seancolsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgodey Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

mathemancer left a comment

Choose a reason for hiding this comment

seancolsen commented Dec 8, 2021

kgodey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silentninja Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgodey Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgodey commented Dec 9, 2021

kgodey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgodey left a comment • edited Loading

Choose a reason for hiding this comment

kgodey left a comment

Choose a reason for hiding this comment

ghislaineguerin commented Nov 11, 2021 •

edited

Loading

kgodey commented Dec 3, 2021 •

edited

Loading

kgodey Dec 8, 2021 •

edited

Loading

silentninja Dec 9, 2021 •

edited

Loading

kgodey Dec 8, 2021 •

edited

Loading

kgodey left a comment •

edited

Loading