Skip to content

[WEB-2628] fix: Sorting by estimates #5988

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 4 commits into from
Nov 13, 2024
Merged

[WEB-2628] fix: Sorting by estimates #5988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4e03ffe
fix estimates sorting in Front end side
rahulramesha Nov 11, 2024
2dea5cb
change estimate sorting keys
rahulramesha Nov 11, 2024
12e35d6
- Fix estimate sorting when local db is enabled
SatishGandham Nov 12, 2024
e21e9ce
Add logic for order by based on layout for special cases of no load
rahulramesha Nov 12, 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
4 changes: 2 additions & 2 deletions packages/types/src/view-props.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ export type TIssueOrderByOptions =
| "-issue_cycle__cycle__name"
| "target_date"
| "-target_date"
| "estimate_point"
| "-estimate_point"
| "estimate_point__key"
| "-estimate_point__key"
| "start_date"
| "-start_date"
| "link_count"
Expand Down
4 changes: 2 additions & 2 deletions web/core/constants/spreadsheet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,9 @@ export const SPREADSHEET_PROPERTY_DETAILS: {
},
estimate: {
title: "Estimate",
ascendingOrderKey: "estimate_point",
ascendingOrderKey: "estimate_point__key",
ascendingOrderTitle: "Low",
descendingOrderKey: "-estimate_point",
descendingOrderKey: "-estimate_point__key",
descendingOrderTitle: "High",
icon: Triangle,
Column: SpreadsheetEstimateColumn,
Expand Down
1 change: 1 addition & 0 deletions web/core/local-db/storage.sqlite.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,7 @@ export class Storage {
const { cursor, group_by, sub_group_by } = queries;

const query = issueFilterQueryConstructor(this.workspaceSlug, projectId, queries);
log("#### Query", query);
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 12, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Inconsistent logging patterns detected in query-related operations.

The added logging statement is not consistent with existing logging practices across other query-related methods. Please ensure that logging is applied uniformly to facilitate debugging and maintenance.

  • web/core/local-db/storage.sqlite.ts (Line 303)
🔗 Analysis chain

LGTM! Helpful debug logging addition.

The added logging statement for the constructed query is well-placed and consistent with the existing logging pattern in the file. This will be particularly useful for debugging the sorting functionality changes related to estimate_point__key.

Let's verify the logging implementation is consistent across query-related operations:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent logging patterns across query-related operations

# Search for logging patterns in query-related code
rg -A 2 'log\(' web/core/local-db/storage.sqlite.ts

Length of output: 3677

const countQuery = issueFilterCountQueryConstructor(this.workspaceSlug, projectId, queries);
const start = performance.now();
let issuesRaw: any[] = [];
Expand Down
11 changes: 7 additions & 4 deletions web/core/local-db/utils/load-workspace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,10 @@ export const getStates = async (workspaceSlug: string) => {
export const getEstimatePoints = async (workspaceSlug: string) => {
const estimateService = new EstimateService();
const estimates = await estimateService.fetchWorkspaceEstimates(workspaceSlug);
const objects: IEstimatePoint[] = [];
let objects: IEstimatePoint[] = [];
(estimates || []).forEach((estimate: IEstimate) => {
if (estimate?.points) {
objects.concat(estimate.points);
objects = objects.concat(estimate.points);
}
Comment on lines +90 to 94
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 12, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Schema does not support the new estimate point key structure

The estimatePointSchema does not include estimate_point__key. Please update the schema to support the new key structure.

🔗 Analysis chain

Verify schema compatibility with estimate point key changes

Given that the PR changes the estimate point key from estimate_point to estimate_point__key, we should verify that the database schema and related components support this change.

Also applies to: 127-127

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the estimate point schema and related components support the new key structure

# Check the estimate point schema definition
rg -A 10 "estimatePointSchema.*=" 

# Check for any references to the old and new estimate point keys
rg "estimate_point[^_]"
rg "estimate_point__key"

Length of output: 35316

});
return objects;
Expand All @@ -104,6 +104,9 @@ export const getMembers = async (workspaceSlug: string) => {
};

export const loadWorkSpaceData = async (workspaceSlug: string) => {
if (!persistence.db || !persistence.db.exec) {
return;
}
log("Loading workspace data");
const promises = [];
promises.push(getLabels(workspaceSlug));
Expand All @@ -112,7 +115,7 @@ export const loadWorkSpaceData = async (workspaceSlug: string) => {
promises.push(getStates(workspaceSlug));
promises.push(getEstimatePoints(workspaceSlug));
promises.push(getMembers(workspaceSlug));
const [labels, modules, cycles, states, estimates, memebers] = await Promise.all(promises);
const [labels, modules, cycles, states, estimates, members] = await Promise.all(promises);

const start = performance.now();
await persistence.db.exec("BEGIN;");
Expand All @@ -121,7 +124,7 @@ export const loadWorkSpaceData = async (workspaceSlug: string) => {
await batchInserts(cycles, "cycles", cycleSchema);
await batchInserts(states, "states", stateSchema);
await batchInserts(estimates, "estimate_points", estimatePointSchema);
await batchInserts(memebers, "members", memberSchema);
await batchInserts(members, "members", memberSchema);
await persistence.db.exec("COMMIT;");
const end = performance.now();
log("Time taken to load workspace data", end - start);
Expand Down
14 changes: 6 additions & 8 deletions web/core/local-db/utils/query-constructor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ export const SPECIAL_ORDER_BY = {
"-issue_cycle__cycle__name": "cycles",
state__name: "states",
"-state__name": "states",
estimate_point__key: "estimate_point",
"-estimate_point__key": "estimate_point",
};
export const issueFilterQueryConstructor = (workspaceSlug: string, projectId: string, queries: any) => {
const {
Expand Down Expand Up @@ -48,8 +50,6 @@ export const issueFilterQueryConstructor = (workspaceSlug: string, projectId: st

`;

log("###", sql);

return sql;
}
if (group_by) {
Expand All @@ -64,8 +64,6 @@ export const issueFilterQueryConstructor = (workspaceSlug: string, projectId: st
WHERE rank <= ${per_page}
`;

log("###", sql);

return sql;
}

Expand All @@ -78,16 +76,18 @@ export const issueFilterQueryConstructor = (workspaceSlug: string, projectId: st
sql += `SELECT fi.* , `;
if (order_by.includes("assignee")) {
sql += ` s.first_name as ${name} `;
} else if (order_by.includes("estimate")) {
sql += ` s.key as ${name} `;
} else {
sql += ` s.name as ${name} `;
sql += ` s.name as ${name} `;
}
sql += `FROM fi `;
if (order_by && Object.keys(SPECIAL_ORDER_BY).includes(order_by)) {
if (order_by.includes("cycle")) {
sql += `
LEFT JOIN cycles s on fi.cycle_id = s.id`;
}
if (order_by.includes("estimate_point")) {
if (order_by.includes("estimate_point__key")) {
sql += `
LEFT JOIN estimate_points s on fi.estimate_point = s.id`;
}
Expand Down Expand Up @@ -120,7 +120,6 @@ export const issueFilterQueryConstructor = (workspaceSlug: string, projectId: st
`;
sql += ` group by i.id ${orderByString} LIMIT ${pageSize} OFFSET ${offset * 1 + page * pageSize};`;

log("######$$$", sql);
return sql;
}

Expand Down Expand Up @@ -149,7 +148,6 @@ export const issueFilterQueryConstructor = (workspaceSlug: string, projectId: st
// Add offset and paging to query
sql += ` LIMIT ${pageSize} OFFSET ${offset * 1 + page * pageSize};`;

log("$$$", sql);
return sql;
};

Expand Down
5 changes: 4 additions & 1 deletion web/core/local-db/utils/query.utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export const translateQueryParams = (queries: any) => {
}

// Fix invalid orderby when switching from spreadsheet layout
if (layout === "spreadsheet" && Object.keys(SPECIAL_ORDER_BY).includes(order_by)) {
if (layout !== "spreadsheet" && Object.keys(SPECIAL_ORDER_BY).includes(order_by)) {
otherProps.order_by = "sort_order";
}
// For each property value, replace None with empty string
Expand Down Expand Up @@ -336,6 +336,9 @@ const getSingleFilterFields = (queries: any) => {
if (state_group) {
fields.add("states.'group' as state_group");
}
if (order_by?.includes("estimate_point__key")) {
fields.add("estimate_point");
}
return Array.from(fields);
};

Expand Down
92 changes: 74 additions & 18 deletions web/core/store/issue/helpers/base-issues.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { EIssueLayoutTypes, ISSUE_PRIORITIES } from "@/constants/issue";
// helpers
import { convertToISODateString } from "@/helpers/date-time.helper";
// local-db
import { SPECIAL_ORDER_BY } from "@/local-db/utils/query-constructor";
import { updatePersistentLayer } from "@/local-db/utils/utils";
// services
import { CycleService } from "@/services/cycle.service";
Expand Down Expand Up @@ -164,8 +165,8 @@ const ISSUE_ORDERBY_KEY: Record<TIssueOrderByOptions, keyof TIssue> = {
"-issue_cycle__cycle__name": "cycle_id",
target_date: "target_date",
"-target_date": "target_date",
estimate_point: "estimate_point",
"-estimate_point": "estimate_point",
estimate_point__key: "estimate_point",
"-estimate_point__key": "estimate_point",
start_date: "start_date",
"-start_date": "start_date",
link_count: "link_count",
Expand Down Expand Up @@ -282,6 +283,19 @@ export abstract class BaseIssuesStore implements IBaseIssuesStore {
const displayFilters = this.issueFilterStore?.issueFilters?.displayFilters;
if (!displayFilters) return;

const layout = displayFilters.layout;
const orderBy = displayFilters.order_by;

// Temporary code to fix no load order by
if (
this.rootIssueStore.rootStore.user.localDBEnabled &&
layout !== EIssueLayoutTypes.SPREADSHEET &&
orderBy &&
Object.keys(SPECIAL_ORDER_BY).includes(orderBy)
) {
return "sort_order";
}

return displayFilters?.order_by;
}

Expand Down Expand Up @@ -1701,13 +1715,14 @@ export abstract class BaseIssuesStore implements IBaseIssuesStore {
* @returns string | string[] of sortable fields to be used for sorting
*/
populateIssueDataForSorting(
dataType: "state_id" | "label_ids" | "assignee_ids" | "module_ids" | "cycle_id",
dataType: "state_id" | "label_ids" | "assignee_ids" | "module_ids" | "cycle_id" | "estimate_point",
dataIds: string | string[] | null | undefined,
projectId: string | undefined | null,
order?: "asc" | "desc"
) {
if (!dataIds) return;

const dataValues: string[] = [];
const dataValues: (string | number)[] = [];
const isDataIdsArray = Array.isArray(dataIds);
const dataIdsArray = isDataIdsArray ? dataIds : [dataIds];

Expand Down Expand Up @@ -1757,6 +1772,26 @@ export abstract class BaseIssuesStore implements IBaseIssuesStore {
}
break;
}
case "estimate_point": {
// return if project Id does not exist
if (!projectId) break;
// get the estimate ID for the current Project
const currentProjectEstimateId =
this.rootIssueStore.rootStore.projectEstimate.currentActiveEstimateIdByProjectId(projectId);
// return if current Estimate Id for the project is not available
if (!currentProjectEstimateId) break;
// get Estimate based on Id
const estimate = this.rootIssueStore.rootStore.projectEstimate.estimateById(currentProjectEstimateId);
// If Estimate is not available, then return
if (!estimate) break;
// Get Estimate Value
const estimateKey = estimate?.estimatePointById(dataIds as string)?.key;

// If Value string i not available or empty then return
if (estimateKey === undefined) break;

dataValues.push(estimateKey);
}
Comment on lines +1775 to +1794
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 12, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle dataIds type correctly in estimate_point case

In the "estimate_point" case, dataIds is cast to string with dataIds as string, but dataIds can be string | string[] | null | undefined. Casting without checking can cause runtime errors if dataIds is an array or null.

Consider modifying the code to handle both string and string[] types for dataIds. Apply the following diff to fix the issue:

 case "estimate_point": {
   if (!projectId) break;
   const currentProjectEstimateId =
     this.rootIssueStore.rootStore.projectEstimate.currentActiveEstimateIdByProjectId(projectId);
   if (!currentProjectEstimateId) break;
   const estimate = this.rootIssueStore.rootStore.projectEstimate.estimateById(currentProjectEstimateId);
   if (!estimate) break;
-  const estimateKey = estimate?.estimatePointById(dataIds as string)?.key;
-  if (estimateKey === undefined) break;
-  dataValues.push(estimateKey);
+  const dataIdsArray = Array.isArray(dataIds) ? dataIds : [dataIds];
+  for (const id of dataIdsArray) {
+    if (!id) continue;
+    const estimateKey = estimate.estimatePointById(id)?.key;
+    if (estimateKey !== undefined) {
+      dataValues.push(estimateKey);
+    }
+  }
 }

This change ensures that both single and multiple dataIds are handled correctly, preventing potential runtime errors.

📝 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
case "estimate_point": {
// return if project Id does not exist
if (!projectId) break;
// get the estimate ID for the current Project
const currentProjectEstimateId =
this.rootIssueStore.rootStore.projectEstimate.currentActiveEstimateIdByProjectId(projectId);
// return if current Estimate Id for the project is not available
if (!currentProjectEstimateId) break;
// get Estimate based on Id
const estimate = this.rootIssueStore.rootStore.projectEstimate.estimateById(currentProjectEstimateId);
// If Estimate is not available, then return
if (!estimate) break;
// Get Estimate Value
const estimateKey = estimate?.estimatePointById(dataIds as string)?.key;
// If Value string i not available or empty then return
if (estimateKey === undefined) break;
dataValues.push(estimateKey);
}
case "estimate_point": {
// return if project Id does not exist
if (!projectId) break;
// get the estimate ID for the current Project
const currentProjectEstimateId =
this.rootIssueStore.rootStore.projectEstimate.currentActiveEstimateIdByProjectId(projectId);
// return if current Estimate Id for the project is not available
if (!currentProjectEstimateId) break;
// get Estimate based on Id
const estimate = this.rootIssueStore.rootStore.projectEstimate.estimateById(currentProjectEstimateId);
// If Estimate is not available, then return
if (!estimate) break;
// Get Estimate Value
const dataIdsArray = Array.isArray(dataIds) ? dataIds : [dataIds];
for (const id of dataIdsArray) {
if (!id) continue;
const estimateKey = estimate.estimatePointById(id)?.key;
if (estimateKey !== undefined) {
dataValues.push(estimateKey);
}
}
}

}

return isDataIdsArray ? (order ? orderBy(dataValues, undefined, [order]) : dataValues) : dataValues;
Expand All @@ -1771,11 +1806,17 @@ export abstract class BaseIssuesStore implements IBaseIssuesStore {
return getIssueIds(orderBy(array, "sort_order"));
case "state__name":
return getIssueIds(
orderBy(array, (issue) => this.populateIssueDataForSorting("state_id", issue?.["state_id"]))
orderBy(array, (issue) =>
this.populateIssueDataForSorting("state_id", issue?.["state_id"], issue?.["project_id"])
)
);
case "-state__name":
return getIssueIds(
orderBy(array, (issue) => this.populateIssueDataForSorting("state_id", issue?.["state_id"]), ["desc"])
orderBy(
array,
(issue) => this.populateIssueDataForSorting("state_id", issue?.["state_id"], issue?.["project_id"]),
["desc"]
)
);
// dates
case "created_at":
Expand Down Expand Up @@ -1826,15 +1867,23 @@ export abstract class BaseIssuesStore implements IBaseIssuesStore {
case "-attachment_count":
return getIssueIds(orderBy(array, "attachment_count", ["desc"]));

case "estimate_point":
case "estimate_point__key":
return getIssueIds(
orderBy(array, [getSortOrderToFilterEmptyValues.bind(null, "estimate_point"), "estimate_point"])
orderBy(array, [
getSortOrderToFilterEmptyValues.bind(null, "estimate_point"),
(issue) =>
this.populateIssueDataForSorting("estimate_point", issue?.["estimate_point"], issue?.["project_id"]),
])
); //preferring sorting based on empty values to always keep the empty values below
case "-estimate_point":
case "-estimate_point__key":
return getIssueIds(
orderBy(
array,
[getSortOrderToFilterEmptyValues.bind(null, "estimate_point"), "estimate_point"], //preferring sorting based on empty values to always keep the empty values below
[
getSortOrderToFilterEmptyValues.bind(null, "estimate_point"),
(issue) =>
this.populateIssueDataForSorting("estimate_point", issue?.["estimate_point"], issue?.["project_id"]),
], //preferring sorting based on empty values to always keep the empty values below
["asc", "desc"]
)
);
Expand All @@ -1854,7 +1903,8 @@ export abstract class BaseIssuesStore implements IBaseIssuesStore {
return getIssueIds(
orderBy(array, [
getSortOrderToFilterEmptyValues.bind(null, "label_ids"), //preferring sorting based on empty values to always keep the empty values below
(issue) => this.populateIssueDataForSorting("label_ids", issue?.["label_ids"], "asc"),
(issue) =>
this.populateIssueDataForSorting("label_ids", issue?.["label_ids"], issue?.["project_id"], "asc"),
])
);
case "-labels__name":
Expand All @@ -1863,7 +1913,8 @@ export abstract class BaseIssuesStore implements IBaseIssuesStore {
array,
[
getSortOrderToFilterEmptyValues.bind(null, "label_ids"), //preferring sorting based on empty values to always keep the empty values below
(issue) => this.populateIssueDataForSorting("label_ids", issue?.["label_ids"], "asc"),
(issue) =>
this.populateIssueDataForSorting("label_ids", issue?.["label_ids"], issue?.["project_id"], "asc"),
],
["asc", "desc"]
)
Expand All @@ -1873,7 +1924,8 @@ export abstract class BaseIssuesStore implements IBaseIssuesStore {
return getIssueIds(
orderBy(array, [
getSortOrderToFilterEmptyValues.bind(null, "module_ids"), //preferring sorting based on empty values to always keep the empty values below
(issue) => this.populateIssueDataForSorting("module_ids", issue?.["module_ids"], "asc"),
(issue) =>
this.populateIssueDataForSorting("module_ids", issue?.["module_ids"], issue?.["project_id"], "asc"),
])
);
case "-issue_module__module__name":
Expand All @@ -1882,7 +1934,8 @@ export abstract class BaseIssuesStore implements IBaseIssuesStore {
array,
[
getSortOrderToFilterEmptyValues.bind(null, "module_ids"), //preferring sorting based on empty values to always keep the empty values below
(issue) => this.populateIssueDataForSorting("module_ids", issue?.["module_ids"], "asc"),
(issue) =>
this.populateIssueDataForSorting("module_ids", issue?.["module_ids"], issue?.["project_id"], "asc"),
],
["asc", "desc"]
)
Expand All @@ -1892,7 +1945,7 @@ export abstract class BaseIssuesStore implements IBaseIssuesStore {
return getIssueIds(
orderBy(array, [
getSortOrderToFilterEmptyValues.bind(null, "cycle_id"), //preferring sorting based on empty values to always keep the empty values below
(issue) => this.populateIssueDataForSorting("cycle_id", issue?.["cycle_id"], "asc"),
(issue) => this.populateIssueDataForSorting("cycle_id", issue?.["cycle_id"], issue?.["project_id"], "asc"),
])
);
case "-issue_cycle__cycle__name":
Expand All @@ -1901,7 +1954,8 @@ export abstract class BaseIssuesStore implements IBaseIssuesStore {
array,
[
getSortOrderToFilterEmptyValues.bind(null, "cycle_id"), //preferring sorting based on empty values to always keep the empty values below
(issue) => this.populateIssueDataForSorting("cycle_id", issue?.["cycle_id"], "asc"),
(issue) =>
this.populateIssueDataForSorting("cycle_id", issue?.["cycle_id"], issue?.["project_id"], "asc"),
],
["asc", "desc"]
)
Expand All @@ -1911,7 +1965,8 @@ export abstract class BaseIssuesStore implements IBaseIssuesStore {
return getIssueIds(
orderBy(array, [
getSortOrderToFilterEmptyValues.bind(null, "assignee_ids"), //preferring sorting based on empty values to always keep the empty values below
(issue) => this.populateIssueDataForSorting("assignee_ids", issue?.["assignee_ids"], "asc"),
(issue) =>
this.populateIssueDataForSorting("assignee_ids", issue?.["assignee_ids"], issue?.["project_id"], "asc"),
])
);
case "-assignees__first_name":
Expand All @@ -1920,7 +1975,8 @@ export abstract class BaseIssuesStore implements IBaseIssuesStore {
array,
[
getSortOrderToFilterEmptyValues.bind(null, "assignee_ids"), //preferring sorting based on empty values to always keep the empty values below
(issue) => this.populateIssueDataForSorting("assignee_ids", issue?.["assignee_ids"], "asc"),
(issue) =>
this.populateIssueDataForSorting("assignee_ids", issue?.["assignee_ids"], issue?.["project_id"], "asc"),
],
["asc", "desc"]
)
Expand Down