Saved segments spike #4648
Saved segments spike #4648
Conversation
| @@ -491,6 +491,7 @@ defmodule Plausible.Stats.Filters.QueryParserTest do | |||
| "metrics" => ["visitors"], | |||
| "date_range" => "all", | |||
| "filters" => [ | |||
| ["is", "segment", [200]], | |||
There was a problem hiding this comment.
Since it's not clear whether this is intended as the full code: Nit: separate test.
| site: site, | ||
| user: user | ||
| } do | ||
| name = "foo" |
There was a problem hiding this comment.
Nit: I don't think DRY-ing the variables here helps here - it just ends up with a wordier test where you need to scroll back-and-forth.
| "segment" => %{ | ||
| "description" => nil, | ||
| "name" => ^name, | ||
| "segment_data" => ^segment_data |
There was a problem hiding this comment.
Nit: This is confusing to follow - it isn't clear what the response actually looks like. I suggest the following structure.
segment = from(s in segments, where: %{ site_id: ^site_id }) |> Repo.one()
assert json_response(conn, 200) == %{
"role" => "owner",
"segment" => %{ ... }
}
And using fully-hard-coded values
There was a problem hiding this comment.
Thanks! I struggled with the timestamps in this test file. Any tips on how to omit them from the comparison while remaining brief?
There was a problem hiding this comment.
Good question - elixir assertions don't have any clever shorthands like any(datetime) for assert. I'll dig into existing tests for controllers and see how they have solved it. 🤔 Will provide an answer tomorrow!
There was a problem hiding this comment.
Dug a bit.
We don't have a usecase yet for exposing updated_at or inserted_at at the API level. Looking at similar models like lib/plausible/auth/invitation.ex and lib/plausible/site.ex we don't expose them either. I'd remove exposing them.
I'd rewrite the test as follows:
describe "POST /internal-api/:domain/segments" do
setup [:create_user, :create_new_site, :log_in]
test "creates segment successfully", %{conn: conn, site: site} do
conn =
post(conn, "/internal-api/#{site.domain}/segments", %{
"segment_data" => %{"filters" => [["is", "visit:entry_page", ["/blog"]]]},
"name" => "Blog entry"
})
segment = Plausible.Repo.one(Plausible.Segment)
assert json_response(conn, 200) == %{
"role" => "owner",
"segment" => %{
"id" => segment.id,
"name" => "Blog entry",
"segment_data" => %{"filters" => [["is", "visit:entry_page", ["/blog"]]]},
"description" => nil,
}
}
end
end|
|
||
| @filter_tree_operators [:not, :and, :or] | ||
|
|
||
| def parse_filters(filters) when is_list(filters) do |
There was a problem hiding this comment.
This doesn't apply the JSON schema which is a problem.
| def change do | ||
| create table(:segments) do | ||
| add :name, :string, null: false | ||
| add :segment_data, :map, null: false |
There was a problem hiding this comment.
Why not add :segment_filters, :array, null: false or something equivelent? What are we winning by nesting the data under filters and creating an abstraction?
| "maxItems": 3, | ||
| "items": [ | ||
| { | ||
| "$ref": "#/definitions/filter_operation_for_segments" |
There was a problem hiding this comment.
Nit: Let's inline the operation given there's only one and we're not repeating the ref anywhere.
| "$ref": "#/definitions/filter_operation_for_segments" | |
| "const": "is" |
priv/repo/migrations/20240918171300_create_segments_and_segment_users.exs
Outdated
Show resolved
Hide resolved
priv/repo/migrations/20240918171300_create_segments_and_segment_users.exs
Outdated
Show resolved
Hide resolved
lib/plausible/segment/schema.ex
Outdated
|
|
||
| defp validate_segment_data(changeset) do | ||
| case get_field(changeset, :segment_data) do | ||
| %{"filters" => filters} when is_list(filters) -> |
There was a problem hiding this comment.
Should validate here that it's valid and doesn't contain any nested segment references right?
There was a problem hiding this comment.
Agreed! It should be validated, but at what level? I didn't want to make this module depend on query / filters parsing. How about doing that in the API controller?
There was a problem hiding this comment.
Doing it in the controller seems valid and best (and if it's already validated then sorry for missing it!).
test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs
Outdated
Show resolved
Hide resolved
| role = "owner" | ||
|
|
||
| %{id: segment_id} = | ||
| insert(:segment, |
There was a problem hiding this comment.
Rather than insert it here, would it make sense to use the other APIs to create these to simulate user behavior?
apata
force-pushed
the
saved-segments-spec
branch
2 times, most recently
from
ad1bf06
to
5947e73
Compare
|
apata
force-pushed
the
saved-segments-spec
branch
2 times, most recently
from
437784e
to
7fe1c50
Compare
apata
force-pushed
the
saved-segments-spec
branch
from
e9c14ff
to
4a62e35
Compare
|
|
apata
force-pushed
the
saved-segments-spec
branch
from
4a62e35
to
c31ee4d
Compare
apata
force-pushed
the
saved-segments-spec
branch
from
c31ee4d
to
32107cf
Compare
This PR outlines schema and API structure for saved segments. Not intended to be merged as is.