Skip to content

Some Timetracking view improvements #8170

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Nov 11, 2024
Merged

Some Timetracking view improvements #8170

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
3728f25
add option to select annotation state in time tracking overview
Nov 4, 2024
08e83b5
Merge branch 'master' of github.com:scalableminds/webknossos into tim…
Nov 5, 2024
d2b73cd
include annotation state in user specific time tracking csv download
Nov 5, 2024
d5821e6
show full long project names in timetracking filter input
Nov 5, 2024
f530d00
fix tests
Nov 5, 2024
07cc3d3
update timetracking snapshots
Nov 5, 2024
5496895
fix cyclic frontend dependencies
Nov 5, 2024
c1d6301
Merge branch 'master' into timetracking-followup
MichaelBuessemeyer Nov 7, 2024
3419184
move AnnotationTypeFilterEnum and AnnotationStateFilterEnum to const…
Nov 7, 2024
a92f6c5
add missing changes of last commit
Nov 7, 2024
1bbb675
mend
Nov 7, 2024
939fd16
Update frontend/javascripts/admin/statistic/project_and_annotation_ty…
MichaelBuessemeyer Nov 8, 2024
a958867
Merge branch 'master' into timetracking-followup
MichaelBuessemeyer Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions app/models/user/time/TimeSpan.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ class TimeSpanDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContext)
val projectQuery = projectIdsFilterQuery(projectIds)
for {
tuples <- run(
q"""SELECT ts._user, mu.email, o._id, d.name, a._id, t._id, p.name, tt._id, tt.summary, ts._id, ts.created, ts.time
q"""SELECT ts._user, mu.email, o._id, d.name, a._id, a.state, t._id, p.name, tt._id, tt.summary, ts._id, ts.created, ts.time
FROM webknossos.timespans_ ts
JOIN webknossos.annotations_ a on ts._annotation = a._id
JOIN webknossos.users_ u on ts._user = u._id
Expand All @@ -149,6 +149,7 @@ class TimeSpanDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContext)
String,
String,
String,
String,
Option[String],
Option[String],
Option[String],
Expand All @@ -165,6 +166,7 @@ class TimeSpanDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContext)
String,
String,
String,
String,
Option[String],
Option[String],
Option[String],
Expand All @@ -178,13 +180,14 @@ class TimeSpanDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContext)
"datasetOrganization" -> tuple._3,
"datasetName" -> tuple._4,
"annotationId" -> tuple._5,
"taskId" -> tuple._6,
"projectName" -> tuple._7,
"taskTypeId" -> tuple._8,
"taskTypeSummary" -> tuple._9,
"timeSpanId" -> tuple._10,
"timeSpanCreated" -> tuple._11,
"timeSpanTimeMillis" -> tuple._12
"annotationState" -> tuple._6,
"taskId" -> tuple._7,
"projectName" -> tuple._8,
"taskTypeId" -> tuple._9,
"taskTypeSummary" -> tuple._10,
"timeSpanId" -> tuple._11,
"timeSpanCreated" -> tuple._12,
"timeSpanTimeMillis" -> tuple._13
)

private def projectIdsFilterQuery(projectIds: List[ObjectId]): SqlToken =
Expand Down
28 changes: 24 additions & 4 deletions frontend/javascripts/admin/admin_rest_api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,10 @@ import { doWithToken } from "./api/token";
import type BoundingBox from "oxalis/model/bucket_data_handling/bounding_box";
import type { ArbitraryObject } from "types/globals";
import { assertResponseLimit } from "./api/api_utils";
import type { AnnotationTypeFilterEnum } from "admin/statistic/project_and_annotation_type_dropdown";
import {
AnnotationStateFilterEnum,
type AnnotationTypeFilterEnum,
} from "admin/statistic/project_and_annotation_type_dropdown";

