Skip to content

remove custom_dimension_filter feature and views_per_visit_metric flags - #2996

Merged
ukutaht merged 4 commits into
masterfrom
enable-custom-dimension-filter-for-all
Jun 7, 2023
Merged

remove custom_dimension_filter feature and views_per_visit_metric flags#2996
ukutaht merged 4 commits into
masterfrom
enable-custom-dimension-filter-for-all

Conversation

@ruslandoga

@ruslandoga ruslandoga commented Jun 1, 2023

Copy link
Copy Markdown
Contributor

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

bundlemon Bot commented Jun 1, 2023

Copy link
Copy Markdown

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

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)

@ruslandoga ruslandoga Jun 1, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

ukutaht commented Jun 1, 2023

Copy link
Copy Markdown
Contributor

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

}

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

Copy link
Copy Markdown
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?

@ruslandoga ruslandoga Jun 1, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

aerosol commented Jun 1, 2023

Copy link
Copy Markdown
Member

@ruslandoga could you resolve the CHANGELOG conflict please?

@aerosol
aerosol requested a review from RobertJoonas June 1, 2023 13:41

@RobertJoonas RobertJoonas left a comment

Copy link
Copy Markdown
Contributor

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
Comment thread 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
Copy Markdown
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
Copy Markdown
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 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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants