-
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
#1954 - Request an Offering Change - Submit/View UI (Part 1) #2105
#1954 - Request an Offering Change - Submit/View UI (Part 1) #2105
Conversation
.../services/application-offering-change-request/application-offering-change-request.service.ts
Show resolved
Hide resolved
...ation-offering-change-request/application-offering-change-request.institutions.controller.ts
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
.../services/application-offering-change-request/application-offering-change-request.service.ts
Outdated
Show resolved
Hide resolved
@Param("locationId", ParseIntPipe) locationId: number, | ||
@Body() payload: CreateApplicationOfferingChangeRequestAPIInDTO, | ||
): Promise<PrimaryIdentifierAPIOutDTO> { | ||
// TODO: Apply the same validations from PIR. |
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.
👍
...ation-offering-change-request/application-offering-change-request.institutions.controller.ts
Outdated
Show resolved
Hide resolved
@@ -185,7 +185,7 @@ export class EducationProgramOfferingAESTController extends BaseController { | |||
): Promise<EducationProgramOfferingAPIOutDTO> { | |||
const offering = await this.programOfferingService.getOfferingById( | |||
offeringId, | |||
true, | |||
{ isPrecedingOffering: true }, |
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.
👍
.../services/application-offering-change-request/application-offering-change-request.service.ts
Outdated
Show resolved
Hide resolved
.../services/application-offering-change-request/application-offering-change-request.service.ts
Show resolved
Hide resolved
...oute-controllers/education-program-offering/education-program-offering.controller.service.ts
Outdated
Show resolved
Hide resolved
...controllers/education-program-offering/education-program-offering.institutions.controller.ts
Outdated
Show resolved
Hide resolved
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.
Added some comments
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.
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.
About number 1, I can create a common style to be applied to the search inputs in general and have a max-width associated with it. Thoughts? I will check to have it done maybe in an upcoming PR.
For item 2 I found an issue where the payload has an id
property present it will replace the value. The fix was applied in the two tabs.
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.
Iam good with it @andrewsignori-aot or else, as discussed we can create a small story for the style change
.leftJoin( | ||
"application.applicationOfferingChangeRequest", | ||
"applicationOfferingChangeRequest", | ||
"applicationOfferingChangeRequest.applicationOfferingChangeRequestStatus IN (:...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.
I get the point we discussed. 👍
*/ | ||
async getOfferingById( | ||
offeringId: number, | ||
options?: { |
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 we please remove optional for options?. As the one reference that is using it use options.
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.
It is expected that the method will be consumed and also not provide the location id.
Please let me know if it would be considered a blocker.
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 its not a blocker, I saw there was only one reference so suggested the change.
sources/packages/web/src/services/ApplicationOfferingChangeRequestService.ts
Outdated
Show resolved
Hide resolved
@@ -14,7 +14,7 @@ import { User } from "./user.model"; | |||
import { Note } from "./note.model"; | |||
import { ApplicationOfferingChangeRequestStatus } from "./application-offering-change-request-status.type"; | |||
|
|||
const REASON_MAX_LENGTH = 500; | |||
export const REASON_MAX_LENGTH = 500; |
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 have a look at the comments. |
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.
Done with a round of review, nice work @andrewsignori-aot
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 doing the changes 👍 Good work with the component and the submit
.../services/application-offering-change-request/application-offering-change-request.service.ts
Outdated
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed!
|
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.
LGTM,nice work @andrewsignori-aot
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. 👍
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.
Nice work. LGTM 👍 @andrewsignori-aot
@Get(":applicationOfferingChangeRequestId")
getEligibleApplications
was reused to also retrieve the "change request"/"application details" to allow the submission. In this way, the same query constraints are used to load the summary list and also to get more details for the same item that will be submitted. New endpoint added as@Get("available/application/:applicationId")
.Request a new offering change from the available applications.
Required validations
Submitted application in read-only mode
Approved application with StudentAid BC notes
Fixes and UI refactors
detail-header
after the label. Instead of showinglabel : value
it will now showlabel: value
.detail-header
with a pipe ("|") as per Figma.body-header
#status-chip
that was being displayed in the second line.How the changed query looks like with the change.
TODO