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

After adding a foreign key constraint the record summary for linked cells doesn't display correctly #4237

Open
zackkrida opened this issue Feb 5, 2025 · 9 comments · May be fixed by #4267
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 work: frontend Related to frontend code in the mathesar_ui directory
Milestone

Comments

@zackkrida
Copy link
Contributor

zackkrida commented Feb 5, 2025

After adding a foreign key constraint to an existing column which references another table, the column with the constraint has the styling of a 'linked record' applied but still only shows the ID of the record rather than the linked table's record summary.

I think this is most simply explained via this quick video:

Screencast.From.2025-02-06.10-05-56.mp4
@zackkrida zackkrida added needs: triage This issue has not yet been reviewed by a maintainer type: bug labels Feb 5, 2025
@zackkrida
Copy link
Contributor Author

@seancolsen I've updated this issue and thought I'd tell you since you expressed curiosity.

@seancolsen seancolsen added good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. help wanted Community contributors can implement this work: frontend Related to frontend code in the mathesar_ui directory ready Ready for implementation and removed needs: triage This issue has not yet been reviewed by a maintainer labels Feb 6, 2025
@seancolsen seancolsen added this to the Backlog milestone Feb 6, 2025
@seancolsen
Copy link
Contributor

seancolsen commented Feb 6, 2025

Thanks @zackkrida. Good find! I was able to reproduce this too.

I'll add some repro steps based on our sample data:

  1. Use our library sample data.
  2. Open the Items table.
  3. Observe record summaries displaying for the Book column.
  4. Navigate to the Inspector > Table > Advanced > Constraints > Foreign Keys
  5. Delete the foreign key constraint on the Book column
  6. Click the Refresh button at the bottom right.
  7. Open the constraints modal again and re-add the constraint.
  8. Expect to see the same record summaries as in step 3.
  9. Instead, observe record summaries to display as numbers.
  10. Click Refresh again.
  11. Expect and observe the correct record summaries to display again.

Note

EDIT: The steps above are now out-of-date. See updated steps below

Here's why this is happening:

  1. That refresh button at the bottom right loads columns, constraints, and records data.
  2. When the records data is loaded, PostgreSQL sends record summary data for foreign key columns. So if a column doesn't have a FK when pg generates the records data, the the front end won't get the record summaries.
  3. Within that constraints dialog
    • When you add a new FK constraint, the form system in the dialog has a mechanism to call back up to the tabularData store and refresh the constraints. This refreshing is clearly happening because the icon for the Book column changes to a link, and the rendering of cells changes to pills. That's good.
    • What also should be happening (but is not) is: apparently we need to refresh the records data too!

Given that we're already refreshing the constraints, I would imagine this should be pretty easy to fix. We need to dig into the code, find the place where the constraints are being refreshed, and also refresh the records. This is probably a one or two line fix.

As such, I'm labeling this "good first issue".

@atleehlavinka
Copy link

Hey, I'd like to take a shot at this issue.

@seancolsen
Copy link
Contributor

Sure, feel free to submit a PR, @atleehlavinka

@atleehlavinka
Copy link

I wasn't able to replicate this issue. I used the Library Management sample dataset and followed the replication steps given above. Here's the video where I try to replicate the issue:

https://www.youtube.com/watch?v=LGJ0s6Y6Kbs

@atleehlavinka
Copy link

I recreated the data as shown in the video. The cause of this issue seems to be that the data is being linked via a column that is not the PK (notice that the link occurs with 'original_id' and not 'id'). When I link the 'favorite_food' column to the PK in the Food table, no refresh is required. When I link the same column to a non-PK field that is unique, I have to press the Refresh button.

@seancolsen
Copy link
Contributor

@atleehlavinka Thanks for the video. Yep, you certainly took all the steps to reproduce it! I promise I was able to reproduce it with exactly these steps before. I suspect there is some nondeterminism at play here due to JavaScript garbage collection.

Here are some modified repro steps:

  1. Use our library sample data.
  2. Open the Items table.
  3. Observe record summaries displaying for the Book column.
  4. Navigate to the Inspector > Table > Advanced > Constraints > Foreign Keys
  5. Delete the foreign key constraint on the Book column
  6. Click the Refresh button at the bottom right in your browser.
  7. Open the constraints modal again and re-add the constraint.
  8. Expect to see the same record summaries as in step 3.
  9. Instead, observe record summaries to display as numbers.
  10. Click Refresh again.
  11. Expect and observe the correct record summaries to display again.
Screencast_20250211_072755.webm

Refreshing the whole page avoids any GC issues.

Would you care to have another go at this using the steps above?

@atleehlavinka
Copy link

Thanks Sean! I can reliably reproduce this now. I'll see what I can do.

cwyoan pushed a commit to cwyoan/mathesar that referenced this issue Feb 17, 2025
@cwyoan
Copy link

cwyoan commented Feb 18, 2025

This solved the issue on my end, I followed the steps to recreate the issue and it solves it after adding that snippet.

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 work: frontend Related to frontend code in the mathesar_ui directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants