-
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
#1981 - Request Offering Change: Ministry View [Approve / Deny Request] - Part 3 #2422
#1981 - Request Offering Change: Ministry View [Approve / Deny Request] - Part 3 #2422
Conversation
initialValues: { | ||
applicationOfferingChangeRequestStatus: | ||
ApplicationOfferingChangeRequestStatus.InProgressWithSABC, | ||
} as ApplicationOfferingChangeRequest, |
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's no need for the as ApplicationOfferingChangeRequest
here.
* @param auditUserId user that should be considered the one that is causing the changes. | ||
* @param entityManager optional entity manager to execute in transaction. | ||
*/ | ||
async saveApplicationOfferingChangeRequestCompletedByMinistry( |
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 name is not related to "create a notification". It sounds like it is saving the offering change request instead.
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.
Maybe not for this PR but the method name in line 161 also have this problem (missing "Notification" in the end of the name)
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.
@andrepestana-aot I believe that must be part of the PR.
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.
Fixed in both the places.
async saveApplicationOfferingChangeRequestCompletedByMinistry( | ||
notification: ApplicationOfferingChangeRequestCompletedByMinistryNotification, | ||
auditUserId: number, | ||
entityManager?: EntityManager, |
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.
In this method entityManager
should be mandatory.
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 agree. I also see that all the methods in this class have entityManager mandatory but it is not utilized in an optional way.
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.
Good job. Left some comments. Please take a look.
...n-offering-change-request.aest.controller.assessApplicationOfferingChangeRequest.e2e-spec.ts
Outdated
Show resolved
Hide resolved
.../services/application-offering-change-request/application-offering-change-request.service.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.
Nice work, just some minor comments.
...ackages/backend/libs/services/src/notifications/notification/notification-actions.service.ts
Outdated
Show resolved
Hide resolved
email_address: notification.toAddress, | ||
template_id: templateId, | ||
personalisation: { | ||
givenNames: notification.givenNames ?? "", |
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.
Not a part of this PR, we can have a common filler method in notification scope as this code is repeating for all methods.
...n-offering-change-request.aest.controller.assessApplicationOfferingChangeRequest.e2e-spec.ts
Outdated
Show resolved
Hide resolved
}, | ||
); | ||
const note = "Some dummy note."; | ||
const token = await getAESTToken(AESTGroups.BusinessAdministrators); |
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.
Just wondering should we need a test with non business administrator trying to approve/decline?
Nice work @sh16011993 . Please have a look at the comments. |
...n-offering-change-request.aest.controller.assessApplicationOfferingChangeRequest.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...n-offering-change-request.aest.controller.assessApplicationOfferingChangeRequest.e2e-spec.ts
Outdated
Show resolved
Hide resolved
const updatedApplicationOfferingChangeRequest = | ||
await db.applicationOfferingChangeRequest.findOne({ | ||
select: { | ||
id: 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.
looks like id here and in line 67 is not used , do we need it?
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.
IMU, this id
is required by the relations for doing the join. I have removed the id
from line 67.
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 sorry, why id is required for join? I am not following.
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.
Yes, I checked that and I had an error without selecting the id. I had a similar issue with another findOne
query.
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.
@ann-aot I saw the missing ID issue happening yesterday while @andrepestana-aot was sharing the screen but the queries that you provided prove that it can work.
It may be a not-so-evident error or something that we are missing in the entity model classes.
...n-offering-change-request.aest.controller.assessApplicationOfferingChangeRequest.e2e-spec.ts
Show resolved
Hide resolved
...n-offering-change-request.aest.controller.assessApplicationOfferingChangeRequest.e2e-spec.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.
Looks good 👍
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 👍 added a reply for a comment #2422 (comment), if there is an error, we might need to create a ticket to revisit/validate those queries in our code base.
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 👍
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.
Thanks for doing the changes, looks good 👍
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! Thanks for the changes!
As a part of this PR, the following tasks were completed:
Screenshot:
ApplicationOfferingChangeRequestAESTController(e2e)-assessApplicationOfferingChangeRequest
√ Should assess the application offering change request for the provided application offering change request id.
√ Should not create a new assessment when the application offering change request has been declined by the ministry user.
√ Should throw a HttpStatus Not Found (404) error when an application offering change is not in a valid status to be assessed.
√ Should throw a HttpStatus Bad Request (400) error when an invalid application offering change request status is provided.
√ Should throw a HttpStatus Forbidden (403) error when an unauthorized AEST user tries to assess the application offering change request.