export * from "./api/token";
export * from "./api/jobs";
Expand Down Expand Up @@ -1720,6 +1723,7 @@ export async function getTimeTrackingForUserSummedPerAnnotation(
startDate: dayjs.Dayjs,
endDate: dayjs.Dayjs,
annotationTypes: "Explorational" | "Task" | "Task,Explorational",
annotationState: AnnotationStateFilterEnum,
projectIds?: string[] | null,
): Promise<Array<APITimeTrackingPerAnnotation>> {
const params = new URLSearchParams({
Expand All @@ -1729,7 +1733,11 @@ export async function getTimeTrackingForUserSummedPerAnnotation(
if (annotationTypes != null) params.append("annotationTypes", annotationTypes);
if (projectIds != null && projectIds.length > 0)
params.append("projectIds", projectIds.join(","));
params.append("annotationStates", "Active,Finished");
if (annotationState !== AnnotationStateFilterEnum.ALL) {
params.append("annotationStates", annotationState);
} else {
params.append("annotationStates", "Active,Finished");
}
const timeTrackingData = await Request.receiveJSON(
`/api/time/user/${userId}/summedByAnnotation?${params}`,
);
Expand All @@ -1742,16 +1750,22 @@ export async function getTimeTrackingForUserSpans(
startDate: number,
endDate: number,
annotationTypes: "Explorational" | "Task" | "Task,Explorational",
selectedState: AnnotationStateFilterEnum,
projectIds?: string[] | null,
): Promise<Array<APITimeTrackingSpan>> {
const params = new URLSearchParams({
start: startDate.toString(),
end: endDate.toString(),
});
if (annotationTypes != null) params.append("annotationTypes", annotationTypes);
if (projectIds != null && projectIds.length > 0)
if (projectIds != null && projectIds.length > 0) {
params.append("projectIds", projectIds.join(","));
params.append("annotationStates", "Active,Finished");
}
if (selectedState !== AnnotationStateFilterEnum.ALL) {
params.append("annotationStates", selectedState);
} else {
params.append("annotationStates", "Active,Finished");
}
return await Request.receiveJSON(`/api/time/user/${userId}/spans?${params}`);
}

Expand All @@ -1760,13 +1774,19 @@ export async function getTimeEntries(
endMs: number,
teamIds: string[],
selectedTypes: AnnotationTypeFilterEnum,
selectedState: AnnotationStateFilterEnum,
projectIds: string[],
): Promise<Array<APITimeTrackingPerUser>> {
const params = new URLSearchParams({
start: startMs.toString(),
end: endMs.toString(),
annotationTypes: selectedTypes,
});
if (selectedState !== AnnotationStateFilterEnum.ALL) {
params.append("annotationStates", selectedState);
} else {
params.append("annotationStates", "Active,Finished");
}
Comment on lines +1773 to +1785
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove duplicate annotationStates parameter.

There is a critical issue where the annotationStates parameter is being set twice:

  1. First in the conditional block (lines 1785-1789)
  2. Then again unconditionally (line 1793)

This could lead to unexpected behavior as the second append will add a duplicate parameter to the URL.

   if (selectedState !== AnnotationStateFilterEnum.ALL) {
     params.append("annotationStates", selectedState);
   } else {
     params.append("annotationStates", "Active,Finished");
   }
   // Omit empty parameters in request
   if (projectIds.length > 0) params.append("projectIds", projectIds.join(","));
   if (teamIds.length > 0) params.append("teamIds", teamIds.join(","));
-  params.append("annotationStates", "Active,Finished");
   return await Request.receiveJSON(`api/time/overview?${params}`);

Also applies to: 1793-1793

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, thanks for saving me here 🙏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MichaelBuessemeyer, glad I could help!

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

// Omit empty parameters in request
if (projectIds.length > 0) params.append("projectIds", projectIds.join(","));
if (teamIds.length > 0) params.append("teamIds", teamIds.join(","));
Expand Down
35 changes: 31 additions & 4 deletions frontend/javascripts/admin/statistic/project_and_annotation_type_dropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,19 @@ export enum AnnotationTypeFilterEnum {
TASKS_AND_ANNOTATIONS_KEY = "Task,Explorational",
}

export enum AnnotationStateFilterEnum {
ALL = "All",
ACTIVE = "Active",
FINISHED_OR_ARCHIVED = "Finished",
}
type ProjectAndTypeDropdownProps = {
selectedProjectIds: string[];
setSelectedProjectIds: (projectIds: string[]) => void;
selectedAnnotationType: AnnotationTypeFilterEnum;
setSelectedAnnotationType: (type: AnnotationTypeFilterEnum) => void;
selectedAnnotationState: string;
setSelectedAnnotationState: (state: AnnotationStateFilterEnum) => void;

style?: React.CSSProperties;
};

Expand All @@ -38,11 +46,21 @@ const ANNOTATION_TYPE_FILTERS: NestedSelectOptions = {
],
};

const ANNOTATION_STATE_FILTERS: NestedSelectOptions = {
label: "Filter by state",
options: [
{ label: "Active", value: AnnotationStateFilterEnum.ACTIVE },
{ label: "Finished / Archived", value: AnnotationStateFilterEnum.FINISHED_OR_ARCHIVED },
],
};

function ProjectAndAnnotationTypeDropdown({
selectedProjectIds,
setSelectedProjectIds,
selectedAnnotationType,
setSelectedAnnotationType,
selectedAnnotationState,
setSelectedAnnotationState,
style,
}: ProjectAndTypeDropdownProps) {
// This state property is derived from selectedProjectIds and selectedAnnotationType.
Expand All @@ -60,12 +78,14 @@ function ProjectAndAnnotationTypeDropdown({
);

useEffect(() => {
const selectedKeys =
selectedAnnotationState !== AnnotationStateFilterEnum.ALL ? [selectedAnnotationState] : [];
if (selectedProjectIds.length > 0) {
setSelectedFilters(selectedProjectIds);
setSelectedFilters([...selectedProjectIds, ...selectedKeys]);
} else {
setSelectedFilters([selectedAnnotationType]);
setSelectedFilters([selectedAnnotationType, ...selectedKeys]);
}
}, [selectedProjectIds, selectedAnnotationType]);
}, [selectedProjectIds, selectedAnnotationType, selectedAnnotationState]);

useEffect(() => {
const projectOptions = allProjects.map((project) => {
Expand All @@ -74,14 +94,18 @@ function ProjectAndAnnotationTypeDropdown({
value: project.id,
};
});
let allOptions = [ANNOTATION_TYPE_FILTERS];
let allOptions = [ANNOTATION_TYPE_FILTERS, ANNOTATION_STATE_FILTERS];
if (projectOptions.length > 0) {
allOptions.push({ label: "Filter projects (only tasks)", options: projectOptions });
}
setFilterOptions(allOptions);
}, [allProjects]);

const setSelectedProjects = async (_prevSelection: string[], selectedValue: string) => {
if (Object.values<string>(AnnotationStateFilterEnum).includes(selectedValue)) {
setSelectedAnnotationState(selectedValue as AnnotationStateFilterEnum);
return;
}
if (Object.values<string>(AnnotationTypeFilterEnum).includes(selectedValue)) {
setSelectedAnnotationType(selectedValue as AnnotationTypeFilterEnum);
setSelectedProjectIds([]);
Expand All @@ -94,6 +118,8 @@ function ProjectAndAnnotationTypeDropdown({
const onDeselect = (removedKey: string) => {
if (Object.values<string>(AnnotationTypeFilterEnum).includes(removedKey)) {
setSelectedAnnotationType(AnnotationTypeFilterEnum.TASKS_AND_ANNOTATIONS_KEY);
} else if (Object.values<string>(AnnotationStateFilterEnum).includes(removedKey)) {
setSelectedAnnotationState(AnnotationStateFilterEnum.ALL);
} else {
setSelectedProjectIds(selectedProjectIds.filter((projectId) => projectId !== removedKey));
}
Expand All @@ -108,6 +134,7 @@ function ProjectAndAnnotationTypeDropdown({
options={filterOptions}
optionFilterProp="label"
value={selectedFilters}
popupMatchSelectWidth={false}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prop enables fully showing long project names as stated in the issue.

But the antd docs to this prop state:

Determine whether the popup menu and the select input are the same width. Default set min-width same as input. Will ignore when value less than select width. false will disable virtual scroll.

Where the last sentence is important IMO -> virtual scrolling is now disabled by this change. This means that for an organization with many projects, that the list might be slow.

Do you think this is acceptable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not aware of any orga making such extensive use of projects that this would be an issue. An alternative could be to set a reasonable width (somewhat larger than before) and then show the full project name on hover. No real preference on my side.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative could be to set a reasonable width (somewhat larger than before) and then show the full project name on hover. No real preference on my side.

That's definitely also doable :)

I'll switch to this behavior 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Based on the search results, I can now generate a final response. The code shows that the component fetches all projects at once without pagination, and the search results reveal that virtual scrolling is commonly used throughout the codebase for performance optimization, particularly in components with large datasets.

Consider keeping virtual scrolling enabled for better performance

Setting popupMatchSelectWidth={false} disables virtual scrolling in the antd Select component. While this allows showing full project names, it could impact performance as the component loads all projects at once. The codebase shows extensive use of virtualization for performance optimization in similar scenarios.

Alternative solutions that maintain virtual scrolling:

  1. Use CSS ellipsis with tooltip: text-overflow: ellipsis in combination with antd's title prop
  2. Set a fixed dropdownStyle={{ width: 'max-content', maxWidth: '500px' }} that's wider than the input but still enables virtual scrolling
  3. Use antd's built-in ellipsis prop with tooltip for long text
🔗 Analysis chain

Consider performance impact of disabled virtual scrolling.

Setting popupMatchSelectWidth={false} addresses the requirement to show long project names but disables virtual scrolling. This could impact performance for organizations with many projects.

Consider these alternatives:

  1. Use CSS to handle long text (e.g., text-overflow with tooltip) while keeping virtual scrolling
  2. Set a reasonable fixed width for the popup that's wider than the input but still enables virtual scrolling

Let's check how many projects might be affected:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for project-related components to understand the scale
rg -l 'projects.*map' --type ts --type tsx

Length of output: 72

Script:

#!/bin/bash
# Search for project-related components and mapping patterns in TypeScript files
rg -l 'projects.*map' --type typescript

# Look for Select/Dropdown components with project lists
rg -l 'Select.*projects' --type typescript

# Search for array patterns that might indicate project lists
rg -A 5 'projects: \[' --type typescript

# Check the specific component's file for project handling
cat "frontend/javascripts/admin/statistic/project_and_annotation_type_dropdown.tsx"

Length of output: 5410

Script:

#!/bin/bash
# Let's try with proper file extensions for TypeScript/React files
rg -l '\.tsx?$' | xargs rg -l 'projects.*map'

# Look for project data fetching to understand the scale
rg -A 5 'getProjects\(' --type-add 'ts:*.{ts,tsx}'

# Check for any pagination or virtualization related code
rg -A 3 'virtualiz|pagination' --type-add 'ts:*.{ts,tsx}'

# Look for Select component usage patterns
rg -A 3 '<Select.*mode="multiple"' --type-add 'ts:*.{ts,tsx}'

Length of output: 24571

onDeselect={(removedKey: string) => onDeselect(removedKey)}
onSelect={(newSelection: string) => setSelectedProjects(selectedFilters, newSelection)}
/>
Expand Down
7 changes: 6 additions & 1 deletion frontend/javascripts/admin/statistic/time_tracking_detail_view.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import { useFetch } from "libs/react_helpers";
import type { AnnotationTypeFilterEnum } from "./project_and_annotation_type_dropdown";
import type {
AnnotationStateFilterEnum,
AnnotationTypeFilterEnum,
} from "./project_and_annotation_type_dropdown";
import { getTimeTrackingForUserSummedPerAnnotation } from "admin/admin_rest_api";
import dayjs from "dayjs";
import { Col, Divider, Row } from "antd";
Expand All @@ -13,6 +16,7 @@ type TimeTrackingDetailViewProps = {
userId: string;
dateRange: [number, number];
annotationType: AnnotationTypeFilterEnum;
annotationState: AnnotationStateFilterEnum;
projectIds: string[];
};

Expand Down Expand Up @@ -84,6 +88,7 @@ function TimeTrackingDetailView(props: TimeTrackingDetailViewProps) {
dayjs(props.dateRange[0]),
dayjs(props.dateRange[1]),
props.annotationType,
props.annotationState,
props.projectIds,
);
},
Expand Down
22 changes: 19 additions & 3 deletions frontend/javascripts/admin/statistic/time_tracking_overview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { DownloadOutlined, FilterOutlined } from "@ant-design/icons";
import saveAs from "file-saver";
import { formatMilliseconds } from "libs/format_utils";
import ProjectAndAnnotationTypeDropdown, {
AnnotationStateFilterEnum,
AnnotationTypeFilterEnum,
} from "./project_and_annotation_type_dropdown";
import { isUserAdminOrTeamManager, transformToCSVRow } from "libs/utils";
Expand All @@ -23,7 +24,7 @@ const { RangePicker } = DatePicker;

const TIMETRACKING_CSV_HEADER_PER_USER = ["userId,userFirstName,userLastName,timeTrackedInSeconds"];
const TIMETRACKING_CSV_HEADER_SPANS = [
"userId,email,datasetOrga,datasetName,annotation,startTimeUnixTimestamp,durationInSeconds,taskId,projectName,taskTypeId,taskTypeSummary",
"userId,email,datasetOrga,datasetName,annotation,annotationState,startTimeUnixTimestamp,durationInSeconds,taskId,projectName,taskTypeId,taskTypeSummary",
];

function TimeTrackingOverview() {
Expand All @@ -50,6 +51,7 @@ function TimeTrackingOverview() {
const [selectedTypes, setSelectedTypes] = useState(
AnnotationTypeFilterEnum.TASKS_AND_ANNOTATIONS_KEY,
);
const [selectedState, setSelectedState] = useState(AnnotationStateFilterEnum.ALL);
const [selectedTeams, setSelectedTeams] = useState(allTeams.map((team) => team.id));
const filteredTimeEntries = useFetch(
async () => {
Expand All @@ -59,13 +61,14 @@ function TimeTrackingOverview() {
endDate.valueOf(),
selectedTeams,
selectedTypes,
selectedState,
selectedProjectIds,
);
setIsFetching(false);
return filteredEntries;
},
[],
[selectedTeams, selectedTypes, selectedProjectIds, startDate, endDate],
[selectedTeams, selectedTypes, selectedState, selectedProjectIds, startDate, endDate],
);
const filterStyle = { marginInline: 10 };

Expand All @@ -74,13 +77,15 @@ function TimeTrackingOverview() {
start: Dayjs,
end: Dayjs,
annotationTypes: AnnotationTypeFilterEnum,
selectedState: AnnotationStateFilterEnum,
projectIds: string[] | null | undefined,
) => {
const timeSpans = await getTimeTrackingForUserSpans(
userId,
start.valueOf(),
end.valueOf(),
annotationTypes,
selectedState,
projectIds,
);
const timeEntriesAsString = timeSpans
Expand All @@ -91,6 +96,7 @@ function TimeTrackingOverview() {
row.datasetOrganization,
row.datasetName,
row.annotationId,
row.annotationState,
row.timeSpanCreated,
Math.ceil(row.timeSpanTimeMillis / 1000),
row.taskId,
Expand Down Expand Up @@ -186,7 +192,14 @@ function TimeTrackingOverview() {
return (
<LinkButton
onClick={async () => {
downloadTimeSpans(user.id, startDate, endDate, selectedTypes, selectedProjectIds);
downloadTimeSpans(
user.id,
startDate,
endDate,
selectedTypes,
selectedState,
selectedProjectIds,
);
}}
>
<DownloadOutlined className="icon-margin-right" />
Expand Down Expand Up @@ -239,6 +252,8 @@ function TimeTrackingOverview() {
selectedProjectIds={selectedProjectIds}
setSelectedAnnotationType={setSelectedTypes}
selectedAnnotationType={selectedTypes}
selectedAnnotationState={selectedState}
setSelectedAnnotationState={setSelectedState}
style={{ ...filterStyle }}
/>
<Select
Expand Down Expand Up @@ -289,6 +304,7 @@ function TimeTrackingOverview() {
userId={entry.user.id}
dateRange={[startDate.valueOf(), endDate.valueOf()]}
annotationType={selectedTypes}
annotationState={selectedState}
projectIds={selectedProjectIds}
/>
),
Expand Down
3 changes: 3 additions & 0 deletions frontend/javascripts/test/backend-snapshot-tests/timetracking.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
import * as api from "admin/admin_rest_api";
import test from "ava";
import type { APITeam, APIUser } from "types/api_flow_types";
import { AnnotationStateFilterEnum } from "admin/statistic/project_and_annotation_type_dropdown";

let activeUser: APIUser;
let firstTeam: APITeam;
Expand All @@ -32,6 +33,7 @@ test("getTimeTrackingForUserSpans", async (t) => {
dayjs("20180101", "YYYYMMDD").valueOf(),
dayjs("20181001", "YYYYMMDD").valueOf(),
"Task",
AnnotationStateFilterEnum.ALL,
);
t.true(timeTrackingForUser.length > 0);
t.snapshot(replaceVolatileValues(timeTrackingForUser));
Expand All @@ -44,6 +46,7 @@ test("getTimeTrackingForUser for a user other than the active user", async (t) =
dayjs("20160401", "YYYYMMDD").valueOf(),
dayjs("20160420", "YYYYMMDD").valueOf(),
"Task",
AnnotationStateFilterEnum.ALL,
);
t.true(timeTrackingForUser.length > 0);
t.snapshot(replaceVolatileValues(timeTrackingForUser));
Expand Down
Loading