Prevent special goal renames#6468
Conversation
| test "update/2 allows updating non-name fields of a special goal" do | ||
| site = new_site() | ||
| {:ok, goal} = Goals.create(site, %{"event_name" => "File Download"}) | ||
|
|
||
| assert {:ok, updated} = | ||
| Goals.update(goal, %{ | ||
| "event_name" => "File Download", | ||
| "display_name" => "File Download", | ||
| "custom_props" => %{"0" => "path"} | ||
| }) | ||
|
|
||
| assert updated.custom_props == %{"0" => "path"} | ||
| end |
There was a problem hiding this comment.
question, non-blocking: I know it's allowed right now, but now that we're looking into special goals, should we keep allowing this? Special goals with some user-defined properties seem likely to bite us in the future.
There was a problem hiding this comment.
Let's say we want to make File Download dashboard component more special. Instead of relying on the system-defined setup for the goal that's the same for everyone, we need to take into account variants where the otherwise system-defined goal has arbitrary custom props defined.
There was a problem hiding this comment.
Or ignore any unrecognizable configuration. Nothing prevents you from sending ANY goal data. There is no way to tell whether this is legitimate event from the tracker, matching an auto-created goal or just random payload that happens to be named "File Download" with any custom props attached. We are relying on display names to recognize special goals, nothing is guaranteed in such setup.
There was a problem hiding this comment.
Also, special goals with custom props work fine, granted there's data. If you use the seeded db and attach logged_in: true to "Outbound Link: Click", the report will render narrowed down to that. I don't see why should we prevent that based on pure speculation.
| assert render(lv) =~ "cannot be changed for an automated goal" | ||
| end | ||
|
|
||
| test "renders error when goal is invalid", %{conn: conn, site: site} do |
There was a problem hiding this comment.
meganitpick, non-blocking: I see now that there's several ways for the goal to be invalid, this test could be renamed to something like "renders error when renaming goal to be the same as an existing goal name"
There was a problem hiding this comment.
IMHO at the form level, it does not matter, we only care if the changeset error shows up, whatever that might be. The test is fine. Lower level tests can go into details.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
2 participants
This PR locks special goal renames so they don't lose their front-end meaning.
In case of goals whose display names fall within the special set, but their corresponding event names don't, we allow the rename in case a manual status regain is needed.
Ref https://app.basecamp.com/5308029/buckets/36789884/card_tables/cards/9834018285
Ref #6464