Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Onboarding improvements #4459

Merged
merged 66 commits into from
Sep 2, 2024
Merged

Onboarding improvements #4459

merged 66 commits into from
Sep 2, 2024

Conversation

aerosol
Copy link
Member

@aerosol aerosol commented Aug 21, 2024

Changes

Migration will be extracted to a separate PR once the review is conducted. migration

This PR aims to make the onboarding (and related flows, such as e.g. domain change) more helpful and visually consistent. The main functional change is the "customize installation" screen, where site type is determined early (WordPress/GTM/manual) and installation instructions are provided according to that inspection result.

For manual and GTM installations, users can build the script snippet by selecting tracker extensions they're interested it (some of which will have side effects - e.g. including the "file downloads" extension will automatically create a relevant goal).

As a consequence, the snippet window has been removed from general settings section (snippet may not be relevant at all, in case of WP installations) and replaced by 2-step flow allowing users to "review their installations". Similarly, the snippet window became no longer relevant during domain change.

By the way the following changes were also made:

  • focus.html layout has been removed, the distinction leads to unnecessary maintenance
  • site verification is no longer optional, VERIFICATION_ENABLED variable has been deprecated, it no longer has any effect. For self-hosted installations, verification will simply fail by default, unless browserless stack is set up. Instructions on how to do that for CE can be provided later on
  • minor visual rearrangements according to the new designs

Docs: major changes pending

Tests

  • Automated tests have been added
  • This PR does not require tests

Changelog

  • Entry has been added to changelog
  • This PR does not make a user-facing change

Documentation

  • Docs have been updated
  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode
  • This PR does not change the UI

Copy link

Preview environment👷🏼‍♀️🏗️
PR-4459

@aerosol aerosol force-pushed the snippet-wizard branch 3 times, most recently from eef1bc9 to ebd8660 Compare August 26, 2024 08:19
@aerosol aerosol changed the title wip Onboarding improvements Aug 26, 2024
@aerosol aerosol marked this pull request as ready for review August 26, 2024 09:00
@aerosol aerosol requested review from zoldar, RobertJoonas, metmarkosaric and a team and removed request for zoldar and RobertJoonas August 26, 2024 09:03

@type t() :: %__MODULE__{}

embedded_schema do
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next up, we'll probably have "real domain" in here

@aerosol
Copy link
Member Author

aerosol commented Aug 29, 2024

Data migration for CE is failing: https://github.com/plausible/analytics/actions/runs/10612221725/job/29413425673?pr=4459

This is because Plausible.DataMigration.SiteImports relies on Site schema that gets migrated later on. How should we proceed in such case? cc @zoldar @ruslandoga

@ruslandoga
Copy link
Contributor

ruslandoga commented Aug 29, 2024

I'll need to take a closer look, but I wonder if #4466 would fix this?

@ruslandoga
Copy link
Contributor

ruslandoga commented Aug 29, 2024

Ah, the issue seems a bit different. I think the fix here would be to not use Ecto schemas in data migrations and instead rely on "raw" selects.

That is, instead of doing this

Plausible.Site |> ... |> Repo.all()

we can do this in code that runs in migrations

"sites" |> select([s], map(s, [...necessary fields for the migration...])) |> Repo.all()

@aerosol
Copy link
Member Author

aerosol commented Aug 29, 2024

hmm how about:

