From a75939c99e5ca3d08ecd53e2536f8a2ee6819483 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 12 Feb 2025 15:41:41 +0200 Subject: [PATCH 01/25] WIP --- assets/js/dashboard.tsx | 11 +- assets/js/dashboard/components/popover.tsx | 4 + assets/js/dashboard/dashboard-keybinds.tsx | 8 +- assets/js/dashboard/filtering/segments.ts | 64 +++ assets/js/dashboard/nav-menu/filter-menu.tsx | 64 +-- assets/js/dashboard/nav-menu/filters-bar.tsx | 22 +- .../nav-menu/nav-menu-components.tsx | 7 + .../query-periods/comparison-period-menu.tsx | 2 +- .../query-periods/move-period-arrows.tsx | 6 +- .../query-periods/query-period-menu.tsx | 2 +- .../query-periods/shared-menu-items.tsx | 18 - .../segments/searchable-segments-section.tsx | 270 +++++++++++ .../nav-menu/segments/segment-menu.tsx | 150 ++++++ assets/js/dashboard/nav-menu/top-bar.tsx | 4 + assets/js/dashboard/router.tsx | 13 +- .../dashboard/segments/segment-authorship.tsx | 55 +++ .../segments/segment-expanded-context.tsx | 106 +++++ .../js/dashboard/segments/segment-modals.tsx | 430 ++++++++++++++++++ .../segments/transient-segment-modals.tsx | 233 ++++++++++ assets/js/dashboard/site-context.test.tsx | 7 +- assets/js/dashboard/site-context.tsx | 8 +- .../js/dashboard/stats/modals/filter-modal.js | 11 +- assets/js/dashboard/user-context.tsx | 23 +- assets/js/dashboard/util/filters.js | 93 +++- assets/test-utils/app-context-providers.tsx | 5 +- .../controllers/stats_controller.ex | 15 + .../templates/stats/stats.html.heex | 5 + .../controllers/stats_controller_test.exs | 1 + 28 files changed, 1535 insertions(+), 102 deletions(-) create mode 100644 assets/js/dashboard/filtering/segments.ts create mode 100644 assets/js/dashboard/nav-menu/nav-menu-components.tsx create mode 100644 assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx create mode 100644 assets/js/dashboard/nav-menu/segments/segment-menu.tsx create mode 100644 assets/js/dashboard/segments/segment-authorship.tsx create mode 100644 assets/js/dashboard/segments/segment-expanded-context.tsx create mode 100644 assets/js/dashboard/segments/segment-modals.tsx create mode 100644 assets/js/dashboard/segments/transient-segment-modals.tsx diff --git a/assets/js/dashboard.tsx b/assets/js/dashboard.tsx index 227ba57c924b..ed227c6f8c58 100644 --- a/assets/js/dashboard.tsx +++ b/assets/js/dashboard.tsx @@ -57,8 +57,15 @@ if (container && container.dataset) { diff --git a/assets/js/dashboard/components/popover.tsx b/assets/js/dashboard/components/popover.tsx index 81a41811c6f4..5ceac56d88f3 100644 --- a/assets/js/dashboard/components/popover.tsx +++ b/assets/js/dashboard/components/popover.tsx @@ -55,6 +55,10 @@ const items = { roundedStartEnd: classNames( 'first-of-type:rounded-t-md', 'last-of-type:rounded-b-md' + ), + groupRoundedStartEnd: classNames( + 'group-first-of-type:rounded-t-md', + 'group-last-of-type:rounded-b-md' ) } } diff --git a/assets/js/dashboard/dashboard-keybinds.tsx b/assets/js/dashboard/dashboard-keybinds.tsx index f40b244df05f..6153b4c9f7ba 100644 --- a/assets/js/dashboard/dashboard-keybinds.tsx +++ b/assets/js/dashboard/dashboard-keybinds.tsx @@ -1,6 +1,7 @@ /* @format */ import React from 'react' import { NavigateKeybind } from './keybinding' +import { useSegmentExpandedContext } from './segments/segment-expanded-context' const ClearFiltersKeybind = () => ( ( ) export function DashboardKeybinds() { - return ( - <> - - - ) + const { modal } = useSegmentExpandedContext() + return modal === null && } diff --git a/assets/js/dashboard/filtering/segments.ts b/assets/js/dashboard/filtering/segments.ts new file mode 100644 index 000000000000..b626a4d6703e --- /dev/null +++ b/assets/js/dashboard/filtering/segments.ts @@ -0,0 +1,64 @@ +/** @format */ + +import { DashboardQuery, Filter } from '../query' +import { remapFromApiFilters } from '../util/filters' +import { plainFilterText } from '../util/filter-text' + +export enum SegmentType { + personal = 'personal', + site = 'site' +} + +export type SavedSegment = { + id: number + name: string + type: SegmentType + owner_id: number + inserted_at: string + updated_at: string +} + +export type SegmentData = { + filters: Filter[] + labels: Record +} + +const SEGMENT_LABEL_KEY_PREFIX = 'segment-' + +export function getFilterSegmentsByNameInsensitive( + search?: string +): (s: SavedSegment) => boolean { + return (s) => + search?.trim().length + ? s.name.toLowerCase().includes(search.trim().toLowerCase()) + : true +} + +export const getSegmentNamePlaceholder = (query: DashboardQuery) => + query.filters + .reduce( + (combinedName, filter) => + combinedName.length > 100 + ? combinedName + : `${combinedName}${combinedName.length ? ' and ' : ''}${plainFilterText(query, filter)}`, + '' + ) + .slice(0, 255) + +export function isSegmentIdLabelKey(labelKey: string): boolean { + return labelKey.startsWith(SEGMENT_LABEL_KEY_PREFIX) +} + +export function formatSegmentIdAsLabelKey(id: number): string { + return `${SEGMENT_LABEL_KEY_PREFIX}${id}` +} + +export const isSegmentFilter = (f: Filter): boolean => f[1] === 'segment' + +export const parseApiSegmentData = ({ + filters, + ...rest +}: SegmentData): SegmentData => ({ + filters: remapFromApiFilters(filters), + ...rest +}) diff --git a/assets/js/dashboard/nav-menu/filter-menu.tsx b/assets/js/dashboard/nav-menu/filter-menu.tsx index 6b88f6605a89..b383e066b25b 100644 --- a/assets/js/dashboard/nav-menu/filter-menu.tsx +++ b/assets/js/dashboard/nav-menu/filter-menu.tsx @@ -13,6 +13,7 @@ import { popover } from '../components/popover' import classNames from 'classnames' import { AppNavigationLink } from '../navigation/use-app-navigate' import { BlurMenuButtonOnEscape } from '../keybinding' +import { SearchableSegmentsSection } from './segments/searchable-segments-section' export function getFilterListItems({ propsAvailable @@ -50,6 +51,7 @@ const FilterMenuItems = ({ closeDropdown }: { closeDropdown: () => void }) => { const site = useSiteContext() const columns = useMemo(() => getFilterListItems(site), [site]) const buttonRef = useRef(null) + return ( <> @@ -75,37 +77,38 @@ const FilterMenuItems = ({ closeDropdown }: { closeDropdown: () => void }) => { )} > - {columns.map((filterGroups, index) => ( -
- {filterGroups.map(({ title, modals }) => ( -
-
- {title} +
+ {columns.map((filterGroups, index) => ( +
+ {filterGroups.map(({ title, modals }) => ( +
+
{title}
+ {modals + .filter((m) => !!m) + .map((modalKey) => ( + closeDropdown()} + key={modalKey} + path={filterRoute.path} + params={{ field: modalKey }} + search={(s) => s} + > + {formatFilterGroup(modalKey)} + + ))}
- {modals - .filter((m) => !!m) - .map((modalKey) => ( - closeDropdown()} - key={modalKey} - path={filterRoute.path} - params={{ field: modalKey }} - search={(s) => s} - > - {formatFilterGroup(modalKey)} - - ))} -
- ))} -
- ))} + ))} +
+ ))} +
+ @@ -117,3 +120,6 @@ export const FilterMenu = () => ( {({ close }) => } ) + +const titleClassName = + 'text-sm pb-1 px-4 pt-2 font-bold uppercase text-indigo-500 dark:text-indigo-400' diff --git a/assets/js/dashboard/nav-menu/filters-bar.tsx b/assets/js/dashboard/nav-menu/filters-bar.tsx index 27155605af04..1de54801a708 100644 --- a/assets/js/dashboard/nav-menu/filters-bar.tsx +++ b/assets/js/dashboard/nav-menu/filters-bar.tsx @@ -9,6 +9,7 @@ import { AppNavigationLink } from '../navigation/use-app-navigate' import { Popover, Transition } from '@headlessui/react' import { popover } from '../components/popover' import { BlurMenuButtonOnEscape } from '../keybinding' +import { useSegmentExpandedContext } from '../segments/segment-expanded-context' // Component structure is // `..[ filter (x) ]..[ filter (x) ]..[ three dot menu ]..` @@ -154,6 +155,7 @@ export const FiltersBar = ({ accessors }: FiltersBarProps) => { } const canClear = query.filters.length > 1 + const canSaveAsSegment = canClear return (
{ /> )} {canClear && } + {canSaveAsSegment && } @@ -234,7 +237,7 @@ const ClearAction = () => ( ({ ...search, @@ -245,3 +248,20 @@ const ClearAction = () => ( Clear all filters ) + +const SaveAsSegmentAction = () => { + const { expandedSegment, setModal } = useSegmentExpandedContext() + return !expandedSegment ? ( + s} + onClick={() => setModal('create')} + state={{ expandedSegment: null }} + > + Save as segment + + ) : null +} diff --git a/assets/js/dashboard/nav-menu/nav-menu-components.tsx b/assets/js/dashboard/nav-menu/nav-menu-components.tsx new file mode 100644 index 000000000000..71607ab14706 --- /dev/null +++ b/assets/js/dashboard/nav-menu/nav-menu-components.tsx @@ -0,0 +1,7 @@ +/** @format */ + +import React from 'react' + +export const MenuSeparator = () => ( +
+) diff --git a/assets/js/dashboard/nav-menu/query-periods/comparison-period-menu.tsx b/assets/js/dashboard/nav-menu/query-periods/comparison-period-menu.tsx index 1cdb7143b2f8..875a8a365df0 100644 --- a/assets/js/dashboard/nav-menu/query-periods/comparison-period-menu.tsx +++ b/assets/js/dashboard/nav-menu/query-periods/comparison-period-menu.tsx @@ -26,12 +26,12 @@ import { DateMenuChevron, PopoverMenuProps, linkClassName, - MenuSeparator, CalendarPanel, hiddenCalendarButtonClassName } from './shared-menu-items' import { DateRangeCalendar } from './date-range-calendar' import { formatISO, nowForSite } from '../../util/date' +import { MenuSeparator } from '../nav-menu-components' export const ComparisonPeriodMenuItems = ({ closeDropdown, diff --git a/assets/js/dashboard/nav-menu/query-periods/move-period-arrows.tsx b/assets/js/dashboard/nav-menu/query-periods/move-period-arrows.tsx index 3fda3fe0138c..7df76136994d 100644 --- a/assets/js/dashboard/nav-menu/query-periods/move-period-arrows.tsx +++ b/assets/js/dashboard/nav-menu/query-periods/move-period-arrows.tsx @@ -82,14 +82,14 @@ export function MovePeriodArrows({ className }: { className?: string }) { return (
( ) -export const MenuSeparator = () => ( -
-) - export interface PopoverMenuProps { closeDropdown: () => void calendarButtonRef: RefObject } -export enum DropdownState { - CLOSED = 'CLOSED', - MENU = 'MENU', - CALENDAR = 'CALENDAR' -} - -export interface DropdownWithCalendarState { - closeDropdown: () => void - toggleDropdown: (mode: 'menu' | 'calendar') => void - dropdownState: DropdownState - buttonRef: RefObject - toggleCalendar: () => void -} - const calendarPositionClassName = '*:!top-auto *:!right-0 *:!absolute' type CalendarPanelProps = { diff --git a/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx b/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx new file mode 100644 index 000000000000..2451dedf3e72 --- /dev/null +++ b/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx @@ -0,0 +1,270 @@ +/** @format */ + +import React, { useCallback, useEffect, useMemo, useState } from 'react' +import { useQueryContext } from '../../query-context' +import { useSiteContext } from '../../site-context' +import { + formatSegmentIdAsLabelKey, + getFilterSegmentsByNameInsensitive, + isSegmentFilter, + parseApiSegmentData, + SavedSegment, + SegmentData +} from '../../filtering/segments' +import { QueryFunction, useQuery, useQueryClient } from '@tanstack/react-query' +import { cleanLabels } from '../../util/filters' +import classNames from 'classnames' +import { Tooltip } from '../../util/tooltip' +import { useSegmentExpandedContext } from '../../segments/segment-expanded-context' +import { SegmentAuthorship } from '../../segments/segment-authorship' +import { SearchInput } from '../../components/search-input' +import { EllipsisHorizontalIcon } from '@heroicons/react/24/solid' +import { popover } from '../../components/popover' +import { AppNavigationLink } from '../../navigation/use-app-navigate' +import { MenuSeparator } from '../nav-menu-components' + +const useSegmentsListQuery = () => { + const site = useSiteContext() + const appliedSegmentIds = [] as number[] + return useQuery({ + queryKey: ['segments'], + placeholderData: (previousData) => previousData, + queryFn: async () => { + const response = await fetch( + `/api/${encodeURIComponent(site.domain)}/segments`, + { + method: 'GET', + headers: { + 'content-type': 'application/json', + accept: 'application/json' + } + } + ).then((res): Promise => res.json()) + + return response.sort( + (a, b) => + appliedSegmentIds.findIndex((id) => id === b.id) - + appliedSegmentIds.findIndex((id) => id === a.id) + ) + } + }) +} + +const linkClassName = classNames( + popover.items.classNames.navigationLink, + popover.items.classNames.selectedOption, + popover.items.classNames.hoverLink, + popover.items.classNames.groupRoundedStartEnd +) + +const initialSliceLength = 5 + +export const SearchableSegmentsSection = ({ + closeList +}: { + closeList: () => void +}) => { + const { query } = useQueryContext() + const segmentFilter = query.filters.find(isSegmentFilter) + const appliedSegmentIds = (segmentFilter ? segmentFilter[2] : []) as number[] + + const { data } = useSegmentsListQuery() + const [searchValue, setSearch] = useState() + const [showAll, setShowAll] = useState(false) + + const searching = !searchValue?.trim().length + + useEffect(() => { + setShowAll(false) + }, [searching]) + + const filteredData = data?.filter( + getFilterSegmentsByNameInsensitive(searchValue) + ) + + const showableSlice = showAll + ? filteredData + : filteredData?.slice(0, initialSliceLength) + + const { expandedSegment } = useSegmentExpandedContext() + + if (expandedSegment) { + return null + } + + return ( + <> + {!!data?.length && ( + <> + +
+
+ Segments +
+ {data.length > initialSliceLength && ( + + )} +
+ + {showableSlice!.map((s) => { + return ( + +
{s.name}
+
+ { + { + personal: 'Personal segment', + site: 'Site segment' + }[s.type] + } +
+ + +
+ } + > + + + ) + })} + {!!filteredData?.length && + !!showableSlice?.length && + filteredData?.length > showableSlice?.length && + showAll === false && ( + + s} + onClick={() => setShowAll(true)} + > + {`Show ${filteredData.length - showableSlice.length} more`} + + + + )} + + )} + + ) +} + +export const useSegmentPrefetch = ({ id }: { id: string }) => { + const site = useSiteContext() + const queryClient = useQueryClient() + const queryKey = useMemo(() => ['segments', id] as const, [id]) + + const getSegmentFn: QueryFunction< + SavedSegment & { segment_data: SegmentData }, + typeof queryKey + > = useCallback( + async ({ queryKey: [_, id] }) => { + const res = await fetch( + `/api/${encodeURIComponent(site.domain)}/segments/${id}`, + { + method: 'GET', + headers: { + 'content-type': 'application/json', + accept: 'application/json' + } + } + ) + const d = await res.json() + return { + ...d, + segment_data: parseApiSegmentData(d.segment_data) + } + }, + [site] + ) + + const getSegment = useQuery({ + enabled: false, + queryKey: queryKey, + queryFn: getSegmentFn + }) + + const prefetchSegment = useCallback( + () => + queryClient.prefetchQuery({ + queryKey, + queryFn: getSegmentFn, + staleTime: 120_000 + }), + [queryClient, getSegmentFn, queryKey] + ) + + const fetchSegment = useCallback( + () => + queryClient.fetchQuery({ + queryKey, + queryFn: getSegmentFn + }), + [queryClient, getSegmentFn, queryKey] + ) + + return { prefetchSegment, data: getSegment.data, fetchSegment } +} + +const SegmentLink = ({ + id, + name, + // type, + // owner_id, + appliedSegmentIds, + closeList +}: SavedSegment & { appliedSegmentIds: number[]; closeList: () => void }) => { + const { query } = useQueryContext() + + const { prefetchSegment } = useSegmentPrefetch({ id: String(id) }) + + return ( + { + const otherFilters = query.filters.filter((f) => !isSegmentFilter(f)) + const updatedSegmentIds = appliedSegmentIds.includes(id) ? [] : [id] + if (!updatedSegmentIds.length) { + return { + ...search, + filters: otherFilters, + labels: cleanLabels(otherFilters, query.labels) + } + } + + const updatedFilters = [ + ['is', 'segment', updatedSegmentIds], + ...otherFilters + ] + + return { + ...search, + filters: updatedFilters, + labels: cleanLabels(updatedFilters, query.labels, 'segment', { + [formatSegmentIdAsLabelKey(id)]: name + }) + } + }} + > +
{name}
+ + ) +} diff --git a/assets/js/dashboard/nav-menu/segments/segment-menu.tsx b/assets/js/dashboard/nav-menu/segments/segment-menu.tsx new file mode 100644 index 000000000000..df35c0cc88a5 --- /dev/null +++ b/assets/js/dashboard/nav-menu/segments/segment-menu.tsx @@ -0,0 +1,150 @@ +/** @format */ + +import React from 'react' +import classNames from 'classnames' +import { useSegmentExpandedContext } from '../../segments/segment-expanded-context' +import { Popover, Transition } from '@headlessui/react' +import { popover } from '../../components/popover' +import { AppNavigationLink } from '../../navigation/use-app-navigate' +import { + CheckIcon, + Square2StackIcon, + TrashIcon, + XMarkIcon +} from '@heroicons/react/24/outline' +import { ChevronDownIcon } from '@heroicons/react/20/solid' + +const linkClassName = classNames( + popover.items.classNames.navigationLink, + popover.items.classNames.selectedOption, + popover.items.classNames.hoverLink, + popover.items.classNames.roundedStartEnd +) +const buttonClassName = classNames( + 'text-white font-medium bg-indigo-600 hover:bg-indigo-700' +) + +export const SegmentMenu = () => { + const { expandedSegment, setModal } = useSegmentExpandedContext() + + if (!expandedSegment) { + return null + } + + return ( +
+ s} + state={{ expandedSegment }} + onClick={() => { + setModal('update') + }} + > + Update segment + + + {({ close: closeDropdown }) => ( + <> + + + + + s} + state={{ expandedSegment }} + onClick={() => { + closeDropdown() + setModal('update') + }} + > +
+ + Update segment +
+ + s} + state={{ expandedSegment }} + onClick={() => { + closeDropdown() + setModal('create') + }} + > +
+ + + Save as a new segment + +
+ + s} + state={{ expandedSegment }} + onClick={() => { + closeDropdown() + setModal('delete') + }} + > +
+ + Delete segment +
+ + ({ + ...s, + filters: [], + labels: {} + // filters: [[['is', 'segment', [expandedSegment.id]]]], + // labels: { + // [formatSegmentIdAsLabelKey(expandedSegment.id)]: + // expandedSegment.name + // } + })} + state={{ expandedSegment: null }} + onClick={closeDropdown} + > +
+ + + Close without saving + +
+ + + + + )} + +
+ ) +} diff --git a/assets/js/dashboard/nav-menu/top-bar.tsx b/assets/js/dashboard/nav-menu/top-bar.tsx index f6b6e43dc413..574d469fed99 100644 --- a/assets/js/dashboard/nav-menu/top-bar.tsx +++ b/assets/js/dashboard/nav-menu/top-bar.tsx @@ -11,6 +11,8 @@ import { useInView } from 'react-intersection-observer' import { FilterMenu } from './filter-menu' import { FiltersBar } from './filters-bar' import { QueryPeriodsPicker } from './query-periods/query-periods-picker' +import { SegmentMenu } from './segments/segment-menu' +import { TransientSegmentModals } from '../segments/transient-segment-modals' interface TopBarProps { showCurrentVisitors: boolean @@ -83,8 +85,10 @@ function TopBarInner({ showCurrentVisitors }: TopBarProps) { />
+ + {}} />
) : ( diff --git a/assets/js/dashboard/router.tsx b/assets/js/dashboard/router.tsx index af2a67505ea0..d208df091316 100644 --- a/assets/js/dashboard/router.tsx +++ b/assets/js/dashboard/router.tsx @@ -27,6 +27,7 @@ import FilterModal from './stats/modals/filter-modal' import QueryContextProvider from './query-context' import { DashboardKeybinds } from './dashboard-keybinds' import LastLoadContextProvider from './last-load-context' +import SegmentExpandedContextProvider from './segments/segment-expanded-context' const queryClient = new QueryClient({ defaultOptions: { @@ -40,11 +41,13 @@ function DashboardElement() { return ( - - - {/** render any children of the root route below */} - - + + + + {/** render any children of the root route below */} + + + ) diff --git a/assets/js/dashboard/segments/segment-authorship.tsx b/assets/js/dashboard/segments/segment-authorship.tsx new file mode 100644 index 000000000000..d19479033285 --- /dev/null +++ b/assets/js/dashboard/segments/segment-authorship.tsx @@ -0,0 +1,55 @@ +/** @format */ + +import React from 'react' +import { SavedSegment } from '../filtering/segments' +import { PlausibleSite, useSiteContext } from '../site-context' +import { formatDayShort, parseUTCDate } from '../util/date' + +const getAuthorLabel = ( + site: Pick, + owner_id: number +) => { + if (!site.members) { + return '' + } + + if (!owner_id || !site.members[owner_id]) { + return '(Removed User)' + } + + // if (owner_id === user.id) { + // return 'You' + // } + + return site.members[owner_id] +} + +export const SegmentAuthorship = ({ + className, + owner_id, + inserted_at, + updated_at +}: SavedSegment & { + className?: string +}) => { + const site = useSiteContext() + + const authorLabel = getAuthorLabel(site, owner_id) + + const showUpdatedAt = updated_at !== inserted_at + + return ( +
+
+ {`Created at ${formatDayShort(parseUTCDate(inserted_at))}`} + {!showUpdatedAt && !!authorLabel && ` by ${authorLabel}`} +
+ {showUpdatedAt && ( +
+ {`Last updated at ${formatDayShort(parseUTCDate(updated_at))}`} + {!!authorLabel && ` by ${authorLabel}`} +
+ )} +
+ ) +} diff --git a/assets/js/dashboard/segments/segment-expanded-context.tsx b/assets/js/dashboard/segments/segment-expanded-context.tsx new file mode 100644 index 000000000000..2d33203a3853 --- /dev/null +++ b/assets/js/dashboard/segments/segment-expanded-context.tsx @@ -0,0 +1,106 @@ +/* @format */ +import React, { + createContext, + ReactNode, + useContext, + useEffect, + useLayoutEffect, + useState +} from 'react' +import { SavedSegment, SegmentData } from '../filtering/segments' +import { useLocation } from 'react-router-dom' +import { useAppNavigate } from '../navigation/use-app-navigate' +import { useQueryContext } from '../query-context' + +export type SegmentExpandedState = { + expandedSegment: (SavedSegment & { segment_data: SegmentData }) | null +} + +export type SegmentModalState = null | 'create' | 'update' | 'delete' + +const segmentExpandedContextDefaultValue: SegmentExpandedState & { + modal: SegmentModalState + setModal: (modal: SegmentModalState) => void +} = { + expandedSegment: null, + modal: null, + setModal: () => {} +} + +const SegmentExpandedContext = createContext< + typeof segmentExpandedContextDefaultValue +>(segmentExpandedContextDefaultValue) + +export const useSegmentExpandedContext = () => { + return useContext(SegmentExpandedContext) +} + +export default function SegmentExpandedContextProvider({ + children +}: { + children: ReactNode +}) { + const location = useLocation() + const [expandedSegmentState, setState] = useState({ + expandedSegment: segmentExpandedContextDefaultValue.expandedSegment + }) + const [modal, setModal] = useState( + segmentExpandedContextDefaultValue.modal + ) + + const { query } = useQueryContext() + const navigate = useAppNavigate() + + useLayoutEffect(() => { + // copy location.state to state + if (location.state?.expandedSegment) { + setState({ + expandedSegment: location.state.expandedSegment + }) + } + if (location.state?.expandedSegment === null) { + setState({ + expandedSegment: segmentExpandedContextDefaultValue.expandedSegment + }) + // setModal(segmentExpandedContextDefaultValue.modal) + } + }, [location.state]) + + useLayoutEffect(() => { + // clear edit mode on clearing all filters + if (!query.filters.length && expandedSegmentState.expandedSegment) { + navigate({ + search: (s) => s, + state: { + expandedSegment: segmentExpandedContextDefaultValue.expandedSegment + }, + replace: true + }) + // overwrite undefined locationState with current expandedSegment, to handle Back navigation correctly + } else if (location.state?.expandedSegment === undefined) { + navigate({ + search: (s) => s, + state: { + expandedSegment: expandedSegmentState.expandedSegment + }, + replace: true + }) + } + }, [query, expandedSegmentState.expandedSegment, navigate, location.state]) + + useEffect(() => { + console.log({ modal }) + }, [modal]) + + return ( + + {children} + + ) +} diff --git a/assets/js/dashboard/segments/segment-modals.tsx b/assets/js/dashboard/segments/segment-modals.tsx new file mode 100644 index 000000000000..b07f05358b08 --- /dev/null +++ b/assets/js/dashboard/segments/segment-modals.tsx @@ -0,0 +1,430 @@ +/** @format */ + +import React, { Fragment, ReactNode, useLayoutEffect, useState } from 'react' +import ModalWithRouting from '../stats/modals/modal' +import { + isSegmentFilter, + SavedSegment, + SegmentData, + SegmentType +} from '../filtering/segments' +import { useSegmentPrefetch } from '../nav-menu/segments/searchable-segments-section' +import { useQueryContext } from '../query-context' +import { AppNavigationLink } from '../navigation/use-app-navigate' +import { cleanLabels } from '../util/filters' +import { plainFilterText, styledFilterText } from '../util/filter-text' +import { rootRoute } from '../router' +import { FilterPillsList } from '../nav-menu/filter-pills-list' +import classNames from 'classnames' +import { SegmentAuthorship } from './segment-authorship' +import { ExclamationTriangleIcon, TrashIcon } from '@heroicons/react/24/outline' + +interface SharedSegmentModalProps { + onClose: () => void + siteSegmentsAvailable: boolean + userCanSelectSiteSegment: boolean + namePlaceholder: string +} + +const primaryNeutralButtonClassName = 'button' + +const primaryNegativeButtonClassName = classNames( + 'button', + 'items-center !bg-red-500 dark:!bg-red-500 hover:!bg-red-600 dark:hover:!bg-red-700 whitespace-nowrap' +) + +const secondaryButtonClassName = classNames( + 'button', + 'border border-gray-300 dark:border-gray-500 text-gray-700 dark:text-gray-300 !bg-transparent hover:!bg-gray-100 dark:hover:!bg-gray-850' +) + +const SegmentActionModal = ({ + children, + onClose +}: { + children: ReactNode + onClose: () => void +}) => { + return ( + +
{children}
+ + ) +} + +export const CreateSegmentModal = ({ + segment, + onClose, + onSave, + siteSegmentsAvailable: siteSegmentsAvailable, + userCanSelectSiteSegment, + namePlaceholder +}: SharedSegmentModalProps & { + segment?: SavedSegment + onSave: (input: Pick) => void +}) => { + const defaultName = segment?.name ? `Copy of ${segment.name}` : '' + const [name, setName] = useState(defaultName) + const defaultType = + segment?.type === SegmentType.site && + siteSegmentsAvailable && + userCanSelectSiteSegment + ? SegmentType.site + : SegmentType.personal + + const [type, setType] = useState(defaultType) + + return ( + + Create segment + + + + + + + + ) +} + +export const DeleteSegmentModal = ({ + onClose, + onSave, + segment +}: { + onClose: () => void + onSave: (input: Pick) => void + segment: SavedSegment & { segment_data?: SegmentData } +}) => { + return ( + + + { + { personal: 'Delete personal segment', site: 'Delete site segment' }[ + segment.type + ] + } + {` "${segment.name}"?`} + + {segment.segment_data && ( + + )} + + + + + + + ) +} + +const FormTitle = ({ children }: { children?: ReactNode }) => ( +

{children}

+) + +const ButtonsRow = ({ + className, + children +}: { + className?: string + children?: ReactNode +}) => ( +
+ {children} +
+) + +const SegmentNameInput = ({ + namePlaceholder, + value, + onChange +}: { + namePlaceholder: string + value: string + onChange: (value: string) => void +}) => { + return ( + <> + + onChange(e.target.value)} + placeholder={namePlaceholder} + id="name" + className="block mt-2 p-2 w-full dark:bg-gray-900 dark:text-gray-300 rounded-md shadow-sm border border-gray-300 dark:border-gray-700 focus-within:border-indigo-500 focus-within:ring-1 focus-within:ring-indigo-500" + /> + + ) +} + +const SegmentTypeInput = ({ + value, + onChange, + siteSegmentsAvailable, + userCanSelectSiteSegment +}: { + value: SegmentType + onChange: (value: SegmentType) => void +} & Pick< + SharedSegmentModalProps, + 'siteSegmentsAvailable' | 'userCanSelectSiteSegment' +>) => { + const options = [ + { + type: SegmentType.personal, + name: 'Personal segment', + description: 'Visible only to you', + disabled: false + }, + { + type: SegmentType.site, + name: 'Site segment', + description: 'Visible to others on the site', + disabled: !userCanSelectSiteSegment || !siteSegmentsAvailable, + disabledReason: !userCanSelectSiteSegment ? ( + <> + {"You don't have enough permissions to change segment to this type."} + + ) : !siteSegmentsAvailable ? ( + <> + {`Your account does not have access to site segments. To use this + segment type, `} + + please upgrade your subscription + + . + + ) : null + } + ] + + return ( +
+ {options.map(({ type, name, description, disabled, disabledReason }) => ( +
+
+ onChange(type)} + className="mt-4 w-4 h-4 text-indigo-600 bg-gray-100 border-gray-300 focus:ring-2 focus:ring-offset-2 focus:ring-indigo-500 dark:border-gray-600" + // disabled={disabled} + /> + +
+ {value === type && disabled && !!disabledReason && ( +
+ +
{disabledReason}
+
+ )} +
+ ))} +
+ ) +} + +export const UpdateSegmentModal = ({ + onClose, + onSave, + segment, + siteSegmentsAvailable, + userCanSelectSiteSegment, + namePlaceholder +}: SharedSegmentModalProps & { + onSave: (input: Pick) => void + segment: SavedSegment +}) => { + const [name, setName] = useState(segment.name) + const [type, setType] = useState(segment.type) + + return ( + + Update segment + + + + + + + + ) +} + +const FiltersInSegment = ({ segment_data }: { segment_data: SegmentData }) => { + return ( + <> +

Filters in segment

+
+ ({ + plainText: plainFilterText({ labels: segment_data.labels }, filter), + children: styledFilterText({ labels: segment_data.labels }, filter), + interactive: false + }))} + /> +
+ + ) +} + +export const SegmentModal = () => { + const { query } = useQueryContext() + const segmentsFilter = query.filters.find(isSegmentFilter)! + const { data, fetchSegment } = useSegmentPrefetch({ + id: String(segmentsFilter[2][0]) + }) + + useLayoutEffect(() => { + fetchSegment() + }, [fetchSegment]) + + return ( + +
+
+
+

+ {data ? data.name : 'Segment'} +

+
+
+ {data?.segment_data ? ( +
+ { + { + [SegmentType.personal]: 'Personal segment', + [SegmentType.site]: 'Site segment' + }[data.type] + } +
+ ) : null} +
+ {data?.segment_data ? ( + <> + + + +
+ + ({ + ...s, + filters: data.segment_data.filters, + labels: data.segment_data.labels + })} + state={{ + expandedSegment: data, + modal: null + }} + > + Edit segment + + + { + const nonSegmentFilters = query.filters.filter( + (f) => !isSegmentFilter(f) + ) + return { + ...s, + filters: nonSegmentFilters, + labels: cleanLabels( + nonSegmentFilters, + query.labels, + 'segment', + {} + ) + } + }} + > + + Remove filter + + +
+ + ) : ( +
+ )} +
+ + ) +} diff --git a/assets/js/dashboard/segments/transient-segment-modals.tsx b/assets/js/dashboard/segments/transient-segment-modals.tsx new file mode 100644 index 000000000000..53a8ca92adf8 --- /dev/null +++ b/assets/js/dashboard/segments/transient-segment-modals.tsx @@ -0,0 +1,233 @@ +/** @format */ + +import React from 'react' +import { useSegmentExpandedContext } from './segment-expanded-context' +import { + CreateSegmentModal, + DeleteSegmentModal, + UpdateSegmentModal +} from './segment-modals' +import { + formatSegmentIdAsLabelKey, + getSegmentNamePlaceholder, + parseApiSegmentData, + SavedSegment, + SegmentData +} from '../filtering/segments' +import { useMutation, useQueryClient } from '@tanstack/react-query' +import { useSiteContext } from '../site-context' +import { cleanLabels, remapToApiFilters } from '../util/filters' +import { useAppNavigate } from '../navigation/use-app-navigate' +import { useQueryContext } from '../query-context' +import { Role, useUserContext } from '../user-context' + +export const TransientSegmentModals = (_p: { closeList: () => void }) => { + const navigate = useAppNavigate() + const queryClient = useQueryClient() + const site = useSiteContext() + const { expandedSegment, modal, setModal } = useSegmentExpandedContext() + const { query } = useQueryContext() + const user = useUserContext() + + const patchSegment = useMutation({ + mutationFn: ({ + id, + name, + type, + segment_data + }: Pick & + Partial> & { + segment_data?: SegmentData + }) => { + return fetch(`/api/${encodeURIComponent(site.domain)}/segments/${id}`, { + method: 'PATCH', + body: JSON.stringify({ + name, + type, + ...(segment_data && { + segment_data: { + filters: remapToApiFilters(segment_data.filters), + labels: cleanLabels(segment_data.filters, segment_data.labels) + } + }) + }), + headers: { + 'content-type': 'application/json', + accept: 'application/json' + } + }) + .then((res) => res.json()) + .then((d) => ({ + ...d, + segment_data: parseApiSegmentData(d.segment_data) + })) + }, + onSuccess: async (d) => { + queryClient.invalidateQueries({ queryKey: ['segments'] }) + navigate({ + search: (search) => { + const filters = [['is', 'segment', [d.id]]] + const labels = cleanLabels(filters, {}, 'segment', { + [formatSegmentIdAsLabelKey(d.id)]: d.name + }) + return { + ...search, + filters, + labels + } + }, + state: { + expandedSegment: null + } + // replace: true + }) + setModal(null) + } + }) + + const createSegment = useMutation({ + mutationFn: ({ + name, + type, + segment_data + }: { + name: string + type: 'personal' | 'site' + segment_data: SegmentData + }) => { + return fetch(`/api/${encodeURIComponent(site.domain)}/segments`, { + method: 'POST', + body: JSON.stringify({ + name, + type, + segment_data: { + filters: remapToApiFilters(segment_data.filters), + labels: cleanLabels(segment_data.filters, segment_data.labels) + } + }), + headers: { 'content-type': 'application/json' } + }) + .then((res) => res.json()) + .then((d) => ({ + ...d, + segment_data: parseApiSegmentData(d.segment_data) + })) + }, + onSuccess: async (d) => { + navigate({ + search: (search) => { + queryClient.invalidateQueries({ queryKey: ['segments'] }) + const filters = [['is', 'segment', [d.id]]] + const labels = cleanLabels(filters, {}, 'segment', { + [formatSegmentIdAsLabelKey(d.id)]: d.name + }) + return { + ...search, + filters, + labels + } + }, + state: { + expandedSegment: null + } + // replace: true + }) + setModal(null) + } + }) + + const deleteSegment = useMutation({ + mutationFn: (data: Pick) => { + return fetch( + `/api/${encodeURIComponent(site.domain)}/segments/${data.id}`, + { + method: 'DELETE' + } + ) + .then((res) => res.json()) + .then((d) => ({ + ...d, + segment_data: parseApiSegmentData(d.segment_data) + })) + }, + onSuccess: (_d): void => { + queryClient.invalidateQueries({ queryKey: ['segments'] }) + navigate({ + search: (s) => { + return { + ...s, + filters: null, + labels: null + } + }, + state: { + expandedSegment: null + } + // replace: true + }) + setModal(null) + } + }) + + if (!user.loggedIn || !site.flags.saved_segments) { + return null + } + + const userCanSelectSiteSegment = [ + Role.admin, + Role.owner, + Role.editor, + 'super_admin' + ].includes(user.role) + + return ( + <> + {modal === 'update' && expandedSegment && ( + setModal(null)} + onSave={({ id, name, type }) => + patchSegment.mutate({ + id, + name, + type, + segment_data: { + filters: query.filters, + labels: query.labels + } + }) + } + /> + )} + {modal === 'create' && ( + setModal(null)} + onSave={({ name, type }) => + createSegment.mutate({ + name, + type, + segment_data: { + filters: query.filters, + labels: query.labels + } + }) + } + /> + )} + {modal === 'delete' && expandedSegment && ( + setModal(null)} + onSave={({ id }) => deleteSegment.mutate({ id })} + /> + )} + + ) +} diff --git a/assets/js/dashboard/site-context.test.tsx b/assets/js/dashboard/site-context.test.tsx index 9c4109bee1e8..1ad2701104c4 100644 --- a/assets/js/dashboard/site-context.test.tsx +++ b/assets/js/dashboard/site-context.test.tsx @@ -16,6 +16,7 @@ describe('parseSiteFromDataset', () => { data-funnels-opted-out="false" data-props-opted-out="false" data-funnels-available="true" + data-site-segments-available="true" data-props-available="true" data-revenue-goals='[{"currency":"USD","display_name":"Purchase"}]' data-funnels='[{"id":1,"name":"From homepage to login","steps_count":3}]' @@ -27,6 +28,8 @@ describe('parseSiteFromDataset', () => { data-embedded="" data-is-dbip="false" data-current-user-role="owner" + data-current-user-id="1" + data-members='{"1":"Test User"}' data-flags="{}" data-valid-intervals-by-period='{"12mo":["day","week","month"],"30d":["day","week"],"6mo":["day","week","month"],"7d":["hour","day"],"all":["week","month"],"custom":["day","week","month"],"day":["minute","hour"],"month":["day","week"],"realtime":["minute"],"year":["day","week","month"]}' {...attrs} @@ -41,6 +44,7 @@ describe('parseSiteFromDataset', () => { propsOptedOut: false, funnelsAvailable: true, propsAvailable: true, + siteSegmentsAvailable: true, revenueGoals: [{ currency: 'USD', display_name: 'Purchase' }], funnels: [{ id: 1, name: 'From homepage to login', steps_count: 3 }], hasProps: true, @@ -63,7 +67,8 @@ describe('parseSiteFromDataset', () => { realtime: ['minute'], year: ['day', 'week', 'month'] }, - shared: false + shared: false, + members: { '1': 'Test User' } } it('parses from dom string map correctly', () => { diff --git a/assets/js/dashboard/site-context.tsx b/assets/js/dashboard/site-context.tsx index 215bcba48db3..fbd03c3f5fde 100644 --- a/assets/js/dashboard/site-context.tsx +++ b/assets/js/dashboard/site-context.tsx @@ -10,6 +10,7 @@ export function parseSiteFromDataset(dataset: DOMStringMap): PlausibleSite { scrollDepthVisible: dataset.scrollDepthVisible === 'true', funnelsAvailable: dataset.funnelsAvailable === 'true', propsAvailable: dataset.propsAvailable === 'true', + siteSegmentsAvailable: dataset.siteSegmentsAvailable === 'true', conversionsOptedOut: dataset.conversionsOptedOut === 'true', funnelsOptedOut: dataset.funnelsOptedOut === 'true', propsOptedOut: dataset.propsOptedOut === 'true', @@ -22,7 +23,8 @@ export function parseSiteFromDataset(dataset: DOMStringMap): PlausibleSite { isDbip: dataset.isDbip === 'true', flags: JSON.parse(dataset.flags!), validIntervalsByPeriod: JSON.parse(dataset.validIntervalsByPeriod!), - shared: !!dataset.sharedLinkAuth + shared: !!dataset.sharedLinkAuth, + members: JSON.parse(dataset.members!) } } @@ -40,6 +42,7 @@ const siteContextDefaultValue = { scrollDepthVisible: false, funnelsAvailable: false, propsAvailable: false, + siteSegmentsAvailable: false, conversionsOptedOut: false, funnelsOptedOut: false, propsOptedOut: false, @@ -54,7 +57,8 @@ const siteContextDefaultValue = { isDbip: false, flags: {} as FeatureFlags, validIntervalsByPeriod: {} as Record>, - shared: false + shared: false, + members: null as null | Record } export type PlausibleSite = typeof siteContextDefaultValue diff --git a/assets/js/dashboard/stats/modals/filter-modal.js b/assets/js/dashboard/stats/modals/filter-modal.js index 6762b3bee36d..2dffc4d7e6fa 100644 --- a/assets/js/dashboard/stats/modals/filter-modal.js +++ b/assets/js/dashboard/stats/modals/filter-modal.js @@ -9,6 +9,8 @@ import { isModifierPressed, isTyping } from '../../keybinding'; import FilterModalGroup from "./filter-modal-group"; import { rootRoute } from '../../router'; import { useAppNavigate } from '../../navigation/use-app-navigate'; +import { SegmentModal } from '../../segments/segment-modals'; +import { TrashIcon } from '@heroicons/react/24/outline'; function partitionFilters(modalType, filters) { const otherFilters = [] @@ -171,18 +173,18 @@ class FilterModal extends React.Component { className="button" disabled={this.isDisabled()} > - Apply Filter + Apply filter {this.state.hasRelevantFilters && ( )} @@ -199,6 +201,9 @@ export default function FilterModalWithRouter(props) { const { field } = useParams() const { query } = useQueryContext() const site = useSiteContext() + if (field === 'segment') { + return + } return ( (userContextDefaultValue) export const useUserContext = () => { return useContext(UserContext) } export default function UserContextProvider({ - role, - loggedIn, + user, children }: { - role: Role - loggedIn: boolean + user: UserContextValue children: ReactNode }) { - return ( - - {children} - - ) + return {children} } diff --git a/assets/js/dashboard/util/filters.js b/assets/js/dashboard/util/filters.js index d59b5f7378ba..dc3986a7db7f 100644 --- a/assets/js/dashboard/util/filters.js +++ b/assets/js/dashboard/util/filters.js @@ -1,6 +1,7 @@ /** @format */ import * as api from '../api' +import { formatSegmentIdAsLabelKey } from '../filtering/segments' export const FILTER_MODAL_TO_FILTER_GROUP = { page: ['page', 'entry_page', 'exit_page'], @@ -12,7 +13,8 @@ export const FILTER_MODAL_TO_FILTER_GROUP = { utm: ['utm_medium', 'utm_source', 'utm_campaign', 'utm_term', 'utm_content'], goal: ['goal'], props: ['props'], - hostname: ['hostname'] + hostname: ['hostname'], + segment: ['segment'] } export const FILTER_GROUP_TO_MODAL_TYPE = Object.fromEntries( @@ -65,9 +67,13 @@ export function isFreeChoiceFilterOperation(operation) { export function getLabel(labels, filterKey, value) { if (['country', 'region', 'city'].includes(filterKey)) { return labels[value] - } else { - return value } + + if (filterKey === 'segment') { + return labels[formatSegmentIdAsLabelKey(value)] + } + + return value } export function getPropertyKeyFromFilterKey(filterKey) { @@ -135,11 +141,18 @@ export function formatFilterGroup(filterGroup) { export function cleanLabels(filters, labels, mergedFilterKey, mergedLabels) { const filteredBy = Object.fromEntries( filters - .flatMap(([_operation, filterKey, clauses]) => - ['country', 'region', 'city'].includes(filterKey) ? clauses : [] - ) + .flatMap(([_operation, filterKey, clauses]) => { + if (filterKey === 'segment') { + return clauses.map(formatSegmentIdAsLabelKey) + } + if (['country', 'region', 'city'].includes(filterKey)) { + return clauses + } + return [] + }) .map((value) => [value, true]) ) + let result = { ...labels } for (const value in labels) { if (!filteredBy[value]) { @@ -149,7 +162,7 @@ export function cleanLabels(filters, labels, mergedFilterKey, mergedLabels) { if ( mergedFilterKey && - ['country', 'region', 'city'].includes(mergedFilterKey) + ['country', 'region', 'city', 'segment'].includes(mergedFilterKey) ) { result = { ...result, @@ -160,20 +173,65 @@ export function cleanLabels(filters, labels, mergedFilterKey, mergedLabels) { return result } +const NO_PREFIX_KEYS = new Set(['segment']) const EVENT_FILTER_KEYS = new Set(['name', 'page', 'goal', 'hostname']) +const EVENT_PREFIX = 'event:' +const VISIT_PREFIX = 'visit:' + +function remapFilterKey(filterKey) { + if (NO_PREFIX_KEYS.has(filterKey)) { + return filterKey + } + if (EVENT_FILTER_KEYS.has(filterKey)) { + return `${EVENT_PREFIX}${filterKey}` + } + return `${VISIT_PREFIX}${filterKey}` +} + +function remapApiFilterKey(apiFilterKey) { + const isNoPrefixKey = NO_PREFIX_KEYS.has(apiFilterKey) + + if (isNoPrefixKey) { + return apiFilterKey + } + + const isEventKey = apiFilterKey.startsWith(EVENT_PREFIX) + const isVisitKey = apiFilterKey.startsWith(VISIT_PREFIX) + + if (isEventKey) { + return apiFilterKey.substring(EVENT_PREFIX.length) + } + if (isVisitKey) { + return apiFilterKey.substring(VISIT_PREFIX.length) + } + + return apiFilterKey // maybe throw? +} + +export function remapToApiFilters(filters) { + return filters.map(remapToApiFilter) +} + +export function remapFromApiFilters(apiFilters) { + return apiFilters.map((apiFilter) => { + const [operation, ...rest] = apiFilter + if (operation === 'has_not_done') { + const [[_, key, clauses]] = rest + return [FILTER_OPERATIONS.has_not_done, key, clauses] + } + const [apiFilterKey, clauses] = rest + return [operation, remapApiFilterKey(apiFilterKey), clauses] + }) +} export function serializeApiFilters(filters) { - const apiFilters = filters.map(serializeFilter) - return JSON.stringify(apiFilters) + return JSON.stringify(remapToApiFilters(filters)) } -function serializeFilter([operation, filterKey, clauses, ...modifiers]) { - let apiFilterKey = `visit:${filterKey}` - if ( - filterKey.startsWith(EVENT_PROPS_PREFIX) || - EVENT_FILTER_KEYS.has(filterKey) - ) { - apiFilterKey = `event:${filterKey}` +function remapToApiFilter([operation, filterKey, clauses, ...modifiers]) { + const apiFilterKey = remapFilterKey(filterKey) + if (apiFilterKey === 'segment') { + return [operation, apiFilterKey, clauses.map((v) => parseInt(v, 10))] } if (operation === FILTER_OPERATIONS.has_not_done) { // :NOTE: Frontend does not support advanced query building that's used in the backend. @@ -232,5 +290,6 @@ export const formattedFilters = { page: 'Page', hostname: 'Hostname', entry_page: 'Entry Page', - exit_page: 'Exit Page' + exit_page: 'Exit Page', + segment: 'Segment' } diff --git a/assets/test-utils/app-context-providers.tsx b/assets/test-utils/app-context-providers.tsx index 96819be93934..8b83ae2cdf90 100644 --- a/assets/test-utils/app-context-providers.tsx +++ b/assets/test-utils/app-context-providers.tsx @@ -41,7 +41,8 @@ export const TestContextProviders = ({ isDbip: false, flags: {}, validIntervalsByPeriod: {}, - shared: false + shared: false, + members: { 1: 'Test User' } } const site = { ...defaultSite, ...siteOptions } @@ -59,7 +60,7 @@ export const TestContextProviders = ({ return ( // not interactive component, default value is suitable - + Map.new() + defp get_members(nil, _site) do + nil + end + + defp get_members(_user, site = %Plausible.Site{}) do + memberships = + Plausible.Repo.preload(site, :guest_memberships).guest_memberships |> IO.inspect() + + %{"0" => "0"} + # s = Plausible.Repo.preload(site, :guest_memberships) + # s.guest_memberships |> Enum.map(fn member -> {member.id, member.name} end) |> Map.new() + end + defp is_dbip() do on_ee do false diff --git a/lib/plausible_web/templates/stats/stats.html.heex b/lib/plausible_web/templates/stats/stats.html.heex index 8bbecc573aa8..854792f00a32 100644 --- a/lib/plausible_web/templates/stats/stats.html.heex +++ b/lib/plausible_web/templates/stats/stats.html.heex @@ -27,6 +27,9 @@ data-props-available={ to_string(Plausible.Billing.Feature.Props.check_availability(@site.team) == :ok) } + data-site-segments-available={ + to_string(Plausible.Billing.Feature.Props.check_availability(@site.team) == :ok) + } data-revenue-goals={Jason.encode!(@revenue_goals)} data-funnels={Jason.encode!(@funnels)} data-has-props={to_string(@has_props)} @@ -39,6 +42,8 @@ data-background={@conn.assigns[:background]} data-is-dbip={to_string(@is_dbip)} data-current-user-role={@conn.assigns[:site_role]} + data-current-user-id={if user = @conn.assigns[:current_user], do: user.id, else: Jason.encode!(nil)} + data-members={Jason.encode!(@members)} data-flags={Jason.encode!(@flags)} data-valid-intervals-by-period={ Plausible.Stats.Interval.valid_by_period(site: @site) |> Jason.encode!() diff --git a/test/plausible_web/controllers/stats_controller_test.exs b/test/plausible_web/controllers/stats_controller_test.exs index 52ecadeed5b0..b971d3b9fbb2 100644 --- a/test/plausible_web/controllers/stats_controller_test.exs +++ b/test/plausible_web/controllers/stats_controller_test.exs @@ -22,6 +22,7 @@ defmodule PlausibleWeb.StatsControllerTest do assert text_of_attr(resp, @react_container, "data-funnels-opted-out") == "false" assert text_of_attr(resp, @react_container, "data-props-opted-out") == "false" assert text_of_attr(resp, @react_container, "data-props-available") == "true" + assert text_of_attr(resp, @react_container, "data-site-segments-available") == "true" assert text_of_attr(resp, @react_container, "data-funnels-available") == "true" assert text_of_attr(resp, @react_container, "data-has-props") == "false" assert text_of_attr(resp, @react_container, "data-logged-in") == "false" From 46f4540738521fb32aec4310fa83e953cdfcf5c5 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 12 Feb 2025 23:10:45 +0200 Subject: [PATCH 02/25] Load members --- .../controllers/stats_controller.ex | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/plausible_web/controllers/stats_controller.ex b/lib/plausible_web/controllers/stats_controller.ex index 96926df8ae9b..072eb352f168 100644 --- a/lib/plausible_web/controllers/stats_controller.ex +++ b/lib/plausible_web/controllers/stats_controller.ex @@ -404,12 +404,22 @@ defmodule PlausibleWeb.StatsController do end defp get_members(_user, site = %Plausible.Site{}) do - memberships = - Plausible.Repo.preload(site, :guest_memberships).guest_memberships |> IO.inspect() + site = + site + |> Plausible.Repo.preload( + team: [team_memberships: [:user]], + guest_memberships: [team_membership: [:user]] + ) - %{"0" => "0"} - # s = Plausible.Repo.preload(site, :guest_memberships) - # s.guest_memberships |> Enum.map(fn member -> {member.id, member.name} end) |> Map.new() + site.guest_memberships + |> Enum.map(fn i = %Plausible.Teams.GuestMembership{} -> + i.team_membership + end) + |> Enum.concat(site.team.team_memberships) + |> Enum.map(fn i = %Plausible.Teams.Membership{} -> + {i.user.id, i.user.name} + end) + |> Map.new() end defp is_dbip() do From 0a192cf22fc7ce280d5e3a4c239d9fdb8c73baf1 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 13 Feb 2025 10:27:39 +0200 Subject: [PATCH 03/25] Assert that we know has_not_done will not work without changes --- .../api/internal_controller/segments_controller_test.exs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs b/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs index d6269ebee005..d7c4af5bbca6 100644 --- a/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs +++ b/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs @@ -445,7 +445,9 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do "Invalid filters. Dimension `event:goal` can only be filtered at the top level.", [] ] - ]} + ]}, + {[["has_not_done", ["is", "event:goal", ["a goal"]]]], + [["segment_data", "Invalid filters. Deep filters are not supported.", []]]} ] do test "prevents owners from updating segments to invalid filters #{inspect(filters)} with error 400", %{ From 8b5c9bd264a88176e1d2ddeb5be315024db86816 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 13 Feb 2025 11:16:15 +0200 Subject: [PATCH 04/25] Add tests --- .../js/dashboard/filtering/segments.test.ts | 88 +++++++++++++++++++ assets/js/dashboard/filtering/segments.ts | 11 ++- .../js/dashboard/segments/segment-modals.tsx | 41 ++++----- assets/js/dashboard/util/filters.js | 13 ++- assets/test-utils/app-context-providers.tsx | 1 + 5 files changed, 128 insertions(+), 26 deletions(-) create mode 100644 assets/js/dashboard/filtering/segments.test.ts diff --git a/assets/js/dashboard/filtering/segments.test.ts b/assets/js/dashboard/filtering/segments.test.ts new file mode 100644 index 000000000000..6be2d7748cc2 --- /dev/null +++ b/assets/js/dashboard/filtering/segments.test.ts @@ -0,0 +1,88 @@ +/** @format */ + +import { remapToApiFilters } from '../util/filters' +import { + formatSegmentIdAsLabelKey, + getFilterSegmentsByNameInsensitive, + getSegmentNamePlaceholder, + isSegmentIdLabelKey, + parseApiSegmentData +} from './segments' + +describe(`${getFilterSegmentsByNameInsensitive.name}`, () => { + const unfilteredSegments = [ + { name: 'APAC Region' }, + { name: 'EMEA Region' }, + { name: 'Scandinavia' } + ] + it('generates insensitive filter function', () => { + expect( + unfilteredSegments.filter(getFilterSegmentsByNameInsensitive('region')) + ).toEqual([{ name: 'APAC Region' }, { name: 'EMEA Region' }]) + }) + + it('ignores preceding and following whitespace', () => { + expect( + unfilteredSegments.filter(getFilterSegmentsByNameInsensitive(' scandi ')) + ).toEqual([{ name: 'Scandinavia' }]) + }) + + it.each([[undefined], [''], [' '], ['\n\n']])( + 'generates always matching filter for search value %p', + (searchValue) => { + expect( + unfilteredSegments.filter( + getFilterSegmentsByNameInsensitive(searchValue) + ) + ).toEqual(unfilteredSegments) + } + ) +}) + +describe(`${getSegmentNamePlaceholder.name}`, () => { + it('gives readable result', () => { + const placeholder = getSegmentNamePlaceholder({ + labels: { US: 'United States' }, + filters: [ + ['is', 'country', ['US']], + ['contains', 'page', ['/blog', `${new Array(250).fill('c').join('')}`]] + ] + }) + + expect(placeholder).toEqual( + 'Country is United States and Page contains /blog or ccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc' + ) + expect(placeholder).toHaveLength(255) + }) +}) + +describe('segment labels in URL search params', () => { + test(`${formatSegmentIdAsLabelKey.name} and ${isSegmentIdLabelKey.name} work`, () => { + const formatted = formatSegmentIdAsLabelKey(5) + expect(formatted).toEqual('segment-5') + expect(isSegmentIdLabelKey(formatted)).toEqual(true) + }) +}) + +describe(`${parseApiSegmentData.name}`, () => { + it('correctly formats values stored as API filters in their dashboard format', () => { + const apiFormatFilters = [ + ['is', 'visit:country', ['PL']], + ['is', 'event:props:logged_in', ['true']], + ['has_not_done', ['is', 'event:goal', ['Signup']]] + ] + const dashboardFormat = parseApiSegmentData({ + filters: apiFormatFilters, + labels: { PL: 'Poland' } + }) + expect(dashboardFormat).toEqual({ + filters: [ + ['is', 'country', ['PL']], + ['is', 'props:logged_in', ['true']], + ['has_not_done', 'goal', ['Signup']] + ], + labels: { PL: 'Poland' } + }) + expect(remapToApiFilters(dashboardFormat.filters)).toEqual(apiFormatFilters) + }) +}) diff --git a/assets/js/dashboard/filtering/segments.ts b/assets/js/dashboard/filtering/segments.ts index b626a4d6703e..f0020f2d424d 100644 --- a/assets/js/dashboard/filtering/segments.ts +++ b/assets/js/dashboard/filtering/segments.ts @@ -27,14 +27,16 @@ const SEGMENT_LABEL_KEY_PREFIX = 'segment-' export function getFilterSegmentsByNameInsensitive( search?: string -): (s: SavedSegment) => boolean { +): (s: Pick) => boolean { return (s) => search?.trim().length ? s.name.toLowerCase().includes(search.trim().toLowerCase()) : true } -export const getSegmentNamePlaceholder = (query: DashboardQuery) => +export const getSegmentNamePlaceholder = ( + query: Pick +) => query.filters .reduce( (combinedName, filter) => @@ -58,7 +60,10 @@ export const isSegmentFilter = (f: Filter): boolean => f[1] === 'segment' export const parseApiSegmentData = ({ filters, ...rest -}: SegmentData): SegmentData => ({ +}: { + filters: unknown[] + labels: Record +}): SegmentData => ({ filters: remapFromApiFilters(filters), ...rest }) diff --git a/assets/js/dashboard/segments/segment-modals.tsx b/assets/js/dashboard/segments/segment-modals.tsx index b07f05358b08..fe844271d38a 100644 --- a/assets/js/dashboard/segments/segment-modals.tsx +++ b/assets/js/dashboard/segments/segment-modals.tsx @@ -1,6 +1,6 @@ /** @format */ -import React, { Fragment, ReactNode, useLayoutEffect, useState } from 'react' +import React, { ReactNode, useLayoutEffect, useState } from 'react' import ModalWithRouting from '../stats/modals/modal' import { isSegmentFilter, @@ -19,10 +19,13 @@ import classNames from 'classnames' import { SegmentAuthorship } from './segment-authorship' import { ExclamationTriangleIcon, TrashIcon } from '@heroicons/react/24/outline' -interface SharedSegmentModalProps { - onClose: () => void +interface SegmentTypeSelectorProps { siteSegmentsAvailable: boolean userCanSelectSiteSegment: boolean +} + +interface SharedSegmentModalProps { + onClose: () => void namePlaceholder: string } @@ -63,10 +66,11 @@ export const CreateSegmentModal = ({ siteSegmentsAvailable: siteSegmentsAvailable, userCanSelectSiteSegment, namePlaceholder -}: SharedSegmentModalProps & { - segment?: SavedSegment - onSave: (input: Pick) => void -}) => { +}: SharedSegmentModalProps & + SegmentTypeSelectorProps & { + segment?: SavedSegment + onSave: (input: Pick) => void + }) => { const defaultName = segment?.name ? `Copy of ${segment.name}` : '' const [name, setName] = useState(defaultName) const defaultType = @@ -86,7 +90,7 @@ export const CreateSegmentModal = ({ onChange={setName} namePlaceholder={namePlaceholder} /> - void -} & Pick< - SharedSegmentModalProps, - 'siteSegmentsAvailable' | 'userCanSelectSiteSegment' ->) => { +}) => { const options = [ { type: SegmentType.personal, @@ -251,7 +252,6 @@ const SegmentTypeInput = ({ value="" onChange={() => onChange(type)} className="mt-4 w-4 h-4 text-indigo-600 bg-gray-100 border-gray-300 focus:ring-2 focus:ring-offset-2 focus:ring-indigo-500 dark:border-gray-600" - // disabled={disabled} />
) : ( diff --git a/assets/js/dashboard/segments/transient-segment-modals.tsx b/assets/js/dashboard/segments/routeless-segment-modals.tsx similarity index 62% rename from assets/js/dashboard/segments/transient-segment-modals.tsx rename to assets/js/dashboard/segments/routeless-segment-modals.tsx index 53a8ca92adf8..7ef94d2657ef 100644 --- a/assets/js/dashboard/segments/transient-segment-modals.tsx +++ b/assets/js/dashboard/segments/routeless-segment-modals.tsx @@ -8,7 +8,7 @@ import { UpdateSegmentModal } from './segment-modals' import { - formatSegmentIdAsLabelKey, + getSearchToApplySingleSegmentFilter, getSegmentNamePlaceholder, parseApiSegmentData, SavedSegment, @@ -20,17 +20,18 @@ import { cleanLabels, remapToApiFilters } from '../util/filters' import { useAppNavigate } from '../navigation/use-app-navigate' import { useQueryContext } from '../query-context' import { Role, useUserContext } from '../user-context' +import { mutation } from '../api' -export const TransientSegmentModals = (_p: { closeList: () => void }) => { +export const RoutelessSegmentModals = () => { const navigate = useAppNavigate() const queryClient = useQueryClient() const site = useSiteContext() - const { expandedSegment, modal, setModal } = useSegmentExpandedContext() const { query } = useQueryContext() const user = useUserContext() + const { expandedSegment, modal, setModal } = useSegmentExpandedContext() const patchSegment = useMutation({ - mutationFn: ({ + mutationFn: async ({ id, name, type, @@ -39,54 +40,43 @@ export const TransientSegmentModals = (_p: { closeList: () => void }) => { Partial> & { segment_data?: SegmentData }) => { - return fetch(`/api/${encodeURIComponent(site.domain)}/segments/${id}`, { - method: 'PATCH', - body: JSON.stringify({ - name, - type, - ...(segment_data && { - segment_data: { - filters: remapToApiFilters(segment_data.filters), - labels: cleanLabels(segment_data.filters, segment_data.labels) + const response: SavedSegment & { segment_data: SegmentData } = + await mutation( + `/api/${encodeURIComponent(site.domain)}/segments/${id}`, + { + method: 'PATCH', + body: { + name, + type, + ...(segment_data && { + segment_data: { + filters: remapToApiFilters(segment_data.filters), + labels: cleanLabels(segment_data.filters, segment_data.labels) + } + }) } - }) - }), - headers: { - 'content-type': 'application/json', - accept: 'application/json' - } - }) - .then((res) => res.json()) - .then((d) => ({ - ...d, - segment_data: parseApiSegmentData(d.segment_data) - })) + } + ) + + return { + ...response, + segment_data: parseApiSegmentData(response.segment_data) + } }, - onSuccess: async (d) => { + onSuccess: async (segment) => { queryClient.invalidateQueries({ queryKey: ['segments'] }) navigate({ - search: (search) => { - const filters = [['is', 'segment', [d.id]]] - const labels = cleanLabels(filters, {}, 'segment', { - [formatSegmentIdAsLabelKey(d.id)]: d.name - }) - return { - ...search, - filters, - labels - } - }, + search: getSearchToApplySingleSegmentFilter(segment), state: { expandedSegment: null } - // replace: true }) setModal(null) } }) const createSegment = useMutation({ - mutationFn: ({ + mutationFn: async ({ name, type, segment_data @@ -95,62 +85,50 @@ export const TransientSegmentModals = (_p: { closeList: () => void }) => { type: 'personal' | 'site' segment_data: SegmentData }) => { - return fetch(`/api/${encodeURIComponent(site.domain)}/segments`, { - method: 'POST', - body: JSON.stringify({ - name, - type, - segment_data: { - filters: remapToApiFilters(segment_data.filters), - labels: cleanLabels(segment_data.filters, segment_data.labels) + const response: SavedSegment & { segment_data: SegmentData } = + await mutation(`/api/${encodeURIComponent(site.domain)}/segments`, { + method: 'POST', + body: { + name, + type, + segment_data: { + filters: remapToApiFilters(segment_data.filters), + labels: cleanLabels(segment_data.filters, segment_data.labels) + } } - }), - headers: { 'content-type': 'application/json' } - }) - .then((res) => res.json()) - .then((d) => ({ - ...d, - segment_data: parseApiSegmentData(d.segment_data) - })) + }) + return { + ...response, + segment_data: parseApiSegmentData(response.segment_data) + } }, - onSuccess: async (d) => { + onSuccess: async (segment) => { + queryClient.invalidateQueries({ queryKey: ['segments'] }) navigate({ - search: (search) => { - queryClient.invalidateQueries({ queryKey: ['segments'] }) - const filters = [['is', 'segment', [d.id]]] - const labels = cleanLabels(filters, {}, 'segment', { - [formatSegmentIdAsLabelKey(d.id)]: d.name - }) - return { - ...search, - filters, - labels - } - }, + search: getSearchToApplySingleSegmentFilter(segment), state: { expandedSegment: null } - // replace: true }) setModal(null) } }) const deleteSegment = useMutation({ - mutationFn: (data: Pick) => { - return fetch( - `/api/${encodeURIComponent(site.domain)}/segments/${data.id}`, - { - method: 'DELETE' - } - ) - .then((res) => res.json()) - .then((d) => ({ - ...d, - segment_data: parseApiSegmentData(d.segment_data) - })) + mutationFn: async (data: Pick) => { + const response: SavedSegment & { segment_data: SegmentData } = + await mutation( + `/api/${encodeURIComponent(site.domain)}/segments/${data.id}`, + { + method: 'DELETE' + } + ) + return { + ...response, + segment_data: parseApiSegmentData(response.segment_data) + } }, - onSuccess: (_d): void => { + onSuccess: (_segment): void => { queryClient.invalidateQueries({ queryKey: ['segments'] }) navigate({ search: (s) => { @@ -163,7 +141,6 @@ export const TransientSegmentModals = (_p: { closeList: () => void }) => { state: { expandedSegment: null } - // replace: true }) setModal(null) } @@ -200,6 +177,8 @@ export const TransientSegmentModals = (_p: { closeList: () => void }) => { } }) } + status={patchSegment.status} + error={patchSegment.error} /> )} {modal === 'create' && ( @@ -219,6 +198,8 @@ export const TransientSegmentModals = (_p: { closeList: () => void }) => { } }) } + status={createSegment.status} + error={createSegment.error} /> )} {modal === 'delete' && expandedSegment && ( @@ -226,6 +207,8 @@ export const TransientSegmentModals = (_p: { closeList: () => void }) => { segment={expandedSegment} onClose={() => setModal(null)} onSave={({ id }) => deleteSegment.mutate({ id })} + status={deleteSegment.status} + error={deleteSegment.error} /> )} diff --git a/assets/js/dashboard/segments/segment-authorship.tsx b/assets/js/dashboard/segments/segment-authorship.tsx index d19479033285..7f405c471f59 100644 --- a/assets/js/dashboard/segments/segment-authorship.tsx +++ b/assets/js/dashboard/segments/segment-authorship.tsx @@ -3,7 +3,7 @@ import React from 'react' import { SavedSegment } from '../filtering/segments' import { PlausibleSite, useSiteContext } from '../site-context' -import { formatDayShort, parseUTCDate } from '../util/date' +import { dateForSite, formatDayShort } from '../util/date' const getAuthorLabel = ( site: Pick, @@ -29,7 +29,7 @@ export const SegmentAuthorship = ({ owner_id, inserted_at, updated_at -}: SavedSegment & { +}: Pick & { className?: string }) => { const site = useSiteContext() @@ -41,12 +41,12 @@ export const SegmentAuthorship = ({ return (
- {`Created at ${formatDayShort(parseUTCDate(inserted_at))}`} + {`Created at ${formatDayShort(dateForSite(inserted_at, site))}`} {!showUpdatedAt && !!authorLabel && ` by ${authorLabel}`}
{showUpdatedAt && (
- {`Last updated at ${formatDayShort(parseUTCDate(updated_at))}`} + {`Last updated at ${formatDayShort(dateForSite(updated_at, site))}`} {!!authorLabel && ` by ${authorLabel}`}
)} diff --git a/assets/js/dashboard/segments/segment-modals.tsx b/assets/js/dashboard/segments/segment-modals.tsx index fe844271d38a..b1256e44791b 100644 --- a/assets/js/dashboard/segments/segment-modals.tsx +++ b/assets/js/dashboard/segments/segment-modals.tsx @@ -18,6 +18,12 @@ import { FilterPillsList } from '../nav-menu/filter-pills-list' import classNames from 'classnames' import { SegmentAuthorship } from './segment-authorship' import { ExclamationTriangleIcon, TrashIcon } from '@heroicons/react/24/outline' +import { MutationStatus } from '@tanstack/react-query' + +interface ApiRequestState { + status: MutationStatus + error?: unknown +} interface SegmentTypeSelectorProps { siteSegmentsAvailable: boolean @@ -67,6 +73,7 @@ export const CreateSegmentModal = ({ userCanSelectSiteSegment, namePlaceholder }: SharedSegmentModalProps & + ApiRequestState & SegmentTypeSelectorProps & { segment?: SavedSegment onSave: (input: Pick) => void @@ -125,7 +132,7 @@ export const DeleteSegmentModal = ({ onClose: () => void onSave: (input: Pick) => void segment: SavedSegment & { segment_data?: SegmentData } -}) => { +} & ApiRequestState) => { return ( @@ -281,6 +288,7 @@ export const UpdateSegmentModal = ({ userCanSelectSiteSegment, namePlaceholder }: SharedSegmentModalProps & + ApiRequestState & SegmentTypeSelectorProps & { onSave: (input: Pick) => void segment: SavedSegment @@ -342,11 +350,11 @@ const FiltersInSegment = ({ segment_data }: { segment_data: SegmentData }) => { ) } -export const SegmentModal = () => { +export const SegmentModal = ({ id }: { id: SavedSegment['id'] }) => { const { query } = useQueryContext() - const segmentsFilter = query.filters.find(isSegmentFilter)! + const { data, fetchSegment } = useSegmentPrefetch({ - id: String(segmentsFilter[2][0]) + id: String(id) }) useLayoutEffect(() => { diff --git a/assets/js/dashboard/stats/modals/filter-modal.js b/assets/js/dashboard/stats/modals/filter-modal.js index 2dffc4d7e6fa..b250b70f2646 100644 --- a/assets/js/dashboard/stats/modals/filter-modal.js +++ b/assets/js/dashboard/stats/modals/filter-modal.js @@ -11,6 +11,7 @@ import { rootRoute } from '../../router'; import { useAppNavigate } from '../../navigation/use-app-navigate'; import { SegmentModal } from '../../segments/segment-modals'; import { TrashIcon } from '@heroicons/react/24/outline'; +import { isSegmentFilter } from '../../filtering/segments'; function partitionFilters(modalType, filters) { const otherFilters = [] @@ -201,8 +202,10 @@ export default function FilterModalWithRouter(props) { const { field } = useParams() const { query } = useQueryContext() const site = useSiteContext() - if (field === 'segment') { - return + const firstSegmentFilter = field === 'segment' ? query.filters?.find(isSegmentFilter) : null + if (firstSegmentFilter) { + const firstSegmentId = firstSegmentFilter[2][0] + return } return ( date2.year(); + return date1.year() > date2.year() } - if (period === "year") { - return false; + if (period === 'year') { + return false } if (date1.month() !== date2.month()) { - return date1.month() > date2.month(); + return date1.month() > date2.month() } - if (period === "month") { - return false; + if (period === 'month') { + return false } return date1.date() > date2.date() } diff --git a/assets/js/dashboard/util/date.test.ts b/assets/js/dashboard/util/date.test.ts index 99a2085fa70f..ba085e7fce73 100644 --- a/assets/js/dashboard/util/date.test.ts +++ b/assets/js/dashboard/util/date.test.ts @@ -1,6 +1,13 @@ /** @format */ -import { formatISO, nowForSite, shiftMonths, yesterday } from './date' +import { + dateForSite, + formatDayShort, + formatISO, + nowForSite, + shiftMonths, + yesterday +} from './date' jest.useFakeTimers() @@ -102,3 +109,13 @@ for (const [timezone, suite] of sets) { ) }) } + +describe('formatting UTC dates from database', () => { + it('is able to enrich UTC date string with site timezone, formatting the value correctly', () => { + expect( + formatDayShort( + dateForSite('2025-01-01T14:00:00', { offset: 60 * 60 * 11 }) + ) + ).toEqual('2 Jan') + }) +}) From 12099fec18e6ee3f42b543e7396e8c71b79b4b08 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 13 Feb 2025 18:29:39 +0200 Subject: [PATCH 07/25] Add error panels --- .../js/dashboard/components/error-panel.tsx | 51 ++++++++ .../segments/searchable-segments-section.tsx | 34 ++++-- .../segments/routeless-segment-modals.tsx | 3 + .../segments/segment-expanded-context.tsx | 29 +++-- .../js/dashboard/segments/segment-modals.tsx | 115 +++++++++++++----- 5 files changed, 179 insertions(+), 53 deletions(-) create mode 100644 assets/js/dashboard/components/error-panel.tsx diff --git a/assets/js/dashboard/components/error-panel.tsx b/assets/js/dashboard/components/error-panel.tsx new file mode 100644 index 000000000000..39832ae11569 --- /dev/null +++ b/assets/js/dashboard/components/error-panel.tsx @@ -0,0 +1,51 @@ +/** @format */ + +import React from 'react' +import classNames from 'classnames' +import { + ArrowPathIcon, + ExclamationTriangleIcon, + XMarkIcon +} from '@heroicons/react/24/outline' + +export const ErrorPanel = ({ + errorMessage, + className, + onClose, + onRetry +}: { + errorMessage: string + className?: string + onClose?: () => void + onRetry?: () => void +}) => ( +
+
+ +
+
{errorMessage}
+ {typeof onClose === 'function' && ( + + )} + {typeof onRetry === 'function' && ( + + )} +
+) diff --git a/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx b/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx index 2451dedf3e72..5245d246d8b7 100644 --- a/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx +++ b/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx @@ -22,6 +22,8 @@ import { EllipsisHorizontalIcon } from '@heroicons/react/24/solid' import { popover } from '../../components/popover' import { AppNavigationLink } from '../../navigation/use-app-navigate' import { MenuSeparator } from '../nav-menu-components' +import { ErrorPanel } from '../../components/error-panel' +import { get } from '../../api' const useSegmentsListQuery = () => { const site = useSiteContext() @@ -30,16 +32,9 @@ const useSegmentsListQuery = () => { queryKey: ['segments'], placeholderData: (previousData) => previousData, queryFn: async () => { - const response = await fetch( - `/api/${encodeURIComponent(site.domain)}/segments`, - { - method: 'GET', - headers: { - 'content-type': 'application/json', - accept: 'application/json' - } - } - ).then((res): Promise => res.json()) + const response: SavedSegment[] = await get( + `/api/${encodeURIComponent(site.domain)}/segments` + ) return response.sort( (a, b) => @@ -68,7 +63,7 @@ export const SearchableSegmentsSection = ({ const segmentFilter = query.filters.find(isSegmentFilter) const appliedSegmentIds = (segmentFilter ? segmentFilter[2] : []) as number[] - const { data } = useSegmentsListQuery() + const { data, ...listQuery } = useSegmentsListQuery() const [searchValue, setSearch] = useState() const [showAll, setShowAll] = useState(false) @@ -98,7 +93,7 @@ export const SearchableSegmentsSection = ({ <>
-
+
Segments
{data.length > initialSliceLength && ( @@ -159,6 +154,21 @@ export const SearchableSegmentsSection = ({ )} )} + {listQuery.status === 'pending' && ( +
+
+
+
+
+ )} + {listQuery.error && ( +
+ listQuery.refetch()} + /> +
+ )} ) } diff --git a/assets/js/dashboard/segments/routeless-segment-modals.tsx b/assets/js/dashboard/segments/routeless-segment-modals.tsx index 7ef94d2657ef..665854de02d4 100644 --- a/assets/js/dashboard/segments/routeless-segment-modals.tsx +++ b/assets/js/dashboard/segments/routeless-segment-modals.tsx @@ -179,6 +179,7 @@ export const RoutelessSegmentModals = () => { } status={patchSegment.status} error={patchSegment.error} + reset={patchSegment.reset} /> )} {modal === 'create' && ( @@ -200,6 +201,7 @@ export const RoutelessSegmentModals = () => { } status={createSegment.status} error={createSegment.error} + reset={createSegment.reset} /> )} {modal === 'delete' && expandedSegment && ( @@ -209,6 +211,7 @@ export const RoutelessSegmentModals = () => { onSave={({ id }) => deleteSegment.mutate({ id })} status={deleteSegment.status} error={deleteSegment.error} + reset={deleteSegment.reset} /> )} diff --git a/assets/js/dashboard/segments/segment-expanded-context.tsx b/assets/js/dashboard/segments/segment-expanded-context.tsx index 2d33203a3853..93bdc1b088b9 100644 --- a/assets/js/dashboard/segments/segment-expanded-context.tsx +++ b/assets/js/dashboard/segments/segment-expanded-context.tsx @@ -4,7 +4,6 @@ import React, { ReactNode, useContext, useEffect, - useLayoutEffect, useState } from 'react' import { SavedSegment, SegmentData } from '../filtering/segments' @@ -51,22 +50,24 @@ export default function SegmentExpandedContextProvider({ const { query } = useQueryContext() const navigate = useAppNavigate() - useLayoutEffect(() => { + const locationStateExpandedSegment = location.state?.expandedSegment + + useEffect(() => { // copy location.state to state - if (location.state?.expandedSegment) { + if (locationStateExpandedSegment) { setState({ - expandedSegment: location.state.expandedSegment + expandedSegment: locationStateExpandedSegment }) } - if (location.state?.expandedSegment === null) { + if (locationStateExpandedSegment === null) { setState({ expandedSegment: segmentExpandedContextDefaultValue.expandedSegment }) // setModal(segmentExpandedContextDefaultValue.modal) } - }, [location.state]) + }, [locationStateExpandedSegment]) - useLayoutEffect(() => { + useEffect(() => { // clear edit mode on clearing all filters if (!query.filters.length && expandedSegmentState.expandedSegment) { navigate({ @@ -77,7 +78,8 @@ export default function SegmentExpandedContextProvider({ replace: true }) // overwrite undefined locationState with current expandedSegment, to handle Back navigation correctly - } else if (location.state?.expandedSegment === undefined) { + } else if (locationStateExpandedSegment === undefined) { + // console.log('Slowness') navigate({ search: (s) => s, state: { @@ -86,11 +88,12 @@ export default function SegmentExpandedContextProvider({ replace: true }) } - }, [query, expandedSegmentState.expandedSegment, navigate, location.state]) - - useEffect(() => { - console.log({ modal }) - }, [modal]) + }, [ + query, + expandedSegmentState.expandedSegment, + navigate, + locationStateExpandedSegment + ]) return ( void } interface SegmentTypeSelectorProps { @@ -44,7 +47,7 @@ const primaryNegativeButtonClassName = classNames( const secondaryButtonClassName = classNames( 'button', - 'border border-gray-300 dark:border-gray-500 text-gray-700 dark:text-gray-300 !bg-transparent hover:!bg-gray-100 dark:hover:!bg-gray-850' + 'border !border-gray-300 dark:!border-gray-500 !text-gray-700 dark:!text-gray-300 !bg-transparent hover:!bg-gray-100 dark:hover:!bg-gray-850' ) const SegmentActionModal = ({ @@ -71,9 +74,12 @@ export const CreateSegmentModal = ({ onSave, siteSegmentsAvailable: siteSegmentsAvailable, userCanSelectSiteSegment, - namePlaceholder + namePlaceholder, + error, + reset, + status }: SharedSegmentModalProps & - ApiRequestState & + ApiRequestProps & SegmentTypeSelectorProps & { segment?: SavedSegment onSave: (input: Pick) => void @@ -103,23 +109,38 @@ export const CreateSegmentModal = ({ siteSegmentsAvailable={siteSegmentsAvailable} userCanSelectSiteSegment={userCanSelectSiteSegment} /> - + + {error !== null && ( + + )} ) } @@ -127,12 +148,15 @@ export const CreateSegmentModal = ({ export const DeleteSegmentModal = ({ onClose, onSave, - segment + segment, + status, + error, + reset }: { onClose: () => void onSave: (input: Pick) => void segment: SavedSegment & { segment_data?: SegmentData } -} & ApiRequestState) => { +} & ApiRequestProps) => { return ( @@ -147,19 +171,35 @@ export const DeleteSegmentModal = ({ )} - + + {error !== null && ( + + )} ) } @@ -286,9 +326,12 @@ export const UpdateSegmentModal = ({ segment, siteSegmentsAvailable, userCanSelectSiteSegment, - namePlaceholder + namePlaceholder, + status, + error, + reset }: SharedSegmentModalProps & - ApiRequestState & + ApiRequestProps & SegmentTypeSelectorProps & { onSave: (input: Pick) => void segment: SavedSegment @@ -310,23 +353,39 @@ export const UpdateSegmentModal = ({ siteSegmentsAvailable={siteSegmentsAvailable} userCanSelectSiteSegment={userCanSelectSiteSegment} /> - + + {error !== null && ( + + )} ) } From ed1a59a4ba269036e39f25385fb01a98caaed96a Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 13 Feb 2025 20:58:16 +0200 Subject: [PATCH 08/25] Flatten errors on the API side --- assets/js/dashboard/api.ts | 2 +- assets/js/dashboard/nav-menu/filters-bar.tsx | 23 +++-- .../segments/searchable-segments-section.tsx | 24 +++-- assets/js/dashboard/nav-menu/top-bar.test.tsx | 4 +- .../js/dashboard/segments/segment-modals.tsx | 69 ++++++++++---- lib/plausible/segments/segments.ex | 15 +++ .../api/internal/segments_controller.ex | 6 +- .../controllers/stats_controller.ex | 2 +- .../templates/stats/stats.html.heex | 4 +- test/plausible/segments/segments_test.exs | 4 + .../segments_controller_test.exs | 91 +++++++------------ 11 files changed, 140 insertions(+), 104 deletions(-) create mode 100644 test/plausible/segments/segments_test.exs diff --git a/assets/js/dashboard/api.ts b/assets/js/dashboard/api.ts index 8742e1cfc7b2..1bc8e50ec144 100644 --- a/assets/js/dashboard/api.ts +++ b/assets/js/dashboard/api.ts @@ -99,7 +99,7 @@ export async function get( query ? url + serializeQuery(query, extraQuery) : url, { signal: abortController.signal, - headers: getHeaders() + headers: { ...getHeaders(), Accept: 'application/json' } } ) return handleApiResponse(response) diff --git a/assets/js/dashboard/nav-menu/filters-bar.tsx b/assets/js/dashboard/nav-menu/filters-bar.tsx index 1de54801a708..92606466f427 100644 --- a/assets/js/dashboard/nav-menu/filters-bar.tsx +++ b/assets/js/dashboard/nav-menu/filters-bar.tsx @@ -10,6 +10,7 @@ import { Popover, Transition } from '@headlessui/react' import { popover } from '../components/popover' import { BlurMenuButtonOnEscape } from '../keybinding' import { useSegmentExpandedContext } from '../segments/segment-expanded-context' +import { isSegmentFilter } from '../filtering/segments' // Component structure is // `..[ filter (x) ]..[ filter (x) ]..[ three dot menu ]..` @@ -235,7 +236,6 @@ export const FiltersBar = ({ accessors }: FiltersBarProps) => { const ClearAction = () => ( ( ) const SaveAsSegmentAction = () => { + const { query } = useQueryContext() const { expandedSegment, setModal } = useSegmentExpandedContext() - return !expandedSegment ? ( + if (expandedSegment) { + return null + } + + const disabledReason = query.filters.some(isSegmentFilter) + ? 'Segment filters can not be saved within other segments' + : undefined + const disabled = !!disabledReason + + return ( s} - onClick={() => setModal('create')} + onClick={disabled ? () => {} : () => setModal('create')} state={{ expandedSegment: null }} > Save as segment - ) : null + ) } diff --git a/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx b/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx index 5245d246d8b7..f1695c7f7a89 100644 --- a/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx +++ b/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx @@ -183,20 +183,12 @@ export const useSegmentPrefetch = ({ id }: { id: string }) => { typeof queryKey > = useCallback( async ({ queryKey: [_, id] }) => { - const res = await fetch( - `/api/${encodeURIComponent(site.domain)}/segments/${id}`, - { - method: 'GET', - headers: { - 'content-type': 'application/json', - accept: 'application/json' - } - } + const response: SavedSegment & { segment_data: SegmentData } = await get( + `/api/${encodeURIComponent(site.domain)}/segments/${id}` ) - const d = await res.json() return { - ...d, - segment_data: parseApiSegmentData(d.segment_data) + ...response, + segment_data: parseApiSegmentData(response.segment_data) } }, [site] @@ -227,7 +219,13 @@ export const useSegmentPrefetch = ({ id }: { id: string }) => { [queryClient, getSegmentFn, queryKey] ) - return { prefetchSegment, data: getSegment.data, fetchSegment } + return { + prefetchSegment, + data: getSegment.data, + fetchSegment, + error: getSegment.error, + status: getSegment.status + } } const SegmentLink = ({ diff --git a/assets/js/dashboard/nav-menu/top-bar.test.tsx b/assets/js/dashboard/nav-menu/top-bar.test.tsx index 832bcdf1e5a6..f75d20c837b8 100644 --- a/assets/js/dashboard/nav-menu/top-bar.test.tsx +++ b/assets/js/dashboard/nav-menu/top-bar.test.tsx @@ -98,7 +98,7 @@ test('user can open and close filters dropdown', async () => { }) test('current visitors renders when visitors are present and disappears after visitors are null', async () => { - mockAPI.get(`/api/stats/${domain}/current-visitors?`, 500) + mockAPI.get(`/api/stats/${domain}/current-visitors`, 500) render(, { wrapper: (props) => ( @@ -111,7 +111,7 @@ test('current visitors renders when visitors are present and disappears after vi ).toBeVisible() }) - mockAPI.get(`/api/stats/${domain}/current-visitors?`, null) + mockAPI.get(`/api/stats/${domain}/current-visitors`, null) fireEvent(document, new CustomEvent('tick')) await waitForElementToBeRemoved(() => screen.queryByRole('link', { name: /current visitors/ }) diff --git a/assets/js/dashboard/segments/segment-modals.tsx b/assets/js/dashboard/segments/segment-modals.tsx index 15c5e80170c6..12903480b77b 100644 --- a/assets/js/dashboard/segments/segment-modals.tsx +++ b/assets/js/dashboard/segments/segment-modals.tsx @@ -1,6 +1,6 @@ /** @format */ -import React, { ReactNode, useLayoutEffect, useState } from 'react' +import React, { ReactNode, useEffect, useState } from 'react' import ModalWithRouting from '../stats/modals/modal' import { isSegmentFilter, @@ -409,14 +409,32 @@ const FiltersInSegment = ({ segment_data }: { segment_data: SegmentData }) => { ) } +const Placeholder = ({ + children, + placeholder +}: { + children: ReactNode | false + placeholder: ReactNode +}) => ( + + {children === false ? placeholder : children} + +) + export const SegmentModal = ({ id }: { id: SavedSegment['id'] }) => { const { query } = useQueryContext() - const { data, fetchSegment } = useSegmentPrefetch({ + const { data, fetchSegment, status, error } = useSegmentPrefetch({ id: String(id) }) - useLayoutEffect(() => { + useEffect(() => { fetchSegment() }, [fetchSegment]) @@ -426,22 +444,23 @@ export const SegmentModal = ({ id }: { id: SavedSegment['id'] }) => {

- {data ? data.name : 'Segment'} + {data ? data.name : 'Segment details'}

- {data?.segment_data ? ( -
- { - { - [SegmentType.personal]: 'Personal segment', - [SegmentType.site]: 'Site segment' - }[data.type] - } -
- ) : null} + +
+ + {data?.segment_data + ? { + [SegmentType.personal]: 'Personal segment', + [SegmentType.site]: 'Site segment' + }[data.type] + : false} + +
- {data?.segment_data ? ( + {!!data?.segment_data && ( <> @@ -489,8 +508,24 @@ export const SegmentModal = ({ id }: { id: SavedSegment['id'] }) => {
- ) : ( -
+ )} + {status === 'pending' && ( +
+
+
+
+
+ )} + {error !== null && ( + fetchSegment()} + /> )}
diff --git a/lib/plausible/segments/segments.ex b/lib/plausible/segments/segments.ex index 63ac290244d8..4a52a60d8a9c 100644 --- a/lib/plausible/segments/segments.ex +++ b/lib/plausible/segments/segments.ex @@ -297,4 +297,19 @@ defmodule Plausible.Segments do Repo.all(query) end + + @doc """ + iex> serialize_first_error([{"name", {"should be at most %{count} byte(s)", [count: 255]}}]) + "name should be at most 255 byte(s)" + """ + def serialize_first_error(errors) do + {field, {message, opts}} = List.first(errors) + + formatted_message = + Enum.reduce(opts, message, fn {key, value}, acc -> + String.replace(acc, "%{#{key}}", to_string(value)) + end) + + "#{field} #{formatted_message}" + end end diff --git a/lib/plausible_web/controllers/api/internal/segments_controller.ex b/lib/plausible_web/controllers/api/internal/segments_controller.ex index a1df13bff790..b28db63f5d39 100644 --- a/lib/plausible_web/controllers/api/internal/segments_controller.ex +++ b/lib/plausible_web/controllers/api/internal/segments_controller.ex @@ -76,8 +76,7 @@ defmodule PlausibleWeb.Api.Internal.SegmentsController do conn |> put_status(400) |> json(%{ - errors: - Enum.map(errors, fn {field_key, {message, opts}} -> [field_key, message, opts] end) + error: Segments.serialize_first_error(errors) }) {:ok, segment} -> @@ -111,8 +110,7 @@ defmodule PlausibleWeb.Api.Internal.SegmentsController do conn |> put_status(400) |> json(%{ - errors: - Enum.map(errors, fn {field_key, {message, opts}} -> [field_key, message, opts] end) + error: Segments.serialize_first_error(errors) }) {:ok, segment} -> diff --git a/lib/plausible_web/controllers/stats_controller.ex b/lib/plausible_web/controllers/stats_controller.ex index 072eb352f168..a06a1cd41c79 100644 --- a/lib/plausible_web/controllers/stats_controller.ex +++ b/lib/plausible_web/controllers/stats_controller.ex @@ -403,7 +403,7 @@ defmodule PlausibleWeb.StatsController do nil end - defp get_members(_user, site = %Plausible.Site{}) do + defp get_members(_user, %Plausible.Site{} = site) do site = site |> Plausible.Repo.preload( diff --git a/lib/plausible_web/templates/stats/stats.html.heex b/lib/plausible_web/templates/stats/stats.html.heex index 854792f00a32..d729e115b6cb 100644 --- a/lib/plausible_web/templates/stats/stats.html.heex +++ b/lib/plausible_web/templates/stats/stats.html.heex @@ -42,7 +42,9 @@ data-background={@conn.assigns[:background]} data-is-dbip={to_string(@is_dbip)} data-current-user-role={@conn.assigns[:site_role]} - data-current-user-id={if user = @conn.assigns[:current_user], do: user.id, else: Jason.encode!(nil)} + data-current-user-id={ + if user = @conn.assigns[:current_user], do: user.id, else: Jason.encode!(nil) + } data-members={Jason.encode!(@members)} data-flags={Jason.encode!(@flags)} data-valid-intervals-by-period={ diff --git a/test/plausible/segments/segments_test.exs b/test/plausible/segments/segments_test.exs new file mode 100644 index 000000000000..4fc53ac01bbd --- /dev/null +++ b/test/plausible/segments/segments_test.exs @@ -0,0 +1,4 @@ +defmodule Plausible.SegmentsTest do + use ExUnit.Case + doctest Plausible.Segments, import: true +end diff --git a/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs b/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs index d306039c8d97..db59c0670669 100644 --- a/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs +++ b/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs @@ -272,36 +272,6 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do "updated_at" => NaiveDateTime.to_iso8601(segment.updated_at) } end - - test "serves dates correctly in site timezone", %{ - conn: conn, - user: user - } do - site = new_site(owner: user, timezone: "Australia/Sydney") - - segment = - insert(:segment, - site: site, - name: "any", - owner: user, - type: :personal, - inserted_at: "2025-02-01T00:00:00", - updated_at: "2025-02-01T00:00:00" - ) - - conn = - get(conn, "/api/#{site.domain}/segments/#{segment.id}") - - assert json_response(conn, 200) == %{ - "id" => segment.id, - "owner_id" => user.id, - "name" => segment.name, - "type" => Atom.to_string(segment.type), - "segment_data" => segment.segment_data, - "inserted_at" => "2025-02-01T11:00:00", - "updated_at" => "2025-02-01T11:00:00" - } - end end describe "POST /api/:domain/segments" do @@ -357,13 +327,8 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do }) assert json_response(conn, 400) == %{ - "errors" => [ - [ - "segment_data", - "#/filters/0: Invalid filter [\"is\", \"entry_page\", [\"/blog\"]]", - [] - ] - ] + "error" => + "segment_data #/filters/0: Invalid filter [\"is\", \"entry_page\", [\"/blog\"]]" } end @@ -447,19 +412,11 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do end end - for {filters, expected_errors} <- [ - {[], - [ - [ - "segment_data", - "property \"filters\" must be an array with at least one member", - [] - ] - ]}, - {[["foo", "bar"]], - [["segment_data", "#/filters/0: Invalid filter [\"foo\", \"bar\"]", []]]}, + for {filters, expected_error} <- [ + {[], "segment_data property \"filters\" must be an array with at least one member"}, + {[["foo", "bar"]], "segment_data #/filters/0: Invalid filter [\"foo\", \"bar\"]"}, {[["not", ["is", "visit:entry_page", ["/campaigns/:campaign_name"]]]], - [["segment_data", "Invalid filters. Deep filters are not supported.", []]]}, + "segment_data Invalid filters. Deep filters are not supported."}, {[ [ "or", @@ -469,15 +426,7 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do ] ] ], - [ - [ - "segment_data", - "Invalid filters. Dimension `event:goal` can only be filtered at the top level.", - [] - ] - ]}, - {[["has_not_done", ["is", "event:goal", ["a goal"]]]], - [["segment_data", "Invalid filters. Deep filters are not supported.", []]]} + "segment_data Invalid filters. Dimension `event:goal` can only be filtered at the top level."} ] do test "prevents owners from updating segments to invalid filters #{inspect(filters)} with error 400", %{ @@ -499,11 +448,35 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do }) assert json_response(conn, 400) == %{ - "errors" => unquote(expected_errors) + "error" => unquote(expected_error) } end end + test "prevents editors from updating segments name beyond 255 characters with error 400", + %{ + conn: conn, + user: user, + site: site + } do + segment = + insert(:segment, + site: site, + name: "any name", + type: :personal, + owner: user + ) + + conn = + patch(conn, "/api/#{site.domain}/segments/#{segment.id}", %{ + "name" => String.duplicate("a", 256) + }) + + assert json_response(conn, 400) == %{ + "error" => "name should be at most 255 byte(s)" + } + end + test "editors can update a segment", %{conn: conn, user: user} do site = new_site() add_guest(site, user: user, role: :editor) From 44fb20f928a40782b2a9fdbc7c5c65dc80d8e073 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Mon, 17 Feb 2025 13:47:35 +0200 Subject: [PATCH 09/25] Stop name copy from getting too long --- assets/js/dashboard/segments/segment-modals.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/assets/js/dashboard/segments/segment-modals.tsx b/assets/js/dashboard/segments/segment-modals.tsx index 12903480b77b..bd3eb2e0d7fe 100644 --- a/assets/js/dashboard/segments/segment-modals.tsx +++ b/assets/js/dashboard/segments/segment-modals.tsx @@ -84,7 +84,9 @@ export const CreateSegmentModal = ({ segment?: SavedSegment onSave: (input: Pick) => void }) => { - const defaultName = segment?.name ? `Copy of ${segment.name}` : '' + const defaultName = segment?.name + ? `Copy of ${segment.name}`.slice(0, 255) + : '' const [name, setName] = useState(defaultName) const defaultType = segment?.type === SegmentType.site && From 317403b03aeb21c72e815acce7ae3d3b0aca28a4 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Tue, 18 Feb 2025 15:24:07 +0200 Subject: [PATCH 10/25] Make comparison mode and edit segment modes exclusive --- .../js/dashboard/components/search-input.tsx | 2 +- .../query-periods/query-period-menu.tsx | 16 ++++++--- .../query-periods/query-periods-picker.tsx | 2 +- .../segments/searchable-segments-section.tsx | 13 +++++--- assets/js/dashboard/nav-menu/top-bar.tsx | 2 +- assets/js/dashboard/query-context.tsx | 10 ++++-- .../js/dashboard/query-time-periods.test.ts | 28 ++++++++++++++-- assets/js/dashboard/query-time-periods.ts | 19 +++++++---- .../segments/segment-expanded-context.tsx | 33 ++++++++++++++++--- 9 files changed, 96 insertions(+), 29 deletions(-) diff --git a/assets/js/dashboard/components/search-input.tsx b/assets/js/dashboard/components/search-input.tsx index 1c7b6e8fbced..245dd8533eab 100644 --- a/assets/js/dashboard/components/search-input.tsx +++ b/assets/js/dashboard/components/search-input.tsx @@ -59,7 +59,7 @@ export const SearchInput = ({ type="text" placeholder={isFocused ? placeholderFocused : placeholderUnfocused} className={classNames( - 'shadow-sm dark:bg-gray-900 dark:text-gray-100 focus:ring-indigo-500 focus:border-indigo-500 block sm:text-sm border-gray-300 dark:border-gray-500 rounded-md dark:bg-gray-800 w-48', + 'shadow-sm dark:bg-gray-900 dark:text-gray-100 focus:ring-indigo-500 focus:border-indigo-500 block border-gray-300 dark:border-gray-500 rounded-md dark:bg-gray-800 w-48', className )} onChange={debouncedOnSearchInputChange} diff --git a/assets/js/dashboard/nav-menu/query-periods/query-period-menu.tsx b/assets/js/dashboard/nav-menu/query-periods/query-period-menu.tsx index a03187cab52f..8f6808b3eeb8 100644 --- a/assets/js/dashboard/nav-menu/query-periods/query-period-menu.tsx +++ b/assets/js/dashboard/nav-menu/query-periods/query-period-menu.tsx @@ -16,16 +16,16 @@ import { useAppNavigate } from '../../navigation/use-app-navigate' import { - COMPARISON_DISABLED_PERIODS, getCompareLinkItem, last6MonthsLinkItem, getDatePeriodGroups, LinkItem, QueryPeriod, getCurrentPeriodDisplayName, - getSearchToApplyCustomDates + getSearchToApplyCustomDates, + isComparisonForbidden } from '../../query-time-periods' -import { useMatch } from 'react-router-dom' +import { useLocation, useMatch } from 'react-router-dom' import { rootRoute } from '../../router' import { Popover, Transition } from '@headlessui/react' import { popover } from '../../components/popover' @@ -40,6 +40,7 @@ import { import { DateRangeCalendar } from './date-range-calendar' import { formatISO, nowForSite } from '../../util/date' import { MenuSeparator } from '../nav-menu-components' +import { useIsSegmentExpanded } from '../../segments/segment-expanded-context' function QueryPeriodMenuKeybinds({ closeDropdown, @@ -122,6 +123,8 @@ const QueryPeriodMenuInner = ({ }) => { const site = useSiteContext() const { query } = useQueryContext() + const location = useLocation() + const segmentIsExpanded = useIsSegmentExpanded({ state: location.state }) const groups = useMemo(() => { const compareLink = getCompareLinkItem({ site, query }) @@ -138,11 +141,14 @@ const QueryPeriodMenuInner = ({ } ] ], - extraGroups: COMPARISON_DISABLED_PERIODS.includes(query.period) + extraGroups: isComparisonForbidden({ + period: query.period, + segmentIsExpanded + }) ? [] : [[compareLink]] }) - }, [site, query, closeDropdown, toggleCalendar]) + }, [site, query, closeDropdown, toggleCalendar, segmentIsExpanded]) return ( <> diff --git a/assets/js/dashboard/nav-menu/query-periods/query-periods-picker.tsx b/assets/js/dashboard/nav-menu/query-periods/query-periods-picker.tsx index 12f69b9912dc..684f4a12f528 100644 --- a/assets/js/dashboard/nav-menu/query-periods/query-periods-picker.tsx +++ b/assets/js/dashboard/nav-menu/query-periods/query-periods-picker.tsx @@ -40,7 +40,7 @@ export function QueryPeriodsPicker({ className }: { className?: string }) { {isComparing && ( <>
- vs. + vs.
{({ close }) => ( diff --git a/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx b/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx index f1695c7f7a89..3164d4b99643 100644 --- a/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx +++ b/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx @@ -99,7 +99,7 @@ export const SearchableSegmentsSection = ({ {data.length > initialSliceLength && ( )} @@ -154,6 +154,14 @@ export const SearchableSegmentsSection = ({ )} )} + {!!data?.length && searchValue && !showableSlice?.length && ( + +
+ No segments found. Clear search to show all. +
+ + )} + {listQuery.status === 'pending' && (
@@ -231,8 +239,6 @@ export const useSegmentPrefetch = ({ id }: { id: string }) => { const SegmentLink = ({ id, name, - // type, - // owner_id, appliedSegmentIds, closeList }: SavedSegment & { appliedSegmentIds: number[]; closeList: () => void }) => { @@ -244,7 +250,6 @@ const SegmentLink = ({ { diff --git a/assets/js/dashboard/nav-menu/top-bar.tsx b/assets/js/dashboard/nav-menu/top-bar.tsx index a4ab21c3e75b..c7586ff59ae1 100644 --- a/assets/js/dashboard/nav-menu/top-bar.tsx +++ b/assets/js/dashboard/nav-menu/top-bar.tsx @@ -85,8 +85,8 @@ function TopBarInner({ showCurrentVisitors }: TopBarProps) { />
- +
diff --git a/assets/js/dashboard/query-context.tsx b/assets/js/dashboard/query-context.tsx index 8acf552e12ee..f4ad680e6914 100644 --- a/assets/js/dashboard/query-context.tsx +++ b/assets/js/dashboard/query-context.tsx @@ -19,6 +19,7 @@ import { queryDefaultValue, postProcessFilters } from './query' +import { useIsSegmentExpanded } from './segments/segment-expanded-context' const queryContextDefaultValue = { query: queryDefaultValue, @@ -55,14 +56,16 @@ export default function QueryContextProvider({ ...otherSearch } = useMemo(() => parseSearch(location.search), [location.search]) + const segmentIsExpanded = useIsSegmentExpanded({ state: location.state }) + const query = useMemo(() => { const defaultValues = queryDefaultValue const storedValues = getSavedTimePreferencesFromStorage({ site }) - const timeQuery = getDashboardTimeSettings({ searchValues: { period, comparison, match_day_of_week }, storedValues, - defaultValues + defaultValues, + segmentIsExpanded }) return { @@ -111,7 +114,8 @@ export default function QueryContextProvider({ period, to, with_imported, - site + site, + segmentIsExpanded ]) useSaveTimePreferencesToStorage({ diff --git a/assets/js/dashboard/query-time-periods.test.ts b/assets/js/dashboard/query-time-periods.test.ts index b2361092d11d..3e9ffc4c5ae1 100644 --- a/assets/js/dashboard/query-time-periods.test.ts +++ b/assets/js/dashboard/query-time-periods.test.ts @@ -45,7 +45,8 @@ describe(`${getDashboardTimeSettings.name}`, () => { getDashboardTimeSettings({ searchValues: emptySearchValues, storedValues: emptyStoredValues, - defaultValues + defaultValues, + segmentIsExpanded: false }) ).toEqual(defaultValues) }) @@ -59,7 +60,8 @@ describe(`${getDashboardTimeSettings.name}`, () => { comparison: ComparisonMode.year_over_year, match_day_of_week: false }, - defaultValues + defaultValues, + segmentIsExpanded: false }) ).toEqual({ period: QueryPeriod['12mo'], @@ -81,7 +83,8 @@ describe(`${getDashboardTimeSettings.name}`, () => { comparison: ComparisonMode.year_over_year, match_day_of_week: false }, - defaultValues + defaultValues, + segmentIsExpanded: false }) ).toEqual({ period: QueryPeriod['year'], @@ -89,4 +92,23 @@ describe(`${getDashboardTimeSettings.name}`, () => { match_day_of_week: true }) }) + + it('respects segmentIsExpanded: true option', () => { + expect( + getDashboardTimeSettings({ + searchValues: { + period: QueryPeriod['custom'], + comparison: ComparisonMode.previous_period, + match_day_of_week: true + }, + storedValues: emptyStoredValues, + defaultValues, + segmentIsExpanded: true + }) + ).toEqual({ + period: QueryPeriod['custom'], + comparison: null, + match_day_of_week: true + }) + }) }) diff --git a/assets/js/dashboard/query-time-periods.ts b/assets/js/dashboard/query-time-periods.ts index d362df647417..cdc2d25fb85a 100644 --- a/assets/js/dashboard/query-time-periods.ts +++ b/assets/js/dashboard/query-time-periods.ts @@ -64,10 +64,15 @@ export const COMPARISON_MATCH_MODE_LABELS = { export const DEFAULT_COMPARISON_MODE = ComparisonMode.previous_period -export const COMPARISON_DISABLED_PERIODS = [ - QueryPeriod.realtime, - QueryPeriod.all -] +const COMPARISON_DISABLED_PERIODS = [QueryPeriod.realtime, QueryPeriod.all] + +export const isComparisonForbidden = ({ + period, + segmentIsExpanded +}: { + period: QueryPeriod + segmentIsExpanded: boolean +}) => COMPARISON_DISABLED_PERIODS.includes(period) || segmentIsExpanded export const DEFAULT_COMPARISON_MATCH_MODE = ComparisonMatchMode.MatchDayOfWeek @@ -501,7 +506,8 @@ export function getSavedTimePreferencesFromStorage({ export function getDashboardTimeSettings({ searchValues, storedValues, - defaultValues + defaultValues, + segmentIsExpanded }: { searchValues: Record<'period' | 'comparison' | 'match_day_of_week', unknown> storedValues: ReturnType @@ -509,6 +515,7 @@ export function getDashboardTimeSettings({ DashboardQuery, 'period' | 'comparison' | 'match_day_of_week' > + segmentIsExpanded: boolean }): Pick { let period: QueryPeriod if (isValidPeriod(searchValues.period)) { @@ -521,7 +528,7 @@ export function getDashboardTimeSettings({ let comparison: ComparisonMode | null - if ([QueryPeriod.realtime, QueryPeriod.all].includes(period)) { + if (isComparisonForbidden({ period, segmentIsExpanded })) { comparison = null } else { comparison = isValidComparison(searchValues.comparison) diff --git a/assets/js/dashboard/segments/segment-expanded-context.tsx b/assets/js/dashboard/segments/segment-expanded-context.tsx index 93bdc1b088b9..3988c8bd9a75 100644 --- a/assets/js/dashboard/segments/segment-expanded-context.tsx +++ b/assets/js/dashboard/segments/segment-expanded-context.tsx @@ -4,6 +4,7 @@ import React, { ReactNode, useContext, useEffect, + useMemo, useState } from 'react' import { SavedSegment, SegmentData } from '../filtering/segments' @@ -34,6 +35,29 @@ export const useSegmentExpandedContext = () => { return useContext(SegmentExpandedContext) } +const getExpandedSegment = ({ + state +}: { + state: Record | undefined +}): SavedSegment & { segment_data: SegmentData } => { + const locationStateExpandedSegment = state?.expandedSegment + return locationStateExpandedSegment as SavedSegment & { + segment_data: SegmentData + } +} + +export const useIsSegmentExpanded = ({ + state +}: { + state: Record +}) => { + const isSegmentExpanded = useMemo( + () => !!getExpandedSegment({ state }), + [state] + ) + return isSegmentExpanded +} + export default function SegmentExpandedContextProvider({ children }: { @@ -50,7 +74,9 @@ export default function SegmentExpandedContextProvider({ const { query } = useQueryContext() const navigate = useAppNavigate() - const locationStateExpandedSegment = location.state?.expandedSegment + const locationStateExpandedSegment = getExpandedSegment({ + state: location.state + }) useEffect(() => { // copy location.state to state @@ -58,12 +84,10 @@ export default function SegmentExpandedContextProvider({ setState({ expandedSegment: locationStateExpandedSegment }) - } - if (locationStateExpandedSegment === null) { + } else if (locationStateExpandedSegment === null) { setState({ expandedSegment: segmentExpandedContextDefaultValue.expandedSegment }) - // setModal(segmentExpandedContextDefaultValue.modal) } }, [locationStateExpandedSegment]) @@ -79,7 +103,6 @@ export default function SegmentExpandedContextProvider({ }) // overwrite undefined locationState with current expandedSegment, to handle Back navigation correctly } else if (locationStateExpandedSegment === undefined) { - // console.log('Slowness') navigate({ search: (s) => s, state: { From 039dcc2cecef7d423b3cef295df3045b45fff2b9 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Tue, 18 Feb 2025 17:21:14 +0200 Subject: [PATCH 11/25] Fix flicker calculating space --- assets/js/dashboard/dashboard-keybinds.tsx | 4 +- assets/js/dashboard/nav-menu/filters-bar.tsx | 4 +- .../query-periods/query-period-menu.tsx | 11 +-- .../segments/searchable-segments-section.tsx | 5 +- .../nav-menu/segments/segment-menu.tsx | 4 +- assets/js/dashboard/query-context.tsx | 85 +++++++++++++++++-- assets/js/dashboard/router.tsx | 13 ++- .../segments/routeless-segment-modals.tsx | 4 +- 8 files changed, 92 insertions(+), 38 deletions(-) diff --git a/assets/js/dashboard/dashboard-keybinds.tsx b/assets/js/dashboard/dashboard-keybinds.tsx index 6153b4c9f7ba..81e8344b353e 100644 --- a/assets/js/dashboard/dashboard-keybinds.tsx +++ b/assets/js/dashboard/dashboard-keybinds.tsx @@ -1,7 +1,7 @@ /* @format */ import React from 'react' import { NavigateKeybind } from './keybinding' -import { useSegmentExpandedContext } from './segments/segment-expanded-context' +import { useQueryContext } from './query-context' const ClearFiltersKeybind = () => ( ( ) export function DashboardKeybinds() { - const { modal } = useSegmentExpandedContext() + const { modal } = useQueryContext() return modal === null && } diff --git a/assets/js/dashboard/nav-menu/filters-bar.tsx b/assets/js/dashboard/nav-menu/filters-bar.tsx index 92606466f427..1ddcdcdfb774 100644 --- a/assets/js/dashboard/nav-menu/filters-bar.tsx +++ b/assets/js/dashboard/nav-menu/filters-bar.tsx @@ -9,7 +9,6 @@ import { AppNavigationLink } from '../navigation/use-app-navigate' import { Popover, Transition } from '@headlessui/react' import { popover } from '../components/popover' import { BlurMenuButtonOnEscape } from '../keybinding' -import { useSegmentExpandedContext } from '../segments/segment-expanded-context' import { isSegmentFilter } from '../filtering/segments' // Component structure is @@ -250,8 +249,7 @@ const ClearAction = () => ( ) const SaveAsSegmentAction = () => { - const { query } = useQueryContext() - const { expandedSegment, setModal } = useSegmentExpandedContext() + const { query, expandedSegment, setModal } = useQueryContext() if (expandedSegment) { return null } diff --git a/assets/js/dashboard/nav-menu/query-periods/query-period-menu.tsx b/assets/js/dashboard/nav-menu/query-periods/query-period-menu.tsx index 8f6808b3eeb8..c4bfb342dcc7 100644 --- a/assets/js/dashboard/nav-menu/query-periods/query-period-menu.tsx +++ b/assets/js/dashboard/nav-menu/query-periods/query-period-menu.tsx @@ -25,7 +25,7 @@ import { getSearchToApplyCustomDates, isComparisonForbidden } from '../../query-time-periods' -import { useLocation, useMatch } from 'react-router-dom' +import { useMatch } from 'react-router-dom' import { rootRoute } from '../../router' import { Popover, Transition } from '@headlessui/react' import { popover } from '../../components/popover' @@ -40,7 +40,6 @@ import { import { DateRangeCalendar } from './date-range-calendar' import { formatISO, nowForSite } from '../../util/date' import { MenuSeparator } from '../nav-menu-components' -import { useIsSegmentExpanded } from '../../segments/segment-expanded-context' function QueryPeriodMenuKeybinds({ closeDropdown, @@ -122,9 +121,7 @@ const QueryPeriodMenuInner = ({ toggleCalendar: () => void }) => { const site = useSiteContext() - const { query } = useQueryContext() - const location = useLocation() - const segmentIsExpanded = useIsSegmentExpanded({ state: location.state }) + const { query, expandedSegment } = useQueryContext() const groups = useMemo(() => { const compareLink = getCompareLinkItem({ site, query }) @@ -143,12 +140,12 @@ const QueryPeriodMenuInner = ({ ], extraGroups: isComparisonForbidden({ period: query.period, - segmentIsExpanded + segmentIsExpanded: !!expandedSegment }) ? [] : [[compareLink]] }) - }, [site, query, closeDropdown, toggleCalendar, segmentIsExpanded]) + }, [site, query, closeDropdown, toggleCalendar, expandedSegment]) return ( <> diff --git a/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx b/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx index 3164d4b99643..9ade6765485a 100644 --- a/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx +++ b/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx @@ -15,7 +15,6 @@ import { QueryFunction, useQuery, useQueryClient } from '@tanstack/react-query' import { cleanLabels } from '../../util/filters' import classNames from 'classnames' import { Tooltip } from '../../util/tooltip' -import { useSegmentExpandedContext } from '../../segments/segment-expanded-context' import { SegmentAuthorship } from '../../segments/segment-authorship' import { SearchInput } from '../../components/search-input' import { EllipsisHorizontalIcon } from '@heroicons/react/24/solid' @@ -59,7 +58,7 @@ export const SearchableSegmentsSection = ({ }: { closeList: () => void }) => { - const { query } = useQueryContext() + const { query, expandedSegment } = useQueryContext() const segmentFilter = query.filters.find(isSegmentFilter) const appliedSegmentIds = (segmentFilter ? segmentFilter[2] : []) as number[] @@ -81,8 +80,6 @@ export const SearchableSegmentsSection = ({ ? filteredData : filteredData?.slice(0, initialSliceLength) - const { expandedSegment } = useSegmentExpandedContext() - if (expandedSegment) { return null } diff --git a/assets/js/dashboard/nav-menu/segments/segment-menu.tsx b/assets/js/dashboard/nav-menu/segments/segment-menu.tsx index df35c0cc88a5..71f6cece84c5 100644 --- a/assets/js/dashboard/nav-menu/segments/segment-menu.tsx +++ b/assets/js/dashboard/nav-menu/segments/segment-menu.tsx @@ -2,7 +2,6 @@ import React from 'react' import classNames from 'classnames' -import { useSegmentExpandedContext } from '../../segments/segment-expanded-context' import { Popover, Transition } from '@headlessui/react' import { popover } from '../../components/popover' import { AppNavigationLink } from '../../navigation/use-app-navigate' @@ -13,6 +12,7 @@ import { XMarkIcon } from '@heroicons/react/24/outline' import { ChevronDownIcon } from '@heroicons/react/20/solid' +import { useQueryContext } from '../../query-context' const linkClassName = classNames( popover.items.classNames.navigationLink, @@ -25,7 +25,7 @@ const buttonClassName = classNames( ) export const SegmentMenu = () => { - const { expandedSegment, setModal } = useSegmentExpandedContext() + const { expandedSegment, setModal } = useQueryContext() if (!expandedSegment) { return null diff --git a/assets/js/dashboard/query-context.tsx b/assets/js/dashboard/query-context.tsx index f4ad680e6914..9e2ca5f4002d 100644 --- a/assets/js/dashboard/query-context.tsx +++ b/assets/js/dashboard/query-context.tsx @@ -1,5 +1,12 @@ /* @format */ -import React, { createContext, useMemo, useContext, ReactNode } from 'react' +import React, { + createContext, + useMemo, + useContext, + ReactNode, + useEffect, + useState +} from 'react' import { useLocation } from 'react-router' import { useMountedEffect } from './custom-hooks' import * as api from './api' @@ -19,11 +26,16 @@ import { queryDefaultValue, postProcessFilters } from './query' -import { useIsSegmentExpanded } from './segments/segment-expanded-context' +import { SavedSegment, SegmentData } from './filtering/segments' +import { useAppNavigate } from './navigation/use-app-navigate' +import { SegmentModalState } from './segments/segment-expanded-context' const queryContextDefaultValue = { query: queryDefaultValue, - otherSearch: {} as Record + otherSearch: {} as Record, + expandedSegment: null as SavedSegment | null, + modal: null as SegmentModalState, + setModal: (_modal: SegmentModalState) => {} } export type QueryContextValue = typeof queryContextDefaultValue @@ -34,13 +46,49 @@ export const useQueryContext = () => { return useContext(QueryContext) } +function useDefiniteLocation() { + const location = useLocation() + const navigate = useAppNavigate() + // Initialize with location.state if defined, otherwise null. + const [definiteState, setDefiniteState] = useState( + location.state !== undefined ? (location.state as T) : null + ) + + // Effect: Whenever explicitState changes, sync it into location.state. + useEffect(() => { + // Normalize location.state so that undefined is treated as null. + if (location.state === undefined) { + navigate({ + search: (s) => s, + replace: true, + state: definiteState ?? null + }) + } + }, [definiteState, location.state, navigate]) + + useEffect(() => { + if (location.state !== undefined && location.state !== definiteState) { + setDefiniteState(location.state as T) + } + }, [location.state, definiteState]) + + return { location, definiteState } +} + export default function QueryContextProvider({ children }: { children: ReactNode }) { - const location = useLocation() + const { location, definiteState } = useDefiniteLocation<{ + expandedSegment: SavedSegment & { segment_data: SegmentData } + }>() + const navigate = useAppNavigate() const site = useSiteContext() + + const expandedSegment = definiteState?.expandedSegment ?? null + const [modal, setModal] = useState(null) + const { compare_from, compare_to, @@ -56,8 +104,6 @@ export default function QueryContextProvider({ ...otherSearch } = useMemo(() => parseSearch(location.search), [location.search]) - const segmentIsExpanded = useIsSegmentExpanded({ state: location.state }) - const query = useMemo(() => { const defaultValues = queryDefaultValue const storedValues = getSavedTimePreferencesFromStorage({ site }) @@ -65,7 +111,7 @@ export default function QueryContextProvider({ searchValues: { period, comparison, match_day_of_week }, storedValues, defaultValues, - segmentIsExpanded + segmentIsExpanded: !!expandedSegment }) return { @@ -115,9 +161,22 @@ export default function QueryContextProvider({ to, with_imported, site, - segmentIsExpanded + expandedSegment ]) + useEffect(() => { + // clear edit mode on clearing all filters + if (!query.filters.length && expandedSegment) { + navigate({ + search: (s) => s, + state: { + expandedSegment: null + }, + replace: true + }) + } + }, [query.filters, expandedSegment, navigate]) + useSaveTimePreferencesToStorage({ site, period, @@ -130,7 +189,15 @@ export default function QueryContextProvider({ }, []) return ( - + {children} ) diff --git a/assets/js/dashboard/router.tsx b/assets/js/dashboard/router.tsx index d208df091316..af2a67505ea0 100644 --- a/assets/js/dashboard/router.tsx +++ b/assets/js/dashboard/router.tsx @@ -27,7 +27,6 @@ import FilterModal from './stats/modals/filter-modal' import QueryContextProvider from './query-context' import { DashboardKeybinds } from './dashboard-keybinds' import LastLoadContextProvider from './last-load-context' -import SegmentExpandedContextProvider from './segments/segment-expanded-context' const queryClient = new QueryClient({ defaultOptions: { @@ -41,13 +40,11 @@ function DashboardElement() { return ( - - - - {/** render any children of the root route below */} - - - + + + {/** render any children of the root route below */} + + ) diff --git a/assets/js/dashboard/segments/routeless-segment-modals.tsx b/assets/js/dashboard/segments/routeless-segment-modals.tsx index 665854de02d4..857f7a205228 100644 --- a/assets/js/dashboard/segments/routeless-segment-modals.tsx +++ b/assets/js/dashboard/segments/routeless-segment-modals.tsx @@ -1,7 +1,6 @@ /** @format */ import React from 'react' -import { useSegmentExpandedContext } from './segment-expanded-context' import { CreateSegmentModal, DeleteSegmentModal, @@ -26,9 +25,8 @@ export const RoutelessSegmentModals = () => { const navigate = useAppNavigate() const queryClient = useQueryClient() const site = useSiteContext() - const { query } = useQueryContext() + const { query, expandedSegment, modal, setModal } = useQueryContext() const user = useUserContext() - const { expandedSegment, modal, setModal } = useSegmentExpandedContext() const patchSegment = useMutation({ mutationFn: async ({ From b009a9afce660f61672f659a84959399e255e01e Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Tue, 18 Feb 2025 20:43:48 +0200 Subject: [PATCH 12/25] Fix issue with definite state not persisting --- assets/js/dashboard/query-context.tsx | 39 ++++++++++--------- .../js/dashboard/segments/segment-modals.tsx | 3 +- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/assets/js/dashboard/query-context.tsx b/assets/js/dashboard/query-context.tsx index 9e2ca5f4002d..03f5341d9da6 100644 --- a/assets/js/dashboard/query-context.tsx +++ b/assets/js/dashboard/query-context.tsx @@ -33,7 +33,9 @@ import { SegmentModalState } from './segments/segment-expanded-context' const queryContextDefaultValue = { query: queryDefaultValue, otherSearch: {} as Record, - expandedSegment: null as SavedSegment | null, + expandedSegment: null as + | (SavedSegment & { segment_data: SegmentData }) + | null, modal: null as SegmentModalState, setModal: (_modal: SegmentModalState) => {} } @@ -46,33 +48,35 @@ export const useQueryContext = () => { return useContext(QueryContext) } -function useDefiniteLocation() { +function useDefiniteLocationState(key: string) { const location = useLocation() + const rawState = useMemo(() => location.state ?? {}, [location.state]) + const navigate = useAppNavigate() // Initialize with location.state if defined, otherwise null. - const [definiteState, setDefiniteState] = useState( - location.state !== undefined ? (location.state as T) : null + const [definiteValue, setDefiniteValue] = useState( + rawState[key] === undefined ? null : (rawState[key] as T) ) // Effect: Whenever explicitState changes, sync it into location.state. useEffect(() => { // Normalize location.state so that undefined is treated as null. - if (location.state === undefined) { + if (rawState[key] === undefined) { navigate({ search: (s) => s, - replace: true, - state: definiteState ?? null + state: { ...rawState, [key]: definiteValue }, + replace: true }) } - }, [definiteState, location.state, navigate]) + }, [definiteValue, rawState, navigate, key]) useEffect(() => { - if (location.state !== undefined && location.state !== definiteState) { - setDefiniteState(location.state as T) + if (rawState[key] !== undefined && rawState[key] !== definiteValue) { + setDefiniteValue(rawState[key] as T) } - }, [location.state, definiteState]) + }, [rawState, definiteValue, key]) - return { location, definiteState } + return { location, definiteValue } } export default function QueryContextProvider({ @@ -80,13 +84,12 @@ export default function QueryContextProvider({ }: { children: ReactNode }) { - const { location, definiteState } = useDefiniteLocation<{ - expandedSegment: SavedSegment & { segment_data: SegmentData } - }>() + const { location, definiteValue: expandedSegment } = useDefiniteLocationState< + SavedSegment & { segment_data: SegmentData } + >('expandedSegment') const navigate = useAppNavigate() const site = useSiteContext() - const expandedSegment = definiteState?.expandedSegment ?? null const [modal, setModal] = useState(null) const { @@ -165,8 +168,8 @@ export default function QueryContextProvider({ ]) useEffect(() => { - // clear edit mode on clearing all filters - if (!query.filters.length && expandedSegment) { + // clear edit mode on clearing all filters or removing last filter + if (!!expandedSegment && !query.filters.length) { navigate({ search: (s) => s, state: { diff --git a/assets/js/dashboard/segments/segment-modals.tsx b/assets/js/dashboard/segments/segment-modals.tsx index bd3eb2e0d7fe..a238643c373b 100644 --- a/assets/js/dashboard/segments/segment-modals.tsx +++ b/assets/js/dashboard/segments/segment-modals.tsx @@ -478,8 +478,7 @@ export const SegmentModal = ({ id }: { id: SavedSegment['id'] }) => { labels: data.segment_data.labels })} state={{ - expandedSegment: data, - modal: null + expandedSegment: data }} > Edit segment From a4e0b51a0352641de17fea83aadddebf8b433911 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 19 Feb 2025 14:22:38 +0200 Subject: [PATCH 13/25] Unhitch modals from query-context --- assets/js/dashboard/nav-menu/filters-bar.tsx | 4 +- .../nav-menu/segments/segment-menu.tsx | 4 +- .../navigation/routeless-modals-context.tsx | 46 ++++++ .../use-definite-location-state.tsx | 42 ++++++ assets/js/dashboard/query-context.tsx | 51 +------ assets/js/dashboard/router.tsx | 17 ++- .../segments/routeless-segment-modals.tsx | 6 +- .../segments/segment-expanded-context.tsx | 132 ------------------ 8 files changed, 115 insertions(+), 187 deletions(-) create mode 100644 assets/js/dashboard/navigation/routeless-modals-context.tsx create mode 100644 assets/js/dashboard/navigation/use-definite-location-state.tsx delete mode 100644 assets/js/dashboard/segments/segment-expanded-context.tsx diff --git a/assets/js/dashboard/nav-menu/filters-bar.tsx b/assets/js/dashboard/nav-menu/filters-bar.tsx index 1ddcdcdfb774..6a00564ec0f9 100644 --- a/assets/js/dashboard/nav-menu/filters-bar.tsx +++ b/assets/js/dashboard/nav-menu/filters-bar.tsx @@ -10,6 +10,7 @@ import { Popover, Transition } from '@headlessui/react' import { popover } from '../components/popover' import { BlurMenuButtonOnEscape } from '../keybinding' import { isSegmentFilter } from '../filtering/segments' +import { useRoutelessModalsContext } from '../navigation/routeless-modals-context' // Component structure is // `..[ filter (x) ]..[ filter (x) ]..[ three dot menu ]..` @@ -249,7 +250,8 @@ const ClearAction = () => ( ) const SaveAsSegmentAction = () => { - const { query, expandedSegment, setModal } = useQueryContext() + const { setModal } = useRoutelessModalsContext() + const { query, expandedSegment } = useQueryContext() if (expandedSegment) { return null } diff --git a/assets/js/dashboard/nav-menu/segments/segment-menu.tsx b/assets/js/dashboard/nav-menu/segments/segment-menu.tsx index 71f6cece84c5..8bb8cf9963d9 100644 --- a/assets/js/dashboard/nav-menu/segments/segment-menu.tsx +++ b/assets/js/dashboard/nav-menu/segments/segment-menu.tsx @@ -13,6 +13,7 @@ import { } from '@heroicons/react/24/outline' import { ChevronDownIcon } from '@heroicons/react/20/solid' import { useQueryContext } from '../../query-context' +import { useRoutelessModalsContext } from '../../navigation/routeless-modals-context' const linkClassName = classNames( popover.items.classNames.navigationLink, @@ -25,7 +26,8 @@ const buttonClassName = classNames( ) export const SegmentMenu = () => { - const { expandedSegment, setModal } = useQueryContext() + const { setModal } = useRoutelessModalsContext() + const { expandedSegment } = useQueryContext() if (!expandedSegment) { return null diff --git a/assets/js/dashboard/navigation/routeless-modals-context.tsx b/assets/js/dashboard/navigation/routeless-modals-context.tsx new file mode 100644 index 000000000000..83c6e969ed73 --- /dev/null +++ b/assets/js/dashboard/navigation/routeless-modals-context.tsx @@ -0,0 +1,46 @@ +/* @format */ +import React, { createContext, ReactNode, useContext, useState } from 'react' +import { + RoutelessSegmentModal, + RoutelessSegmentModals +} from '../segments/routeless-segment-modals' + +type ActiveModal = null | RoutelessSegmentModal + +const routelessModalsContextDefaultValue: { + modal: ActiveModal + setModal: (modal: ActiveModal) => void +} = { + modal: null, + setModal: () => {} +} + +const RoutelessModalsContext = createContext< + typeof routelessModalsContextDefaultValue +>(routelessModalsContextDefaultValue) + +export const useRoutelessModalsContext = () => { + return useContext(RoutelessModalsContext) +} + +export function RoutelessModalsContextProvider({ + children +}: { + children: ReactNode +}) { + const [modal, setModal] = useState( + routelessModalsContextDefaultValue.modal + ) + + return ( + + + {children} + + ) +} diff --git a/assets/js/dashboard/navigation/use-definite-location-state.tsx b/assets/js/dashboard/navigation/use-definite-location-state.tsx new file mode 100644 index 000000000000..0de95bc81e22 --- /dev/null +++ b/assets/js/dashboard/navigation/use-definite-location-state.tsx @@ -0,0 +1,42 @@ +/** @format */ + +import { useEffect, useMemo, useState } from 'react' +import { useLocation } from 'react-router-dom' +import { useAppNavigate } from './use-app-navigate' + +/** + * This hook should be rendered once in the app, since it hooks into each navigation + * (re-navigating with replace: true on most navigations). It does so to + * replace undefined `location.state[key]` with a known definite value. + * The way to unset `location.state[key]` is to set it to null in navigation. + */ +export function useDefiniteLocationState(key: string): { + definiteValue: T | null +} { + const location = useLocation() + const rawState = useMemo(() => location.state ?? {}, [location.state]) + + const navigate = useAppNavigate() + + const [definiteValue, setDefiniteValue] = useState( + rawState[key] === undefined ? null : (rawState[key] as T) + ) + + useEffect(() => { + if (rawState[key] === undefined) { + navigate({ + search: (s) => s, + state: { ...rawState, [key]: definiteValue }, + replace: true + }) + } + }, [definiteValue, rawState, navigate, key]) + + useEffect(() => { + if (rawState[key] !== undefined && rawState[key] !== definiteValue) { + setDefiniteValue(rawState[key] as T) + } + }, [rawState, definiteValue, key]) + + return { definiteValue } +} diff --git a/assets/js/dashboard/query-context.tsx b/assets/js/dashboard/query-context.tsx index 03f5341d9da6..481cf90b6453 100644 --- a/assets/js/dashboard/query-context.tsx +++ b/assets/js/dashboard/query-context.tsx @@ -4,8 +4,7 @@ import React, { useMemo, useContext, ReactNode, - useEffect, - useState + useEffect } from 'react' import { useLocation } from 'react-router' import { useMountedEffect } from './custom-hooks' @@ -28,16 +27,12 @@ import { } from './query' import { SavedSegment, SegmentData } from './filtering/segments' import { useAppNavigate } from './navigation/use-app-navigate' -import { SegmentModalState } from './segments/segment-expanded-context' +import { useDefiniteLocationState } from './navigation/use-definite-location-state' const queryContextDefaultValue = { query: queryDefaultValue, otherSearch: {} as Record, - expandedSegment: null as - | (SavedSegment & { segment_data: SegmentData }) - | null, - modal: null as SegmentModalState, - setModal: (_modal: SegmentModalState) => {} + expandedSegment: null as (SavedSegment & { segment_data: SegmentData }) | null } export type QueryContextValue = typeof queryContextDefaultValue @@ -48,50 +43,18 @@ export const useQueryContext = () => { return useContext(QueryContext) } -function useDefiniteLocationState(key: string) { - const location = useLocation() - const rawState = useMemo(() => location.state ?? {}, [location.state]) - - const navigate = useAppNavigate() - // Initialize with location.state if defined, otherwise null. - const [definiteValue, setDefiniteValue] = useState( - rawState[key] === undefined ? null : (rawState[key] as T) - ) - - // Effect: Whenever explicitState changes, sync it into location.state. - useEffect(() => { - // Normalize location.state so that undefined is treated as null. - if (rawState[key] === undefined) { - navigate({ - search: (s) => s, - state: { ...rawState, [key]: definiteValue }, - replace: true - }) - } - }, [definiteValue, rawState, navigate, key]) - - useEffect(() => { - if (rawState[key] !== undefined && rawState[key] !== definiteValue) { - setDefiniteValue(rawState[key] as T) - } - }, [rawState, definiteValue, key]) - - return { location, definiteValue } -} - export default function QueryContextProvider({ children }: { children: ReactNode }) { - const { location, definiteValue: expandedSegment } = useDefiniteLocationState< + const location = useLocation() + const { definiteValue: expandedSegment } = useDefiniteLocationState< SavedSegment & { segment_data: SegmentData } >('expandedSegment') const navigate = useAppNavigate() const site = useSiteContext() - const [modal, setModal] = useState(null) - const { compare_from, compare_to, @@ -196,9 +159,7 @@ export default function QueryContextProvider({ value={{ query, otherSearch, - expandedSegment, - modal, - setModal + expandedSegment }} > {children} diff --git a/assets/js/dashboard/router.tsx b/assets/js/dashboard/router.tsx index af2a67505ea0..1aaf38ec05b2 100644 --- a/assets/js/dashboard/router.tsx +++ b/assets/js/dashboard/router.tsx @@ -27,6 +27,7 @@ import FilterModal from './stats/modals/filter-modal' import QueryContextProvider from './query-context' import { DashboardKeybinds } from './dashboard-keybinds' import LastLoadContextProvider from './last-load-context' +import { RoutelessModalsContextProvider } from './navigation/routeless-modals-context' const queryClient = new QueryClient({ defaultOptions: { @@ -39,13 +40,15 @@ const queryClient = new QueryClient({ function DashboardElement() { return ( - - - - {/** render any children of the root route below */} - - - + + + + + {/** render any children of the root route below */} + + + + ) } diff --git a/assets/js/dashboard/segments/routeless-segment-modals.tsx b/assets/js/dashboard/segments/routeless-segment-modals.tsx index 857f7a205228..cd96c1b61690 100644 --- a/assets/js/dashboard/segments/routeless-segment-modals.tsx +++ b/assets/js/dashboard/segments/routeless-segment-modals.tsx @@ -20,12 +20,16 @@ import { useAppNavigate } from '../navigation/use-app-navigate' import { useQueryContext } from '../query-context' import { Role, useUserContext } from '../user-context' import { mutation } from '../api' +import { useRoutelessModalsContext } from '../navigation/routeless-modals-context' + +export type RoutelessSegmentModal = 'create' | 'update' | 'delete' export const RoutelessSegmentModals = () => { const navigate = useAppNavigate() const queryClient = useQueryClient() const site = useSiteContext() - const { query, expandedSegment, modal, setModal } = useQueryContext() + const { modal, setModal } = useRoutelessModalsContext() + const { query, expandedSegment } = useQueryContext() const user = useUserContext() const patchSegment = useMutation({ diff --git a/assets/js/dashboard/segments/segment-expanded-context.tsx b/assets/js/dashboard/segments/segment-expanded-context.tsx deleted file mode 100644 index 3988c8bd9a75..000000000000 --- a/assets/js/dashboard/segments/segment-expanded-context.tsx +++ /dev/null @@ -1,132 +0,0 @@ -/* @format */ -import React, { - createContext, - ReactNode, - useContext, - useEffect, - useMemo, - useState -} from 'react' -import { SavedSegment, SegmentData } from '../filtering/segments' -import { useLocation } from 'react-router-dom' -import { useAppNavigate } from '../navigation/use-app-navigate' -import { useQueryContext } from '../query-context' - -export type SegmentExpandedState = { - expandedSegment: (SavedSegment & { segment_data: SegmentData }) | null -} - -export type SegmentModalState = null | 'create' | 'update' | 'delete' - -const segmentExpandedContextDefaultValue: SegmentExpandedState & { - modal: SegmentModalState - setModal: (modal: SegmentModalState) => void -} = { - expandedSegment: null, - modal: null, - setModal: () => {} -} - -const SegmentExpandedContext = createContext< - typeof segmentExpandedContextDefaultValue ->(segmentExpandedContextDefaultValue) - -export const useSegmentExpandedContext = () => { - return useContext(SegmentExpandedContext) -} - -const getExpandedSegment = ({ - state -}: { - state: Record | undefined -}): SavedSegment & { segment_data: SegmentData } => { - const locationStateExpandedSegment = state?.expandedSegment - return locationStateExpandedSegment as SavedSegment & { - segment_data: SegmentData - } -} - -export const useIsSegmentExpanded = ({ - state -}: { - state: Record -}) => { - const isSegmentExpanded = useMemo( - () => !!getExpandedSegment({ state }), - [state] - ) - return isSegmentExpanded -} - -export default function SegmentExpandedContextProvider({ - children -}: { - children: ReactNode -}) { - const location = useLocation() - const [expandedSegmentState, setState] = useState({ - expandedSegment: segmentExpandedContextDefaultValue.expandedSegment - }) - const [modal, setModal] = useState( - segmentExpandedContextDefaultValue.modal - ) - - const { query } = useQueryContext() - const navigate = useAppNavigate() - - const locationStateExpandedSegment = getExpandedSegment({ - state: location.state - }) - - useEffect(() => { - // copy location.state to state - if (locationStateExpandedSegment) { - setState({ - expandedSegment: locationStateExpandedSegment - }) - } else if (locationStateExpandedSegment === null) { - setState({ - expandedSegment: segmentExpandedContextDefaultValue.expandedSegment - }) - } - }, [locationStateExpandedSegment]) - - useEffect(() => { - // clear edit mode on clearing all filters - if (!query.filters.length && expandedSegmentState.expandedSegment) { - navigate({ - search: (s) => s, - state: { - expandedSegment: segmentExpandedContextDefaultValue.expandedSegment - }, - replace: true - }) - // overwrite undefined locationState with current expandedSegment, to handle Back navigation correctly - } else if (locationStateExpandedSegment === undefined) { - navigate({ - search: (s) => s, - state: { - expandedSegment: expandedSegmentState.expandedSegment - }, - replace: true - }) - } - }, [ - query, - expandedSegmentState.expandedSegment, - navigate, - locationStateExpandedSegment - ]) - - return ( - - {children} - - ) -} From dfa441fc9449e9050e0bc2b33f500564a3bb4f26 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 19 Feb 2025 14:37:32 +0200 Subject: [PATCH 14/25] Separate API format and dashboard format of segment_data --- assets/js/dashboard/filtering/segments.ts | 17 ++++++++++++ .../segments/searchable-segments-section.tsx | 15 +++++------ .../segments/routeless-segment-modals.tsx | 26 +++++++------------ .../js/dashboard/segments/segment-modals.tsx | 2 +- 4 files changed, 33 insertions(+), 27 deletions(-) diff --git a/assets/js/dashboard/filtering/segments.ts b/assets/js/dashboard/filtering/segments.ts index 61f4bc140607..f480cbbd8313 100644 --- a/assets/js/dashboard/filtering/segments.ts +++ b/assets/js/dashboard/filtering/segments.ts @@ -19,6 +19,12 @@ export type SavedSegment = { updated_at: string } +export type SegmentDataFromApi = { + filters: unknown[] + labels: Record +} + +/** In this type, filters are parsed to dashboard format */ export type SegmentData = { filters: Filter[] labels: Record @@ -26,6 +32,17 @@ export type SegmentData = { const SEGMENT_LABEL_KEY_PREFIX = 'segment-' +export function handleSegmentResponse( + segment: SavedSegment & { + segment_data: SegmentDataFromApi + } +): SavedSegment & { segment_data: SegmentData } { + return { + ...segment, + segment_data: parseApiSegmentData(segment.segment_data) + } +} + export function getFilterSegmentsByNameInsensitive( search?: string ): (s: Pick) => boolean { diff --git a/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx b/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx index 9ade6765485a..f7b6e0e256ba 100644 --- a/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx +++ b/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx @@ -6,10 +6,11 @@ import { useSiteContext } from '../../site-context' import { formatSegmentIdAsLabelKey, getFilterSegmentsByNameInsensitive, + handleSegmentResponse, isSegmentFilter, - parseApiSegmentData, SavedSegment, - SegmentData + SegmentData, + SegmentDataFromApi } from '../../filtering/segments' import { QueryFunction, useQuery, useQueryClient } from '@tanstack/react-query' import { cleanLabels } from '../../util/filters' @@ -188,13 +189,9 @@ export const useSegmentPrefetch = ({ id }: { id: string }) => { typeof queryKey > = useCallback( async ({ queryKey: [_, id] }) => { - const response: SavedSegment & { segment_data: SegmentData } = await get( - `/api/${encodeURIComponent(site.domain)}/segments/${id}` - ) - return { - ...response, - segment_data: parseApiSegmentData(response.segment_data) - } + const response: SavedSegment & { segment_data: SegmentDataFromApi } = + await get(`/api/${encodeURIComponent(site.domain)}/segments/${id}`) + return handleSegmentResponse(response) }, [site] ) diff --git a/assets/js/dashboard/segments/routeless-segment-modals.tsx b/assets/js/dashboard/segments/routeless-segment-modals.tsx index cd96c1b61690..7b2ecd2cbef5 100644 --- a/assets/js/dashboard/segments/routeless-segment-modals.tsx +++ b/assets/js/dashboard/segments/routeless-segment-modals.tsx @@ -9,9 +9,10 @@ import { import { getSearchToApplySingleSegmentFilter, getSegmentNamePlaceholder, - parseApiSegmentData, + handleSegmentResponse, SavedSegment, - SegmentData + SegmentData, + SegmentDataFromApi } from '../filtering/segments' import { useMutation, useQueryClient } from '@tanstack/react-query' import { useSiteContext } from '../site-context' @@ -42,7 +43,7 @@ export const RoutelessSegmentModals = () => { Partial> & { segment_data?: SegmentData }) => { - const response: SavedSegment & { segment_data: SegmentData } = + const response: SavedSegment & { segment_data: SegmentDataFromApi } = await mutation( `/api/${encodeURIComponent(site.domain)}/segments/${id}`, { @@ -60,10 +61,7 @@ export const RoutelessSegmentModals = () => { } ) - return { - ...response, - segment_data: parseApiSegmentData(response.segment_data) - } + return handleSegmentResponse(response) }, onSuccess: async (segment) => { queryClient.invalidateQueries({ queryKey: ['segments'] }) @@ -87,7 +85,7 @@ export const RoutelessSegmentModals = () => { type: 'personal' | 'site' segment_data: SegmentData }) => { - const response: SavedSegment & { segment_data: SegmentData } = + const response: SavedSegment & { segment_data: SegmentDataFromApi } = await mutation(`/api/${encodeURIComponent(site.domain)}/segments`, { method: 'POST', body: { @@ -99,10 +97,7 @@ export const RoutelessSegmentModals = () => { } } }) - return { - ...response, - segment_data: parseApiSegmentData(response.segment_data) - } + return handleSegmentResponse(response) }, onSuccess: async (segment) => { queryClient.invalidateQueries({ queryKey: ['segments'] }) @@ -118,17 +113,14 @@ export const RoutelessSegmentModals = () => { const deleteSegment = useMutation({ mutationFn: async (data: Pick) => { - const response: SavedSegment & { segment_data: SegmentData } = + const response: SavedSegment & { segment_data: SegmentDataFromApi } = await mutation( `/api/${encodeURIComponent(site.domain)}/segments/${data.id}`, { method: 'DELETE' } ) - return { - ...response, - segment_data: parseApiSegmentData(response.segment_data) - } + return handleSegmentResponse(response) }, onSuccess: (_segment): void => { queryClient.invalidateQueries({ queryKey: ['segments'] }) diff --git a/assets/js/dashboard/segments/segment-modals.tsx b/assets/js/dashboard/segments/segment-modals.tsx index a238643c373b..771f29adaf08 100644 --- a/assets/js/dashboard/segments/segment-modals.tsx +++ b/assets/js/dashboard/segments/segment-modals.tsx @@ -169,7 +169,7 @@ export const DeleteSegmentModal = ({ } {` "${segment.name}"?`} - {segment.segment_data && ( + {!!segment.segment_data && ( )} From 180a16ba30f8136a40ade79c9c9181b9cde58bfe Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 19 Feb 2025 14:42:26 +0200 Subject: [PATCH 15/25] Clarify purpose of useDefiniteLocationState --- assets/js/dashboard/navigation/use-definite-location-state.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/assets/js/dashboard/navigation/use-definite-location-state.tsx b/assets/js/dashboard/navigation/use-definite-location-state.tsx index 0de95bc81e22..db66d6e5f5fe 100644 --- a/assets/js/dashboard/navigation/use-definite-location-state.tsx +++ b/assets/js/dashboard/navigation/use-definite-location-state.tsx @@ -9,6 +9,8 @@ import { useAppNavigate } from './use-app-navigate' * (re-navigating with replace: true on most navigations). It does so to * replace undefined `location.state[key]` with a known definite value. * The way to unset `location.state[key]` is to set it to null in navigation. + * This is to make sure that normal navigations (open Details etc) don't need to declare + * that they want to keep previous location.state. */ export function useDefiniteLocationState(key: string): { definiteValue: T | null From 99ebd025d939751b548feda7f67f044548b19a73 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 19 Feb 2025 15:17:18 +0200 Subject: [PATCH 16/25] Tweak UI: site switcher, save as segment --- assets/js/dashboard/dashboard-keybinds.tsx | 4 +- assets/js/dashboard/nav-menu/filter-menu.tsx | 4 +- .../dashboard/nav-menu/filters-bar.test.tsx | 9 ++-- assets/js/dashboard/nav-menu/filters-bar.tsx | 44 +++++++++++-------- .../nav-menu/segments/segment-menu.tsx | 5 +-- assets/js/dashboard/nav-menu/top-bar.test.tsx | 2 +- assets/js/dashboard/site-switcher.js | 6 +-- 7 files changed, 42 insertions(+), 32 deletions(-) diff --git a/assets/js/dashboard/dashboard-keybinds.tsx b/assets/js/dashboard/dashboard-keybinds.tsx index 81e8344b353e..b4d3ceb46801 100644 --- a/assets/js/dashboard/dashboard-keybinds.tsx +++ b/assets/js/dashboard/dashboard-keybinds.tsx @@ -1,7 +1,7 @@ /* @format */ import React from 'react' import { NavigateKeybind } from './keybinding' -import { useQueryContext } from './query-context' +import { useRoutelessModalsContext } from './navigation/routeless-modals-context' const ClearFiltersKeybind = () => ( ( ) export function DashboardKeybinds() { - const { modal } = useQueryContext() + const { modal } = useRoutelessModalsContext() return modal === null && } diff --git a/assets/js/dashboard/nav-menu/filter-menu.tsx b/assets/js/dashboard/nav-menu/filter-menu.tsx index b383e066b25b..4a077bce4ee0 100644 --- a/assets/js/dashboard/nav-menu/filter-menu.tsx +++ b/assets/js/dashboard/nav-menu/filter-menu.tsx @@ -59,13 +59,13 @@ const FilterMenuItems = ({ closeDropdown }: { closeDropdown: () => void }) => { ref={buttonRef} className={classNames( popover.toggleButton.classNames.rounded, - popover.toggleButton.classNames.ghost, + popover.toggleButton.classNames.shadow, 'justify-center gap-1 px-3' )} > - Add filter + Filter { leftoverWidth: 1000, seeMoreWidth: 100, pillWidths: [200, 200, 200, 200], - pillGap: 25 + pillGap: 25, + mustShowSeeMoreMenu: true } handleVisibility(input) expect(setVisibility).toHaveBeenCalledTimes(1) @@ -148,7 +149,8 @@ describe(`${handleVisibility.name}`, () => { leftoverWidth: 300, seeMoreWidth: 50, pillWidths: [250], - pillGap: 25 + pillGap: 25, + mustShowSeeMoreMenu: false } handleVisibility(input) expect(setVisibility).toHaveBeenCalledTimes(1) @@ -165,7 +167,8 @@ describe(`${handleVisibility.name}`, () => { leftoverWidth: 300, seeMoreWidth: 50, pillWidths: [250, 200], - pillGap: 25 + pillGap: 25, + mustShowSeeMoreMenu: true } handleVisibility(input) expect(setVisibility).toHaveBeenCalledTimes(1) diff --git a/assets/js/dashboard/nav-menu/filters-bar.tsx b/assets/js/dashboard/nav-menu/filters-bar.tsx index 6a00564ec0f9..6607e2c9836f 100644 --- a/assets/js/dashboard/nav-menu/filters-bar.tsx +++ b/assets/js/dashboard/nav-menu/filters-bar.tsx @@ -11,6 +11,7 @@ import { popover } from '../components/popover' import { BlurMenuButtonOnEscape } from '../keybinding' import { isSegmentFilter } from '../filtering/segments' import { useRoutelessModalsContext } from '../navigation/routeless-modals-context' +import { DashboardQuery } from '../query' // Component structure is // `..[ filter (x) ]..[ filter (x) ]..[ three dot menu ]..` @@ -27,13 +28,15 @@ export const handleVisibility = ({ leftoverWidth, seeMoreWidth, pillWidths, - pillGap + pillGap, + mustShowSeeMoreMenu }: { setVisibility: (v: VisibilityState) => void leftoverWidth: number | null pillWidths: (number | null)[] | null seeMoreWidth: number pillGap: number + mustShowSeeMoreMenu: boolean }): void => { if (leftoverWidth === null || pillWidths === null) { return @@ -58,7 +61,7 @@ export const handleVisibility = ({ const fits = fitToWidth(leftoverWidth) const seeMoreWillBePresent = - fits.visibleCount < pillWidths.length || pillWidths.length > 1 + fits.visibleCount < pillWidths.length || mustShowSeeMoreMenu // Check if the appearance of "See more" would cause overflow if (seeMoreWillBePresent) { @@ -106,6 +109,11 @@ interface FiltersBarProps { } } +const canShowClearAllAction = (filters: DashboardQuery['filters']) => + filters.length >= 2 +const canShowSaveAsSegmentAction = (filters: DashboardQuery['filters']) => + filters.length >= 1 && !filters.some(isSegmentFilter) + export const FiltersBar = ({ accessors }: FiltersBarProps) => { const containerRef = useRef(null) const pillsRef = useRef(null) @@ -137,7 +145,12 @@ export const FiltersBar = ({ accessors }: FiltersBarProps) => { BUFFER_RIGHT_PX : null, seeMoreWidth: - SEE_MORE_LEFT_MARGIN_PX + SEE_MORE_WIDTH_PX + SEE_MORE_RIGHT_MARGIN_PX + SEE_MORE_LEFT_MARGIN_PX + + SEE_MORE_WIDTH_PX + + SEE_MORE_RIGHT_MARGIN_PX, + mustShowSeeMoreMenu: + canShowClearAllAction(query.filters) || + canShowClearAllAction(query.filters) }) }) @@ -155,8 +168,8 @@ export const FiltersBar = ({ accessors }: FiltersBarProps) => { return
} - const canClear = query.filters.length > 1 - const canSaveAsSegment = canClear + const showingClearAll = canShowClearAllAction(query.filters) + const showingSaveAsSegment = canShowSaveAsSegmentAction(query.filters) return (
{ />
{visibility !== null && - (query.filters.length !== visibility.visibleCount || canClear) && ( + (query.filters.length !== visibility.visibleCount || + showingClearAll || + showingSaveAsSegment) && ( { }} /> )} - {canClear && } - {canSaveAsSegment && } + {showingClearAll && } + {showingSaveAsSegment && } @@ -251,25 +266,18 @@ const ClearAction = () => ( const SaveAsSegmentAction = () => { const { setModal } = useRoutelessModalsContext() - const { query, expandedSegment } = useQueryContext() + const { expandedSegment } = useQueryContext() if (expandedSegment) { return null } - const disabledReason = query.filters.some(isSegmentFilter) - ? 'Segment filters can not be saved within other segments' - : undefined - const disabled = !!disabledReason - return ( s} - onClick={disabled ? () => {} : () => setModal('create')} + onClick={() => setModal('create')} state={{ expandedSegment: null }} > Save as segment diff --git a/assets/js/dashboard/nav-menu/segments/segment-menu.tsx b/assets/js/dashboard/nav-menu/segments/segment-menu.tsx index 8bb8cf9963d9..88cf5bdc8786 100644 --- a/assets/js/dashboard/nav-menu/segments/segment-menu.tsx +++ b/assets/js/dashboard/nav-menu/segments/segment-menu.tsx @@ -6,7 +6,6 @@ import { Popover, Transition } from '@headlessui/react' import { popover } from '../../components/popover' import { AppNavigationLink } from '../../navigation/use-app-navigate' import { - CheckIcon, Square2StackIcon, TrashIcon, XMarkIcon @@ -76,7 +75,7 @@ export const SegmentMenu = () => { )} > - s} state={{ expandedSegment }} @@ -89,7 +88,7 @@ export const SegmentMenu = () => { Update segment
- + */} s} diff --git a/assets/js/dashboard/nav-menu/top-bar.test.tsx b/assets/js/dashboard/nav-menu/top-bar.test.tsx index f75d20c837b8..89d7559cdb52 100644 --- a/assets/js/dashboard/nav-menu/top-bar.test.tsx +++ b/assets/js/dashboard/nav-menu/top-bar.test.tsx @@ -80,7 +80,7 @@ test('user can open and close filters dropdown', async () => { ) }) - const toggleFilters = screen.getByRole('button', { name: /Add filter/ }) + const toggleFilters = screen.getByRole('button', { name: 'Filter' }) await userEvent.click(toggleFilters) expect(screen.queryAllByRole('link').map((el) => el.textContent)).toEqual([ 'Page', diff --git a/assets/js/dashboard/site-switcher.js b/assets/js/dashboard/site-switcher.js index 4919265dfd94..2612bc23d712 100644 --- a/assets/js/dashboard/site-switcher.js +++ b/assets/js/dashboard/site-switcher.js @@ -249,13 +249,13 @@ export default class SiteSwitcher extends React.Component { > - + {this.props.site.domain} {this.props.loggedIn && ( - + )} From 9955969c4497833fbc1fbbc5f1bc9b02f3de5b0b Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 19 Feb 2025 16:20:10 +0200 Subject: [PATCH 17/25] Fix issues with modals --- assets/js/dashboard/nav-menu/top-bar.tsx | 2 -- assets/js/dashboard/navigation/routeless-modals-context.tsx | 6 +----- assets/js/dashboard/router.tsx | 2 ++ 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/assets/js/dashboard/nav-menu/top-bar.tsx b/assets/js/dashboard/nav-menu/top-bar.tsx index c7586ff59ae1..fa6dd7f7e674 100644 --- a/assets/js/dashboard/nav-menu/top-bar.tsx +++ b/assets/js/dashboard/nav-menu/top-bar.tsx @@ -12,7 +12,6 @@ import { FilterMenu } from './filter-menu' import { FiltersBar } from './filters-bar' import { QueryPeriodsPicker } from './query-periods/query-periods-picker' import { SegmentMenu } from './segments/segment-menu' -import { RoutelessSegmentModals } from '../segments/routeless-segment-modals' interface TopBarProps { showCurrentVisitors: boolean @@ -88,7 +87,6 @@ function TopBarInner({ showCurrentVisitors }: TopBarProps) { -
) : ( diff --git a/assets/js/dashboard/navigation/routeless-modals-context.tsx b/assets/js/dashboard/navigation/routeless-modals-context.tsx index 83c6e969ed73..32e43aef35ec 100644 --- a/assets/js/dashboard/navigation/routeless-modals-context.tsx +++ b/assets/js/dashboard/navigation/routeless-modals-context.tsx @@ -1,9 +1,6 @@ /* @format */ import React, { createContext, ReactNode, useContext, useState } from 'react' -import { - RoutelessSegmentModal, - RoutelessSegmentModals -} from '../segments/routeless-segment-modals' +import { RoutelessSegmentModal } from '../segments/routeless-segment-modals' type ActiveModal = null | RoutelessSegmentModal @@ -39,7 +36,6 @@ export function RoutelessModalsContextProvider({ setModal }} > - {children} ) diff --git a/assets/js/dashboard/router.tsx b/assets/js/dashboard/router.tsx index 1aaf38ec05b2..95136faed703 100644 --- a/assets/js/dashboard/router.tsx +++ b/assets/js/dashboard/router.tsx @@ -28,6 +28,7 @@ import QueryContextProvider from './query-context' import { DashboardKeybinds } from './dashboard-keybinds' import LastLoadContextProvider from './last-load-context' import { RoutelessModalsContextProvider } from './navigation/routeless-modals-context' +import { RoutelessSegmentModals } from './segments/routeless-segment-modals' const queryClient = new QueryClient({ defaultOptions: { @@ -47,6 +48,7 @@ function DashboardElement() { {/** render any children of the root route below */} + From 1bb067e6c09ba64d8a6ff9ef1d09b5097b696a65 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 20 Feb 2025 14:34:07 +0200 Subject: [PATCH 18/25] Remove commented and unnecessary code, better query context --- assets/js/dashboard/nav-menu/filter-menu.tsx | 1 - .../segments/searchable-segments-section.tsx | 8 +-- .../nav-menu/segments/segment-menu.tsx | 50 +++++++++++-------- assets/js/dashboard/query-context.tsx | 25 ++-------- .../js/dashboard/query-time-periods.test.ts | 2 +- 5 files changed, 34 insertions(+), 52 deletions(-) diff --git a/assets/js/dashboard/nav-menu/filter-menu.tsx b/assets/js/dashboard/nav-menu/filter-menu.tsx index 4a077bce4ee0..f3fd7455e12f 100644 --- a/assets/js/dashboard/nav-menu/filter-menu.tsx +++ b/assets/js/dashboard/nav-menu/filter-menu.tsx @@ -92,7 +92,6 @@ const FilterMenuItems = ({ closeDropdown }: { closeDropdown: () => void }) => { className={classNames( popover.items.classNames.navigationLink, popover.items.classNames.hoverLink - // 'text-xs' )} onClick={() => closeDropdown()} key={modalKey} diff --git a/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx b/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx index f7b6e0e256ba..6293ebde53d5 100644 --- a/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx +++ b/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx @@ -27,7 +27,6 @@ import { get } from '../../api' const useSegmentsListQuery = () => { const site = useSiteContext() - const appliedSegmentIds = [] as number[] return useQuery({ queryKey: ['segments'], placeholderData: (previousData) => previousData, @@ -35,12 +34,7 @@ const useSegmentsListQuery = () => { const response: SavedSegment[] = await get( `/api/${encodeURIComponent(site.domain)}/segments` ) - - return response.sort( - (a, b) => - appliedSegmentIds.findIndex((id) => id === b.id) - - appliedSegmentIds.findIndex((id) => id === a.id) - ) + return response } }) } diff --git a/assets/js/dashboard/nav-menu/segments/segment-menu.tsx b/assets/js/dashboard/nav-menu/segments/segment-menu.tsx index 88cf5bdc8786..187cc7441afe 100644 --- a/assets/js/dashboard/nav-menu/segments/segment-menu.tsx +++ b/assets/js/dashboard/nav-menu/segments/segment-menu.tsx @@ -1,10 +1,13 @@ /** @format */ -import React from 'react' +import React, { useEffect } from 'react' import classNames from 'classnames' import { Popover, Transition } from '@headlessui/react' import { popover } from '../../components/popover' -import { AppNavigationLink } from '../../navigation/use-app-navigate' +import { + AppNavigationLink, + useAppNavigate +} from '../../navigation/use-app-navigate' import { Square2StackIcon, TrashIcon, @@ -13,6 +16,8 @@ import { import { ChevronDownIcon } from '@heroicons/react/20/solid' import { useQueryContext } from '../../query-context' import { useRoutelessModalsContext } from '../../navigation/routeless-modals-context' +import { SavedSegment } from '../../filtering/segments' +import { DashboardQuery } from '../../query' const linkClassName = classNames( popover.items.classNames.navigationLink, @@ -24,6 +29,28 @@ const buttonClassName = classNames( 'text-white font-medium bg-indigo-600 hover:bg-indigo-700' ) +export const useClearExpandedSegmentModeOnFilterClear = ({ + expandedSegment, + query +}: { + expandedSegment: SavedSegment | null + query: DashboardQuery +}) => { + const navigate = useAppNavigate() + useEffect(() => { + // clear edit mode on clearing all filters or removing last filter + if (!!expandedSegment && !query.filters.length) { + navigate({ + search: (s) => s, + state: { + expandedSegment: null + }, + replace: true + }) + } + }, [query.filters, expandedSegment, navigate]) +} + export const SegmentMenu = () => { const { setModal } = useRoutelessModalsContext() const { expandedSegment } = useQueryContext() @@ -75,20 +102,6 @@ export const SegmentMenu = () => { )} > - {/* s} - state={{ expandedSegment }} - onClick={() => { - closeDropdown() - setModal('update') - }} - > -
- - Update segment -
- */} s} @@ -125,11 +138,6 @@ export const SegmentMenu = () => { ...s, filters: [], labels: {} - // filters: [[['is', 'segment', [expandedSegment.id]]]], - // labels: { - // [formatSegmentIdAsLabelKey(expandedSegment.id)]: - // expandedSegment.name - // } })} state={{ expandedSegment: null }} onClick={closeDropdown} diff --git a/assets/js/dashboard/query-context.tsx b/assets/js/dashboard/query-context.tsx index 481cf90b6453..d4987ff10d45 100644 --- a/assets/js/dashboard/query-context.tsx +++ b/assets/js/dashboard/query-context.tsx @@ -1,11 +1,5 @@ /* @format */ -import React, { - createContext, - useMemo, - useContext, - ReactNode, - useEffect -} from 'react' +import React, { createContext, useMemo, useContext, ReactNode } from 'react' import { useLocation } from 'react-router' import { useMountedEffect } from './custom-hooks' import * as api from './api' @@ -26,8 +20,8 @@ import { postProcessFilters } from './query' import { SavedSegment, SegmentData } from './filtering/segments' -import { useAppNavigate } from './navigation/use-app-navigate' import { useDefiniteLocationState } from './navigation/use-definite-location-state' +import { useClearExpandedSegmentModeOnFilterClear } from './nav-menu/segments/segment-menu' const queryContextDefaultValue = { query: queryDefaultValue, @@ -52,7 +46,6 @@ export default function QueryContextProvider({ const { definiteValue: expandedSegment } = useDefiniteLocationState< SavedSegment & { segment_data: SegmentData } >('expandedSegment') - const navigate = useAppNavigate() const site = useSiteContext() const { @@ -130,19 +123,7 @@ export default function QueryContextProvider({ expandedSegment ]) - useEffect(() => { - // clear edit mode on clearing all filters or removing last filter - if (!!expandedSegment && !query.filters.length) { - navigate({ - search: (s) => s, - state: { - expandedSegment: null - }, - replace: true - }) - } - }, [query.filters, expandedSegment, navigate]) - + useClearExpandedSegmentModeOnFilterClear({ expandedSegment, query }) useSaveTimePreferencesToStorage({ site, period, diff --git a/assets/js/dashboard/query-time-periods.test.ts b/assets/js/dashboard/query-time-periods.test.ts index 3e9ffc4c5ae1..a8244c6b0b4e 100644 --- a/assets/js/dashboard/query-time-periods.test.ts +++ b/assets/js/dashboard/query-time-periods.test.ts @@ -93,7 +93,7 @@ describe(`${getDashboardTimeSettings.name}`, () => { }) }) - it('respects segmentIsExpanded: true option', () => { + it('respects segmentIsExpanded: true option: comparison and edit segment mode are mutually exclusive', () => { expect( getDashboardTimeSettings({ searchValues: { From e619595c93ab7efca637fc3d0b1634c28ccecaa2 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 20 Feb 2025 14:59:43 +0200 Subject: [PATCH 19/25] Fix too permissive site members dataset --- .../controllers/stats_controller.ex | 31 +++++++++++++------ .../controllers/stats_controller_test.exs | 6 +++- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/lib/plausible_web/controllers/stats_controller.ex b/lib/plausible_web/controllers/stats_controller.ex index a06a1cd41c79..713cceeee329 100644 --- a/lib/plausible_web/controllers/stats_controller.ex +++ b/lib/plausible_web/controllers/stats_controller.ex @@ -71,6 +71,8 @@ defmodule PlausibleWeb.StatsController do cond do (stats_start_date && can_see_stats?) || (can_see_stats? && skip_to_dashboard?) -> + flags = get_flags(current_user, site) + conn |> put_resp_header("x-robots-tag", "noindex, nofollow") |> render("stats.html", @@ -84,8 +86,12 @@ defmodule PlausibleWeb.StatsController do native_stats_start_date: NaiveDateTime.to_date(site.native_stats_start_at), title: title(conn, site), demo: demo, - flags: get_flags(current_user, site), - members: get_members(current_user, site), + flags: flags, + members: + if(flags.saved_segments, + do: get_members(conn.assigns[:site_role], site), + else: nil + ), is_dbip: is_dbip(), dogfood_page_path: dogfood_page_path, load_dashboard_js: true @@ -353,6 +359,8 @@ defmodule PlausibleWeb.StatsController do scroll_depth_visible? = Plausible.Stats.ScrollDepth.check_feature_visible!(shared_link.site, current_user) + flags = get_flags(current_user, shared_link.site) + conn |> put_resp_header("x-robots-tag", "noindex, nofollow") |> delete_resp_header("x-frame-options") @@ -372,8 +380,12 @@ defmodule PlausibleWeb.StatsController do embedded: conn.params["embed"] == "true", background: conn.params["background"], theme: conn.params["theme"], - flags: get_flags(current_user, shared_link.site), - members: get_members(current_user, shared_link.site), + flags: flags, + members: + if(flags.saved_segments, + do: get_members(conn.assigns[:site_role], shared_link.site), + else: nil + ), is_dbip: is_dbip(), load_dashboard_js: true ) @@ -399,11 +411,8 @@ defmodule PlausibleWeb.StatsController do end) |> Map.new() - defp get_members(nil, _site) do - nil - end - - defp get_members(_user, %Plausible.Site{} = site) do + defp get_members(site_role, %Plausible.Site{} = site) + when site_role in [:viewer, :editor, :owner, :admin, :super_admin] do site = site |> Plausible.Repo.preload( @@ -422,6 +431,10 @@ defmodule PlausibleWeb.StatsController do |> Map.new() end + defp get_members(_site_role, _site) do + nil + end + defp is_dbip() do on_ee do false diff --git a/test/plausible_web/controllers/stats_controller_test.exs b/test/plausible_web/controllers/stats_controller_test.exs index b971d3b9fbb2..c94a5a27b6fb 100644 --- a/test/plausible_web/controllers/stats_controller_test.exs +++ b/test/plausible_web/controllers/stats_controller_test.exs @@ -27,6 +27,7 @@ defmodule PlausibleWeb.StatsControllerTest do assert text_of_attr(resp, @react_container, "data-has-props") == "false" assert text_of_attr(resp, @react_container, "data-logged-in") == "false" assert text_of_attr(resp, @react_container, "data-embedded") == "" + assert text_of_attr(resp, @react_container, "data-members") == "null" [{"div", attrs, _}] = find(resp, @react_container) assert Enum.all?(attrs, fn {k, v} -> is_binary(k) and is_binary(v) end) @@ -116,11 +117,14 @@ defmodule PlausibleWeb.StatsControllerTest do describe "GET /:domain - as a logged in user" do setup [:create_user, :log_in, :create_site] - test "can view stats of a website I've created", %{conn: conn, site: site} do + test "can view stats of a website I've created", %{conn: conn, site: site, user: user} do populate_stats(site, [build(:pageview)]) conn = get(conn, "/" <> site.domain) resp = html_response(conn, 200) assert text_of_attr(resp, @react_container, "data-logged-in") == "true" + + assert text_of_attr(resp, @react_container, "data-members") == + "{\"#{user.id}\":\"#{user.name}\"}" end test "can view stats of a website I've created, enforcing pageviews check skip", %{ From 2c3e8e62934fc9f8346fbd2132646a6219775e85 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 20 Feb 2025 15:36:00 +0200 Subject: [PATCH 20/25] Make sure Segment doesn't show up as an option to customer without the FF --- assets/js/dashboard/filters.js | 9 +- .../js/dashboard/stats/modals/filter-modal.js | 5 +- assets/js/dashboard/util/filters.js | 9 ++ assets/js/dashboard/util/filters.test.ts | 86 +++++++++++++++---- assets/test-utils/app-context-providers.tsx | 5 +- 5 files changed, 91 insertions(+), 23 deletions(-) diff --git a/assets/js/dashboard/filters.js b/assets/js/dashboard/filters.js index 31d5d56edd4e..6b4ef30ddd3d 100644 --- a/assets/js/dashboard/filters.js +++ b/assets/js/dashboard/filters.js @@ -10,9 +10,9 @@ import { Menu, Transition } from '@headlessui/react'; import { FILTER_GROUP_TO_MODAL_TYPE, cleanLabels, - FILTER_MODAL_TO_FILTER_GROUP, formatFilterGroup, - EVENT_PROPS_PREFIX + EVENT_PROPS_PREFIX, + getAvailableFilterModals } from "./util/filters" import { plainFilterText, styledFilterText } from "./util/filter-text" @@ -101,10 +101,7 @@ function DropdownContent({ wrapped }) { const [addingFilter, setAddingFilter] = useState(false); if (wrapped === WRAPSTATE.unwrapped || addingFilter) { - let filterModals = { ...FILTER_MODAL_TO_FILTER_GROUP } - if (!site.propsAvailable) delete filterModals.props - - return <>{Object.keys(filterModals).map((option) => )} + return <>{Object.keys(getAvailableFilterModals(site)).map((option) => )} } return ( diff --git a/assets/js/dashboard/stats/modals/filter-modal.js b/assets/js/dashboard/stats/modals/filter-modal.js index b250b70f2646..ea381637096c 100644 --- a/assets/js/dashboard/stats/modals/filter-modal.js +++ b/assets/js/dashboard/stats/modals/filter-modal.js @@ -2,7 +2,7 @@ import React from 'react' import { useParams } from 'react-router-dom'; import Modal from './modal'; -import { EVENT_PROPS_PREFIX, FILTER_GROUP_TO_MODAL_TYPE, formatFilterGroup, FILTER_OPERATIONS, getFilterGroup, FILTER_MODAL_TO_FILTER_GROUP, cleanLabels } from '../../util/filters'; +import { EVENT_PROPS_PREFIX, FILTER_GROUP_TO_MODAL_TYPE, formatFilterGroup, FILTER_OPERATIONS, getFilterGroup, FILTER_MODAL_TO_FILTER_GROUP, cleanLabels, getAvailableFilterModals } from '../../util/filters'; import { useQueryContext } from '../../query-context'; import { useSiteContext } from '../../site-context'; import { isModifierPressed, isTyping } from '../../keybinding'; @@ -202,6 +202,9 @@ export default function FilterModalWithRouter(props) { const { field } = useParams() const { query } = useQueryContext() const site = useSiteContext() + if (!Object.keys(getAvailableFilterModals(site)).includes(field)) { + return null + } const firstSegmentFilter = field === 'segment' ? query.filters?.find(isSegmentFilter) : null if (firstSegmentFilter) { const firstSegmentId = firstSegmentFilter[2][0] diff --git a/assets/js/dashboard/util/filters.js b/assets/js/dashboard/util/filters.js index 044031f6f538..4f6714d1d3c7 100644 --- a/assets/js/dashboard/util/filters.js +++ b/assets/js/dashboard/util/filters.js @@ -17,6 +17,15 @@ export const FILTER_MODAL_TO_FILTER_GROUP = { segment: ['segment'] } +export function getAvailableFilterModals(site) { + const { props, segment, ...rest } = FILTER_MODAL_TO_FILTER_GROUP + return { + ...rest, + ...(site.propsAvailable && { props }), + ...(site.flags.saved_segments && { segment }) + } +} + export const FILTER_GROUP_TO_MODAL_TYPE = Object.fromEntries( Object.entries(FILTER_MODAL_TO_FILTER_GROUP).flatMap( ([modalName, filterGroups]) => diff --git a/assets/js/dashboard/util/filters.test.ts b/assets/js/dashboard/util/filters.test.ts index 10661dc82d9b..cd774b30f27a 100644 --- a/assets/js/dashboard/util/filters.test.ts +++ b/assets/js/dashboard/util/filters.test.ts @@ -1,6 +1,62 @@ -import { serializeApiFilters } from './filters' +/** @format */ -describe('serializeApiFilters', () => { +import { getAvailableFilterModals, serializeApiFilters } from './filters' + +describe(`${getAvailableFilterModals.name}`, () => { + it('gives limited object when props and segments are not available', () => { + expect( + getAvailableFilterModals({ + propsAvailable: false, + flags: { saved_segments: null } + }) + ).toEqual({ + browser: ['browser', 'browser_version'], + goal: ['goal'], + hostname: ['hostname'], + location: ['country', 'region', 'city'], + os: ['os', 'os_version'], + page: ['page', 'entry_page', 'exit_page'], + screen: ['screen'], + source: ['source', 'channel', 'referrer'], + utm: [ + 'utm_medium', + 'utm_source', + 'utm_campaign', + 'utm_term', + 'utm_content' + ] + }) + }) + + it('gives full object when props and segments are available', () => { + expect( + getAvailableFilterModals({ + propsAvailable: true, + flags: { saved_segments: true } + }) + ).toEqual({ + browser: ['browser', 'browser_version'], + goal: ['goal'], + hostname: ['hostname'], + location: ['country', 'region', 'city'], + os: ['os', 'os_version'], + page: ['page', 'entry_page', 'exit_page'], + screen: ['screen'], + source: ['source', 'channel', 'referrer'], + utm: [ + 'utm_medium', + 'utm_source', + 'utm_campaign', + 'utm_term', + 'utm_content' + ], + props: ['props'], + segment: ['segment'] + }) + }) +}) + +describe(`${serializeApiFilters.name}`, () => { it('should prefix filter keys with event: or visit: when appropriate', () => { const filters = [ ['is', 'page', ['/docs', '/blog']], @@ -9,21 +65,21 @@ describe('serializeApiFilters', () => { ['is', 'country', ['US']], ['is_not', 'utm_source', ['google']] ] - expect(serializeApiFilters(filters)).toEqual(JSON.stringify([ - ['is', 'event:page', ['/docs', '/blog']], - ['contains', 'event:goal', ['Signup']], - ['contains_not', 'visit:browser', ['chrom'], { case_sensitive: false }], - ['is', 'visit:country', ['US']], - ['is_not', 'visit:utm_source', ['google']] - ])) + expect(serializeApiFilters(filters)).toEqual( + JSON.stringify([ + ['is', 'event:page', ['/docs', '/blog']], + ['contains', 'event:goal', ['Signup']], + ['contains_not', 'visit:browser', ['chrom'], { case_sensitive: false }], + ['is', 'visit:country', ['US']], + ['is_not', 'visit:utm_source', ['google']] + ]) + ) }) it('wraps has_not_done goal filters in API format', () => { - const filters = [ - ['has_not_done', 'goal', ['Signup']] - ] - expect(serializeApiFilters(filters)).toEqual(JSON.stringify([ - ['has_not_done', ['is', 'event:goal', ['Signup']]] - ])) + const filters = [['has_not_done', 'goal', ['Signup']]] + expect(serializeApiFilters(filters)).toEqual( + JSON.stringify([['has_not_done', ['is', 'event:goal', ['Signup']]]]) + ) }) }) diff --git a/assets/test-utils/app-context-providers.tsx b/assets/test-utils/app-context-providers.tsx index 976c07d8ea8b..012d5391bc21 100644 --- a/assets/test-utils/app-context-providers.tsx +++ b/assets/test-utils/app-context-providers.tsx @@ -9,6 +9,7 @@ import { MemoryRouter, MemoryRouterProps } from 'react-router-dom' import { QueryClient, QueryClientProvider } from '@tanstack/react-query' import QueryContextProvider from '../js/dashboard/query-context' import { getRouterBasepath } from '../js/dashboard/router' +import { RoutelessModalsContextProvider } from '../js/dashboard/navigation/routeless-modals-context' type TestContextProvidersProps = { children: ReactNode @@ -68,7 +69,9 @@ export const TestContextProviders = ({ {...routerProps} > - {children} + + {children} + From 18c420f4028b470b11fae5cfdf169fd27cf48719 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Tue, 25 Feb 2025 09:40:07 +0200 Subject: [PATCH 21/25] Fix issue with 'See more' menu being present when it should not be --- assets/js/dashboard/nav-menu/filters-bar.tsx | 36 +++++++++++--------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/assets/js/dashboard/nav-menu/filters-bar.tsx b/assets/js/dashboard/nav-menu/filters-bar.tsx index 6607e2c9836f..b5ca5b0140df 100644 --- a/assets/js/dashboard/nav-menu/filters-bar.tsx +++ b/assets/js/dashboard/nav-menu/filters-bar.tsx @@ -109,18 +109,30 @@ interface FiltersBarProps { } } -const canShowClearAllAction = (filters: DashboardQuery['filters']) => +const canShowClearAllAction = ({ filters }: Pick) => filters.length >= 2 -const canShowSaveAsSegmentAction = (filters: DashboardQuery['filters']) => - filters.length >= 1 && !filters.some(isSegmentFilter) + +const canShowSaveAsSegmentAction = ({ + filters, + isEditingSegment +}: Pick & { isEditingSegment: boolean }) => + filters.length >= 1 && !filters.some(isSegmentFilter) && !isEditingSegment export const FiltersBar = ({ accessors }: FiltersBarProps) => { const containerRef = useRef(null) const pillsRef = useRef(null) const [visibility, setVisibility] = useState(null) - const { query } = useQueryContext() + const { query, expandedSegment } = useQueryContext() const seeMoreRef = useRef(null) + const showingClearAll = canShowClearAllAction({ filters: query.filters }) + const showingSaveAsSegment = canShowSaveAsSegmentAction({ + filters: query.filters, + isEditingSegment: !!expandedSegment + }) + + const mustShowSeeMoreMenu = showingClearAll || showingSaveAsSegment + useLayoutEffect(() => { const topBar = accessors.topBar(containerRef.current) const leftSection = accessors.leftSection(containerRef.current) @@ -148,9 +160,7 @@ export const FiltersBar = ({ accessors }: FiltersBarProps) => { SEE_MORE_LEFT_MARGIN_PX + SEE_MORE_WIDTH_PX + SEE_MORE_RIGHT_MARGIN_PX, - mustShowSeeMoreMenu: - canShowClearAllAction(query.filters) || - canShowClearAllAction(query.filters) + mustShowSeeMoreMenu }) }) @@ -161,16 +171,13 @@ export const FiltersBar = ({ accessors }: FiltersBarProps) => { return () => { resizeObserver.disconnect() } - }, [accessors, query.filters]) + }, [accessors, query.filters, mustShowSeeMoreMenu]) if (!query.filters.length) { // functions as spacer between elements.leftSection and elements.rightSection return
} - const showingClearAll = canShowClearAllAction(query.filters) - const showingSaveAsSegment = canShowSaveAsSegmentAction(query.filters) - return (
{
{visibility !== null && (query.filters.length !== visibility.visibleCount || - showingClearAll || - showingSaveAsSegment) && ( + mustShowSeeMoreMenu) && ( ( const SaveAsSegmentAction = () => { const { setModal } = useRoutelessModalsContext() - const { expandedSegment } = useQueryContext() - if (expandedSegment) { - return null - } return ( Date: Tue, 25 Feb 2025 11:51:25 +0200 Subject: [PATCH 22/25] Permit :has_not_done filter in segments --- lib/plausible/segments/filters.ex | 2 +- lib/plausible/segments/segment.ex | 14 +- lib/plausible/stats/filters/query_parser.ex | 2 +- test/plausible/stats/query_parser_test.exs | 6 +- .../segments_controller_test.exs | 137 +++++++++++++++++- 5 files changed, 149 insertions(+), 12 deletions(-) diff --git a/lib/plausible/segments/filters.ex b/lib/plausible/segments/filters.ex index 132ce2bd55bc..fb90b6853a35 100644 --- a/lib/plausible/segments/filters.ex +++ b/lib/plausible/segments/filters.ex @@ -64,7 +64,7 @@ defmodule Plausible.Segments.Filters do end defp replace_segment_with_filter_tree([_, "segment", clauses], preloaded_segments) do - if length(clauses) === 1 do + if length(clauses) == 1 do [[:and, Map.get(preloaded_segments, Enum.at(clauses, 0))]] else [[:or, Enum.map(clauses, fn id -> [:and, Map.get(preloaded_segments, id)] end)]] diff --git a/lib/plausible/segments/segment.ex b/lib/plausible/segments/segment.ex index d2edfe2d7aed..43f5f7d231fd 100644 --- a/lib/plausible/segments/segment.ex +++ b/lib/plausible/segments/segment.ex @@ -197,9 +197,15 @@ defmodule Plausible.Segments.Segment do end defp dashboard_compatible_filter?(filter) do - is_list(filter) and length(filter) === 3 and - is_atom(Enum.at(filter, 0)) and - is_binary(Enum.at(filter, 1)) and - is_list(Enum.at(filter, 2)) + regular_filter? = + is_list(filter) and length(filter) == 3 and + is_atom(Enum.at(filter, 0)) and + is_binary(Enum.at(filter, 1)) and + is_list(Enum.at(filter, 2)) + + has_not_done_filter? = + is_list(filter) and length(filter) == 2 and Enum.at(filter, 0) == :has_not_done + + regular_filter? or has_not_done_filter? end end diff --git a/lib/plausible/stats/filters/query_parser.ex b/lib/plausible/stats/filters/query_parser.ex index 38fd78fbe570..392e37528743 100644 --- a/lib/plausible/stats/filters/query_parser.ex +++ b/lib/plausible/stats/filters/query_parser.ex @@ -566,7 +566,7 @@ defmodule Plausible.Stats.Filters.QueryParser do :ok else {:error, - "The goal `#{clause}` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals"} + "Invalid filters. The goal `#{clause}` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals"} end end diff --git a/test/plausible/stats/query_parser_test.exs b/test/plausible/stats/query_parser_test.exs index a523dc7e8708..afccf342652e 100644 --- a/test/plausible/stats/query_parser_test.exs +++ b/test/plausible/stats/query_parser_test.exs @@ -1137,7 +1137,7 @@ defmodule Plausible.Stats.Filters.QueryParserTest do } |> check_error( site, - "The goal `Signup` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals" + "Invalid filters. The goal `Signup` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals" ) end @@ -1152,7 +1152,7 @@ defmodule Plausible.Stats.Filters.QueryParserTest do } |> check_error( site, - "The goal `Visit /thank-you` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals" + "Invalid filters. The goal `Visit /thank-you` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals" ) end @@ -1255,7 +1255,7 @@ defmodule Plausible.Stats.Filters.QueryParserTest do } |> check_error( site, - "The goal `Unknown` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals", + "Invalid filters. The goal `Unknown` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals", :internal ) end diff --git a/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs b/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs index db59c0670669..375a7a31fe06 100644 --- a/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs +++ b/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs @@ -341,12 +341,18 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do %{conn: conn, user: user} do site = new_site() add_guest(site, user: user, role: unquote(role)) + insert(:goal, site: site, event_name: "Conversion") response = post(conn, "/api/#{site.domain}/segments", %{ "name" => "Some segment", "type" => "#{unquote(type)}", - "segment_data" => %{"filters" => [["is", "visit:entry_page", ["/blog"]]]} + "segment_data" => %{ + "filters" => [ + ["is", "visit:entry_page", ["/blog"]], + ["has_not_done", ["is", "event:goal", ["Conversion"]]] + ] + } }) |> json_response(200) @@ -355,7 +361,12 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do "name" => "Some segment", "type" => ^"#{unquote(type)}", "segment_data" => - ^strict_map(%{"filters" => [["is", "visit:entry_page", ["/blog"]]]}), + ^strict_map(%{ + "filters" => [ + ["is", "visit:entry_page", ["/blog"]], + ["has_not_done", ["is", "event:goal", ["Conversion"]]] + ] + }), "owner_id" => ^user.id, "inserted_at" => ^any(:iso8601_naive_datetime), "updated_at" => ^any(:iso8601_naive_datetime) @@ -477,6 +488,75 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do } end + test "updating a segment containing a goal that has been deleted, with the deleted goal still in filters, fails", + %{ + conn: conn, + user: user, + site: site + } do + segment = + insert(:segment, + site: site, + name: "any name", + type: :personal, + owner: user, + segment_data: %{"filters" => [["is", "event:goal", ["Signup"]]]} + ) + + conn = + patch(conn, "/api/#{site.domain}/segments/#{segment.id}", %{ + "segment_data" => %{ + "filters" => [["is", "event:goal", ["Signup"]], ["is", "event:page", ["/register"]]] + } + }) + + assert json_response(conn, 400) == %{ + "error" => + "segment_data Invalid filters. The goal `Signup` is not configured for this site. Find out how to configure goals here: https://plausible.io/docs/stats-api#filtering-by-goals" + } + end + + test "a segment containing a goal that has been deleted can be updated to not contain the goal", + %{ + conn: conn, + user: user, + site: site + } do + segment = + insert(:segment, + site: site, + name: "any name", + type: :personal, + owner: user, + segment_data: %{"filters" => [["is", "event:goal", ["Signup"]]]} + ) + + insert(:goal, site: site, event_name: "a new goal") + + response = + patch(conn, "/api/#{site.domain}/segments/#{segment.id}", %{ + "segment_data" => %{ + "filters" => [["is", "event:goal", ["a new goal"]]] + } + }) + |> json_response(200) + + assert_matches ^strict_map(%{ + "id" => ^segment.id, + "name" => ^segment.name, + "type" => ^any(:string, ~r/#{segment.type}/), + "segment_data" => + ^strict_map(%{ + "filters" => [ + ["is", "event:goal", ["a new goal"]] + ] + }), + "owner_id" => ^user.id, + "inserted_at" => ^any(:iso8601_naive_datetime), + "updated_at" => ^any(:iso8601_naive_datetime) + }) = response + end + test "editors can update a segment", %{conn: conn, user: user} do site = new_site() add_guest(site, user: user, role: :editor) @@ -536,12 +616,33 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do verify_segment_in_db(segment) end + test "even site owners can't delete personal segments of other users", + %{conn: conn, site: site} do + other_user = add_guest(site, role: :editor) + + segment = + insert(:segment, + site: site, + owner_id: other_user.id, + name: "any", + type: :personal + ) + + conn = + delete(conn, "/api/#{site.domain}/segments/#{segment.id}") + + assert %{"error" => "Segment not found with ID \"#{segment.id}\""} == + json_response(conn, 404) + + verify_segment_in_db(segment) + end + for %{role: role, type: type} <- [ %{role: :viewer, type: :personal}, %{role: :editor, type: :personal}, %{role: :editor, type: :site} ] do - test "#{role} can delete segment with type \"#{type}\" successfully", + test "#{role} can delete their own segment with type \"#{type}\" successfully", %{conn: conn, user: user} do site = new_site() add_guest(site, user: user, role: unquote(role)) @@ -571,6 +672,36 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do verify_no_segment_in_db(segment) end end + + test "site owner can delete a site segment owned by someone else, even if it contains a non-existing goal", + %{conn: conn, site: site} do + other_user = add_guest(site, role: :editor) + + segment = + insert(:segment, + site: site, + owner: other_user, + name: "any", + type: :site, + segment_data: %{"filters" => [["is", "event:goal", ["non-existing goal"]]]} + ) + + response = + delete(conn, "/api/#{site.domain}/segments/#{segment.id}") + |> json_response(200) + + assert %{ + "owner_id" => other_user.id, + "id" => segment.id, + "name" => segment.name, + "segment_data" => segment.segment_data, + "type" => "site", + "inserted_at" => NaiveDateTime.to_iso8601(segment.inserted_at), + "updated_at" => NaiveDateTime.to_iso8601(segment.updated_at) + } == response + + verify_no_segment_in_db(segment) + end end defp verify_segment_in_db(segment) do From 8db3ce9a00e5b133896c9a9be767443cacaaf812 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Tue, 25 Feb 2025 14:45:47 +0200 Subject: [PATCH 23/25] Refactor to matching on filter list structure --- lib/plausible/segments/segment.ex | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/lib/plausible/segments/segment.ex b/lib/plausible/segments/segment.ex index 43f5f7d231fd..43978b7cc315 100644 --- a/lib/plausible/segments/segment.ex +++ b/lib/plausible/segments/segment.ex @@ -197,15 +197,10 @@ defmodule Plausible.Segments.Segment do end defp dashboard_compatible_filter?(filter) do - regular_filter? = - is_list(filter) and length(filter) == 3 and - is_atom(Enum.at(filter, 0)) and - is_binary(Enum.at(filter, 1)) and - is_list(Enum.at(filter, 2)) - - has_not_done_filter? = - is_list(filter) and length(filter) == 2 and Enum.at(filter, 0) == :has_not_done - - regular_filter? or has_not_done_filter? + case filter do + [operation, dimension, _clauses] when is_atom(operation) and is_binary(dimension) -> true + [:has_not_done, _] -> true + _ -> false + end end end From 59c99f8a3459148a4f17d28c3da86503a1ca1006 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Tue, 25 Feb 2025 15:51:56 +0200 Subject: [PATCH 24/25] Flatten :and stemming from segment filters on first level --- lib/plausible/segments/filters.ex | 24 ++++++++++++++++--- .../external_stats_controller/query_test.exs | 4 ++-- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/lib/plausible/segments/filters.ex b/lib/plausible/segments/filters.ex index fb90b6853a35..bd62b8dd58b7 100644 --- a/lib/plausible/segments/filters.ex +++ b/lib/plausible/segments/filters.ex @@ -63,6 +63,13 @@ defmodule Plausible.Segments.Filters do end end + defp expand_first_level_and_filters(filter) do + case filter do + [:and, clauses] -> clauses + filter -> [filter] + end + end + defp replace_segment_with_filter_tree([_, "segment", clauses], preloaded_segments) do if length(clauses) == 1 do [[:and, Map.get(preloaded_segments, Enum.at(clauses, 0))]] @@ -84,9 +91,18 @@ defmodule Plausible.Segments.Filters do iex> resolve_segments([[:is, "visit:entry_page", ["/home"]], [:is, "segment", [1]]], %{1 => [[:contains, "visit:entry_page", ["blog"]], [:is, "visit:country", ["PL"]]]}) {:ok, [ [:is, "visit:entry_page", ["/home"]], - [:and, [[:contains, "visit:entry_page", ["blog"]], [:is, "visit:country", ["PL"]]]] + [:contains, "visit:entry_page", ["blog"]], + [:is, "visit:country", ["PL"]] + ]} + + iex> resolve_segments([[:is, "visit:entry_page", ["/home"]], [:is, "segment", [1]], [:is, "segment", [2]]], %{1 => [[:is, "visit:country", ["PL"]]], 2 => [[:is, "event:goal", ["Signup"]]]}) + {:ok, [ + [:is, "visit:entry_page", ["/home"]], + [:is, "visit:country", ["PL"]], + [:is, "event:goal", ["Signup"]] ]} + iex> resolve_segments([[:is, "segment", [1, 2]]], %{1 => [[:contains, "event:goal", ["Singup"]], [:is, "visit:country", ["PL"]]], 2 => [[:contains, "event:goal", ["Sauna"]], [:is, "visit:country", ["EE"]]]}) {:ok, [ [:or, [ @@ -98,9 +114,11 @@ defmodule Plausible.Segments.Filters do def resolve_segments(original_filters, preloaded_segments) do if map_size(preloaded_segments) > 0 do {:ok, - Filters.transform_filters(original_filters, fn f -> + original_filters + |> Filters.transform_filters(fn f -> replace_segment_with_filter_tree(f, preloaded_segments) - end)} + end) + |> Filters.transform_filters(&expand_first_level_and_filters/1)} else {:ok, original_filters} end diff --git a/test/plausible_web/controllers/api/external_stats_controller/query_test.exs b/test/plausible_web/controllers/api/external_stats_controller/query_test.exs index a5acd797b727..104fa08176be 100644 --- a/test/plausible_web/controllers/api/external_stats_controller/query_test.exs +++ b/test/plausible_web/controllers/api/external_stats_controller/query_test.exs @@ -4959,7 +4959,7 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryTest do site: site, name: "Signups", segment_data: %{ - "filters" => [["is", "event:name", ["Signup"]]] + "filters" => [["is", "event:goal", ["Signup"]]] } ) @@ -4998,7 +4998,7 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryTest do # response shows what filters the segment was resolved to assert json_response(conn, 200)["query"]["filters"] == [ - ["and", [["is", "event:name", ["Signup"]]]] + ["is", "event:goal", ["Signup"]] ] end end From da742270d8c5e7c81961bd55a640fcfa334d0391 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Tue, 25 Feb 2025 16:37:18 +0200 Subject: [PATCH 25/25] Update test --- test/plausible/stats/query_parser_test.exs | 23 +++++++++------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/test/plausible/stats/query_parser_test.exs b/test/plausible/stats/query_parser_test.exs index afccf342652e..5615a0ebd567 100644 --- a/test/plausible/stats/query_parser_test.exs +++ b/test/plausible/stats/query_parser_test.exs @@ -2440,23 +2440,18 @@ defmodule Plausible.Stats.Filters.QueryParserTest do metrics: [:visitors, :events], utc_time_range: @date_range_day, filters: [ + [ + :or, + [ + [:and, [[:is, "visit:country", ["AU", "NZ"]]]], + [:and, [[:is, "visit:country", ["FR", "DE"]]]] + ] + ], [ :and, [ - [ - :or, - [ - [:and, [[:is, "visit:country", ["AU", "NZ"]]]], - [:and, [[:is, "visit:country", ["FR", "DE"]]]] - ] - ], - [ - :and, - [ - [:is, "visit:browser", ["Firefox"]], - [:is, "visit:os", ["Linux"]] - ] - ] + [:is, "visit:browser", ["Firefox"]], + [:is, "visit:os", ["Linux"]] ] ] ],