feat: adding new "team_settings" table to Hasura #3289
Conversation
|
🤖 Hasura Change Summary compared a subset of table metadata including permissions: Tracked Tables (1)
|
|
Removed vultr server and associated DNS entries |
There was a problem hiding this comment.
A few comments to take a look at 👍
It's a good decision not to remove the columns from the team table here - we can come back and tidy this up once the feature is complete.
I think there's a bit of a chicken-egg situation with the sync scripts which is causing CI to fail. Without the team_settings table on production, the sync script can't read from it. What this means is that we'll need to -
- Deploy the changes creating the table to staging and then to production
- Then open a PR with the sync script updates which we'll also progress forward
You could just comment out the file in main.sql and we'll just comment it back in later 👍
It's a known issue and not unique to this PR - we're going to need a better solution eventually here. Open to ideas!
hasura.planx.uk/migrations/1718728257637_populate_team_settings_table/down.sql
Outdated
Show resolved
Hide resolved
hasura.planx.uk/migrations/1718719894002_create_table_public_team_settings/up.sql
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,69 @@ | |||
| -- insert teams_settings overwriting conflicts | |||
There was a problem hiding this comment.
Take a look at #3160 which added the sync script for the feedback table. You'll also need to include this file in main.sql.
| @@ -1649,6 +1649,115 @@ | |||
| - role: platformAdmin | |||
| permission: | |||
| filter: {} | |||
| - table: | |||
There was a problem hiding this comment.
Generally, it's worth adopting a principle of least-privileged access to any tables. It's always worth being overly-conservative with permissions initially, and then granting access when we hit a requirement to access them.
What I think this means for this PR is that -
-
The public role does not need access to GovNotify fields (
email_reply_to_id,help_email,help_opening_hours,help_phone) as these are read by the backend when posting out emails. If this assumption is wrong, we can grant access when we hit features that require them. -
teamEditors can only update their ownteam_settings- they cannot update theteam_settingsof a team which they aren't a member of. Hasura has a neat way of handling this - take a look at how this is handled inteam_themeswhere the same restriction applies. Hasura has good docs on this here, which are useful for context. Happy to talk this through if helpful, just let me know 👍
There was a problem hiding this comment.
This type of thing? @DafyddLlyr
There was a problem hiding this comment.
Yep, that's the one! Looks correct to me - you can check that the filter matches team_themes (and I think there may be even a way of copying directly).
| @@ -0,0 +1,15 @@ | |||
| INSERT INTO team_settings (team_id, reference_code, homepage, help_email, help_phone,help_opening_hours ,email_reply_to_id,has_planning_data , external_planning_site_url,external_planning_site_name,boundary_url,boundary_json) | |||
There was a problem hiding this comment.
This looks good and correct to me 👍 Will double check when Pizza is up and test sync script locally also.
| filter: | ||
| team: | ||
| members: | ||
| _and: | ||
| - user_id: | ||
| _eq: x-hasura-user-id | ||
| - role: | ||
| _eq: teamEditor |
What does this PR do?
This PR focuses on the Hasura side of adding a new
team_settingstable to the database. In future, this will allow users to manage their own settings for their Team, and connect to elements of the API and Editor.Where to find more info?
I have been documenting some of what I'm doing here: team_settings notion
Notable exclusions
I am yet to add in space for the computed field
boundary__bboxbut I will look to add this in during the current sprint.settingsandnotify_personalisationhave no been dropped from theTEAMStable - this will be done in clean up near the end of the sprint.