Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inaccurate info for a foreign key column referencing a non-pk column #3247

Closed
Anish9901 opened this issue Oct 17, 2023 · 8 comments · Fixed by #3260
Closed

Inaccurate info for a foreign key column referencing a non-pk column #3247

Anish9901 opened this issue Oct 17, 2023 · 8 comments · Fixed by #3260
Assignees
Labels
good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. help wanted Community contributors can implement this ready Ready for implementation type: bug Something isn't working work: frontend Related to frontend code in the mathesar_ui directory
Milestone

Comments

@Anish9901
Copy link
Member

Description

We've been assuming a foreign key column to be referencing only a primary key column(which is not always true, as a foreign key can also reference a non-pk column), there is an info alert in the data type section of a foreign key column that reflects the same.

Screenshot from 2023-10-17 15-40-56

context: tab2_refferer column refers a unique text column but the info in the data type section is inaccurately assumes it to be referring to a pk column.

Expected behavior

Show accurate info.

To Reproduce

Setup a column with a fk referencing a column which is not a pk, similar to #3232

@Anish9901 Anish9901 added type: bug Something isn't working good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. work: frontend Related to frontend code in the mathesar_ui directory needs: unblocking Blocked by other work ready Ready for implementation and removed needs: unblocking Blocked by other work labels Oct 17, 2023
@kgodey kgodey added the help wanted Community contributors can implement this label Oct 17, 2023
@kgodey
Copy link
Contributor

kgodey commented Oct 17, 2023

Note that @seancolsen @mathemancer or I should review the new text before this work gets merged.

@FidalMathew
Copy link
Contributor

@Anish9901 @kgodey Can I work on this issue?

@Anish9901
Copy link
Member Author

Go ahead @FidalMathew

@seancolsen
Copy link
Contributor

seancolsen commented Oct 25, 2023

I'd like to propose a path towards resolving this issue.

The help text is currently:

The data type of the foreign key column is restricted to the data type of the primary key column and cannot be changed.

I suggest changing that text to the following:

The data type of this column cannot be changed because it has a foreign key constraint requiring that its type match the type of the column it references.

@Anish9901 does that sound okay to you?

I'm making this suggestion here because I was reviewing #3257 but I don't like the approach in that PR. I don't think we need any changes in functionality to resolve the underlying problem described in this issue. I think we only need a minor adjustment to the language used in the UI.

If we can reach an agreement on my proposal, then I will close #3257.

@Anish9901
Copy link
Member Author

I agree @seancolsen, I don't think we need a functionality change either.

I've come up with a couple more suggestions for the help text that you might like:

  1. The data type of this foreign key column cannot be changed as it must match the data type of the column it references.
  2. The data type of the foreign key column is restricted to data type of the underlying referenced column, and hence cannot be changed.

@kgodey
Copy link
Contributor

kgodey commented Oct 25, 2023

I'll throw in a suggestion:

The data type of this column must match the referenced column and cannot be changed.

(I was aiming for short but precise.)

@seancolsen
Copy link
Contributor

I don't feel strongly about this. Happy to go with Kriti's suggestion because it's succinct.

@seancolsen
Copy link
Contributor

seancolsen commented Oct 25, 2023

Ok we have an agreement. The UI text should be changed to the following

The data type of this column must match the referenced column and cannot be changed.

@FidalMathew @ejenk0 if either of you would like to submit a PR for that change, go ahead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. help wanted Community contributors can implement this ready Ready for implementation type: bug Something isn't working work: frontend Related to frontend code in the mathesar_ui directory
Projects
No open projects
5 participants