Update goal in segment #5027
Update goal in segment #5027
Conversation
| {:ok, goal1} = Goals.create(site, %{"event_name" => "Signup"}) | ||
| {:ok, _goal2} = Goals.create(site, %{"event_name" => "Signup from nav"}) | ||
|
|
||
| {:ok, segment} = | ||
| Plausible.Segments.insert_one(user.id, site, :editor, %{ | ||
| "type" => "site", | ||
| "segment_data" => %{ | ||
| "filters" => [ | ||
| ["is", "event:page", ["/blog"]], | ||
| ["is", "event:goal", ["Signup from nav", "Signup"]], | ||
| ["is", "event:props:variant", ["A"]] | ||
| ] | ||
| }, | ||
| "name" => "Segment name" | ||
| }) |
There was a problem hiding this comment.
nitpick, non-blocking: In some other cases, we're using Plausible.Factory to populate data. insert(:segment, owner: user, site: site, ...) would save you the trouble of declaring user role.
There was a problem hiding this comment.
The reason we're using contextual interfaces for Goals setup (above) is that much of the insertion logic is abstracted away. I'm not a big fan of using factories for anything slightly complex - otherwise an unverified contract emerges (we assume insert(s) made in tests are equivalent to actual creation routine). Not sure how it's like with segments, but it is certainly more reliable to use Goals.create for instance. Just my 2c.
| assert updated_segment.segment_data == %{ | ||
| "filters" => [ | ||
| ["is", "event:page", ["/blog"]], | ||
| ["is", "event:goal", ["Signup from nav", "SIGNUP"]], | ||
| ["is", "event:props:variant", ["A"]] | ||
| ] | ||
| } |
There was a problem hiding this comment.
issue: Noticed that we also override segment updated_at.
If that's intentional, it would be good to assert it here as well, with something like segment.updated_at < updated_segment.updated_at.
Alternatively, we may want to not override updated_at on these automatic updates. May be confusing when the message in the UI (Last updated at <stamp> by <name>) doesn't match with their usual online time.
Or we could also just rephrase it in the UI: Last updated at <stamp>, belongs to <name> or similar.
| goal_filter_regex = | ||
| ~s(.*?\\["is",\s*"event:goal",\s*\\[.*?"#{Regex.escape(stale_goal.display_name)}".*?\\]\\].*?) |
There was a problem hiding this comment.
question, non-blocking: Is the filters field as text with whitespace or not?
If not, we could simplify the regex a bit by removing the optional whitespace matchers \s*.
If yes, shouldn't we also include the whitespace matchers after the characters of [ and ]?
| @@ -117,6 +117,37 @@ defmodule Plausible.GoalsTest do | |||
| assert goal2.display_name == "Homepage" | |||
| end | |||
|
|
|||
| test "update/2 also updates any segments the goal is a part of" do | |||
There was a problem hiding this comment.
ultranitpick, non-blocking: test says any segments, but there's only one segment in the case
| goal_filter_regex = | ||
| ~s(.*?\\["is",\s*"event:goal",\s*\\[.*?"#{Regex.escape(stale_goal.display_name)}".*?\\]\\].*?) | ||
|
|
||
| IO.inspect(stale_goal) |
| {:ok, Repo.preload(goal, :funnels)} | ||
| else | ||
| {:ok, goal} | ||
| Repo.transaction(fn -> |
There was a problem hiding this comment.
Wrapping this with Repo.transaction means, that it now returns {:ok, result} on success - so {:ok, {:ok, goal}} while {:ok, goal} is expected. Also note, that if the fn doesn't throw an exception, the transaction is not rolled back, so a manual Repo.rollback(error) might be necessary in case of errors along the way.
I'm out of the loop with this, but AFAIU Segments don't set constraints on any goals they depend on, so should we also handle deleting a goal?
There was a problem hiding this comment.
Migrating segments automatically on goal name change is a bit of a workaround to not having goal IDs in filters.
We're betting on the fact that when user is composing filters like [is, event:goal, <goal-display-name>], they don't mean [contains, event:goal, <goal-display-name>].
On deleting goals, the plan was to just let the user know that it's in segments and that these will not filter right until a goal with the same name is added back again, or the segments manually changed.
| %Plausible.Goal{} = stale_goal, | ||
| %Plausible.Goal{} = updated_goal | ||
| ) do | ||
| goal_filter_regex = |
There was a problem hiding this comment.
This requires inline commentary so the reader won't have to wonder what are we trying to achieve here
When a goal's display name is updated, this PR makes it so the segments containing that goal are also updated accordingly