Skip to content

Fix: Cycle graphs refactor #5745

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
pushya22 merged 2 commits into from
Oct 3, 2024
Merged

Fix: Cycle graphs refactor #5745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
508934a
fix: community changes for cycle graphs
gakshita Oct 3, 2024
65eaf4e
fix: added dependency from root package.json
gakshita Oct 3, 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
3 changes: 2 additions & 1 deletion packages/types/src/cycle/cycle.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,11 @@ export type TCycleEstimateDistribution = {
export type TCycleProgress = {
date: string;
started: number;
actual: number;
pending: number;
ideal: number | null;
scope: number;
completed: number;
actual: number;
unstarted: number;
backlog: number;
cancelled: number;
Expand Down Expand Up @@ -103,6 +103,7 @@ export interface ICycle extends TProgressSnapshot {
workspace_id: string;
project_detail: IProjectDetails;
progress: any[];
version: number;
}

export interface CycleIssueResponse {
Expand Down
89 changes: 0 additions & 89 deletions web/core/components/cycles/active-cycle/root.tsx
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion web/core/components/cycles/list/root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export const CyclesList: FC<ICyclesList> = observer((props) => {
</>
) : (
<>
<ActiveCycleRoot workspaceSlug={workspaceSlug} projectId={projectId} />
<ActiveCycleRoot />

{upcomingCycleIds && (
<Disclosure as="div" className="flex flex-shrink-0 flex-col" defaultOpen>
Expand Down
2 changes: 1 addition & 1 deletion web/core/constants/cycle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export const CYCLE_STATUS: {
{
label: "day left",
value: "current",
title: "Active",
title: "In progress",
color: "#F59E0B",
textColor: "text-amber-500",
bgColor: "bg-amber-50",
Expand Down
19 changes: 7 additions & 12 deletions web/core/store/cycle.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,7 @@ export interface ICycleStore {
fetchArchivedCycleDetails: (workspaceSlug: string, projectId: string, cycleId: string) => Promise<ICycle>;
fetchCycleDetails: (workspaceSlug: string, projectId: string, cycleId: string) => Promise<ICycle>;
fetchActiveCycleProgress: (workspaceSlug: string, projectId: string, cycleId: string) => Promise<TProgressSnapshot>;
fetchActiveCycleProgressPro: (
workspaceSlug: string,
projectId: string,
cycleId: string
) => Promise<TProgressSnapshot> | Promise<null>;
fetchActiveCycleProgressPro: (workspaceSlug: string, projectId: string, cycleId: string) => Promise<void>;
fetchActiveCycleAnalytics: (
workspaceSlug: string,
projectId: string,
Expand Down Expand Up @@ -146,7 +142,6 @@ export class CycleStore implements ICycleStore {
fetchArchivedCycles: action,
fetchArchivedCycleDetails: action,
fetchActiveCycleProgress: action,
fetchActiveCycleProgressPro: action,
fetchActiveCycleAnalytics: action,
fetchCycleDetails: action,
createCycle: action,
Expand Down Expand Up @@ -282,7 +277,7 @@ export class CycleStore implements ICycleStore {
*/
getActiveCycleProgress = computedFn((cycleId?: string) => {
const cycle = cycleId ? this.cycleMap[cycleId] : this.currentProjectActiveCycle;
if (!cycle?.progress) return null;
if (!cycle) return null;

const isTypeIssue = this.getEstimateTypeByCycleId(cycle.id) === "issues";
const isBurnDown = this.getPlotTypeByCycleId(cycle.id) === "burndown";
Expand Down Expand Up @@ -403,13 +398,13 @@ export class CycleStore implements ICycleStore {
await this.cycleService.cycleDateCheck(workspaceSlug, projectId, payload);

/**
* @description gets the plot type for the module store
* @description gets the plot type for the cycle store
* @param {TCyclePlotType} plotType
*/
getPlotTypeByCycleId = computedFn((cycleId: string) => this.plotType[cycleId] || "burndown");

/**
* @description gets the estimate type for the module store
* @description gets the estimate type for the cycle store
* @param {TCycleEstimateType} estimateType
*/
getEstimateTypeByCycleId = computedFn((cycleId: string) => {
Expand All @@ -421,15 +416,15 @@ export class CycleStore implements ICycleStore {
});

/**
* @description updates the plot type for the module store
* @description updates the plot type for the cycle store
* @param {TCyclePlotType} plotType
*/
setPlotType = (cycleId: string, plotType: TCyclePlotType) => {
set(this.plotType, [cycleId], plotType);
};

/**
* @description updates the estimate type for the module store
* @description updates the estimate type for the cycle store
* @param {TCycleEstimateType} estimateType
*/
setEstimateType = (cycleId: string, estimateType: TCycleEstimateType) => {
Expand Down Expand Up @@ -545,7 +540,7 @@ export class CycleStore implements ICycleStore {
* @param cycleId
* @returns
*/
fetchActiveCycleProgressPro = async (workspaceSlug: string, projectId: string, cycleId: string) => null;
fetchActiveCycleProgressPro = action(async (workspaceSlug: string, projectId: string, cycleId: string) => {});
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 3, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Implement the fetchActiveCycleProgressPro method.

The fetchActiveCycleProgressPro method has been replaced with an empty async function. This implementation doesn't match the method's name or its expected behavior. Please implement the method to fetch the active cycle progress for pro users.

Consider implementing the method similar to the non-pro version, adjusting for any pro-specific features:

fetchActiveCycleProgressPro = action(async (workspaceSlug: string, projectId: string, cycleId: string) => {
  this.progressLoader = true;
  try {
    const progress = await this.cycleService.workspaceActiveCyclesProgressPro(workspaceSlug, projectId, cycleId);
    runInAction(() => {
      set(this.cycleMap, [cycleId], { ...this.cycleMap[cycleId], ...progress });
    });
  } catch (error) {
    console.error("Failed to fetch active cycle progress for pro users", error);
  } finally {
    runInAction(() => {
      this.progressLoader = false;
    });
  }
});


/**
* @description fetches active cycle analytics
Expand Down
103 changes: 77 additions & 26 deletions web/helpers/cycle.helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { startOfToday, format } from "date-fns";
import { isEmpty, orderBy, uniqBy } from "lodash";
import sortBy from "lodash/sortBy";
import { ICycle, TCycleFilters } from "@plane/types";
// helpers
import { generateDateArray, getDate, getToday } from "@/helpers/date-time.helper";
import { findTotalDaysInRange, generateDateArray, getDate } from "@/helpers/date-time.helper";
import { satisfiesDateFilter } from "@/helpers/filter.helper";

export type TProgressChartData = {
Expand Down Expand Up @@ -75,47 +76,97 @@ export const shouldFilterCycle = (cycle: ICycle, filter: TCycleFilters): boolean
return fallsInFilters;
};

export const formatActiveCycle = (args: {
cycle: ICycle;
isBurnDown?: boolean | undefined;
isTypeIssue?: boolean | undefined;
}) => {
const { cycle, isBurnDown, isTypeIssue } = args;
let today = getToday();
const endDate: Date | string = new Date(cycle.end_date!);
const scope = (p: any, isTypeIssue: boolean) => (isTypeIssue ? p.total_issues : p.total_estimate_points);
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 3, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid using the any type for parameter p.

Using any defeats the purpose of TypeScript's static typing and can lead to runtime errors. Consider defining a specific interface or type for p to ensure type safety.

Apply this diff to specify the parameter type:

-const scope = (p: any, isTypeIssue: boolean) => (isTypeIssue ? p.total_issues : p.total_estimate_points);
+interface ProgressData {
+  total_issues?: number;
+  total_estimate_points?: number;
+}
+
+const scope = (p: ProgressData, isTypeIssue: boolean) => (isTypeIssue ? p.total_issues : p.total_estimate_points);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const scope = (p: any, isTypeIssue: boolean) => (isTypeIssue ? p.total_issues : p.total_estimate_points);
interface ProgressData {
total_issues?: number;
total_estimate_points?: number;
}
const scope = (p: ProgressData, isTypeIssue: boolean) => (isTypeIssue ? p.total_issues : p.total_estimate_points);

const ideal = (date: string, scope: number, cycle: ICycle) =>
Math.floor(
((findTotalDaysInRange(date, cycle.end_date) || 0) /
(findTotalDaysInRange(cycle.start_date, cycle.end_date) || 0)) *
scope
Comment on lines +80 to +84
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 3, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential division by zero in the ideal function.

In the ideal function, there's a risk of dividing by zero if findTotalDaysInRange(cycle.start_date, cycle.end_date) returns 0 or undefined. This will result in a NaN value.

Apply this diff to handle the division by zero:

 const ideal = (date: string, scope: number, cycle: ICycle) =>
   Math.floor(
-    ((findTotalDaysInRange(date, cycle.end_date) || 0) /
-      (findTotalDaysInRange(cycle.start_date, cycle.end_date) || 0)) *
+    ((findTotalDaysInRange(date, cycle.end_date) || 0) /
+      (findTotalDaysInRange(cycle.start_date, cycle.end_date) || 1)) *
       scope
   );

This change ensures that if the denominator is 0 or falsy, it defaults to 1, preventing division by zero.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const ideal = (date: string, scope: number, cycle: ICycle) =>
Math.floor(
((findTotalDaysInRange(date, cycle.end_date) || 0) /
(findTotalDaysInRange(cycle.start_date, cycle.end_date) || 0)) *
scope
const ideal = (date: string, scope: number, cycle: ICycle) =>
Math.floor(
((findTotalDaysInRange(date, cycle.end_date) || 0) /
(findTotalDaysInRange(cycle.start_date, cycle.end_date) || 1)) *
scope
);

);
Comment on lines +79 to +85
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 3, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider typing the scope and ideal functions for clarity.

Explicitly typing the return types and parameters enhances code clarity and TypeScript's type checking capabilities.

Specify types for the functions:

-const scope = (p: ProgressData, isTypeIssue: boolean) => (isTypeIssue ? p.total_issues : p.total_estimate_points);
+const scope = (p: ProgressData, isTypeIssue: boolean): number => (isTypeIssue ? p.total_issues || 0 : p.total_estimate_points || 0);

-const ideal = (date: string, scope: number, cycle: ICycle) =>
+const ideal = (date: string, scope: number, cycle: ICycle): number =>

Committable suggestion was skipped due to low confidence.


const extendedArray = endDate > today ? generateDateArray(today as Date, endDate) : [];
if (isEmpty(cycle.progress)) return extendedArray;
today = getToday(true);
const formatV1Data = (isTypeIssue: boolean, cycle: ICycle, isBurnDown: boolean, endDate: Date | string) => {
const today = format(startOfToday(), "yyyy-MM-dd");
const data = isTypeIssue ? cycle.distribution : cycle.estimate_distribution;
const extendedArray = generateDateArray(endDate, endDate).map((d) => d.date);

if (isEmpty(data)) return [];
const progress = [...Object.keys(data.completion_chart), ...extendedArray].map((p) => {
const pending = data.completion_chart[p] || 0;
const total = isTypeIssue ? cycle.total_issues : cycle.total_estimate_points;
const completed = scope(cycle, isTypeIssue) - pending;

return {
date: p,
scope: p! < today ? scope(cycle, isTypeIssue) : null,
completed,
backlog: isTypeIssue ? cycle.backlog_issues : cycle.backlog_estimate_points,
started: p === today ? cycle[isTypeIssue ? "started_issues" : "started_estimate_points"] : undefined,
unstarted: p === today ? cycle[isTypeIssue ? "unstarted_issues" : "unstarted_estimate_points"] : undefined,
cancelled: p === today ? cycle[isTypeIssue ? "cancelled_issues" : "cancelled_estimate_points"] : undefined,
pending: Math.abs(pending || 0),
ideal:
p < today
? ideal(p, total || 0, cycle)
: p <= cycle.end_date!
? ideal(today as string, total || 0, cycle)
: null,
actual: p <= today ? (isBurnDown ? Math.abs(pending) : completed) : undefined,
};
});

const scope = (p: any) => (isTypeIssue ? p.total_issues : p.total_estimate_points);
const ideal = (p: any) =>
isTypeIssue
? Math.abs(p.total_issues - p.completed_issues + (Math.random() < 0.5 ? -1 : 1))
: Math.abs(p.total_estimate_points - p.completed_estimate_points + (Math.random() < 0.5 ? -1 : 1));
return progress;
};

const formatV2Data = (isTypeIssue: boolean, cycle: ICycle, isBurnDown: boolean, endDate: Date | string) => {
if (!cycle.progress) return [];
let today: Date | string = startOfToday();

const scopeToday = scope(cycle?.progress[cycle?.progress.length - 1]);
const idealToday = ideal(cycle?.progress[cycle?.progress.length - 1]);
const extendedArray = endDate > today ? generateDateArray(today as Date, endDate) : [];
if (isEmpty(cycle.progress)) return extendedArray;
today = format(startOfToday(), "yyyy-MM-dd");
const todaysData = cycle?.progress[cycle?.progress.length - 1];
const scopeToday = scope(todaysData, isTypeIssue);
const idealToday = ideal(todaysData.date, scopeToday, cycle);

const progress = [...orderBy(cycle?.progress, "date"), ...extendedArray].map((p) => {
let progress = [...orderBy(cycle?.progress, "date"), ...extendedArray].map((p) => {
const pending = isTypeIssue
? p.total_issues - p.completed_issues - p.cancelled_issues
: p.total_estimate_points - p.completed_estimate_points - p.cancelled_estimate_points;
const completed = isTypeIssue ? p.completed_issues : p.completed_estimate_points;
const dataDate = p.progress_date ? format(new Date(p.progress_date), "yyyy-MM-dd") : p.date;

return {
date: p.date,
scope: p.date! < today ? scope(p) : p.date! < cycle.end_date! ? scopeToday : null,
date: dataDate,
scope: dataDate! < today ? scope(p, isTypeIssue) : dataDate! <= cycle.end_date! ? scopeToday : null,
completed,
backlog: isTypeIssue ? p.backlog_issues : p.backlog_estimate_points,
started: isTypeIssue ? p.started_issues : p.started_estimate_points,
unstarted: isTypeIssue ? p.unstarted_issues : p.unstarted_estimate_points,
cancelled: isTypeIssue ? p.cancelled_issues : p.cancelled_estimate_points,
pending: Math.abs(pending),
// TODO: This is a temporary logic to show the ideal line in the cycle chart
ideal: p.date! < today ? ideal(p) : p.date! < cycle.end_date! ? idealToday : null,
actual: p.date! <= today ? (isBurnDown ? Math.abs(pending) : completed) : undefined,
ideal:
dataDate! < today
? ideal(dataDate, scope(p, isTypeIssue), cycle)
: dataDate! < cycle.end_date!
? idealToday
: null,
actual: dataDate! <= today ? (isBurnDown ? Math.abs(pending) : completed) : undefined,
Comment on lines +131 to +153
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 3, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle different types of p in the progress mapping.

In formatV2Data, the progress array combines cycle.progress objects and extendedArray dates. When mapping over this array, the code assumes that each p has certain properties, which may not be true for dates from extendedArray.

Separate the handling of progress data and extended dates:

 let progressData = orderBy(cycle.progress, "date").map((p) => {
   // existing logic for progress entries
 });

+let extendedProgress = extendedArray.map((date) => ({
+  date: format(new Date(date), "yyyy-MM-dd"),
+  scope: date <= cycle.end_date! ? scopeToday : null,
+  // other properties set to default or null
+}));

-progress = [...progressData, ...extendedArray].map((p) => { /* ... */ });
+progress = [...progressData, ...extendedProgress];
 progress = uniqBy(progress, "date");

This ensures that the properties are correctly assigned based on whether p is a progress entry or an extended date.

Committable suggestion was skipped due to low confidence.

};
});
return uniqBy(progress, "date");
progress = uniqBy(progress, "date");

return progress;
};
Comment on lines +120 to +159
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 3, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Possible property access on undefined in formatV2Data.

When accessing todaysData.date and other properties of todaysData, there's a risk that todaysData could be undefined if cycle.progress is empty. This would lead to a runtime error.

Apply this diff to add a safety check:

 let today: Date | string = startOfToday();
 
 const extendedArray = endDate > today ? generateDateArray(today as Date, endDate) : [];
 if (isEmpty(cycle.progress)) return extendedArray;
 today = format(startOfToday(), "yyyy-MM-dd");
-const todaysData = cycle?.progress[cycle?.progress.length - 1];
+const todaysData = cycle.progress[cycle.progress.length - 1];
+if (!todaysData) return [];
 const scopeToday = scope(todaysData, isTypeIssue);
 const idealToday = ideal(todaysData.date, scopeToday, cycle);

This ensures that the function returns early if todaysData is undefined.

Committable suggestion was skipped due to low confidence.


export const formatActiveCycle = (args: {
cycle: ICycle;
isBurnDown?: boolean | undefined;
isTypeIssue?: boolean | undefined;
}) => {
const { cycle, isBurnDown, isTypeIssue } = args;
const endDate: Date | string = new Date(cycle.end_date!);

return cycle.version === 1
? formatV1Data(isTypeIssue!, cycle, isBurnDown!, endDate)
: formatV2Data(isTypeIssue!, cycle, isBurnDown!, endDate);
Comment on lines +161 to +171
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 3, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid using non-null assertions with potentially undefined values.

In formatActiveCycle, isTypeIssue and isBurnDown are optional and may be undefined, but non-null assertions (!) are used when passing them to formatV1Data and formatV2Data, which could lead to runtime errors.

Provide default values to ensure safety:

 export const formatActiveCycle = (args: {
   cycle: ICycle;
   isBurnDown?: boolean;
   isTypeIssue?: boolean;
 }) => {
   const { cycle, isBurnDown = false, isTypeIssue = false } = args;
   const endDate: Date | string = new Date(cycle.end_date!);

   return cycle.version === 1
-    ? formatV1Data(isTypeIssue!, cycle, isBurnDown!, endDate)
-    : formatV2Data(isTypeIssue!, cycle, isBurnDown!, endDate);
+    ? formatV1Data(isTypeIssue, cycle, isBurnDown, endDate)
+    : formatV2Data(isTypeIssue, cycle, isBurnDown, endDate);
 };

This approach removes the need for non-null assertions and sets default values if they are not provided.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const formatActiveCycle = (args: {
cycle: ICycle;
isBurnDown?: boolean | undefined;
isTypeIssue?: boolean | undefined;
}) => {
const { cycle, isBurnDown, isTypeIssue } = args;
const endDate: Date | string = new Date(cycle.end_date!);
return cycle.version === 1
? formatV1Data(isTypeIssue!, cycle, isBurnDown!, endDate)
: formatV2Data(isTypeIssue!, cycle, isBurnDown!, endDate);
export const formatActiveCycle = (args: {
cycle: ICycle;
isBurnDown?: boolean;
isTypeIssue?: boolean;
}) => {
const { cycle, isBurnDown = false, isTypeIssue = false } = args;
const endDate: Date | string = new Date(cycle.end_date!);
return cycle.version === 1
? formatV1Data(isTypeIssue, cycle, isBurnDown, endDate)
: formatV2Data(isTypeIssue, cycle, isBurnDown, endDate);
};

};
46 changes: 1 addition & 45 deletions web/helpers/date-time.helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,57 +358,13 @@ export const getReadTimeFromWordsCount = (wordsCount: number): number => {
return minutes * 60;
};

/**
* @description calculates today's date
* @param {boolean} format
* @returns {Date | string} today's date
* @example getToday() // Output: 2024-09-29T00:00:00.000Z
* @example getToday(true) // Output: 2024-09-29
*/
export const getToday = (format: boolean = false) => {
const today = new Date();
today.setHours(0, 0, 0, 0);
if (!format) return today;

const year = today.getFullYear();
const month = String(today.getMonth() + 1).padStart(2, "0"); // Months are 0-based, so add 1
const day = String(today.getDate()).padStart(2, "0"); // Add leading zero for single digits
return `${year}-${month}-${day}`;
};

/**
* @description calculates the date of the day before today
* @param {boolean} format
* @returns {Date | string} date of the day before today
* @example dateFormatter() // Output: "Sept 20, 2024"
*/
export const dateFormatter = (dateString: string) => {
// Convert to Date object
const date = new Date(dateString);

// Options for the desired format (Month Day, Year)
const options: Intl.DateTimeFormatOptions = { year: "numeric", month: "short", day: "numeric" };

// Format the date
const formattedDate = date.toLocaleDateString("en-US", options);

return formattedDate;
};

/**
* @description calculates days left from today to the end date
* @returns {Date | string} number of days left
*/
export const daysLeft = (end_date: string) =>
end_date ? Math.ceil((new Date(end_date).getTime() - new Date().getTime()) / (1000 * 3600 * 24)) : 0;

/**
* @description generates an array of dates between the start and end dates
* @param startDate
* @param endDate
* @returns
*/
export const generateDateArray = (startDate: Date, endDate: Date) => {
export const generateDateArray = (startDate: string | Date, endDate: string | Date) => {
// Convert the start and end dates to Date objects if they aren't already
const start = new Date(startDate);
// start.setDate(start.getDate() + 1);
Expand Down
Loading