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.

Sign up for GitHub

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

Jump to bottom

remove custom_dimension_filter feature and views_per_visit_metric flags #2996

Merged
merged 4 commits into from
Jun 7, 2023
Merged

remove custom_dimension_filter feature and views_per_visit_metric flags #2996

merged 4 commits into from
Jun 7, 2023

Conversation

ruslandoga
Copy link
Contributor

@ruslandoga ruslandoga commented Jun 1, 2023 •
edited
Loading

Changes

This PR enables custom dimension filter for self-hosters.

Relevant discussion: #2984

This PR is based on #2898

Tests

  • This PR does not require tests

Changelog

  • Entry has been added to changelog

Documentation

  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode

@ruslandoga ruslandoga requested a review from a team June 1, 2023 04:53
@bundlemon
Copy link

bundlemon bot commented Jun 1, 2023 •
edited
Loading

BundleMon

Files updated (1)
Status Path Size Limits
✅ static/js/dashboard.js
 304.55KB (-80B -0.03%) -
Unchanged files (6)
Status Path Size Limits
✅ static/css/app.css
 492.34KB -
✅ static/js/app.js
 12.31KB -
✅ static/js/embed.host.js
 5.58KB -
✅ static/js/embed.content.js
 5.06KB -
✅ tracker/js/plausible.js
 742B -
✅ static/js/applyTheme.js
 314B -

Total files change -80B -0.01%

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

ruslandoga
ruslandoga commented Jun 1, 2023
View reviewed changes
lib/plausible_web/controllers/stats_controller.ex Outdated
@@ -326,7 +326,6 @@ defmodule PlausibleWeb.StatsController do

defp get_flags(user) do
%{
custom_dimension_filter: FunWithFlags.enabled?(:custom_dimension_filter, for: user),
views_per_visit_metric: FunWithFlags.enabled?(:views_per_visit_metric, for: user)
Copy link
Contributor Author

@ruslandoga ruslandoga Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can views_per_visit_metric be enabled as well?

Request: #131 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can views_per_visit_metric be enabled as well?

@ruslandoga, the views_per_visit_metric flag is actually redundant. It's a duplicate that can be safely deleted now.

We used a visits_metric flag for both visits and views_per_visits metrics. And that flag was removed in #2898, so in the current master the views_per_visit metric is enabled for everyone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR into this branch: #3001

Copy link
Contributor

Choose a reason for hiding this comment

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

@ruslandoga nice! I've approved :)

@ruslandoga ruslandoga requested a review from ukutaht June 1, 2023 04:58
@ruslandoga ruslandoga force-pushed the enable-custom-dimension-filter-for-all branch from 4d5f19d to 79bcdf4 Compare June 1, 2023 05:02
@ukutaht
Copy link
Contributor

ukutaht commented Jun 1, 2023

@RobertJoonas could you take a look? I think you're most familiar with this stuff

aerosol
aerosol approved these changes Jun 1, 2023
View reviewed changes
assets/js/dashboard/filters.js
@@ -83,10 +79,6 @@ function renderDropdownFilter(site, history, [key, value], query) {
)
}

function shouldLinkToFilterModal(site, key) {
return key !== 'props' || site.flags.custom_dimension_filter
Copy link
Member

Choose a reason for hiding this comment

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

I lack context here but sanity check -- shouldn't we retain the key !== 'props' condition?

Copy link
Contributor Author

@ruslandoga ruslandoga Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I lack context here too (and I was uncertain if I should even open this PR).

My thinking was, if custom_dimension_filter is enabled for all, then it's always true, so even if key !== 'props' returns false, the end result is still true because of ||.

Copy link
Contributor

Choose a reason for hiding this comment

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

The key !== 'props' check makes sure that if key is anything other than 'props' (e.g. 'page') then it should return true, no matter the flag status. We can remove that too as we don't need any special handling for props anymore, all of the options should be links.

@aerosol
Copy link
Member

aerosol commented Jun 1, 2023

@ruslandoga could you resolve the CHANGELOG conflict please?

@aerosol aerosol requested a review from RobertJoonas June 1, 2023 13:41
RobertJoonas
RobertJoonas approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@RobertJoonas RobertJoonas left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@ruslandoga ruslandoga force-pushed the enable-custom-dimension-filter-for-all branch from 60801b8 to a246875 Compare June 2, 2023 03:40
ruslandoga
ruslandoga commented Jun 2, 2023
View reviewed changes
assets/js/dashboard/filters.js Outdated
<span className="inline-block w-full truncate">{filterText(key, value, query)}</span>
<PencilSquareIcon className="w-4 h-4 ml-1 cursor-pointer group-hover:text-indigo-700 dark:group-hover:text-indigo-500" />
</Link>
<div
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, this shouldn't be included, since it was using a !shouldLinkToFilterModal(site, key) check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in bc7a616

@ruslandoga ruslandoga marked this pull request as draft June 2, 2023 03:41
@ruslandoga ruslandoga marked this pull request as ready for review June 2, 2023 03:46
@ruslandoga ruslandoga requested a review from RobertJoonas June 2, 2023 03:46
@ruslandoga ruslandoga mentioned this pull request Jun 2, 2023
Merged
4 tasks
@ruslandoga ruslandoga changed the title remove custom_dimension_filter feature flag remove custom_dimension_filter feature and views_per_visit_metric flags Jun 2, 2023
@ukutaht ukutaht merged commit 131c99c into master Jun 7, 2023
@ukutaht ukutaht deleted the enable-custom-dimension-filter-for-all branch June 7, 2023 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RobertJoonas RobertJoonas RobertJoonas approved these changes

@aerosol aerosol aerosol approved these changes

@ukutaht ukutaht Awaiting requested review from ukutaht

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ruslandoga @ukutaht @aerosol @RobertJoonas