#1953 - Request an Offering Change - Part 2 #2013
Conversation
ann-aot
changed the title
ann-aot
requested review from
JasonCTang,
andrepestana-aot,
andrewsignori-aot,
dheepak-aot,
guru-aot,
hellolynn-tbtb and
sh16011993
.../src/route-controllers/institution-locations/institution-location.institutions.controller.ts
Outdated
Show resolved
Hide resolved
.../src/route-controllers/institution-locations/institution-location.institutions.controller.ts
Outdated
Show resolved
Hide resolved
.../src/route-controllers/institution-locations/institution-location.institutions.controller.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/services/application/application.service.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/services/application/application.service.ts
Show resolved
Hide resolved
...apps/db-migrations/src/sql/Types/Add-application-offering-change-assessment-trigger-type.sql
Outdated
Show resolved
Hide resolved
andrewsignori-aot
left a comment
andrewsignori-aot
left a comment
There was a problem hiding this comment.
Great work, please take a look at the comments.
.../src/route-controllers/institution-locations/institution-location.institutions.controller.ts
Outdated
Show resolved
Hide resolved
| @Get(":locationId/active-applications/request-change") | ||
| async getEligibleApplicationOfferingChangeRecords( | ||
| @Param("locationId", ParseIntPipe) locationId: number, | ||
| @Query() pagination: ApplicationStatusPaginationOptionsAPIInDTO, |
There was a problem hiding this comment.
May be or may not be part of the PR. ApplicationStatusPaginationOptionsAPIInDTO the DTO name and it's extended purpose aren't matching.
There was a problem hiding this comment.
That makes sense, As mentioned in the description, I have reused the DTO. If devs are fine with the approach, I will rename the dto
There was a problem hiding this comment.
I would not mind keeping these DTOs specific for the target endpoint.
@ann-aot just double-checking, the idea would be to reuse the same pagination DTO for multiple endpoints?
There was a problem hiding this comment.
the extended pagination dto
as its the same @andrewsignori-aot
There was a problem hiding this comment.
as mentioned, created a separate one for the endpoint
| }) | ||
| .andWhere("application.isArchived = false"); | ||
|
|
||
| if (paginationOptions.searchCriteria) { |
There was a problem hiding this comment.
if(paginationOptions.searchCriteria?.trim()) ? If search criteria becomes nothing after trim, we can avoid upfront.
There was a problem hiding this comment.
we have been using trim() inside the query search for all the pagination, that's the reason I was also following the same. let me know the way devs prefer @andrewsignori-aot @guru-aot @andrepestana-aot. open for suggestion
There was a problem hiding this comment.
I agree with @dheepak-aot. There is no need to append the query with the searchCriteria if it is a bunch of white spaces and line terminator characters.
There was a problem hiding this comment.
so, we will need a ticket to update all the pagination query
...ation-offering-change-request/application-offering-change-request.institutions.controller.ts
Outdated
Show resolved
Hide resolved
.../services/application-offering-change-request/application-offering-change-request.service.ts
Outdated
Show resolved
Hide resolved
...ation-offering-change-request/application-offering-change-request.institutions.controller.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/route-controllers/models/pagination.dto.ts
Show resolved
Hide resolved
dheepak-aot
left a comment
dheepak-aot
left a comment
There was a problem hiding this comment.
Thanks for making the changes @ann-aot 👍
andrepestana-aot
left a comment
andrepestana-aot
left a comment
There was a problem hiding this comment.
Thanks for the changes and explanations. Looks good to me!
| @@ -0,0 +1,35 @@ | |||
| import { PaginationOptions } from "../types"; | |||
There was a problem hiding this comment.
Please change to import from the index "@/types".
Same for the ApiClient.
|
Kudos, SonarCloud Quality Gate passed!
|
Backend Unit Tests Coverage Report
|
E2E Workflow Workers Coverage Report
|
andrewsignori-aot
left a comment
andrewsignori-aot
left a comment
There was a problem hiding this comment.
Thanks for doing the changes, its look good 👍
E2E SIMS API Coverage Report
|
E2E Queue Consumers Coverage Report
|
@leftjointo join theapplication.applicationOfferingChangeRequestandapplicationOfferingChangeRequestas there will always be one in-progress item for an application at a time.ApplicationStatusPaginationOptionsAPIInDTOused by theactive applicationas it was exactly the same. let me know your thoughts.application-offering-change-assessment-trigger-typesand to addapplication-offering-change-to-assessmentfor theStudentAssessmentstable.v-data table-serverduring the search. When we search for something on any other page other than the first one, there was an unexpected behavior - resolvedScreenshots
Next PRs