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

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7c4d93c
remove custom_dimension_filter feature flag
ruslandoga Jun 1, 2023
a246875
add changelog entry
ruslandoga Jun 2, 2023
bc7a616
fix shouldLinkToFilterModal removal
ruslandoga Jun 2, 2023
58d639a
remove views_per_visit_metric flag
ruslandoga Jun 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ All notable changes to this project will be documented in this file.
### Removed
- Remove Firewall plug and `IP_BLOCKLIST` environment variable
- Remove the ability to collapse the main graph plausible/analytics#2627
- Remove `custom_dimension_filter` feature flag plausible/analytics#2996

## v1.5.1 - 2022-12-06

Expand Down
57 changes: 22 additions & 35 deletions assets/js/dashboard/filters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,26 +55,22 @@ function renderDropdownFilter(site, history, [key, value], query) {
return (
<Menu.Item key={key}>
<div className="px-3 md:px-4 sm:py-2 py-3 text-sm leading-tight flex items-center justify-between" key={key + value}>
{shouldLinkToFilterModal(site, key) &&
<Link
title={`Edit filter: ${formattedFilters[key]}`}
to={{ pathname: `/${encodeURIComponent(site.domain)}/filter/${filterGroupForFilter(key)}`, search: window.location.search }}
className="group flex w-full justify-between items-center"
style={{ width: 'calc(100% - 1.5rem)' }}
>
<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>
}
{!shouldLinkToFilterModal(site, key) &&
<div
onClick={(e) => {e.preventDefault()}}
className="group flex w-full justify-between items-center"
style={{ width: 'calc(100% - 1.5rem)' }}
>
<span className="inline-block w-full truncate">{filterText(key, value, query)}</span>
</div>
}
<Link
title={`Edit filter: ${formattedFilters[key]}`}
to={{ pathname: `/${encodeURIComponent(site.domain)}/filter/${filterGroupForFilter(key)}`, search: window.location.search }}
className="group flex w-full justify-between items-center"
style={{ width: 'calc(100% - 1.5rem)' }}
>
<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

onClick={(e) => {e.preventDefault()}}
className="group flex w-full justify-between items-center"
style={{ width: 'calc(100% - 1.5rem)' }}
>
<span className="inline-block w-full truncate">{filterText(key, value, query)}</span>
</div>
<b title={`Remove filter: ${formattedFilters[key]}`} className="ml-2 cursor-pointer hover:text-indigo-700 dark:hover:text-indigo-500" onClick={() => removeFilter(key, history, query)}>
<XMarkIcon className="w-4 h-4" />
</b>
Expand All @@ -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.

}

function filterDropdownOption(site, option) {
return (
<Menu.Item key={option}>
Expand All @@ -110,7 +102,6 @@ function DropdownContent({ history, site, query, wrapped }) {

if (wrapped === 0 || addingFilter) {
return Object.keys(FILTER_GROUPS)
.filter((option) => option === 'props' ? site.flags.custom_dimension_filter : true)
.map((option) => filterDropdownOption(site, option))
}

Expand Down Expand Up @@ -219,16 +210,12 @@ class Filters extends React.Component {
renderListFilter(history, [key, value], query) {
return (
<span key={key} title={value} className="flex bg-white dark:bg-gray-800 text-gray-700 dark:text-gray-300 shadow text-sm rounded mr-2 items-center">
{shouldLinkToFilterModal(this.props.site, key) &&
<Link title={`Edit filter: ${formattedFilters[key]}`} className="flex w-full h-full items-center py-2 pl-3" to={{ pathname: `/${encodeURIComponent(this.props.site.domain)}/filter/${filterGroupForFilter(key)}`, search: window.location.search }}>
<span className="inline-block max-w-2xs md:max-w-xs truncate">{filterText(key, value, query)}</span>
</Link>
}
{!shouldLinkToFilterModal(this.props.site, key) &&
<div className="flex w-full h-full items-center py-2 pl-3">
<span className="inline-block max-w-2xs md:max-w-xs truncate">{filterText(key, value, query)}</span>
</div>
}
<Link title={`Edit filter: ${formattedFilters[key]}`} className="flex w-full h-full items-center py-2 pl-3" to={{ pathname: `/${encodeURIComponent(this.props.site.domain)}/filter/${filterGroupForFilter(key)}`, search: window.location.search }}>
<span className="inline-block max-w-2xs md:max-w-xs truncate">{filterText(key, value, query)}</span>
</Link>
<div className="flex w-full h-full items-center py-2 pl-3">
<span className="inline-block max-w-2xs md:max-w-xs truncate">{filterText(key, value, query)}</span>
</div>
<span title={`Remove filter: ${formattedFilters[key]}`} className="flex h-full w-full px-2 cursor-pointer hover:text-indigo-700 dark:hover:text-indigo-500 items-center" onClick={() => removeFilter(key, history, query)}>
<XMarkIcon className="w-4 h-4" />
</span>
Expand Down
1 change: 0 additions & 1 deletion lib/plausible_web/controllers/stats_controller.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 :)

}
end
Expand Down