diff --git a/lib/plausible/data_migration/site_imports.ex b/lib/plausible/data_migration/site_imports.ex
index 8aaa03f3e..749fbeaa2 100644
--- a/lib/plausible/data_migration/site_imports.ex
+++ b/lib/plausible/data_migration/site_imports.ex
@@ -29,6 +29,7 @@ defmodule Plausible.DataMigration.SiteImports do
     sites_with_only_legacy_import =
       from(s in Site,
         as: :site,
+        select: %{id: s.id, imported_data: s.imported_data},
         where:
           not is_nil(s.imported_data) and fragment("?->>'status'", s.imported_data) == "ok" and
             not exists(site_import_query)

the test at Plausible.DataMigration.SiteImportsTest runs fine.

@aerosol aerosol merged commit e3af1a3 into master Sep 2, 2024
10 checks passed
@aerosol aerosol deleted the snippet-wizard branch September 2, 2024 10:49
provisioning: [
"Add site info",
"Install Plausible",
"Verify installation"
Copy link
Contributor

@ruslandoga ruslandoga Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 @aerosol

Is it possible to disable this somehow conditionally on CE? It might confuse some users:

Screenshot 2024-09-13 at 16 05 33

Update:

just realized that I used dummy.site which doesn't have Plausible tracker installed. So I tried it on one of my real sites, but it still doesn't seem to work:

Screenshot 2024-09-20 at 18 39 27 Screenshot 2024-09-20 at 18 39 35

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. we can enable it only if ghcr.io/browserless/chromium is running or if some extra ENV var is set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it felt like it was too costly to maintain a separate branch without verification. I think it's going to be easier going forward to set up instructions for self-hosters to run their own browserless stack. WYT?

Copy link
Contributor

@ruslandoga ruslandoga Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be problematic for many users who decide to run on the smallest available instances (e.g. t2-micro on AWS):

[ec2-user@ip-172-31-46-71 ~]$ docker run --rm  ghcr.io/browserless/chromium
Unable to find image 'ghcr.io/browserless/chromium:latest' locally
latest: Pulling from browserless/chromium
602d8ad51b81: Pull complete
7e6a86bffb05: Pull complete
4f4fb700ef54: Pull complete
eadff0eab842: Pull complete
6b08750ce489: Pull complete
6931508ca973: Pull complete
e839281773da: Pull complete
d9fbbce37766: Pull complete
16df0a60c56d: Pull complete
daccdb15ea7d: Pull complete
872e2e99569f: Pull complete
9f74149fc518: Pull complete
35f28019a5e3: Pull complete
43a8a10c249b: Pull complete
b1b4ece4e245: Pull complete
fd5c44f4051d: Pull complete
62ed90b89bd0: Pull complete
0b23a1ec6119: Pull complete
2859dd417a71: Pull complete
a15b9ba04167: Pull complete
c76fa6cc8898: Pull complete
a6f34b5464b1: Pull complete
7aa3df8dfb9b: Pull complete
e15bbf55a158: Pull complete
9354a349fd43: Pull complete
362a19d48bd5: Pull complete
4708bbe9978e: Pull complete
4347e0d884d4: Extracting [==================================================>]  260.2MB/260.2MB
docker: failed to register layer: write /usr/local/bin/playwright-browsers/chromium-1134/chrome-linux/chrome: no space left on device.

^ seems like I'll need to add more disk space to run Chrome on my instance.


Once started it doesn't seem to consume many resources.

$ docker images
REPOSITORY                     TAG         IMAGE ID       CREATED        SIZE
ghcr.io/ruslandoga/plausible   my-build    69ee5ce2a9e4   3 hours ago    243MB
ghcr.io/browserless/chromium   latest      e19c5598adbb   14 hours ago   3.15GB
postgres                       14-alpine   7f76e70684c3   3 months ago   239MB
clickhouse/clickhouse-server   24.3        bbf3c391e720   4 months ago   962MB

$ docker stats --no-stream
CONTAINER ID   NAME                            CPU %     MEM USAGE / LIMIT     MEM %     NET I/O           BLOCK I/O        PIDS
7594e0f4b8cb   jovial_clarke                   0.00%     60.81MiB / 949.6MiB   6.40%     1.09kB / 0B       129MB / 254kB    14
6dc5abe276f2   hosting-plausible-1             0.32%     196.8MiB / 949.6MiB   20.73%    2.37MB / 2.01MB   108MB / 141MB    24
da57d28c65a8   hosting-plausible_events_db-1   9.58%     183.4MiB / 949.6MiB   19.32%    1.58MB / 2.01MB   751MB / 100MB    698
05556c484b88   hosting-plausible_db-1          0.16%     46.97MiB / 949.6MiB   4.95%     437kB / 342kB     120MB / 12.4MB   18

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's a one-time feature for self-hosters, I don't see much benefit in running idle Chrome in the background for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's possible to reach site settings from this page, shouldn't it be just a conditional render call when not ce()? for this "Verify Installation" step?

Copy link
Contributor

@ruslandoga ruslandoga Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aerosol would it be OK for me to explore this conditional approach or should we postpone it until self-hosters' feedback? If nobody complains, then it would mean #4459 (comment) is not a problem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, please make your call regarding this. I’ll be happy to review a PR. My reasoning was - for folks on small VPS boxes, I thought it’d be reasonable to say “in order for verification to work, you need more resources. alternatively verify your installation manually”. They are not missing much in such case. But if you feel it’s simple to make this step skippable based on configuration, go ahead.

@@ -778,11 +778,6 @@ config :plausible, Plausible.PromEx,
grafana: :disabled,
metrics_server: :disabled

config :plausible, Plausible.Verification,
enabled?:
get_var_from_path_or_env(config_dir, "VERIFICATION_ENABLED", "false")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it was being done this way before. Has this configuration been moved elsewhere?

@ruslandoga
Copy link
Contributor

For self-hosted installations, verification will simply fail by default, unless browserless stack is set up. Instructions on how to do that for CE can be provided later on

Ah, I see. I hope this won't cause too much confusion / questions on the forum :)

<li>
<.focus_list>
<:item>
<%= if Keyword.fetch!(Application.get_env(:plausible, :selfhost), :disable_registration) == false do %>
Copy link
Contributor

@ruslandoga ruslandoga Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This renders an empty list element on CE:

Screenshot 2024-09-13 at 16 20 10

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes, sorry

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aerosol would it be OK for me to PR a fix?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, much appreciated, thank you

This was referenced Sep 13, 2024
This was referenced Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants