-
Notifications
You must be signed in to change notification settings - Fork 14
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.
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
#2741 - Ministry Filter Search Programs and Offerings [Location and Program Status Search] #3819
#2741 - Ministry Filter Search Programs and Offerings [Location and Program Status Search] #3819
Conversation
); | ||
queryParams.push(`%${paginationOptions.locationNameSearch}%`); | ||
} | ||
if (paginationOptions.status) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -331,6 +331,33 @@ export class EducationProgramService extends RecordDataModelService<EducationPro | |||
}); | |||
queryParams.push(`%${paginationOptions.searchCriteria}%`); | |||
} | |||
if (paginationOptions.programNameSearch) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct me if i am wrong, the paginationOptions, if searched with only one criteria, paginationOptions.searchCriteria is used else the paginationOptions.programNameSearch and paginationOptions.locationNameSearch is pushed. If that is the case, either paginationOptions.searchCriteria should be pushed or the paginationOptions.programNameSearch and paginationOptions.locationNameSearch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
searchCriteria
should not be used for programs.
@sh16011993 what if we change the service models as below to ensure the searchCriteria
will not be present.
export interface BasePaginationOptions {
sortField?: string;
sortOrder?: FieldSortOrder;
page: number;
pageLimit: number;
}
/**
* Pagination option.
*/
export interface PaginationOptions extends BasePaginationOptions {
searchCriteria?: string;
}
export interface ProgramPaginationOptions extends BasePaginationOptions {
programNameSearch?: string;
locationNameSearch?: string;
status?: string;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, please have a look on the comment
sources/packages/backend/apps/api/src/route-controllers/models/pagination.dto.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/route-controllers/models/pagination.dto.ts
Outdated
Show resolved
Hide resolved
} | ||
if (paginationOptions.status) { | ||
paginatedProgramQuery.andWhere( | ||
"programs.programStatus Ilike :programStatusSearchCriteria", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
programStatus
does not require Ilike
.
@@ -11,6 +11,12 @@ export interface PaginationOptions { | |||
pageLimit: number; | |||
} | |||
|
|||
export interface ProgramPaginationOptions extends PaginationOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see suggested changes here: https://github.com/bcgov/SIMS/pull/3819/files#r1813295340
if ( | ||
paginationOptions.searchCriteria && | ||
typeof paginationOptions.searchCriteria === "object" | ||
) { | ||
const searchCriteria = paginationOptions.searchCriteria; | ||
for (const searchCriterion in searchCriteria) { | ||
parameters.push(`${searchCriterion}=${searchCriteria[searchCriterion]}`); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
if (!paginationOptions.searchCriteria) {
return parameters.join("&");
}
// Search criteria.
if (typeof paginationOptions.searchCriteria === "string") {
// Single search criteria.
parameters.push(
`${PaginationParams.SearchCriteria}=${paginationOptions.searchCriteria}`,
);
return parameters.join("&");
}
// Multiple search criteria.
const searchCriteria = paginationOptions.searchCriteria;
for (const searchCriterion in paginationOptions.searchCriteria) {
parameters.push(`${searchCriterion}=${searchCriteria[searchCriterion]}`);
}
return parameters.join("&");
@@ -123,7 +123,7 @@ export enum PaginationParams { | |||
* todo: remove sortOrder: DataTableSortOrder when all primevue datatables are removed. | |||
*/ | |||
export interface PaginationOptions { | |||
searchCriteria?: string; | |||
searchCriteria?: string | Record<string, string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a comment to this parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. Please take a look at the comments.
sources/packages/backend/apps/api/src/route-controllers/models/pagination.dto.ts
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/route-controllers/models/pagination.dto.ts
Show resolved
Hide resolved
// query parameter assigned to paginatedProgramQuery like | ||
// paginationOptions.searchCriteria or institutionId | ||
// queryParams should follow the order/index. | ||
const queryParams: any[] = [...offeringTypes, institutionId]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even being from the old code, it can be changed to unknown
without a need for further modifications.
institutionId: number, | ||
): { | ||
programQuery: SelectQueryBuilder<EducationProgram>; | ||
queryParams: any[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( | ||
!paginationOptions.statusSearch && | ||
!paginationOptions.inactiveProgramSearch | ||
) { | ||
return { | ||
results: [], | ||
count: 0, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not following up on why "nothing" should be returned for this case. Can you please elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we talked let's move this to the beginning of the method.
It seems okay to stay if it respects the business requirements.
My advise would be to UI and DTO enforce that one of these should be present.
private async preparePaginatedProgramQuery( | ||
paginatedProgramQuery: SelectQueryBuilder<EducationProgram>, | ||
paginationOptions: PaginationOptions, | ||
queryParams: any[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change to unknown
.
// sort | ||
if ( | ||
paginationOptions.sortField === "programStatus" && | ||
paginationOptions.sortOrder === "DESC" | ||
) { | ||
paginatedProgramQuery.orderBy( | ||
`CASE | ||
WHEN programs.programStatus = '${ProgramStatus.Pending}' and programs.isActive = true THEN ${SortPriority.Priority1} | ||
WHEN programs.isActive = false THEN ${SortPriority.Priority2} | ||
WHEN programs.programStatus = '${ProgramStatus.Declined}' and programs.isActive = true THEN ${SortPriority.Priority3} | ||
WHEN programs.programStatus = '${ProgramStatus.Approved}' and programs.isActive = true THEN ${SortPriority.Priority4} | ||
ELSE ${SortPriority.Priority5} | ||
END`, | ||
); | ||
} else if ( | ||
paginationOptions.sortField === "programStatus" && | ||
paginationOptions.sortOrder === "ASC" | ||
) { | ||
paginatedProgramQuery.orderBy( | ||
`CASE | ||
WHEN programs.programStatus = '${ProgramStatus.Approved}' and programs.isActive = true THEN ${SortPriority.Priority1} | ||
WHEN programs.programStatus = '${ProgramStatus.Declined}' and programs.isActive = true THEN ${SortPriority.Priority2} | ||
WHEN programs.isActive = false THEN ${SortPriority.Priority3} | ||
WHEN programs.programStatus = '${ProgramStatus.Pending}' and programs.isActive = true THEN ${SortPriority.Priority4} | ||
ELSE ${SortPriority.Priority5} | ||
END`, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the requirement is to have this ordered alphabetically, can the first part be simplified as below?
// Sort
if (paginationOptions.sortField === "programStatus") {
paginatedProgramQuery.orderBy(
`CASE WHEN programs.isActive = false THEN '${INACTIVE_PROGRAM_STATUS}' ELSE programs.programStatus :: text end`,
paginationOptions.sortOrder,
);
} else if (paginationOptions.sortField && paginationOptions.sortOrder) {
paginatedProgramQuery.orderBy(
sortProgramsColumnMap(paginationOptions.sortField),
paginationOptions.sortOrder,
);
}
} from "@/types"; | ||
import { AESTRoutesConst } from "@/constants/routes/RouteConstants"; | ||
import StatusChipProgram from "@/components/generic/StatusChipProgram.vue"; | ||
import { EducationProgramService } from "@/services/EducationProgramService"; | ||
|
||
const INACTIVE_PROGRAM = "Inactive"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be added to a common location and shared with the program status chip.
const statusSearchList = JSON.parse( | ||
JSON.stringify(searchProgramStatus.value), | ||
); | ||
const searchInactiveProgram = statusSearchList.indexOf("Inactive") > -1; | ||
if (searchInactiveProgram) { | ||
statusSearchList.splice(statusSearchList.indexOf("Inactive"), 1); | ||
} | ||
let searchCriteria: Record<string, string | string[] | boolean>; | ||
if (statusSearchList.length) { | ||
searchCriteria = { | ||
programNameSearch: searchProgramName.value, | ||
locationNameSearch: searchLocationName.value, | ||
statusSearch: statusSearchList, | ||
inactiveProgramSearch: searchInactiveProgram, | ||
}; | ||
} else { | ||
searchCriteria = { | ||
programNameSearch: searchProgramName.value, | ||
locationNameSearch: searchLocationName.value, | ||
inactiveProgramSearch: searchInactiveProgram, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be refactored as below? The idea is to execute more array checks than string manipulations.
const statusSearchList = searchProgramStatus.value.filter(
(searchItem) => searchItem !== INACTIVE_PROGRAM,
);
const searchInactiveProgram = searchProgramStatus.value.some(
(searchItem) => searchItem === INACTIVE_PROGRAM,
);
let searchCriteria: Record<string, string | string[] | boolean> = {
programNameSearch: searchProgramName.value,
locationNameSearch: searchLocationName.value,
statusSearch: statusSearchList,
inactiveProgramSearch: searchInactiveProgram,
};
if (statusSearchList.length) {
searchCriteria.statusSearch = statusSearchList.toString(); // Check if we need JSON.stringify.
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No conversion needed here as filter
returns a string array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work doing the changes and refactors. Please take a look a the comments.
@@ -0,0 +1 @@ | |||
export const INACTIVE_PROGRAM = "Inactive"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the same name from the API: INACTIVE_PROGRAM_STATUS
.
// sort | ||
if (paginationOptions.sortField === "programStatus") { | ||
paginatedProgramQuery.orderBy( | ||
`CASE WHEN programs.isActive = false THEN '${INACTIVE_PROGRAM_STATUS}' ELSE programs.programStatus :: text end`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change it to "END".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes and for all the effort. The search seems a great addition. Looks good 👍
Quality Gate passedIssues Measures |
As a part of the PR, the following were completed:
&&
operation.api
to include thelocation
and the program status in the pagination for the search and updated thesql
query fetching the result.Screenshots:
Location Search and Program Status Search added:
Location Search with Program Search Empty and Program Statuses: Pending and Inactive selected:
Both Location and Program Search with Program Statuses: Pending and Approved selected
Location Search along with the Program Status: Pending selected:
Inactive Programs selected:
Nothing selected from the Program Status filter:
Descending Sort by Program Status:
Natural Sorting (without ASC / DESC selected) on page load:
Ascending Sort by Program Status: