Add a special page that lists spreadsheet columns #35

YaxelPerez · 2020-07-27T12:36:19Z

In reference to issue #23. Adds a special page in TorqueDataConnect and a corresponding API endpoint in torquedata.

YaxelPerez · 2020-07-27T12:40:22Z

torquedata/torquedata/__init__.py

+class Spreadsheet(UserDict):
+    @classmethod
+    def read(cls, name):
+        """Load an existing spreadsheet"""
+        raise NotImplementedError
+
+    @classmethod
+    def write(cls):
+        """Save spreadsheet to filesystem"""
+        raise NotImplementedError
+
+    def __init__(self, name, object_name, key_column, columns=[]):
+        self.name = name
+        self.object_name = object_name
+        self.key_column = key_column
+        self.columns = columns
+
+        super().__init__()
+
+    def apply_permissions(self, permissions):
+        """
+        Return a new spreadsheet with only data that is allowed by a given
+        permissions object
+        """
+        new_data = {}
+        # cull invalid rows, columns
+        for key, entry in self.data.items():
+            if 'valid_ids' in permissions and key not in permissions['valid_ids']:
+                continue
+            if 'columns' in permissions:
+                new_entry = {}
+                for column, value in entry.items():
+                    if column not in permissions['columns']:
+                        continue
+                    new_entry[column] = value
+                new_data[key] = new_entry
+            else:
+                new_data[key] = entry
+
+        new_sheet = Spreadsheet(
+            name=self.name,
+            object_name=self.object_name,
+            key_column=self.key_column,
+            columns=permissions.get('columns', None) or self.columns
+        )
+        new_sheet.data = new_data
+        return new_sheet
+
+    def json(self):
+        return json.dumps({self.name: self.data.values()})
+


This is out of the scope of this issue, but I started adding an interface for spreadsheets that I think is nicer to work with. All the other code works with or without it except for the new API endpoint. Is this something I should keep pursuing?

Well, there are two separate questions here, I think:

Is it okay to introduce this new class and use it in the new code that is introduced in this commit?

Do we want to go back and modify existing code to take advantage of this new class? (If so, doing that would be a separate change, out of scope for this PR and this issue.)

I'm inclined to say "yes" on (1) and "maybe" on (2), but I'd like to know what @frankduncan thinks.

kfogel · 2020-07-27T15:32:16Z

TorqueDataConnect/include/specials/TorqueDataConnectColumns.php

+    curl_setopt(
+      $ch,
+      CURLOPT_URL,
+      'http:/localhost:5000/api/sheets' .


This isn't necessarily a comment about this commit, it's just a more general question, and it's as much for @frankduncan as it is for @YaxelPerez:

Is the "localhost:5000" convention for TorqueDataConnect something we've documented, and there's no reason for us to parameterize it? If we have "http://localhost:5000/..." all over the code, that feels a bit weird to me, but maybe it's not weird as long as we've documented that that's always the address? Still, I'd kind of prefer to see a variable like $TorqueDataConnectPoint or whatever -- if nothing else, it would prevent other reviewers from having the same question pop up for them.

Nope! It is the base address on which flask boots up, and never got around to parameterizing it. Now is as good a time as any.

kfogel · 2020-07-27T15:34:41Z

torquedata/torquedata/__init__.py

+class Spreadsheet(UserDict):
+    @classmethod
+    def read(cls, name):
+        """Load an existing spreadsheet"""
+        raise NotImplementedError
+
+    @classmethod
+    def write(cls):
+        """Save spreadsheet to filesystem"""
+        raise NotImplementedError
+
+    def __init__(self, name, object_name, key_column, columns=[]):
+        self.name = name
+        self.object_name = object_name
+        self.key_column = key_column
+        self.columns = columns
+
+        super().__init__()
+
+    def apply_permissions(self, permissions):
+        """
+        Return a new spreadsheet with only data that is allowed by a given
+        permissions object
+        """
+        new_data = {}
+        # cull invalid rows, columns
+        for key, entry in self.data.items():
+            if 'valid_ids' in permissions and key not in permissions['valid_ids']:
+                continue
+            if 'columns' in permissions:
+                new_entry = {}
+                for column, value in entry.items():
+                    if column not in permissions['columns']:
+                        continue
+                    new_entry[column] = value
+                new_data[key] = new_entry
+            else:
+                new_data[key] = entry
+
+        new_sheet = Spreadsheet(
+            name=self.name,
+            object_name=self.object_name,
+            key_column=self.key_column,
+            columns=permissions.get('columns', None) or self.columns
+        )
+        new_sheet.data = new_data
+        return new_sheet
+
+    def json(self):
+        return json.dumps({self.name: self.data.values()})
+


Well, there are two separate questions here, I think:

Is it okay to introduce this new class and use it in the new code that is introduced in this commit?

Do we want to go back and modify existing code to take advantage of this new class? (If so, doing that would be a separate change, out of scope for this PR and this issue.)

I'm inclined to say "yes" on (1) and "maybe" on (2), but I'd like to know what @frankduncan thinks.

kfogel · 2020-07-27T17:14:25Z

See the related Zulip discussion too.

kfogel · 2020-07-28T21:24:49Z

torquedata/torquedata/routes.py

    group = request.args.get("group")
    wiki_key = request.args.get("wiki_key")

    response = []
    for sheet_name, sheet in data.items():
-        sheet = sheet.apply_permissions(permissions[sheet_name][wiki_key][group])


We now no longer call sheet.apply_permissions(), so I would expect to see the permissions check happening manually somewhere in the new code introduced in this commit... but I don't see that anywhere?

frankduncan

Thanks for the change! It's a great start. Some changes needed for the first iteration:

I don't think we need to pass in group as part of the api request
We should use $wgTorqueDataConnectSheetName to return only the columns that this wiki is interested in (this is the variable used to tell what to save when looking at MainConfig). When we upgrade MainConfig to be able to configure multiple sheets, we'll have to upgrade this call as well, but for now they should be lock step.
We need to restrict access to this page to only people in the torquedataconnect-admin group

YaxelPerez · 2020-08-04T17:54:37Z

New API endpoint was unnecessary, given @frankduncan's comment. Removed in latest commits.

frankduncan

Thanks for the update!

For the general case, can you squash all the commits into one? Because things were added and then removed as we nailed down the true scope of this, the commit history is a little bit messier than the very clean single view of the whole change. So having the commit log message and the commit history be simpler would be great!

There's another change in line from a design point of view.

frankduncan · 2020-08-11T12:43:54Z

TorqueDataConnect/include/specials/TorqueDataConnectColumns.php

+      CURLOPT_URL,
+      'http:/localhost:5000/api/' .
+      $wgTorqueDataConnectSheetName . '.json' .
+      '?group=' . $wgTorqueDataConnectGroup .


Because this is only going to return the columns for which this group is configured to have since you're passing in the $wgTorqueDataConfigGroup, it's effectively the same list as the configuration for this group. This issue is about giving admins a list of all columns, regardless of what is currently set up for their group in the MainConfig. The main thing we want to solve here is when columns get added to the spreadsheet, we want a place for admins to see them.

Or, even better, a way for them to set up the MainConfig to point to this special page so they just always have all columns (this is a good use case for testing this).

In reference to issue #23. This commit adds a special page in TorqueDataConnect and a corresponding API endpoint in torquedata. Issue #23: Add a Special Page that lists available spreadsheet columns

YaxelPerez commented Jul 27, 2020

View reviewed changes

YaxelPerez requested a review from frankduncan July 27, 2020 12:41

kfogel reviewed Jul 27, 2020

View reviewed changes

kfogel reviewed Jul 28, 2020

View reviewed changes

frankduncan requested changes Jul 31, 2020

View reviewed changes

YaxelPerez force-pushed the 23-spreadsheet-column-special-page branch from e595788 to 5d184fb Compare August 4, 2020 16:36

frankduncan requested changes Aug 11, 2020

View reviewed changes

frankduncan changed the base branch from master to main September 23, 2020 14:28

frankduncan force-pushed the 23-spreadsheet-column-special-page branch from 9e2a491 to 87d902e Compare June 3, 2021 15:21

frankduncan approved these changes Jun 3, 2021

View reviewed changes

Fix #23: Add a special page that lists spreadsheet columns

1f14532

In reference to issue #23. This commit adds a special page in TorqueDataConnect and a corresponding API endpoint in torquedata. Issue #23: Add a Special Page that lists available spreadsheet columns

frankduncan force-pushed the 23-spreadsheet-column-special-page branch from 87d902e to 1f14532 Compare June 10, 2021 18:23

frankduncan merged commit a1e2427 into main Jun 10, 2021

frankduncan deleted the 23-spreadsheet-column-special-page branch June 10, 2021 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a special page that lists spreadsheet columns #35

Add a special page that lists spreadsheet columns #35

YaxelPerez commented Jul 27, 2020

YaxelPerez Jul 27, 2020

kfogel Jul 27, 2020

kfogel Jul 27, 2020

frankduncan Jul 27, 2020

kfogel Jul 27, 2020

kfogel commented Jul 27, 2020

kfogel Jul 28, 2020

frankduncan left a comment

YaxelPerez commented Aug 4, 2020

frankduncan left a comment

frankduncan Aug 11, 2020

Add a special page that lists spreadsheet columns #35

Add a special page that lists spreadsheet columns #35

Conversation

YaxelPerez commented Jul 27, 2020

YaxelPerez Jul 27, 2020

Choose a reason for hiding this comment

kfogel Jul 27, 2020

Choose a reason for hiding this comment

kfogel Jul 27, 2020

Choose a reason for hiding this comment

frankduncan Jul 27, 2020

Choose a reason for hiding this comment

kfogel Jul 27, 2020

Choose a reason for hiding this comment

kfogel commented Jul 27, 2020

kfogel Jul 28, 2020

Choose a reason for hiding this comment

frankduncan left a comment

Choose a reason for hiding this comment

YaxelPerez commented Aug 4, 2020

frankduncan left a comment

Choose a reason for hiding this comment

frankduncan Aug 11, 2020

Choose a reason for hiding this comment