From dc618137ff12cceaaf92ccd072d2fc73e7d9b823 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 27 Feb 2025 11:47:31 +0200 Subject: [PATCH] Fix SegmentAuthorship --- assets/js/dashboard/filtering/segments.ts | 13 ++- .../segments/searchable-segments-section.tsx | 31 ++++-- .../segments/segment-authorship.test.tsx | 98 +++++++++++++++++++ .../dashboard/segments/segment-authorship.tsx | 24 +++-- .../js/dashboard/segments/segment-modals.tsx | 6 +- lib/plausible/segments/segment.ex | 2 +- .../segments_controller_test.exs | 6 +- 7 files changed, 156 insertions(+), 24 deletions(-) create mode 100644 assets/js/dashboard/segments/segment-authorship.test.tsx diff --git a/assets/js/dashboard/filtering/segments.ts b/assets/js/dashboard/filtering/segments.ts index 6db811e357919..5fbb23f79df84 100644 --- a/assets/js/dashboard/filtering/segments.ts +++ b/assets/js/dashboard/filtering/segments.ts @@ -14,18 +14,23 @@ export type SavedSegment = { id: number name: string type: SegmentType - owner_id: number - owner_name: string + /** null owner_id or owner_name signifies that the owner has been removed from the site */ + owner_id: number | null + owner_name: string | null /** datetime in site timezone, example 2025-02-26 10:00:00 */ inserted_at: string /** datetime in site timezone, example 2025-02-26 10:00:00 */ updated_at: string } -export type SavedPublicSiteSegment = Pick< +export type SavedSegmentPublic = Pick< SavedSegment, 'id' | 'type' | 'name' | 'inserted_at' | 'updated_at' -> +> & { + /** null owner_id signifies that the owner can't be shown */ + owner_id: null + owner_name: null +} export type SegmentDataFromApi = { filters: unknown[] 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 6293ebde53d59..cd0e76180acf7 100644 --- a/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx +++ b/assets/js/dashboard/nav-menu/segments/searchable-segments-section.tsx @@ -8,6 +8,7 @@ import { getFilterSegmentsByNameInsensitive, handleSegmentResponse, isSegmentFilter, + SavedSegmentPublic, SavedSegment, SegmentData, SegmentDataFromApi @@ -24,14 +25,15 @@ 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' -const useSegmentsListQuery = () => { +function useSegmentsListQuery(_isPublicRequest: T) { const site = useSiteContext() - return useQuery({ + return useQuery({ queryKey: ['segments'], placeholderData: (previousData) => previousData, queryFn: async () => { - const response: SavedSegment[] = await get( + const response = await get( `/api/${encodeURIComponent(site.domain)}/segments` ) return response @@ -56,8 +58,11 @@ export const SearchableSegmentsSection = ({ const { query, expandedSegment } = useQueryContext() const segmentFilter = query.filters.find(isSegmentFilter) const appliedSegmentIds = (segmentFilter ? segmentFilter[2] : []) as number[] + const user = useUserContext() + + const isPublicListQuery = !user.loggedIn || user.role === Role.public + const { data, ...listQuery } = useSegmentsListQuery(isPublicListQuery) - const { data, ...listQuery } = useSegmentsListQuery() const [searchValue, setSearch] = useState() const [showAll, setShowAll] = useState(false) @@ -114,7 +119,18 @@ export const SearchableSegmentsSection = ({ } - + } > @@ -229,7 +245,10 @@ const SegmentLink = ({ name, appliedSegmentIds, closeList -}: SavedSegment & { appliedSegmentIds: number[]; closeList: () => void }) => { +}: Pick & { + appliedSegmentIds: number[] + closeList: () => void +}) => { const { query } = useQueryContext() const { prefetchSegment } = useSegmentPrefetch({ id: String(id) }) diff --git a/assets/js/dashboard/segments/segment-authorship.test.tsx b/assets/js/dashboard/segments/segment-authorship.test.tsx new file mode 100644 index 0000000000000..af7eb65ba0c5c --- /dev/null +++ b/assets/js/dashboard/segments/segment-authorship.test.tsx @@ -0,0 +1,98 @@ +/** @format */ + +import React from 'react' +import { render, screen } from '@testing-library/react' +import { SegmentAuthorship } from './segment-authorship' +import { SegmentType } from '../filtering/segments' + +describe('public (no "by " shown)', () => { + const showOnlyPublicData = true + + test('shows only insert date if its the same as updated at', async () => { + render( + + ) + + expect(screen.getByText('Created at 1 Feb')).toBeVisible() + expect(screen.queryByText(/Last updated/)).toBeNull() + expect(screen.queryByText(/by /)).toBeNull() + }) + + test('shows both insert date and updated at', async () => { + render( + + ) + + expect(screen.getByText('Created at 1 Feb')).toBeVisible() + expect(screen.getByText('Last updated at 1 Feb')).toBeVisible() + expect(screen.queryByText(/by /)).toBeNull() + }) +}) + +describe('shown to a site member ("by " shown)', () => { + const showOnlyPublicData = false + + test('shows only insert date if its the same as updated at', async () => { + render( + + ) + + expect(screen.getByText('Created at 1 Feb by (Removed User)')).toBeVisible() + expect(screen.queryByText(/Last updated/)).toBeNull() + }) + + test('shows both insert date and updated at', async () => { + render( + + ) + + expect(screen.getByText('Created at 1 Feb')).toBeVisible() + expect( + screen.queryByText('Last updated at 5 Feb by Jane Smith') + ).toBeVisible() + }) +}) diff --git a/assets/js/dashboard/segments/segment-authorship.tsx b/assets/js/dashboard/segments/segment-authorship.tsx index 3b087ea3da8b9..a3a91c0a50161 100644 --- a/assets/js/dashboard/segments/segment-authorship.tsx +++ b/assets/js/dashboard/segments/segment-authorship.tsx @@ -1,19 +1,25 @@ /** @format */ import React from 'react' -import { SavedSegment } from '../filtering/segments' +import { SavedSegmentPublic, SavedSegment } from '../filtering/segments' import { formatDayShort, parseNaiveDate } from '../util/date' -export const SegmentAuthorship = ({ +type SegmentAuthorshipProps = { className?: string } & ( + | { showOnlyPublicData: true; segment: SavedSegmentPublic } + | { showOnlyPublicData: false; segment: SavedSegment } +) + +export function SegmentAuthorship({ className, - owner_name, - inserted_at, - updated_at -}: Pick & { - className?: string -}) => { - const authorLabel = owner_name + showOnlyPublicData, + segment +}: SegmentAuthorshipProps) { + const authorLabel = + showOnlyPublicData === true + ? null + : (segment.owner_name ?? '(Removed User)') + const { updated_at, inserted_at } = segment const showUpdatedAt = updated_at !== inserted_at return ( diff --git a/assets/js/dashboard/segments/segment-modals.tsx b/assets/js/dashboard/segments/segment-modals.tsx index 771f29adaf083..bc848346c637a 100644 --- a/assets/js/dashboard/segments/segment-modals.tsx +++ b/assets/js/dashboard/segments/segment-modals.tsx @@ -466,7 +466,11 @@ export const SegmentModal = ({ id }: { id: SavedSegment['id'] }) => { <> - +
Plausible.Timezones.to_datetime_in_timezone(segment.site.timezone) 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 1b65c483735a0..82781acf008bb 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 @@ -41,10 +41,10 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do "id" => s.id, "name" => s.name, "type" => Atom.to_string(s.type), - "owner_id" => nil, - "owner_name" => nil, "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) @@ -125,7 +125,7 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do type: :site, name: "Another region" ) - |> Map.put(:owner_name, "(Removed User)") + |> Map.put(:owner_name, nil) conn = get(conn, "/api/#{site.domain}/segments")