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

Expand foreign key references in row view as well #2031

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tmcl-it
Copy link

@tmcl-it tmcl-it commented Mar 1, 2023

Unlike the table view, the single row view does not resolve foreign key references into labels. This patch extracts the foreign key reference expansion code from TableView.data() into a standalone function that is then called by both TableView.data() and RowView.data().


📚 Documentation preview 📚: https://datasette--2031.org.readthedocs.build/en/2031/

@simonw
Copy link
Owner

simonw commented Mar 6, 2023

This is a very neat fix, for something I've been wanting for a while.

Add a unit test for the row HTML page - I suggest against this page: https://latest.datasette.io/fixtures/foreign_key_references/1 - and I'll land this PR.

You can model it on this test here:

@pytest.mark.asyncio
async def test_table_html_foreign_key_links(ds_client):
response = await ds_client.get("/fixtures/foreign_key_references")
assert response.status_code == 200
table = Soup(response.text, "html.parser").find("table")
actual = [[str(td) for td in tr.select("td")] for tr in table.select("tbody tr")]
assert actual == [
[
'<td class="col-pk type-pk"><a href="/fixtures/foreign_key_references/1">1</a></td>',
'<td class="col-foreign_key_with_label type-str"><a href="/fixtures/simple_primary_key/1">hello</a>\xa0<em>1</em></td>',
'<td class="col-foreign_key_with_blank_label type-str"><a href="/fixtures/simple_primary_key/3">-</a>\xa0<em>3</em></td>',
'<td class="col-foreign_key_with_no_label type-str"><a href="/fixtures/primary_key_multiple_columns/1">1</a></td>',
'<td class="col-foreign_key_compound_pk1 type-str">a</td>',
'<td class="col-foreign_key_compound_pk2 type-str">b</td>',
],
[
'<td class="col-pk type-pk"><a href="/fixtures/foreign_key_references/2">2</a></td>',
'<td class="col-foreign_key_with_label type-none">\xa0</td>',
'<td class="col-foreign_key_with_blank_label type-none">\xa0</td>',
'<td class="col-foreign_key_with_no_label type-none">\xa0</td>',
'<td class="col-foreign_key_compound_pk1 type-none">\xa0</td>',
'<td class="col-foreign_key_compound_pk2 type-none">\xa0</td>',
],
]

I think adding it to test_table_html.py is OK, even though it's technically for the row page and not the table page.

Thanks!

@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Patch coverage: 97.72% and project coverage change: -0.36 ⚠️

Comparison is base (3feed1f) 92.46% compared to head (c8a2904) 92.11%.

❗ Current head c8a2904 differs from pull request most recent head ef25867. Consider uploading reports for the commit ef25867 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2031      +/-   ##
==========================================
- Coverage   92.46%   92.11%   -0.36%     
==========================================
  Files          38       38              
  Lines        5750     5556     -194     
==========================================
- Hits         5317     5118     -199     
- Misses        433      438       +5     
Impacted Files Coverage Δ
datasette/views/table.py 92.57% <97.56%> (-3.35%) ⬇️
datasette/views/base.py 95.17% <100.00%> (+2.38%) ⬆️
datasette/views/row.py 87.93% <100.00%> (+0.10%) ⬆️

... and 8 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tmcl-it tmcl-it force-pushed the patch-row-view-expand-labels branch 3 times, most recently from d39fa8b to 98c7206 Compare March 6, 2023 23:31
@tmcl-it
Copy link
Author

tmcl-it commented Mar 7, 2023

I've implemented the test (thanks for pointing me in the right direction!).

At tmcl-it/datasette:0.64.1+row-view-expand-labels I also have a variant of this patch that applies to the 0.64.x branch. Please let me know if you'd be interested in merging that as well and I'll open another PR.

@simonw
Copy link
Owner

simonw commented Mar 9, 2023

I've implemented the test (thanks for pointing me in the right direction!).

At tmcl-it/datasette:0.64.1+row-view-expand-labels I also have a variant of this patch that applies to the 0.64.x branch. Please let me know if you'd be interested in merging that as well and I'll open another PR.

Sure, let's merge that one too - it can go out in the next 0.64.x series release (maybe even a 0.65).

@tmcl-it tmcl-it force-pushed the patch-row-view-expand-labels branch from 98c7206 to ef25867 Compare March 24, 2023 18:28
@tmcl-it
Copy link
Author

tmcl-it commented Mar 24, 2023

I've rebased my patch on the latest main. It should be ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants