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

?_col= and ?_nocol= support for toggling columns on table view #615

Closed
simonw opened this issue Nov 4, 2019 · 16 comments
Closed

?_col= and ?_nocol= support for toggling columns on table view #615

simonw opened this issue Nov 4, 2019 · 16 comments

Comments

@simonw
Copy link
Owner

simonw commented Nov 4, 2019

Split off from #292 (I guess this is a re-opening of #312).

@simonw
Copy link
Owner Author

simonw commented Nov 4, 2019

First step: stop using select * and switch to selecting columns instead. This will also make the View and edit SQL button more useful.

@simonw
Copy link
Owner Author

simonw commented Nov 6, 2019

I'm going to always include the primary key (or rowid) - otherwise the table view will sometimes not include links to the row pages, which seems bad.

@simonw
Copy link
Owner Author

simonw commented May 23, 2021

I started looking at this again, inspired by #1326. I have a new diff that works against the latest main branch.

diff --git a/datasette/views/table.py b/datasette/views/table.py
index 4879228..f4b2ee2 100644
--- a/datasette/views/table.py
+++ b/datasette/views/table.py
@@ -64,6 +64,36 @@ class Row:
 
 
 class RowTableShared(DataView):
+    async def columns_to_select(self, db, table, request):
+        table_columns = await db.table_columns(table)
+        if "_col" in request.args and "_nocol" in request.args:
+            raise DatasetteError("Cannot use _col and _nocol at the same time")
+        if "_col" in request.args:
+            new_columns = []
+            for column in request.args.getlist("_col"):
+                if column not in table_columns:
+                    raise DatasetteError("_col={} is an invalid column".format(column))
+                new_columns.append(column)
+            return new_columns
+        elif "_nocol" in request.args:
+            # Return all columns EXCEPT these
+            bad_columns = [
+                column
+                for column in request.args.getlist("_nocol")
+                if column not in table_columns
+            ]
+            if bad_columns:
+                raise DatasetteError(
+                    "_nocol={} - invalid columns".format(", ".join(bad_columns))
+                )
+            return [
+                column
+                for column in table_columns
+                if column not in request.args.getlist("_nocol")
+            ]
+        else:
+            return table_columns
+
     async def sortable_columns_for_table(self, database, table, use_rowid):
         db = self.ds.databases[database]
         table_metadata = self.ds.table_metadata(database, table)
@@ -321,18 +351,16 @@ class TableView(RowTableShared):
         )
 
         pks = await db.primary_keys(table)
-        table_column_details = await db.table_column_details(table)
-        table_columns = [column.name for column in table_column_details]
-
-        select_columns = ", ".join(escape_sqlite(t) for t in table_columns)
+        table_columns = await self.columns_to_select(db, table, request)
+        select_clause = ", ".join(escape_sqlite(t) for t in table_columns)
 
         use_rowid = not pks and not is_view
         if use_rowid:
-            select = f"rowid, {select_columns}"
+            select = f"rowid, {select_clause}"
             order_by = "rowid"
             order_by_pks = "rowid"
         else:
-            select = select_columns
+            select = select_clause
             order_by_pks = ", ".join([escape_sqlite(pk) for pk in pks])
             order_by = order_by_pks
 
@@ -715,6 +743,8 @@ class TableView(RowTableShared):
                 column = fk["column"]
                 if column not in columns_to_expand:
                     continue
+                if column not in columns:
+                    continue
                 expanded_columns.append(column)
                 # Gather the values
                 column_index = columns.index(column)

@simonw
Copy link
Owner Author

simonw commented May 23, 2021

Still needed:

  • Unit tests
  • Documentation

And maybe a UI mechanism for this? I could at least add a "remove this column" item to the cog menu on columns.

@simonw
Copy link
Owner Author

simonw commented May 23, 2021

Here's that prototype of a "Hide this column" cog menu button:

hide

Need a way to undo that once you've hidden a column - maybe a list of currently hidden columns that lets you un-hide them.

@simonw
Copy link
Owner Author

simonw commented May 23, 2021

Natalie suggests a quick way to implement "undo" would be to add a "Show all columns" item to that menu which only appears when at least one column is hidden.

@simonw
Copy link
Owner Author

simonw commented May 23, 2021

I've changed my mind about forbidding ?_col= and ?_nocol= from being applied at the same time - I'm going to come up with a simple resolution rule instead.

@simonw
Copy link
Owner Author

simonw commented May 23, 2021

Here's a bug: removing the rowid column returns an error.

rowid-bug

@simonw
Copy link
Owner Author

simonw commented May 23, 2021

A better interface for this would be a full list of columns each with a checkbox for making it visible on invisible - this could then be used to apply a bulk change (rather than refreshing the interface after every removed column) and it could also be easily designed to work on narrow mobile screens where the cog icon is not visible.

@simonw
Copy link
Owner Author

simonw commented May 23, 2021

Interesting side-effect of this implementation is that you can both control column order and request the same column multiple times:

/fivethirtyeight/airline-safety%2Fairline-safety?_col=fatal_accidents_00_14&_col=fatalities_00_14&_col=airline&_col=airline

fivethirtyeight__airline-safety_airline-safety__56_rows

@simonw
Copy link
Owner Author

simonw commented May 23, 2021

Would it be useful to allow this mechanism to alias columns, for example supporting one of the following:

  • ?_col=airline as name_of_airline
  • ?_col=airline:name_of_airline

This could be handy for renaming columns to match a specific expected JSON output.

@simonw
Copy link
Owner Author

simonw commented May 24, 2021

I can't think of any good reasons to support passing the same column twice ?col=airline&_col=airline so I'm going to de-duplicate and silently ignore the second one.

@simonw
Copy link
Owner Author

simonw commented May 24, 2021

Here's a bug: removing the rowid column returns an error.

Removing the rowid column should work. We can continue to show the Link column, ensuring users can still navigate to the row page for each row.

@simonw
Copy link
Owner Author

simonw commented May 24, 2021

I tried allowing the removal of rowid and got this exception:

  File "/Users/simon/Dropbox/Development/datasette/datasette/views/table.py", line 831, in extra_template
    display_columns, display_rows = await self.display_columns_and_rows(
  File "/Users/simon/Dropbox/Development/datasette/datasette/views/table.py", line 163, in display_columns_and_rows
    pk_path = path_from_row_pks(row, pks, not pks, False)
  File "/Users/simon/Dropbox/Development/datasette/datasette/utils/__init__.py", line 82, in path_from_row_pks
    bits = [row["rowid"]]
IndexError: No item with that key

I'm going to disable the removal of rowid - or indeed any of the primary keys, since they are needed to construct the row permalink.

simonw added a commit that referenced this issue May 27, 2021
- Allow both ?_col= and ?_nocol=
- De-duplicate if ?_col= passed multiple times
- 400 error if user tries to ?_nocol= a primary key
@simonw simonw closed this as completed in f1c29fd May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant