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

Conditionally determine visibility of columns in datatables #2044

Merged
merged 3 commits into from
Sep 28, 2021

Conversation

HarmvZ
Copy link
Member

@HarmvZ HarmvZ commented Sep 27, 2021

This PR allows us to conditionally show / hide columns in a datatable.

It works by defining an optional_condition callable on a Column definition for datatables. The optional_condition will be called with the object for that row as parameter for each visible row on the page. If non of the calls evaluate to True, the column will be hidden. The visibility is recalculated each time the table data changes: pagination changes, search changes, ordering changes, etc.

This will be useful when the fields in DIAGNijmegen/rse-panimg#51 are implemented here and we want to show these columns in the archive cases list view, but hide the columns that contain empty values.

As an example I made the "Algorithm Results" column in the cases list view only visible if it actually contains algorithm results.

@HarmvZ HarmvZ requested a review from jmsmkn September 27, 2021 13:14
@jmsmkn
Copy link
Member

jmsmkn commented Sep 27, 2021

At a quick glance, I think the way this is implemented has an N+1 problem with 2 extra calls per object. Have you checked by using the debug toolbar, including the history tab to monitor the AJAX calls?

@HarmvZ
Copy link
Member Author

HarmvZ commented Sep 27, 2021

By the optional_condition for algorithm results? The data for that is prefetched in get_queryset so it should not need extra calls to get it. The toolbar indicates 14 queries total independent of the page length / nr of objects.

@codecov
Copy link

codecov bot commented Sep 27, 2021

Codecov Report

Merging #2044 (6dc7463) into master (51b99c8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2044   +/-   ##
=======================================
  Coverage   93.42%   93.43%           
=======================================
  Files         692      692           
  Lines       26452    26456    +4     
=======================================
+ Hits        24713    24718    +5     
+ Misses       1739     1738    -1     
Impacted Files Coverage Δ
app/grandchallenge/archives/views.py 88.67% <ø> (ø)
app/tests/core_tests/test_views.py 100.00% <ø> (ø)
app/grandchallenge/datatables/views.py 100.00% <100.00%> (ø)
app/grandchallenge/components/models.py 93.08% <0.00%> (+0.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51b99c8...6dc7463. Read the comment docs.

@jmsmkn
Copy link
Member

jmsmkn commented Sep 28, 2021

Great! Then my mistake :-)

@jmsmkn jmsmkn merged commit c292604 into master Sep 28, 2021
@jmsmkn jmsmkn deleted the optional_datatable_columns branch September 28, 2021 08:04
jmsmkn added a commit that referenced this pull request Sep 4, 2024
Due to strange behaviour across pages. Solving this for multiple
pages would involve lookups across all items.

See #2044
Closes #3523
jmsmkn added a commit that referenced this pull request Sep 4, 2024
Due to strange behaviour across pages. Solving this for multiple pages
would involve lookups across all items.

See #2044
Closes #3523
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.

2 participants