diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b02c23b38..e2dd777050 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,7 @@ All notable changes to this project will be documented in this file. - Fix current visitors loading when viewing a dashboard with a shared link - Fix Conversion Rate graph being unselectable when "Goal is ..." filter is within a segment - Fix Channels filter input appearing when clicking Sources in filter menu or clicking an applied "Channel is..." filter +- Fix Conversion Rate metrics column disappearing from reports when "Goal is ..." filter is within a segment ## v2.1.5-rc.1 - 2025-01-17 diff --git a/assets/js/dashboard.tsx b/assets/js/dashboard.tsx index c6489db675..5232786a1e 100644 --- a/assets/js/dashboard.tsx +++ b/assets/js/dashboard.tsx @@ -20,6 +20,10 @@ import { GoToSites, SomethingWentWrongMessage } from './dashboard/error/something-went-wrong' +import { + parsePreloadedSegments, + SegmentsContextProvider +} from './dashboard/filtering/segments-context' timer.start() @@ -71,7 +75,11 @@ if (container && container.dataset) { } } > - + + + diff --git a/assets/js/dashboard/filtering/segments-context.test.tsx b/assets/js/dashboard/filtering/segments-context.test.tsx new file mode 100644 index 0000000000..af3309955a --- /dev/null +++ b/assets/js/dashboard/filtering/segments-context.test.tsx @@ -0,0 +1,120 @@ +/** @format */ + +import React from 'react' +import { render, screen, fireEvent } from '@testing-library/react' +import { SegmentsContextProvider, useSegmentsContext } from './segments-context' +import { SavedSegment, SegmentData, SegmentType } from './segments' + +function TestComponent() { + const { segments, addOne, removeOne, updateOne } = useSegmentsContext() + + return ( +
+ + {segments.map((segment) => ( +
+ {segment.name} + + +
+ ))} +
+ ) +} + +const getRenderedSegmentNames = () => + screen.queryAllByTestId('name').map((e) => e.textContent) + +describe('SegmentsContext functions', () => { + test('deleteOne works', () => { + render( + + + + ) + + expect(getRenderedSegmentNames()).toEqual([ + segmentOpenSource.name, + segmentAPAC.name + ]) + fireEvent.click(screen.getByText(`Delete ${segmentOpenSource.name}`)) + expect(getRenderedSegmentNames()).toEqual([segmentAPAC.name]) + }) + + test('addOne adds to head of list', async () => { + render( + + + + ) + + expect(getRenderedSegmentNames()).toEqual([segmentAPAC.name]) + + fireEvent.click(screen.getByText('Add Segment')) + expect(screen.queryAllByTestId('name').map((e) => e.textContent)).toEqual([ + segmentOpenSource.name, + segmentAPAC.name + ]) + }) + + test('updateOne works: updated segment is at head of list', () => { + render( + + + + ) + + expect(getRenderedSegmentNames()).toEqual([ + segmentOpenSource.name, + segmentAPAC.name + ]) + fireEvent.click(screen.getByText(`Update ${segmentAPAC.name}`)) + expect(getRenderedSegmentNames()).toEqual([ + `${segmentAPAC.name} (Updated)`, + segmentOpenSource.name + ]) + }) +}) + +const segmentAPAC: SavedSegment & { segment_data: SegmentData } = { + id: 1, + name: 'APAC region', + type: SegmentType.personal, + owner_id: 100, + owner_name: 'Test User', + inserted_at: '2025-03-10T10:00:00', + updated_at: '2025-03-11T10:00:00', + segment_data: { + filters: [['is', 'country', ['JP', 'NZ']]], + labels: { JP: 'Japan', NZ: 'New Zealand' } + } +} + +const segmentOpenSource: SavedSegment & { segment_data: SegmentData } = { + id: 2, + name: 'Open source fans', + type: SegmentType.site, + owner_id: 200, + owner_name: 'Other User', + inserted_at: '2025-03-11T10:00:00', + updated_at: '2025-03-12T10:00:00', + segment_data: { + filters: [ + ['is', 'browser', ['Firefox']], + ['is', 'os', ['Linux']] + ], + labels: {} + } +} diff --git a/assets/js/dashboard/filtering/segments-context.tsx b/assets/js/dashboard/filtering/segments-context.tsx new file mode 100644 index 0000000000..0037aab22b --- /dev/null +++ b/assets/js/dashboard/filtering/segments-context.tsx @@ -0,0 +1,82 @@ +/** @format */ +import React, { + createContext, + ReactNode, + useCallback, + useContext, + useState +} from 'react' +import { + handleSegmentResponse, + SavedSegment, + SavedSegmentPublic, + SavedSegments, + SegmentData +} from './segments' + +export function parsePreloadedSegments(dataset: DOMStringMap): SavedSegments { + return JSON.parse(dataset.segments!).map(handleSegmentResponse) +} + +type ChangeSegmentState = ( + segment: (SavedSegment | SavedSegmentPublic) & { segment_data: SegmentData } +) => void + +const initialValue: { + segments: SavedSegments + updateOne: ChangeSegmentState + addOne: ChangeSegmentState + removeOne: ChangeSegmentState +} = { + segments: [], + updateOne: () => {}, + addOne: () => {}, + removeOne: () => {} +} + +const SegmentsContext = createContext(initialValue) + +export const useSegmentsContext = () => { + return useContext(SegmentsContext) +} + +export const SegmentsContextProvider = ({ + preloadedSegments, + children +}: { + preloadedSegments: SavedSegments + children: ReactNode +}) => { + const [segments, setSegments] = useState(preloadedSegments) + + const removeOne: ChangeSegmentState = useCallback( + ({ id }) => + setSegments((currentSegments) => + currentSegments.filter((s) => s.id !== id) + ), + [] + ) + + const updateOne: ChangeSegmentState = useCallback( + (segment) => + setSegments((currentSegments) => [ + segment, + ...currentSegments.filter((s) => s.id !== segment.id) + ]), + [] + ) + + const addOne: ChangeSegmentState = useCallback( + (segment) => + setSegments((currentSegments) => [segment, ...currentSegments]), + [] + ) + + return ( + + {children} + + ) +} diff --git a/assets/js/dashboard/filtering/segments.test.ts b/assets/js/dashboard/filtering/segments.test.ts index c45ee406f7..d896fefeca 100644 --- a/assets/js/dashboard/filtering/segments.test.ts +++ b/assets/js/dashboard/filtering/segments.test.ts @@ -7,8 +7,17 @@ import { getSearchToApplySingleSegmentFilter, getSegmentNamePlaceholder, isSegmentIdLabelKey, - parseApiSegmentData + parseApiSegmentData, + isListableSegment, + resolveFilters, + SegmentType, + SavedSegment, + SegmentData, + canSeeSegmentDetails } from './segments' +import { Filter } from '../query' +import { PlausibleSite } from '../site-context' +import { Role, UserContextValue } from '../user-context' describe(`${getFilterSegmentsByNameInsensitive.name}`, () => { const unfilteredSegments = [ @@ -109,3 +118,111 @@ describe(`${getSearchToApplySingleSegmentFilter.name}`, () => { }) }) }) + +describe(`${isListableSegment.name}`, () => { + const site: Pick = { + siteSegmentsAvailable: true + } + const user: UserContextValue = { loggedIn: true, id: 1, role: Role.editor } + + it('should return true for site segment when siteSegmentsAvailable is true', () => { + const segment = { id: 1, type: SegmentType.site, owner_id: 1 } + expect(isListableSegment({ segment, site, user })).toBe(true) + }) + + it('should return false for personal segment when user is not logged in', () => { + const segment = { id: 1, type: SegmentType.personal, owner_id: 1 } + expect( + isListableSegment({ + segment, + site, + user: { loggedIn: false, role: Role.public, id: null } + }) + ).toBe(false) + }) + + it('should return true for personal segment when user is the owner', () => { + const segment = { id: 1, type: SegmentType.personal, owner_id: 1 } + expect(isListableSegment({ segment, site, user })).toBe(true) + }) + + it('should return false for personal segment when user is not the owner', () => { + const segment = { id: 1, type: SegmentType.personal, owner_id: 2 } + expect(isListableSegment({ segment, site, user })).toBe(false) + }) +}) + +describe(`${resolveFilters.name}`, () => { + const segmentData: SegmentData = { + filters: [['is', 'browser', ['Chrome']]], + labels: {} + } + const segments: Array< + Pick & { segment_data: SegmentData } + > = [{ id: 1, segment_data: segmentData }] + + it('should resolve segment filters to their actual filters', () => { + const resolvedFilters = resolveFilters( + [ + ['is', 'segment', [1]], + ['is', 'browser', ['Firefox']] + ], + segments + ) + expect(resolvedFilters).toEqual([ + ...segmentData.filters, + ['is', 'browser', ['Firefox']] + ]) + }) + + it('should return the original filter if it is not a segment filter', () => { + const filters: Filter[] = [['is', 'browser', ['Firefox']]] + const resolvedFilters = resolveFilters(filters, segments) + expect(resolvedFilters).toEqual(filters) + }) + + it('should return the original filter if the segment is not found', () => { + const filters: Filter[] = [['is', 'segment', [2]]] + const resolvedFilters = resolveFilters(filters, segments) + expect(resolvedFilters).toEqual(filters) + }) + + const cases: Array<{ filters: Filter[] }> = [ + { + filters: [ + ['is', 'segment', [1]], + ['is', 'segment', [2]] + ] + }, + { filters: [['is', 'segment', [1, 2]]] } + ] + it.each(cases)( + 'should throw an error if more than one segment filter is applied, as in %p', + ({ filters }) => { + expect(() => resolveFilters(filters, segments)).toThrow( + 'Dashboard can be filtered by only one segment' + ) + } + ) +}) + +describe(`${canSeeSegmentDetails.name}`, () => { + it('should return true if the user is logged in and not a public role', () => { + const user: UserContextValue = { loggedIn: true, role: Role.admin, id: 1 } + expect(canSeeSegmentDetails({ user })).toBe(true) + }) + + it('should return false if the user is not logged in', () => { + const user: UserContextValue = { + loggedIn: false, + role: Role.editor, + id: null + } + expect(canSeeSegmentDetails({ user })).toBe(false) + }) + + it('should return false if the user has a public role', () => { + const user: UserContextValue = { loggedIn: true, role: Role.public, id: 1 } + expect(canSeeSegmentDetails({ user })).toBe(false) + }) +}) diff --git a/assets/js/dashboard/filtering/segments.ts b/assets/js/dashboard/filtering/segments.ts index 4760e11fcb..37750e5b41 100644 --- a/assets/js/dashboard/filtering/segments.ts +++ b/assets/js/dashboard/filtering/segments.ts @@ -4,6 +4,8 @@ import { DashboardQuery, Filter } from '../query' import { cleanLabels, remapFromApiFilters } from '../util/filters' import { plainFilterText } from '../util/filter-text' import { AppNavigationTarget } from '../navigation/use-app-navigate' +import { PlausibleSite } from '../site-context' +import { Role, UserContextValue } from '../user-context' export enum SegmentType { personal = 'personal', @@ -47,6 +49,12 @@ export type SegmentData = { labels: Record } +export type SavedSegments = Array< + (SavedSegment | SavedSegmentPublic) & { + segment_data: SegmentData + } +> + const SEGMENT_LABEL_KEY_PREFIX = 'segment-' export function handleSegmentResponse( @@ -86,11 +94,16 @@ export function isSegmentIdLabelKey(labelKey: string): boolean { return labelKey.startsWith(SEGMENT_LABEL_KEY_PREFIX) } -export function formatSegmentIdAsLabelKey(id: number): string { +export function formatSegmentIdAsLabelKey(id: number | string): string { return `${SEGMENT_LABEL_KEY_PREFIX}${id}` } -export const isSegmentFilter = (f: Filter): boolean => f[1] === 'segment' +export const isSegmentFilter = ( + filter: Filter +): filter is ['is', 'segment', (number | string)[]] => { + const [operation, dimension, clauses] = filter + return operation === 'is' && dimension === 'segment' && Array.isArray(clauses) +} export const parseApiSegmentData = ({ filters, @@ -123,3 +136,66 @@ export const SEGMENT_TYPE_LABELS = { [SegmentType.personal]: 'Personal segment', [SegmentType.site]: 'Site segment' } + +export function resolveFilters( + filters: Filter[], + segments: Array & { segment_data: SegmentData }> +): Filter[] { + let segmentsInFilter = 0 + return filters.flatMap((filter): Filter[] => { + if (isSegmentFilter(filter)) { + segmentsInFilter++ + const [_operation, _dimension, clauses] = filter + if (segmentsInFilter > 1 || clauses.length !== 1) { + throw new Error('Dashboard can be filtered by only one segment') + } + const segment = segments.find( + (segment) => String(segment.id) == String(clauses[0]) + ) + return segment ? segment.segment_data.filters : [filter] + } else { + return [filter] + } + }) +} + +export function isListableSegment({ + segment, + site, + user +}: { + segment: + | Pick + | Pick + site: Pick + user: UserContextValue +}) { + if (segment.type === SegmentType.site && site.siteSegmentsAvailable) { + return true + } + + if (segment.type === SegmentType.personal) { + if (!user.loggedIn || user.id === null || user.role === Role.public) { + return false + } + return segment.owner_id === user.id + } + + return false +} + +export function canSeeSegmentDetails({ user }: { user: UserContextValue }) { + return user.loggedIn && user.role !== Role.public +} + +export function findAppliedSegmentFilter({ filters }: { filters: Filter[] }) { + const segmentFilter = filters.find(isSegmentFilter) + if (!segmentFilter) { + return undefined + } + const [_operation, _dimension, clauses] = segmentFilter + if (clauses.length !== 1) { + throw new Error('Dashboard can be filtered by only one segment') + } + return segmentFilter +} 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 f5a9693c6a..d2dfb30135 100644 --- a/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx +++ b/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx @@ -1,25 +1,17 @@ /** @format */ -import React, { useCallback, useEffect, useMemo, useState } from 'react' +import React, { useEffect, useState } from 'react' import { useQueryContext } from '../../query-context' import { useSiteContext } from '../../site-context' import { formatSegmentIdAsLabelKey, getFilterSegmentsByNameInsensitive, - handleSegmentResponse, isSegmentFilter, SavedSegmentPublic, SavedSegment, - SegmentData, - SegmentDataFromApi, - SEGMENT_TYPE_LABELS + SEGMENT_TYPE_LABELS, + isListableSegment } from '../../filtering/segments' -import { - QueryFunction, - useQuery, - useQueryClient, - UseQueryResult -} from '@tanstack/react-query' import { cleanLabels } from '../../util/filters' import classNames from 'classnames' import { Tooltip } from '../../util/tooltip' @@ -29,27 +21,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' import { Role, useUserContext } from '../../user-context' - -function useSegmentsListQuery( - _isPublicRequest: boolean -): typeof _isPublicRequest extends true - ? UseQueryResult - : UseQueryResult { - const site = useSiteContext() - return useQuery({ - queryKey: ['segments'], - placeholderData: (previousData) => previousData, - queryFn: async () => { - const response = await get( - `/api/${encodeURIComponent(site.domain)}/segments` - ) - return response - } - }) -} +import { useSegmentsContext } from '../../filtering/segments-context' const linkClassName = classNames( popover.items.classNames.navigationLink, @@ -65,13 +38,17 @@ export const SearchableSegmentsSection = ({ }: { closeList: () => void }) => { - const { query, expandedSegment } = useQueryContext() - const segmentFilter = query.filters.find(isSegmentFilter) - const appliedSegmentIds = (segmentFilter ? segmentFilter[2] : []) as number[] + const site = useSiteContext() + const segmentsContext = useSegmentsContext() + + const { expandedSegment } = useQueryContext() const user = useUserContext() const isPublicListQuery = !user.loggedIn || user.role === Role.public - const { data, ...listQuery } = useSegmentsListQuery(isPublicListQuery) + + const data = segmentsContext.segments.filter((segment) => + isListableSegment({ segment, site, user }) + ) const [searchValue, setSearch] = useState() const [showAll, setShowAll] = useState(false) @@ -139,11 +116,7 @@ export const SearchableSegmentsSection = ({ } > - + ) })} @@ -174,111 +147,28 @@ export const SearchableSegmentsSection = ({ )} - - {listQuery.status === 'pending' && ( -
-
-
-
-
- )} - {listQuery.error && ( -
- listQuery.refetch()} - /> -
- )} ) } -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 response: SavedSegment & { segment_data: SegmentDataFromApi } = - await get(`/api/${encodeURIComponent(site.domain)}/segments/${id}`) - return handleSegmentResponse(response) - }, - [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, - error: getSegment.error, - status: getSegment.status - } -} - const SegmentLink = ({ id, name, - appliedSegmentIds, closeList }: Pick & { - 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 - ] + const updatedFilters = [['is', 'segment', [id]], ...otherFilters] return { ...search, diff --git a/assets/js/dashboard/query-context.tsx b/assets/js/dashboard/query-context.tsx index 2de1f2b881..fb0f37cc0d 100644 --- a/assets/js/dashboard/query-context.tsx +++ b/assets/js/dashboard/query-context.tsx @@ -19,9 +19,10 @@ import { queryDefaultValue, postProcessFilters } from './query' -import { SavedSegment, SegmentData } from './filtering/segments' +import { resolveFilters, SavedSegment, SegmentData } from './filtering/segments' import { useDefiniteLocationState } from './navigation/use-definite-location-state' import { useClearExpandedSegmentModeOnFilterClear } from './nav-menu/segments/segment-menu' +import { useSegmentsContext } from './filtering/segments-context' const queryContextDefaultValue = { query: queryDefaultValue, @@ -42,6 +43,7 @@ export default function QueryContextProvider({ }: { children: ReactNode }) { + const segmentsContext = useSegmentsContext() const location = useLocation() const { definiteValue: expandedSegment } = useDefiniteLocationState< SavedSegment & { segment_data: SegmentData } @@ -53,7 +55,7 @@ export default function QueryContextProvider({ compare_to, comparison, date, - filters, + filters: rawFilters, from, labels, match_day_of_week, @@ -74,6 +76,12 @@ export default function QueryContextProvider({ segmentIsExpanded: !!expandedSegment }) + const filters = Array.isArray(rawFilters) + ? postProcessFilters(rawFilters as Filter[]) + : defaultValues.filters + + const resolvedFilters = resolveFilters(filters, segmentsContext.segments) + return { ...timeQuery, compare_from: @@ -103,9 +111,8 @@ export default function QueryContextProvider({ with_imported: [true, false].includes(with_imported as boolean) ? (with_imported as boolean) : defaultValues.with_imported, - filters: Array.isArray(filters) - ? postProcessFilters(filters as Filter[]) - : defaultValues.filters, + filters, + resolvedFilters, labels: (labels as FilterClauseLabels) || defaultValues.labels, legacy_time_on_page_cutoff: site.flags.new_time_on_page ? (legacy_time_on_page_cutoff as string) || site.legacyTimeOnPageCutoff @@ -116,7 +123,7 @@ export default function QueryContextProvider({ compare_to, comparison, date, - filters, + rawFilters, from, labels, match_day_of_week, @@ -125,7 +132,8 @@ export default function QueryContextProvider({ with_imported, legacy_time_on_page_cutoff, site, - expandedSegment + expandedSegment, + segmentsContext.segments ]) useClearExpandedSegmentModeOnFilterClear({ expandedSegment, query }) diff --git a/assets/js/dashboard/query.ts b/assets/js/dashboard/query.ts index 5c4005df32..ab0154d42a 100644 --- a/assets/js/dashboard/query.ts +++ b/assets/js/dashboard/query.ts @@ -33,22 +33,43 @@ export type Filter = [FilterOperator, FilterKey, FilterClause[]] * */ export type FilterClauseLabels = Record -export const queryDefaultValue = { - period: '30d' as QueryPeriod, - comparison: null as ComparisonMode | null, - match_day_of_week: true, - date: null as Dayjs | null, - from: null as Dayjs | null, - to: null as Dayjs | null, - compare_from: null as Dayjs | null, - compare_to: null as Dayjs | null, - filters: [] as Filter[], - labels: {} as FilterClauseLabels, - with_imported: true, - legacy_time_on_page_cutoff: undefined as string | undefined +export type DashboardQuery = { + period: QueryPeriod + comparison: ComparisonMode | null + match_day_of_week: boolean + date: Dayjs | null + from: Dayjs | null + to: Dayjs | null + compare_from: Dayjs | null + compare_to: Dayjs | null + filters: Filter[] + /** + * This property is the same as `filters` always, except when + * `filters` contains a "Segment is {segment ID}" filter. In this case, + * `resolvedFilters` has the segment filter replaced with its constituent filters, + * so the FE could be aware of what filters are applied. + */ + resolvedFilters: Filter[] + labels: FilterClauseLabels + with_imported: boolean + legacy_time_on_page_cutoff: string | undefined } -export type DashboardQuery = typeof queryDefaultValue +export const queryDefaultValue: DashboardQuery = { + period: '30d' as QueryPeriod, + comparison: null, + match_day_of_week: true, + date: null, + from: null, + to: null, + compare_from: null, + compare_to: null, + filters: [], + resolvedFilters: [], + labels: {}, + with_imported: true, + legacy_time_on_page_cutoff: undefined +} export type BreakdownResultMeta = { date_range_label: string diff --git a/assets/js/dashboard/segments/routeless-segment-modals.tsx b/assets/js/dashboard/segments/routeless-segment-modals.tsx index d829e37189..181247b500 100644 --- a/assets/js/dashboard/segments/routeless-segment-modals.tsx +++ b/assets/js/dashboard/segments/routeless-segment-modals.tsx @@ -22,10 +22,12 @@ import { useQueryContext } from '../query-context' import { Role, useUserContext } from '../user-context' import { mutation } from '../api' import { useRoutelessModalsContext } from '../navigation/routeless-modals-context' +import { useSegmentsContext } from '../filtering/segments-context' export type RoutelessSegmentModal = 'create' | 'update' | 'delete' export const RoutelessSegmentModals = () => { + const { updateOne, addOne, removeOne } = useSegmentsContext() const navigate = useAppNavigate() const queryClient = useQueryClient() const site = useSiteContext() @@ -64,6 +66,7 @@ export const RoutelessSegmentModals = () => { return handleSegmentResponse(response) }, onSuccess: async (segment) => { + updateOne(segment) queryClient.invalidateQueries({ queryKey: ['segments'] }) navigate({ search: getSearchToApplySingleSegmentFilter(segment), @@ -100,6 +103,7 @@ export const RoutelessSegmentModals = () => { return handleSegmentResponse(response) }, onSuccess: async (segment) => { + addOne(segment) queryClient.invalidateQueries({ queryKey: ['segments'] }) navigate({ search: getSearchToApplySingleSegmentFilter(segment), @@ -122,7 +126,8 @@ export const RoutelessSegmentModals = () => { ) return handleSegmentResponse(response) }, - onSuccess: (_segment): void => { + onSuccess: (segment): void => { + removeOne(segment) queryClient.invalidateQueries({ queryKey: ['segments'] }) navigate({ search: (s) => { diff --git a/assets/js/dashboard/segments/segment-authorship.tsx b/assets/js/dashboard/segments/segment-authorship.tsx index a3a91c0a50..605cc2bb3d 100644 --- a/assets/js/dashboard/segments/segment-authorship.tsx +++ b/assets/js/dashboard/segments/segment-authorship.tsx @@ -2,7 +2,8 @@ import React from 'react' import { SavedSegmentPublic, SavedSegment } from '../filtering/segments' -import { formatDayShort, parseNaiveDate } from '../util/date' +import { dateForSite, formatDayShort } from '../util/date' +import { useSiteContext } from '../site-context' type SegmentAuthorshipProps = { className?: string } & ( | { showOnlyPublicData: true; segment: SavedSegmentPublic } @@ -14,6 +15,7 @@ export function SegmentAuthorship({ showOnlyPublicData, segment }: SegmentAuthorshipProps) { + const site = useSiteContext() const authorLabel = showOnlyPublicData === true ? null @@ -25,12 +27,12 @@ export function SegmentAuthorship({ return (
- {`Created at ${formatDayShort(parseNaiveDate(inserted_at))}`} + {`Created at ${formatDayShort(dateForSite(inserted_at, site))}`} {!showUpdatedAt && !!authorLabel && ` by ${authorLabel}`}
{showUpdatedAt && (
- {`Last updated at ${formatDayShort(parseNaiveDate(updated_at))}`} + {`Last updated at ${formatDayShort(dateForSite(updated_at, site))}`} {!!authorLabel && ` by ${authorLabel}`}
)} diff --git a/assets/js/dashboard/segments/segment-modals.test.tsx b/assets/js/dashboard/segments/segment-modals.test.tsx new file mode 100644 index 0000000000..030f525556 --- /dev/null +++ b/assets/js/dashboard/segments/segment-modals.test.tsx @@ -0,0 +1,148 @@ +/** @format */ + +import React from 'react' +import { render, screen } from '@testing-library/react' +import { SegmentModal } from './segment-modals' +import { TestContextProviders } from '../../../test-utils/app-context-providers' +import { + SavedSegment, + SavedSegments, + SegmentData, + SegmentType +} from '../filtering/segments' +import { Role, UserContextValue } from '../user-context' +import { PlausibleSite } from '../site-context' + +beforeEach(() => { + const modalRoot = document.createElement('div') + modalRoot.id = 'modal_root' + document.body.appendChild(modalRoot) +}) + +const flags = { saved_segments: true, saved_segments_fe: true } + +describe('Segment details modal - errors', () => { + const anySiteSegment: SavedSegment & { segment_data: SegmentData } = { + id: 1, + type: SegmentType.site, + owner_id: 1, + owner_name: 'Test User', + name: 'Blog or About', + segment_data: { + filters: [['is', 'page', ['/blog', '/about']]], + labels: {} + }, + inserted_at: '2025-03-13T13:00:00', + updated_at: '2025-03-13T16:00:00' + } + + const anyPersonalSegment: SavedSegment & { segment_data: SegmentData } = { + ...anySiteSegment, + id: 2, + type: SegmentType.personal + } + + const cases: { + case: string + segments: SavedSegments + segmentId: number + user: UserContextValue + message: string + siteOptions: Partial + }[] = [ + { + case: 'segment is not in list', + segments: [anyPersonalSegment, anySiteSegment], + segmentId: 202020, + user: { loggedIn: true, id: 1, role: Role.owner }, + message: `Segment not found with with ID "202020"`, + siteOptions: { flags, siteSegmentsAvailable: true } + }, + { + case: 'site segment is in list but not listable because site segments are not available', + segments: [anyPersonalSegment, anySiteSegment], + segmentId: anySiteSegment.id, + user: { loggedIn: true, id: 1, role: Role.owner }, + message: `Segment not found with with ID "${anySiteSegment.id}"`, + siteOptions: { flags, siteSegmentsAvailable: false } + }, + { + case: 'personal segment is in list but not listable because it is a public dashboard', + segments: [{ ...anyPersonalSegment, owner_id: null, owner_name: null }], + segmentId: anyPersonalSegment.id, + user: { loggedIn: false, id: null, role: Role.public }, + message: `Segment not found with with ID "${anyPersonalSegment.id}"`, + siteOptions: { flags, siteSegmentsAvailable: true } + }, + { + case: 'segment is in list and listable, but detailed view is not available because user is not logged in', + segments: [{ ...anySiteSegment, owner_id: null, owner_name: null }], + segmentId: anySiteSegment.id, + user: { loggedIn: false, id: null, role: Role.public }, + message: 'Not enough permissions to see segment details', + siteOptions: { flags, siteSegmentsAvailable: true } + } + ] + it.each(cases)( + 'shows error `$message` when $case', + ({ user, segments, segmentId, message, siteOptions }) => { + render(, { + wrapper: (props) => ( + + ) + }) + + expect(screen.getByText(message)).toBeVisible() + expect(screen.queryByText(`Edit segment`)).not.toBeInTheDocument() + } + ) +}) + +describe('Segment details modal - other cases', () => { + it('displays site segment correctly', () => { + const anySiteSegment: SavedSegment & { segment_data: SegmentData } = { + id: 100, + type: SegmentType.site, + owner_id: 100100, + owner_name: 'Test User', + name: 'Blog or About', + segment_data: { + filters: [['is', 'page', ['/blog', '/about']]], + labels: {} + }, + inserted_at: '2025-03-13T13:00:00', + updated_at: '2025-03-13T16:00:00' + } + + render(, { + wrapper: (props) => ( + + ) + }) + expect(screen.getByText(anySiteSegment.name)).toBeVisible() + expect(screen.getByText('Site segment')).toBeVisible() + + expect(screen.getByText('Filters in segment')).toBeVisible() + expect(screen.getByTitle('Page is /blog or /about')).toBeVisible() + + expect( + screen.getByText(`Last updated at 13 Mar by ${anySiteSegment.owner_name}`) + ).toBeVisible() + expect(screen.getByText(`Created at 13 Mar`)).toBeVisible() + + expect(screen.getByText('Edit segment')).toBeVisible() + expect(screen.getByText('Remove filter')).toBeVisible() + }) +}) diff --git a/assets/js/dashboard/segments/segment-modals.tsx b/assets/js/dashboard/segments/segment-modals.tsx index 822bf8ee6e..a1c372fb82 100644 --- a/assets/js/dashboard/segments/segment-modals.tsx +++ b/assets/js/dashboard/segments/segment-modals.tsx @@ -1,15 +1,16 @@ /** @format */ -import React, { ReactNode, useEffect, useState } from 'react' +import React, { ReactNode, useState } from 'react' import ModalWithRouting from '../stats/modals/modal' import { + canSeeSegmentDetails, + isListableSegment, isSegmentFilter, SavedSegment, SEGMENT_TYPE_LABELS, 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' @@ -22,6 +23,9 @@ import { ExclamationTriangleIcon, TrashIcon } from '@heroicons/react/24/outline' import { MutationStatus } from '@tanstack/react-query' import { ApiError } from '../api' import { ErrorPanel } from '../components/error-panel' +import { useSegmentsContext } from '../filtering/segments-context' +import { useSiteContext } from '../site-context' +import { useUserContext } from '../user-context' interface ApiRequestProps { status: MutationStatus @@ -427,15 +431,28 @@ const Placeholder = ({ ) export const SegmentModal = ({ id }: { id: SavedSegment['id'] }) => { + const site = useSiteContext() + const user = useUserContext() const { query } = useQueryContext() + const { segments } = useSegmentsContext() - const { data, fetchSegment, status, error } = useSegmentPrefetch({ - id: String(id) - }) + const segment = segments + .filter((s) => isListableSegment({ segment: s, site, user })) + .find((s) => String(s.id) === String(id)) - useEffect(() => { - fetchSegment() - }, [fetchSegment]) + let error: ApiError | null = null + + if (!segment) { + error = new ApiError(`Segment not found with with ID "${id}"`, { + error: `Segment not found with with ID "${id}"` + }) + } else if (!canSeeSegmentDetails({ user })) { + error = new ApiError('Not enough permissions to see segment details', { + error: `Not enough permissions to see segment details` + }) + } + + const data = !error ? segment : null return ( @@ -506,13 +523,6 @@ export const SegmentModal = ({ id }: { id: SavedSegment['id'] }) => {
)} - {status === 'pending' && ( -
-
-
-
-
- )} {error !== null && ( { ? error.message : 'Something went wrong loading segment' } - onRetry={() => fetchSegment()} + onRetry={() => window.location.reload()} /> )}
diff --git a/assets/js/dashboard/stats/modals/filter-modal.js b/assets/js/dashboard/stats/modals/filter-modal.js index db7b6900e9..65d542022c 100644 --- a/assets/js/dashboard/stats/modals/filter-modal.js +++ b/assets/js/dashboard/stats/modals/filter-modal.js @@ -11,7 +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'; +import { findAppliedSegmentFilter } from '../../filtering/segments'; function partitionFilters(modalType, filters) { const otherFilters = [] @@ -202,10 +202,13 @@ export default function FilterModalWithRouter(props) { 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] - return + const appliedSegmentFilter = + field === 'segment' + ? findAppliedSegmentFilter({ filters: query.filters }) + : null + if (appliedSegmentFilter) { + const [_operation, _dimension, [segmentId]] = appliedSegmentFilter + return } return ( - filterKey.startsWith(prefix) - ) + return query.filters.filter(hasDimensionPrefix(prefix)) } +const hasDimensionPrefix = + (prefix) => + ([_operation, dimension, _clauses]) => + dimension.startsWith(prefix) + function omitFiltersByKeyPrefix(query, prefix) { return query.filters.filter( ([_operation, filterKey, _clauses]) => !filterKey.startsWith(prefix) @@ -120,9 +123,11 @@ export function isFilteringOnFixedValue(query, filterKey, expectedValue) { } export function hasConversionGoalFilter(query) { - const goalFilters = getFiltersByKeyPrefix(query, 'goal') + const resolvedGoalFilters = query.resolvedFilters.filter( + hasDimensionPrefix('goal') + ) - return goalFilters.some(([operation, _filterKey, _clauses]) => { + return resolvedGoalFilters.some(([operation, _filterKey, _clauses]) => { return operation !== FILTER_OPERATIONS.has_not_done }) } diff --git a/assets/test-utils/app-context-providers.tsx b/assets/test-utils/app-context-providers.tsx index 7bfd0339b5..ec0a3cacdf 100644 --- a/assets/test-utils/app-context-providers.tsx +++ b/assets/test-utils/app-context-providers.tsx @@ -4,23 +4,32 @@ import React, { ReactNode } from 'react' import SiteContextProvider, { PlausibleSite } from '../js/dashboard/site-context' -import UserContextProvider, { Role } from '../js/dashboard/user-context' +import UserContextProvider, { + Role, + UserContextValue +} from '../js/dashboard/user-context' 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' +import { SegmentsContextProvider } from '../js/dashboard/filtering/segments-context' +import { SavedSegments } from '../js/dashboard/filtering/segments' type TestContextProvidersProps = { children: ReactNode routerProps?: Pick siteOptions?: Partial + user?: UserContextValue + preloaded?: { segments?: SavedSegments } } export const TestContextProviders = ({ children, routerProps, - siteOptions + siteOptions, + preloaded, + user }: TestContextProvidersProps) => { const defaultSite: PlausibleSite = { domain: 'plausible.io/unit', @@ -62,18 +71,22 @@ export const TestContextProviders = ({ return ( // not interactive component, default value is suitable - - - - - {children} - - - + + + + + + {children} + + + + // diff --git a/lib/plausible/segments/segment.ex b/lib/plausible/segments/segment.ex index 39aabbc97c..22cd86b4d1 100644 --- a/lib/plausible/segments/segment.ex +++ b/lib/plausible/segments/segment.ex @@ -203,14 +203,8 @@ defimpl Jason.Encoder, for: Plausible.Segments.Segment do segment_data: segment.segment_data, owner_id: segment.owner_id, owner_name: if(is_nil(segment.owner_id), do: nil, else: segment.owner.name), - inserted_at: - segment.inserted_at - |> Plausible.Timezones.to_datetime_in_timezone(segment.site.timezone) - |> Calendar.strftime("%Y-%m-%d %H:%M:%S"), - updated_at: - segment.updated_at - |> Plausible.Timezones.to_datetime_in_timezone(segment.site.timezone) - |> Calendar.strftime("%Y-%m-%d %H:%M:%S") + inserted_at: segment.inserted_at, + updated_at: segment.updated_at } |> Jason.Encode.map(opts) end diff --git a/lib/plausible/segments/segments.ex b/lib/plausible/segments/segments.ex index 91d73f76a4..4bc41348ef 100644 --- a/lib/plausible/segments/segments.ex +++ b/lib/plausible/segments/segments.ex @@ -17,25 +17,32 @@ defmodule Plausible.Segments do @max_segments 500 - @spec index(pos_integer() | nil, Plausible.Site.t(), atom()) :: - {:ok, [Segment.t()]} | error_not_enough_permissions() - def index(user_id, %Plausible.Site{} = site, site_role) do - fields = [:id, :name, :type, :inserted_at, :updated_at, :owner_id] - - site_segments_available? = - site_segments_available?(site) + def get_all_for_site(%Plausible.Site{} = site, site_role) do + fields = [:id, :name, :type, :inserted_at, :updated_at, :segment_data] cond do - site_role in [:public] and - site_segments_available? -> - {:ok, get_public_site_segments(site.id, fields -- [:owner_id])} + site_role in [:public] -> + {:ok, + Repo.all( + from(segment in Segment, + select: ^fields, + where: segment.site_id == ^site.id, + order_by: [desc: segment.updated_at, desc: segment.id] + ) + )} - site_role in @roles_with_maybe_site_segments and - site_segments_available? -> - {:ok, get_segments(user_id, site.id, fields)} + site_role in @roles_with_personal_segments or site_role in @roles_with_maybe_site_segments -> + fields = fields ++ [:owner_id] - site_role in @roles_with_personal_segments -> - {:ok, get_segments(user_id, site.id, fields, only: :personal)} + {:ok, + Repo.all( + from(segment in Segment, + select: ^fields, + where: segment.site_id == ^site.id, + order_by: [desc: segment.updated_at, desc: segment.id], + preload: [:owner] + ) + )} true -> {:error, :not_enough_permissions} @@ -348,42 +355,6 @@ defmodule Plausible.Segments do def site_segments_available?(%Plausible.Site{} = site), do: Plausible.Billing.Feature.SiteSegments.check_availability(site.team) == :ok - @spec get_public_site_segments(pos_integer(), list(atom())) :: [Segment.t()] - defp get_public_site_segments(site_id, fields) do - Repo.all( - from(segment in Segment, - select: ^fields, - where: segment.site_id == ^site_id, - where: segment.type == :site, - order_by: [desc: segment.updated_at, desc: segment.id] - ) - ) - end - - @spec get_segments(pos_integer(), pos_integer(), list(atom()), Keyword.t()) :: [Segment.t()] - defp get_segments(user_id, site_id, fields, opts \\ []) do - query = - from(segment in Segment, - select: ^fields, - where: segment.site_id == ^site_id, - order_by: [desc: segment.updated_at, desc: segment.id], - preload: [:owner] - ) - - query = - if Keyword.get(opts, :only) == :personal do - where(query, [segment], segment.type == :personal and segment.owner_id == ^user_id) - else - where( - query, - [segment], - segment.type == :site or (segment.type == :personal and segment.owner_id == ^user_id) - ) - end - - 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)" @@ -399,15 +370,6 @@ defmodule Plausible.Segments do "#{field} #{formatted_message}" end - @doc """ - This function enriches the segment with site, without actually querying the database for the site again. - Needed for Plausible.Segments.Segment custom JSON serialization. - """ - @spec enrich_with_site(Segment.t(), Plausible.Site.t()) :: Segment.t() - def enrich_with_site(%Segment{} = segment, %Plausible.Site{} = site) do - Map.put(segment, :site, site) - end - @spec get_site_segments_usage_query(list(pos_integer())) :: Ecto.Query.t() def get_site_segments_usage_query(site_ids) do from(segment in Segment, diff --git a/lib/plausible_web/controllers/api/internal/segments_controller.ex b/lib/plausible_web/controllers/api/internal/segments_controller.ex index 25454e980d..c7962ba6b8 100644 --- a/lib/plausible_web/controllers/api/internal/segments_controller.ex +++ b/lib/plausible_web/controllers/api/internal/segments_controller.ex @@ -8,59 +8,6 @@ defmodule PlausibleWeb.Api.Internal.SegmentsController do alias PlausibleWeb.Api.Helpers, as: H alias Plausible.Segments - def index( - %Plug.Conn{ - assigns: %{ - site: site, - site_role: site_role - } - } = conn, - %{} = _params - ) do - user_id = normalize_current_user_id(conn) - - case Segments.index(user_id, site, site_role) do - {:error, :not_enough_permissions} -> - H.not_enough_permissions(conn, "Not enough permissions to get segments") - - {:ok, segments} -> - json( - conn, - Enum.map(segments, fn segment -> Segments.enrich_with_site(segment, site) end) - ) - end - end - - def get( - %Plug.Conn{ - assigns: %{ - site: site, - site_role: site_role - } - } = conn, - %{} = params - ) do - segment_id = normalize_segment_id_param(params["segment_id"]) - - user_id = normalize_current_user_id(conn) - - case Segments.get_one( - user_id, - site, - site_role, - segment_id - ) do - {:error, :not_enough_permissions} -> - H.not_enough_permissions(conn, "Not enough permissions to get segment data") - - {:error, :segment_not_found} -> - segment_not_found(conn, params["segment_id"]) - - {:ok, segment} -> - json(conn, Segments.enrich_with_site(segment, site)) - end - end - def create( %Plug.Conn{ assigns: %{ @@ -86,7 +33,7 @@ defmodule PlausibleWeb.Api.Internal.SegmentsController do }) {:ok, segment} -> - json(conn, Segments.enrich_with_site(segment, site)) + json(conn, segment) end end @@ -120,7 +67,7 @@ defmodule PlausibleWeb.Api.Internal.SegmentsController do }) {:ok, segment} -> - json(conn, Segments.enrich_with_site(segment, site)) + json(conn, segment) end end @@ -147,16 +94,12 @@ defmodule PlausibleWeb.Api.Internal.SegmentsController do segment_not_found(conn, params["segment_id"]) {:ok, segment} -> - json(conn, Segments.enrich_with_site(segment, site)) + json(conn, segment) end end def delete(%Plug.Conn{} = conn, _params), do: invalid_request(conn) - @spec normalize_current_user_id(Plug.Conn.t()) :: nil | pos_integer() - defp normalize_current_user_id(conn), - do: if(is_nil(conn.assigns[:current_user]), do: nil, else: conn.assigns[:current_user].id) - @spec normalize_segment_id_param(any()) :: nil | pos_integer() defp normalize_segment_id_param(input) do case Integer.parse(input) do diff --git a/lib/plausible_web/controllers/stats_controller.ex b/lib/plausible_web/controllers/stats_controller.ex index f8bd1082de..c51ee9beef 100644 --- a/lib/plausible_web/controllers/stats_controller.ex +++ b/lib/plausible_web/controllers/stats_controller.ex @@ -52,9 +52,10 @@ defmodule PlausibleWeb.StatsController do def stats(%{assigns: %{site: site}} = conn, _params) do site = Plausible.Repo.preload(site, :owners) + site_role = conn.assigns[:site_role] current_user = conn.assigns[:current_user] stats_start_date = Plausible.Sites.stats_start_date(site) - can_see_stats? = not Sites.locked?(site) or conn.assigns[:site_role] == :super_admin + can_see_stats? = not Sites.locked?(site) or site_role == :super_admin demo = site.domain == PlausibleWeb.Endpoint.host() dogfood_page_path = if demo, do: "/#{site.domain}", else: "/:dashboard" skip_to_dashboard? = conn.params["skip_to_dashboard"] == "true" @@ -62,6 +63,8 @@ defmodule PlausibleWeb.StatsController do scroll_depth_visible? = Plausible.Stats.ScrollDepth.check_feature_visible!(site, current_user) + {:ok, segments} = Plausible.Segments.get_all_for_site(site, site_role) + cond do (stats_start_date && can_see_stats?) || (can_see_stats? && skip_to_dashboard?) -> flags = get_flags(current_user, site) @@ -70,6 +73,7 @@ defmodule PlausibleWeb.StatsController do |> put_resp_header("x-robots-tag", "noindex, nofollow") |> render("stats.html", site: site, + site_role: site_role, has_goals: Plausible.Sites.has_goals?(site), revenue_goals: list_revenue_goals(site), funnels: list_funnels(site), @@ -83,6 +87,7 @@ defmodule PlausibleWeb.StatsController do flags: flags, is_dbip: is_dbip(), dogfood_page_path: dogfood_page_path, + segments: segments, load_dashboard_js: true ) @@ -342,6 +347,7 @@ defmodule PlausibleWeb.StatsController do cond do !shared_link.site.locked -> current_user = conn.assigns[:current_user] + site_role = get_fallback_site_role(conn) shared_link = Plausible.Repo.preload(shared_link, site: :owners) stats_start_date = Plausible.Sites.stats_start_date(shared_link.site) @@ -350,11 +356,14 @@ defmodule PlausibleWeb.StatsController do flags = get_flags(current_user, shared_link.site) + {:ok, segments} = Plausible.Segments.get_all_for_site(shared_link.site, site_role) + conn |> put_resp_header("x-robots-tag", "noindex, nofollow") |> delete_resp_header("x-frame-options") |> render("stats.html", site: shared_link.site, + site_role: site_role, has_goals: Sites.has_goals?(shared_link.site), revenue_goals: list_revenue_goals(shared_link.site), funnels: list_funnels(shared_link.site), @@ -372,6 +381,7 @@ defmodule PlausibleWeb.StatsController do theme: conn.params["theme"], flags: flags, is_dbip: is_dbip(), + segments: segments, load_dashboard_js: true ) @@ -386,6 +396,9 @@ defmodule PlausibleWeb.StatsController do end end + defp get_fallback_site_role(conn), + do: if(role = conn.assigns[:site_role], do: role, else: :public) + defp shared_link_cookie_name(slug), do: "shared-link-" <> slug defp get_flags(user, site), diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index 44acf6843a..39db08b7a7 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -221,9 +221,7 @@ defmodule PlausibleWeb.Router do end scope "/:domain/segments", PlausibleWeb.Api.Internal do - get "/", SegmentsController, :index post "/", SegmentsController, :create - get "/:segment_id", SegmentsController, :get patch "/:segment_id", SegmentsController, :update delete "/:segment_id", SegmentsController, :delete end diff --git a/lib/plausible_web/templates/stats/stats.html.heex b/lib/plausible_web/templates/stats/stats.html.heex index f32e6d21e1..41d3d15bda 100644 --- a/lib/plausible_web/templates/stats/stats.html.heex +++ b/lib/plausible_web/templates/stats/stats.html.heex @@ -42,13 +42,12 @@ data-embedded={to_string(@conn.assigns[:embedded])} data-background={@conn.assigns[:background]} data-is-dbip={to_string(@is_dbip)} - data-current-user-role={ - if site_role = @conn.assigns[:site_role], do: site_role, else: :public - } + data-current-user-role={@site_role} data-current-user-id={ if user = @conn.assigns[:current_user], do: user.id, else: Jason.encode!(nil) } data-flags={Jason.encode!(@flags)} + data-segments={Jason.encode!(@segments)} data-valid-intervals-by-period={ Plausible.Stats.Interval.valid_by_period(site: @site) |> Jason.encode!() } 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 0aa44b2c89..733ea17e73 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 @@ -3,304 +3,6 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do use Plausible.Repo use Plausible.Teams.Test - describe "GET /api/:domain/segments" do - setup [:create_user, :log_in, :create_site] - - test "returns empty list when no segments", %{conn: conn, site: site} do - conn = - get(conn, "/api/#{site.domain}/segments") - - assert json_response(conn, 200) == [] - end - - test "returns site segments list when looking at a public dashboard", %{conn: conn} do - other_user = new_user() - site = new_site(owner: other_user, public: true) - - site_segments = - insert_list(2, :segment, - site: site, - owner: other_user, - type: :site, - name: "other site segment" - ) - - insert_list(10, :segment, - site: site, - owner: other_user, - type: :personal, - name: "other user personal segment" - ) - - conn = get(conn, "/api/#{site.domain}/segments") - - assert json_response(conn, 200) == - Enum.reverse( - Enum.map(site_segments, fn s -> - %{ - "id" => s.id, - "name" => s.name, - "type" => Atom.to_string(s.type), - "inserted_at" => Calendar.strftime(s.inserted_at, "%Y-%m-%d %H:%M:%S"), - "updated_at" => Calendar.strftime(s.updated_at, "%Y-%m-%d %H:%M:%S"), - "owner_id" => nil, - "owner_name" => nil, - "segment_data" => nil - } - end) - ) - end - - test "forbids owners on growth plan from seeing site segments", %{ - conn: conn, - user: user, - site: site - } do - user |> subscribe_to_growth_plan() - - insert_list(2, :segment, - site: site, - owner: user, - type: :site, - name: "site segment" - ) - - conn = - get(conn, "/api/#{site.domain}/segments") - - assert json_response(conn, 200) == [] - end - - for role <- [:viewer, :owner] do - test "returns list with personal and site segments for #{role}, avoiding segments from other site", - %{conn: conn, user: user, site: site} do - team = team_of(user) - other_user = new_user(name: "Other User") - other_site = new_site(team: team) - add_member(team, user: other_user, role: :owner) - - insert_list(2, :segment, - site: other_site, - owner: user, - type: :site, - name: "other site segment" - ) - - insert_list(10, :segment, - site: site, - owner: other_user, - type: :personal, - name: "other user personal segment" - ) - - personal_segment = - insert(:segment, - site: site, - owner: user, - type: :personal, - name: "a personal segment" - ) - |> Map.put(:owner_name, user.name) - - emea_site_segment = - insert(:segment, - site: site, - owner: other_user, - type: :site, - name: "EMEA region" - ) - |> Map.put(:owner_name, other_user.name) - - apac_site_segment = - insert(:segment, - site: site, - owner: user, - type: :site, - name: "APAC region" - ) - |> Map.put(:owner_name, user.name) - - dangling_site_segment = - insert(:segment, - site: site, - owner: nil, - type: :site, - name: "Another region" - ) - |> Map.put(:owner_name, nil) - - conn = - get(conn, "/api/#{site.domain}/segments") - - assert json_response(conn, 200) == - Enum.map( - [ - dangling_site_segment, - apac_site_segment, - emea_site_segment, - personal_segment - ], - fn s -> - %{ - "id" => s.id, - "name" => s.name, - "type" => Atom.to_string(s.type), - "owner_id" => s.owner_id, - "owner_name" => s.owner_name, - "inserted_at" => Calendar.strftime(s.inserted_at, "%Y-%m-%d %H:%M:%S"), - "updated_at" => Calendar.strftime(s.updated_at, "%Y-%m-%d %H:%M:%S"), - "segment_data" => nil - } - end - ) - end - end - end - - describe "GET /api/:domain/segments/:segment_id" do - setup [:create_user, :create_site, :log_in] - - test "serves 404 when invalid segment key used", %{conn: conn, site: site} do - conn = - get(conn, "/api/#{site.domain}/segments/any-id") - - assert json_response(conn, 404) == %{"error" => "Segment not found with ID \"any-id\""} - end - - test "serves 404 when no segment found", %{conn: conn, site: site} do - conn = - get(conn, "/api/#{site.domain}/segments/100100") - - assert json_response(conn, 404) == %{"error" => "Segment not found with ID \"100100\""} - end - - test "serves 404 when segment is for another site", %{conn: conn, site: site, user: user} do - other_site = new_site(owner: user) - - segment = - insert(:segment, - site: other_site, - owner: user, - type: :site, - name: "any" - ) - - conn = - get(conn, "/api/#{site.domain}/segments/#{segment.id}") - - assert json_response(conn, 404) == %{ - "error" => "Segment not found with ID \"#{segment.id}\"" - } - end - - test "serves 404 for viewing contents of site segments for viewers of public dashboards", - %{ - conn: conn - } do - site = new_site(public: true) - other_user = add_guest(site, user: new_user(), role: :editor) - - segment = - insert(:segment, - type: :site, - owner: other_user, - site: site, - name: "any" - ) - - conn = - get(conn, "/api/#{site.domain}/segments/#{segment.id}") - - assert json_response(conn, 403) == %{ - "error" => "Not enough permissions to get segment data" - } - end - - test "serves 404 when user is not the segment owner and segment is personal", - %{ - conn: conn, - site: site - } do - other_user = add_guest(site, role: :editor) - - segment = - insert(:segment, - type: :personal, - owner: other_user, - site: site, - name: "any" - ) - - conn = - get(conn, "/api/#{site.domain}/segments/#{segment.id}") - - assert json_response(conn, 404) == %{ - "error" => "Segment not found with ID \"#{segment.id}\"" - } - end - - test "serves 200 with segment when user is not the segment owner and segment is not personal", - %{ - conn: conn, - user: user - } do - site = new_site(owner: user, timezone: "Asia/Tokyo") - other_user = add_guest(site, role: :editor) - - segment = - insert(:segment, - type: :site, - owner: other_user, - site: site, - name: "any", - inserted_at: "2024-09-01T22:00:00.000Z", - updated_at: "2024-09-01T23:00:00.000Z" - ) - - conn = - get(conn, "/api/#{site.domain}/segments/#{segment.id}") - - assert json_response(conn, 200) == %{ - "id" => segment.id, - "owner_id" => other_user.id, - "owner_name" => other_user.name, - "name" => segment.name, - "type" => Atom.to_string(segment.type), - "segment_data" => segment.segment_data, - "inserted_at" => "2024-09-02 07:00:00", - "updated_at" => "2024-09-02 08:00:00" - } - end - - test "serves 200 with segment when user is segment owner", %{ - conn: conn, - site: site, - user: user - } do - segment = - insert(:segment, - site: site, - name: "any", - owner: user, - type: :personal - ) - - conn = - get(conn, "/api/#{site.domain}/segments/#{segment.id}") - - assert json_response(conn, 200) == %{ - "id" => segment.id, - "owner_id" => user.id, - "owner_name" => user.name, - "name" => segment.name, - "type" => Atom.to_string(segment.type), - "segment_data" => segment.segment_data, - "inserted_at" => Calendar.strftime(segment.inserted_at, "%Y-%m-%d %H:%M:%S"), - "updated_at" => Calendar.strftime(segment.updated_at, "%Y-%m-%d %H:%M:%S") - } - end - end - describe "POST /api/:domain/segments" do setup [:create_user, :log_in, :create_site] @@ -638,7 +340,7 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do "inserted_at" => ^any( :string, - ~r/#{Calendar.strftime(segment.inserted_at, "%Y-%m-%d %H:%M:%S")}/ + ~r/#{Calendar.strftime(segment.inserted_at, "%Y-%m-%dT%H:%M:%S")}/ ), "updated_at" => ^any(:iso8601_naive_datetime) }) = response @@ -678,7 +380,7 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do "inserted_at" => ^any( :string, - ~r/#{Calendar.strftime(segment.inserted_at, "%Y-%m-%d %H:%M:%S")}/ + ~r/#{Calendar.strftime(segment.inserted_at, "%Y-%m-%dT%H:%M:%S")}/ ), "updated_at" => ^any(:iso8601_naive_datetime) }) = response @@ -760,8 +462,8 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do "name" => segment.name, "segment_data" => segment.segment_data, "type" => "#{unquote(type)}", - "inserted_at" => Calendar.strftime(segment.inserted_at, "%Y-%m-%d %H:%M:%S"), - "updated_at" => Calendar.strftime(segment.updated_at, "%Y-%m-%d %H:%M:%S") + "inserted_at" => Calendar.strftime(segment.inserted_at, "%Y-%m-%dT%H:%M:%S"), + "updated_at" => Calendar.strftime(segment.updated_at, "%Y-%m-%dT%H:%M:%S") } == response verify_no_segment_in_db(segment) @@ -792,8 +494,8 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do "name" => segment.name, "segment_data" => segment.segment_data, "type" => "site", - "inserted_at" => Calendar.strftime(segment.inserted_at, "%Y-%m-%d %H:%M:%S"), - "updated_at" => Calendar.strftime(segment.updated_at, "%Y-%m-%d %H:%M:%S") + "inserted_at" => Calendar.strftime(segment.inserted_at, "%Y-%m-%dT%H:%M:%S"), + "updated_at" => Calendar.strftime(segment.updated_at, "%Y-%m-%dT%H:%M:%S") } == response verify_no_segment_in_db(segment) diff --git a/test/plausible_web/controllers/stats_controller_test.exs b/test/plausible_web/controllers/stats_controller_test.exs index 821eeb7c39..d546bc5722 100644 --- a/test/plausible_web/controllers/stats_controller_test.exs +++ b/test/plausible_web/controllers/stats_controller_test.exs @@ -41,6 +41,41 @@ defmodule PlausibleWeb.StatsControllerTest do assert text_of_element(resp, "title") == "Plausible ยท #{site.domain}" end + test "public site - all segments (personal or site) are stuffed into dataset, without their owner_id and owner_name", + %{conn: conn} do + user = new_user() + site = new_site(owner: user, public: true) + + populate_stats(site, [build(:pageview)]) + + emea_site_segment = + insert(:segment, + site: site, + owner: user, + type: :site, + name: "EMEA region" + ) + |> Map.put(:owner_name, nil) + |> Map.put(:owner_id, nil) + + foo_personal_segment = + insert(:segment, + site: site, + owner: user, + type: :personal, + name: "FOO" + ) + |> Map.put(:owner_name, nil) + |> Map.put(:owner_id, nil) + + conn = get(conn, "/#{site.domain}") + resp = html_response(conn, 200) + assert element_exists?(resp, @react_container) + + assert text_of_attr(resp, @react_container, "data-segments") == + Jason.encode!([foo_personal_segment, emea_site_segment]) + end + test "plausible.io live demo - shows site stats", %{conn: conn} do site = new_site(domain: "plausible.io", public: true) populate_stats(site, [build(:pageview)]) @@ -187,6 +222,36 @@ defmodule PlausibleWeb.StatsControllerTest do conn = get(conn, conn |> get("/" <> site.domain) |> redirected_to()) refute html_response(conn, 200) =~ "/crm/sites/site/#{site.id}" end + + test "all segments (personal or site) are stuffed into dataset, with associated their owner_id and owner_name", + %{conn: conn, site: site, user: user} do + populate_stats(site, [build(:pageview)]) + + emea_site_segment = + insert(:segment, + site: site, + owner: user, + type: :site, + name: "EMEA region" + ) + |> Map.put(:owner_name, user.name) + + foo_personal_segment = + insert(:segment, + site: site, + owner: user, + type: :personal, + name: "FOO" + ) + |> Map.put(:owner_name, user.name) + + conn = get(conn, "/#{site.domain}") + resp = html_response(conn, 200) + assert element_exists?(resp, @react_container) + + assert text_of_attr(resp, @react_container, "data-segments") == + Jason.encode!([foo_personal_segment, emea_site_segment]) + end end describe "GET /:domain - as a super admin" do @@ -1172,6 +1237,39 @@ defmodule PlausibleWeb.StatsControllerTest do assert text_of_attr(html, @react_container, "data-scroll-depth-visible") == "true" assert not is_nil(Repo.reload!(site).scroll_depth_visible_at) end + + test "all segments (personal or site) are stuffed into dataset, without their owner_id and owner_name", + %{conn: conn} do + user = new_user() + site = new_site(domain: "test-site.com", owner: user) + link = insert(:shared_link, site: site) + + emea_site_segment = + insert(:segment, + site: site, + owner: user, + type: :site, + name: "EMEA region" + ) + |> Map.put(:owner_name, nil) + |> Map.put(:owner_id, nil) + + foo_personal_segment = + insert(:segment, + site: site, + owner: user, + type: :personal, + name: "FOO" + ) + |> Map.put(:owner_name, nil) + |> Map.put(:owner_id, nil) + + conn = get(conn, "/share/#{site.domain}/?auth=#{link.slug}") + resp = html_response(conn, 200) + + assert text_of_attr(resp, @react_container, "data-segments") == + Jason.encode!([foo_personal_segment, emea_site_segment]) + end end describe "GET /share/:slug - backwards compatibility" do