-
Notifications
You must be signed in to change notification settings - Fork 1
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
Saved locations #105
base: main
Are you sure you want to change the base?
Saved locations #105
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally this looks good but there are some changes that might make a stronger schema design.
cli/macrostrat/cli/subsystems/user_features/saved_locations_schema.sql
Outdated
Show resolved
Hide resolved
cli/macrostrat/cli/subsystems/user_features/saved_locations_schema.sql
Outdated
Show resolved
Hide resolved
longitude numeric not null, | ||
created_at timestamp default now() not null, | ||
updated_at timestamp default now() not null, | ||
category saved_locations_enum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be an enum or a link to a related table? the nice thing about the latter is that it is easier to update and can have "details" (e.g., an explanation of what the category means, a symbol ID, etc.) attached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great question. I think having set categories as an enum type so that the user can quickly select...and then have a separate tags table so that the user can define and search by their locations using their own tag definitions. I'll add this to the schema to get your thoughts.
cli/macrostrat/cli/subsystems/user_features/saved_locations_schema.sql
Outdated
Show resolved
Hide resolved
alter table saved_locations | ||
owner to "macrostrat-admin"; | ||
|
||
grant delete, insert, select, update on saved_locations to web_anon; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want anonymous users to be able to delete and insert locations? We could lock this down a bit more even without implementing row-level security...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thoughts were to only give CRUD ops only to authorized users and not web_anon. Does web_anon require a login to access macrostrat? If not, I would assume they wouldn't get access to the saved_locations feature at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep agreed
location_name varchar(120) not null, | ||
location_description text, | ||
latitude numeric not null, | ||
longitude numeric not null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will you decide how far to zoom in for specific saved locations? Is this something that needs to be saved in the database?
- Maybe we could add a
view_params
JSON column to house view parameters in a less structured way? - We could alternatively/additionally set a
radius
value that would define whether the location was small or large
The Zoom level/radius is mostly only a problem because we can add locations from a very zoomed out perspective, so maybe the solution would be to just disallow that in the UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've stored this as an additional text column for now. I'm going to look into the how we can take these extra params, transform them into json and import them into the db.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having optional radius
(number in meters) and camera_position
(JSON column with format matching https://github.com/UW-Macrostrat/web-components/blob/3f7eb7aa9e1c38dca0d2689d9a127a67a2cd7e05/packages/mapbox-utils/src/position.ts#L13) would do nicely.
If we store the elevation of the point, we can infer the camera position from pitch
and bearing
also, so maybe that's better.
cli/macrostrat/cli/subsystems/user_features/update_saved_locations.sql
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Location information updates
references macrostrat_auth."user", | ||
location_name varchar(120) not null, | ||
location_description text, | ||
latitude numeric not null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latitude and longitude should be changed to a point geometry with SRID 4326. Geometry(Point, 4326)
Merging the database schema setup for the saved_locations user features