Hide dashboard import icons when redundant #2 #5102
Conversation
…s. Switch now works off of main query
macobo
force-pushed
the
fix-include-imported
branch
from
9bded43 to
421bf32
Compare
| %{visible: true, togglable: true, tooltip_msg: "Click to include imported data"} | ||
|
|
||
| {false, :unsupported_query} -> | ||
| case Map.new(meta) do |
There was a problem hiding this comment.
There's one case missing here:
%{imports_included_for_main_query: false, imports_included: true} ->
%{visible: true, togglable: true, tooltip_msg: "Click to exclude imported data"}When imports were included only in comparison range, it still means that imports were included, and the tooltip should say "click to exclude".
| @@ -791,7 +791,7 @@ defmodule PlausibleWeb.Api.StatsController.TopStatsTest do | |||
| assert res["with_imported_switch"] == %{ | |||
| "visible" => true, | |||
| "togglable" => true, | |||
| "tooltip_msg" => "Click to exclude imported data" | |||
| "tooltip_msg" => "Click to include imported data" | |||
There was a problem hiding this comment.
Related to my other comment - this should not change.
| "visible" => false, | ||
| "togglable" => false, | ||
| "tooltip_msg" => nil |
There was a problem hiding this comment.
This shouldn't change either.
I believe instead we should add another test in this describe block that:
- makes a top-stats request without asking for comparisons (but top stats still gives the default comparisons)
- populates data that falls only in that default comparison range
- asserts the same that this test does -
assert res["with_imported_switch"] == %{"visible" => false, "togglable" => false, "tooltip_msg" => nil}
| @@ -26,7 +25,9 @@ defmodule Plausible.Stats.Query do | |||
| revenue_warning: nil, | |||
| remove_unavailable_revenue_metrics: false, | |||
| site_id: nil, | |||
| site_native_stats_start_at: nil | |||
| site_native_stats_start_at: nil, | |||
| comparison_utc_time_range: nil, | |||
There was a problem hiding this comment.
How about getting rid of comparison_utc_time_range? That information might as well be included under the comparison_query itself.
| site | ||
| |> Query.from(params, debug_metadata(conn)) | ||
| |> Query.set_include(:imports_meta, true) | ||
| query = Query.from(site, params, debug_metadata(conn)) |
There was a problem hiding this comment.
One of the changes in #5097 was to provide the default "previous_period" comparison to the query here (instead of visitor-graph.js)
I think that's still required - Top Stats are always queried with a comparison option, however,
- By default, comparison mode is
previous_period, which is set invisitor-graph.js. In that "default" case. We don't want to display the imported switch / imported warnings. - When the user is in the comparison mode however (press X on the dashboard, other reports are showing comparison data too), this is when we do want to display the imported switch / imported warnings.
There was a problem hiding this comment.
The business logic changes you are describing in this and other comments are pretty complicated, non-intuitive and seem contradictory to what we discussed in the call. Is there a ticket somewhere with the full context?
I don't see why the frontend change is at all necessary here to fix the 'bug' that including comparison query in meta.include_imported added but I'm likely missing something.
Here's what I've got based on your original PR description:
Bug description
User opens dashboard for month of March. User has imported data in february (but none in march)
Expected result: Imports button should say "Click to include imported data"
Actual result: imports button says "Click to exclude imported data"
There was a problem hiding this comment.
Is there a ticket somewhere with the full context?
There's no ticket at the moment, as it seemed to me like a niche thing to get out of the way with a single PR. Should we create one?
I don't see why the frontend change is at all necessary here to fix the 'bug' that including comparison query in meta.include_imported added but I'm likely missing something.
The "bug" here is not very straight forward indeed as the desired behaviours are somewhat conflicting.
The Top Stats query currently serves another purpose in addition to simply aggregating the stats. It is to set the state of whether imported data is currently visible on the dashboard or not, AND whether it exists at all in the given query (with comparison) range.
From the API perspective, I wouldn't even say there's a bug - imported data is in the Top Stats comparison range so the request responds with:
- yes - imported data WAS included
- When the imports button (aka "with_imported_switch") is clicked, it will disable imported data globally in the dashboard, and hence the tooltip should say "click to exclude imported data"
The issue/weirdness comes from the fact that we're not in the "comparison mode" and yet the backend ends up running a comparison query because Top Stats fetches comparisons even when not in comparison mode - that is to render the small change percentages with arrows. The desired behaviour (as pointed out by Marko) is that when we're not explicitly in comparison mode, the top stats comparison range should "silently" count imported data.
The issue at hand could theoretically be solved by:
- Hide dashboard import icons when redundant #5097 (fixes exactly the problem I'm describing - nothing more, nothing less)
- not rendering the comparison change arrows UNLESS in comparison mode - I recall we did this at some point and people saw the top stats too "blank" so we put the change arrows back even in the non-comparison mode
- create two separate queries in the Top Stats endpoint when not in comparison mode
None of these solutions is perfect in my head.
Also, I believe this issue was there already before any imports-related work/refactoring that was part of the scroll depth project.
Alternative solution to #5097 to not show include/exclude imported data for comparisons in top stats.
What's changing:
query.imports_includedonly applies to that query and not comparison queryinclude_meta->dashboard_include_metaflag renameimports_exist->completed_importslist persisted on queryPrevious solution was causing unneeded joins with imported tables.
Sadly now QueryResult needs to be somewhat aware of comparisons. I've tried to minimize the footprint though.