-
adam-james authored
* Migrations for the new 'Annotate Useful Events' project Users will be able to create 'events' in timelines, which are accessible via collections. Two migrations are introduced to facilitate the new feature: timeline table, and event table. The following columns are presumed necessary based on the design document: ** TimeLine - id - name - description - collection_id - archived - creator_id - created_at - updated_by ?? - updated_at *** Event - id - timeline_id - name - description markdown (max length 255 for some reason) - date - time is optional - timezone (optional) - icon (set of 6) - archived status - created by - created at - created through: api or UI - modified by ?? - modified at * Changes to events schema - add icon onto timeline - make icon on event nullable - just move to a single timestamp on event, no boolean or anything - rename event table to `timeline_events` since "event" is so generic * dir-locals indentation * Timeline api and model - patched up the migration to make collection_id nullable since that is the root collection - followed the layout of api/metric and did the generic model stuff * Select keys with keywords, not destructured nils :( also, can't return the boolean from the update, but select it again * Enable the automatic updated_at * Boolean for "time matters" and string timezone column on events * clean up migration, rename modified_at -> updated_at for benefit of `:timestamped? true` bits * basic timeline event model namespace * Timeline Events initial API Just beginning the basics for the API for timeline events. - need to find a way to check permissions - have to still check if the endpoint is returning stuff properly * Singularize timeline_event tablename * Timeline events api hooked up * Singularize timeline event api namespace * unused comment * indent spec maps on routes * Make name optional on timeline PUT * Update collection api for timelines endpoints - add /root/timelines endpoint for when collection id is null - add /:id/timelines endpoint Created a hydration function to hydrate the timeline events when fetching timelines. * TimelineEvent permissions - crate a permissions objects set via the timeline's permissions * Move to using new permissions setup previously was explicitly checking permissions of the underlying timeline from the event, but now we just do the standard read/write check on the event, and the permissions set knows it is based on the permission set of the underlying timeline (which is based on the permissions set of the underlying collection) * Items api includes timelines * Strip of icon from other rows * Indices on timeline and timeline_event timeline access by collection_id timeline_event by both timeline_id and (timeline_id, timestamp) for when looking for events within a certain range. We will always have been narrowed by timeline ids (per the collection containing the question, or by manually enabling certain timelines) so we don't need a huge index on the timestamp itself. * Skeleton of range-based query * Initial timeline API tests Began with some simple Auth tests and basic GET tests, using a Collection with an id as well as the special root collection. * Fix docstring on api timeline namespace * Put timeline's events at `:events` not `timeline-events` * Add api/card/:id/timelines At the moment api/card/:id/timelines and api/collection/:id/timelines do the same thing: they return all of the timelines in the collection or in the collection belonging to the card. In the future this may drift so they share an implementation at the moment. * Put creator on timeline on api endpoints for timeline by id and timelines by card and collection * Hydrate creator on events Bit tricky stuff here. The FE wants the events at "events" not "timeline-events" i guess due to the hyphen that js can never quite be sure isn't subtraction. To use the magic of nested hydration `(hydrate timelines [:events :creator])`, the events have to be put at the declared hydration key. I had wanted the hydration `:timeline-events` for clarity in the code but want `:events` to be the key. But when you do this, the hydration thinks it didn't do any work and cannot add the creator. So the layout of the datastructure is tied to the name of the hydration key. Makes sense but I wish I had more control since `:events` is so generic. * Add include param check to allow timeline GET to hydrate w/ events. The basic check is in place for include=events in the following ways: GET /api/timeline/:id?include=events GET /api/collection/root/timelines?include=events If include param is omitted, timelines are hydrated only with the creator. Events are always hydrated with creator. * fix hyphen typo in api doc * Change schema for include=events to s/enum to enforce proper param Had used just a s/Str validation, which allows the include parameter value to be anything, which isn't great. So, switched it to an enum. Can always change this later to be a set of valid enums, similar to the `model` parameter on collection items. * Fixed card/:id/timelines failing w/ wrong arity The `include` parameter is passed to `timeline/timelines-for-collection` to control hydration of the events for that timeline. Since the card API currently calls this function too, it requires that parameter be passed along. As well, I abstracted the `include-events-schema` and have it in metabase.api.timeline. Not married to it, but since the schema was being used across timeline, collection, and card API, it seems like a good idea to define it in one place only. Not sure if it should be metabase.api.timeline or metabase.models.timeline * used proper migration numbers (claimed in migrations slack channel) * Add archived=true functionality There's one subtle issue remaining, and that is that when archived=true, :creator is not hydrated on the events. I'll look into fixing that. In the meantime, the following changes are made: - :events returs empty list `[]` instead of `null` when there are no events in a timeline - archived events can be fetched via `/api/timeline/:id?include=events&archived=true` - similarly, archived events can be fetched with: - `/api/collection/root|:id/timelines?include=events&archived=true` - `/api/card/:id/timelines?include=events&archived=true` Just note the caveat for the time being (no creator hydrated on events when fetching archived) Fix pending. * Altered the hydration so creator is always part of a hydrated event Adjusted the hydration of timelines as follows: - hydration always grabs all events - at the endpoint's implementation function, the timeline(s) are altered by filtering on archived true or false Not sure if this is the best approach, but for now it should work * Create GET endpoint for /api/timeline and allow archived=true Added a missed GET endpoint for fetching all the timelines (optionally fetch archived) * reformat def with docstring * Timeline api updated to properly filter archived/un-archived Use archived=true pattern in the query params for the timeline endpoint to allow FE to get what they need. Work in progress on the API tests too. * Timeline-events API tests Timeline Events API endpoint tests, at least the very basics. * Timeline Hydration w/ Events tests Added a test for Events hydration in the timeline API test namespace. * TimelineEvent Model test NS May be a bit unnecessary to test the hydrate function, but the namespace is there in case we need to add more tests over time. Also adjusted out a comment and added a library to timeline_test ns * Added metabase.models.timeline-test NS Once again, this may be a bit unnecessary as a test ns for now, but could be the right place to add tests if the feature grows in the future. * Clean up handling of archived - we only want to show archived events when viewing a timeline by id and specifically asking for archived events. Presumably this is some administrative screen. We don't want to allow all these options when viewing a viz as its just overload and they are archived for a reason. If FE really want them they can go by id of the timeline. Note it will return all events, not just the archived ones. - use namespaced keywords in the options map. Getting timelines necessarily requires getting their events sometimes. And to avoid confusion about the archived state, we have options like `:timeline/events?`, `:timeline/archived?`, `:events/start`, `:events/end`, and `:events/all?` so they are all in one map and not nested or ambiguous. * Clean up some tests noticable differences: timelines with archived=true return all events as if it is "include archived" rather than only show archived. * PUT /api/timeline/:id now archives all events when TL is archived We can archive/un-archive timelines, and all timeline events associated with that timeline will follow suit. This does lose state when, for example, there is a mix of archived/un-archived events, as there is no notion of 'archived-by' to check against. But, per discussion in the pod-discovery slack channel, this is ok for now. It is also how we handle other archiving scenarios anyway, with items on collections being archived when a collection is archived. * cleanup tests * Include Timeline and TimelineEvent in models * Add tt/WithTempDefaults impls for Timeline/TimelineEvent lets us remove lots of the `(merge defaults {...})` that was in the code. Defaults are ```clojure Timeline (fn [_] {:name "Timeline of bird squawks" :creator_id (rasta-id)}) TimelineEvent (fn [_] {:name "default timeline event" :timestamp (t/zoned-date-time) :timezone "US/Pacific" :time_matters true :creator_id (rasta-id)}) ``` * Add timeline and timeline event to copy infra * Timeline Test checking that archiving a Timeline archives its events A Timeline can be archived and its events should also be archived. A Timeline can also be unarchived, which should also unarchive the events. * Docstring clarity on apis * Remove commented out form * Reorder migrations * Clean ns clj-kondo ignores those but our linter does not * Correct casing on api schema for include * Ensure cleanup of timeline from test * DELETE for timeline and timeline-event * Poison collection items timeline query when is_pinned timelines have no notion of pinning and were coming back in both queries for pinned and not pinned. This adds a poison clause 1=2 when searching for pinned timelines since none are but they don't have a column saying that. * Clean up old comment and useless tests comment still said to poison the query when getting pinned state and that function had been added. Tests were asserting count = 2 and also the set of names had two things in them. Close enough since there are no duplicate names * Use TemporalString schema on timeline event routes Co-authored-by:
dan sutton <dan@dpsutton.com>
adam-james authored* Migrations for the new 'Annotate Useful Events' project Users will be able to create 'events' in timelines, which are accessible via collections. Two migrations are introduced to facilitate the new feature: timeline table, and event table. The following columns are presumed necessary based on the design document: ** TimeLine - id - name - description - collection_id - archived - creator_id - created_at - updated_by ?? - updated_at *** Event - id - timeline_id - name - description markdown (max length 255 for some reason) - date - time is optional - timezone (optional) - icon (set of 6) - archived status - created by - created at - created through: api or UI - modified by ?? - modified at * Changes to events schema - add icon onto timeline - make icon on event nullable - just move to a single timestamp on event, no boolean or anything - rename event table to `timeline_events` since "event" is so generic * dir-locals indentation * Timeline api and model - patched up the migration to make collection_id nullable since that is the root collection - followed the layout of api/metric and did the generic model stuff * Select keys with keywords, not destructured nils :( also, can't return the boolean from the update, but select it again * Enable the automatic updated_at * Boolean for "time matters" and string timezone column on events * clean up migration, rename modified_at -> updated_at for benefit of `:timestamped? true` bits * basic timeline event model namespace * Timeline Events initial API Just beginning the basics for the API for timeline events. - need to find a way to check permissions - have to still check if the endpoint is returning stuff properly * Singularize timeline_event tablename * Timeline events api hooked up * Singularize timeline event api namespace * unused comment * indent spec maps on routes * Make name optional on timeline PUT * Update collection api for timelines endpoints - add /root/timelines endpoint for when collection id is null - add /:id/timelines endpoint Created a hydration function to hydrate the timeline events when fetching timelines. * TimelineEvent permissions - crate a permissions objects set via the timeline's permissions * Move to using new permissions setup previously was explicitly checking permissions of the underlying timeline from the event, but now we just do the standard read/write check on the event, and the permissions set knows it is based on the permission set of the underlying timeline (which is based on the permissions set of the underlying collection) * Items api includes timelines * Strip of icon from other rows * Indices on timeline and timeline_event timeline access by collection_id timeline_event by both timeline_id and (timeline_id, timestamp) for when looking for events within a certain range. We will always have been narrowed by timeline ids (per the collection containing the question, or by manually enabling certain timelines) so we don't need a huge index on the timestamp itself. * Skeleton of range-based query * Initial timeline API tests Began with some simple Auth tests and basic GET tests, using a Collection with an id as well as the special root collection. * Fix docstring on api timeline namespace * Put timeline's events at `:events` not `timeline-events` * Add api/card/:id/timelines At the moment api/card/:id/timelines and api/collection/:id/timelines do the same thing: they return all of the timelines in the collection or in the collection belonging to the card. In the future this may drift so they share an implementation at the moment. * Put creator on timeline on api endpoints for timeline by id and timelines by card and collection * Hydrate creator on events Bit tricky stuff here. The FE wants the events at "events" not "timeline-events" i guess due to the hyphen that js can never quite be sure isn't subtraction. To use the magic of nested hydration `(hydrate timelines [:events :creator])`, the events have to be put at the declared hydration key. I had wanted the hydration `:timeline-events` for clarity in the code but want `:events` to be the key. But when you do this, the hydration thinks it didn't do any work and cannot add the creator. So the layout of the datastructure is tied to the name of the hydration key. Makes sense but I wish I had more control since `:events` is so generic. * Add include param check to allow timeline GET to hydrate w/ events. The basic check is in place for include=events in the following ways: GET /api/timeline/:id?include=events GET /api/collection/root/timelines?include=events If include param is omitted, timelines are hydrated only with the creator. Events are always hydrated with creator. * fix hyphen typo in api doc * Change schema for include=events to s/enum to enforce proper param Had used just a s/Str validation, which allows the include parameter value to be anything, which isn't great. So, switched it to an enum. Can always change this later to be a set of valid enums, similar to the `model` parameter on collection items. * Fixed card/:id/timelines failing w/ wrong arity The `include` parameter is passed to `timeline/timelines-for-collection` to control hydration of the events for that timeline. Since the card API currently calls this function too, it requires that parameter be passed along. As well, I abstracted the `include-events-schema` and have it in metabase.api.timeline. Not married to it, but since the schema was being used across timeline, collection, and card API, it seems like a good idea to define it in one place only. Not sure if it should be metabase.api.timeline or metabase.models.timeline * used proper migration numbers (claimed in migrations slack channel) * Add archived=true functionality There's one subtle issue remaining, and that is that when archived=true, :creator is not hydrated on the events. I'll look into fixing that. In the meantime, the following changes are made: - :events returs empty list `[]` instead of `null` when there are no events in a timeline - archived events can be fetched via `/api/timeline/:id?include=events&archived=true` - similarly, archived events can be fetched with: - `/api/collection/root|:id/timelines?include=events&archived=true` - `/api/card/:id/timelines?include=events&archived=true` Just note the caveat for the time being (no creator hydrated on events when fetching archived) Fix pending. * Altered the hydration so creator is always part of a hydrated event Adjusted the hydration of timelines as follows: - hydration always grabs all events - at the endpoint's implementation function, the timeline(s) are altered by filtering on archived true or false Not sure if this is the best approach, but for now it should work * Create GET endpoint for /api/timeline and allow archived=true Added a missed GET endpoint for fetching all the timelines (optionally fetch archived) * reformat def with docstring * Timeline api updated to properly filter archived/un-archived Use archived=true pattern in the query params for the timeline endpoint to allow FE to get what they need. Work in progress on the API tests too. * Timeline-events API tests Timeline Events API endpoint tests, at least the very basics. * Timeline Hydration w/ Events tests Added a test for Events hydration in the timeline API test namespace. * TimelineEvent Model test NS May be a bit unnecessary to test the hydrate function, but the namespace is there in case we need to add more tests over time. Also adjusted out a comment and added a library to timeline_test ns * Added metabase.models.timeline-test NS Once again, this may be a bit unnecessary as a test ns for now, but could be the right place to add tests if the feature grows in the future. * Clean up handling of archived - we only want to show archived events when viewing a timeline by id and specifically asking for archived events. Presumably this is some administrative screen. We don't want to allow all these options when viewing a viz as its just overload and they are archived for a reason. If FE really want them they can go by id of the timeline. Note it will return all events, not just the archived ones. - use namespaced keywords in the options map. Getting timelines necessarily requires getting their events sometimes. And to avoid confusion about the archived state, we have options like `:timeline/events?`, `:timeline/archived?`, `:events/start`, `:events/end`, and `:events/all?` so they are all in one map and not nested or ambiguous. * Clean up some tests noticable differences: timelines with archived=true return all events as if it is "include archived" rather than only show archived. * PUT /api/timeline/:id now archives all events when TL is archived We can archive/un-archive timelines, and all timeline events associated with that timeline will follow suit. This does lose state when, for example, there is a mix of archived/un-archived events, as there is no notion of 'archived-by' to check against. But, per discussion in the pod-discovery slack channel, this is ok for now. It is also how we handle other archiving scenarios anyway, with items on collections being archived when a collection is archived. * cleanup tests * Include Timeline and TimelineEvent in models * Add tt/WithTempDefaults impls for Timeline/TimelineEvent lets us remove lots of the `(merge defaults {...})` that was in the code. Defaults are ```clojure Timeline (fn [_] {:name "Timeline of bird squawks" :creator_id (rasta-id)}) TimelineEvent (fn [_] {:name "default timeline event" :timestamp (t/zoned-date-time) :timezone "US/Pacific" :time_matters true :creator_id (rasta-id)}) ``` * Add timeline and timeline event to copy infra * Timeline Test checking that archiving a Timeline archives its events A Timeline can be archived and its events should also be archived. A Timeline can also be unarchived, which should also unarchive the events. * Docstring clarity on apis * Remove commented out form * Reorder migrations * Clean ns clj-kondo ignores those but our linter does not * Correct casing on api schema for include * Ensure cleanup of timeline from test * DELETE for timeline and timeline-event * Poison collection items timeline query when is_pinned timelines have no notion of pinning and were coming back in both queries for pinned and not pinned. This adds a poison clause 1=2 when searching for pinned timelines since none are but they don't have a column saying that. * Clean up old comment and useless tests comment still said to poison the query when getting pinned state and that function had been added. Tests were asserting count = 2 and also the set of names had two things in them. Close enough since there are no duplicate names * Use TemporalString schema on timeline event routes Co-authored-by:
dan sutton <dan@dpsutton.com>
Code owners
Assign users and groups as approvers for specific file changes. Learn more.
.dir-locals.el 3.83 KiB