Skip to content
Snippets Groups Projects
Unverified Commit 5a67efea authored by adam-james's avatar adam-james Committed by GitHub
Browse files

Events migration (#20452)


* 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: default avatardan sutton <dan@dpsutton.com>
parent a8dabd6a
No related merge requests found
Showing
with 884 additions and 28 deletions
Loading
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment