Skip to content
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

#3815 - Identification of changed information during edit #4221

Merged
merged 26 commits into from
Jan 13, 2025
Merged
Changes from 16 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
3ad2b33
Detecting changes and testing UI
andrewsignori-aot Jan 8, 2025
ece7202
Changes detection working close to expected
andrewsignori-aot Jan 8, 2025
45d09ac
Adjusting read-only
andrewsignori-aot Jan 8, 2025
d969f2e
Adjusted comparison for read-only
andrewsignori-aot Jan 8, 2025
ddc7a3d
Code refactor for PR preparation
andrewsignori-aot Jan 9, 2025
5012f57
Enabled debug for unit tests
andrewsignori-aot Jan 9, 2025
20f09e9
Added unit tests and DTO conversion
andrewsignori-aot Jan 9, 2025
d7ec513
E2E tests
andrewsignori-aot Jan 9, 2025
168f042
Sonarcloud issues
andrewsignori-aot Jan 9, 2025
5c39f11
Reused method from another open PR
andrewsignori-aot Jan 9, 2025
ea1c3e8
Allow changes array to be null
andrewsignori-aot Jan 9, 2025
1648afd
Removed empty arrays from API result
andrewsignori-aot Jan 10, 2025
094f568
Minor unit test renaming
andrewsignori-aot Jan 10, 2025
69a87f3
Fix E2E test result
andrewsignori-aot Jan 10, 2025
19be56b
Pre-review adjustments
andrewsignori-aot Jan 10, 2025
4356d2d
Fix highlight logic
andrewsignori-aot Jan 10, 2025
32e42a4
Changed behavior for child list items
andrewsignori-aot Jan 11, 2025
40ac7c9
Added void
andrewsignori-aot Jan 13, 2025
015951c
Merge branch 'main' into feature/#3815-detect-application-changes
andrewsignori-aot Jan 13, 2025
87cc4dd
Post merge fixes and sonarcloud issue
andrewsignori-aot Jan 13, 2025
7c56184
Data comparison improvement
andrewsignori-aot Jan 13, 2025
69dad6b
Unit test for itemsAppended and PR comment fix
andrewsignori-aot Jan 13, 2025
524fe44
E2E fix
andrewsignori-aot Jan 13, 2025
a88feb5
PR review fixes
andrewsignori-aot Jan 13, 2025
4039fb6
PR review comment
andrewsignori-aot Jan 13, 2025
a9f371a
CSS improvement as Dheepak's suggestion
andrewsignori-aot Jan 13, 2025
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
19 changes: 19 additions & 0 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
@@ -241,6 +241,25 @@
"ENVIRONMENT": "test",
"TZ": "UTC"
}
},
{
"type": "node",
"request": "launch",
"name": "Unit tests - Current test file",
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

"program": "${workspaceFolder}/sources/packages/backend/node_modules/.bin/jest",
"args": [
"${fileBasenameNoExtension}",
"--runInBand",
"--config",
"./sources/packages/backend/jest-unit-tests.json",
"--forceExit"
],
"console": "integratedTerminal",
"internalConsoleOptions": "neverOpen",
"disableOptimisticBPs": true,
"windows": {
"program": "${workspaceFolder}/sources/packages/backend/node_modules/jest/bin/jest"
}
}
]
}
Original file line number Diff line number Diff line change
@@ -11,7 +11,12 @@ import {
E2EDataSources,
saveFakeApplication,
} from "@sims/test-utils";
import { EducationProgramOffering, OfferingIntensity } from "@sims/sims-db";
import {
ApplicationData,
ApplicationStatus,
EducationProgramOffering,
OfferingIntensity,
} from "@sims/sims-db";
import { getUserFullName } from "../../../utilities";
import { addDays, getISODateOnlyString } from "@sims/utilities";

@@ -110,6 +115,90 @@ describe("ApplicationAESTController(e2e)-getApplicationDetails", () => {
});
});

it("Should get the student application changes when the application has a previous version and its dynamic data was changed.", async () => {
// Arrange
const previousApplication = await saveFakeApplication(
db.dataSource,
{},
{
applicationStatus: ApplicationStatus.Overwritten,
applicationData: {
studystartDate: "2000-01-01",
studyendDate: "2000-01-31",
selectedOffering: 1,
studentNumber: "1234567",
courseDetails: [
{
courseName: "courseName",
courseCode: "courseCode",
},
],
} as ApplicationData,
},
);
const currentApplication = await saveFakeApplication(
db.dataSource,
{},
{
applicationStatus: ApplicationStatus.Completed,
applicationNumber: previousApplication.applicationNumber,
applicationData: {
studystartDate: "2000-12-01",
studyendDate: "2000-12-31",
selectedOffering: 4,
courseDetails: [
{
courseName: "courseName updated",
courseCode: "courseCode",
},
],
} as ApplicationData,
},
);
const token = await getAESTToken(AESTGroups.BusinessAdministrators);
const endpoint = `/aest/application/${currentApplication.id}`;

// Act/Assert
const response = await request(app.getHttpServer())
.get(endpoint)
.auth(token, BEARER_AUTH_TYPE)
.expect(HttpStatus.OK);
expect(response.body.changes).toStrictEqual([
{
changeType: "updated",
key: "studyendDate",
},
{
changeType: "updated",
changes: [
{
changeType: "updated",
changes: [
{
changeType: "updated",
key: "courseName",
},
],
index: 0,
},
],
key: "courseDetails",
},
{
changeType: "updated",
key: "studystartDate",
},
{
changeType: "updated",
key: "selectedOffering",
},
{
changeType: "updated",
key: "selectedOfferingName",
},
]);
});

afterAll(async () => {
await app?.close();
});
Original file line number Diff line number Diff line change
@@ -19,6 +19,7 @@ import {
CompletedApplicationDetailsAPIOutDTO,
EnrolmentApplicationDetailsAPIOutDTO,
InProgressApplicationDetailsAPIOutDTO,
ApplicationFormData,
} from "./models/application.dto";
import {
AllowAuthorizedParty,
@@ -81,16 +82,35 @@ export class ApplicationAESTController extends BaseController {
`Application id ${applicationId} was not found.`,
);
}

let currentReadOnlyData: ApplicationFormData;
let previousReadOnlyData: ApplicationFormData;
if (loadDynamicData) {
application.data =
await this.applicationControllerService.generateApplicationFormData(
const currentReadOnlyDataPromise =
this.applicationControllerService.generateApplicationFormData(
application.data,
);
// Check if a previous application exists.
const [previousApplicationVersion] =
await this.applicationService.getPreviousApplicationVersions(
applicationId,
{ loadDynamicData: true, limit: 1 },
);
Comment on lines +90 to +94
Copy link
Collaborator

@dheepak-aot dheepak-aot Jan 10, 2025

Choose a reason for hiding this comment

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

Minor. Can this part which is blocking can be executed first, so that then next 2 request which are parallel can happen after this. ? Not a blocker.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure if I am following, to be clear, generateApplicationFormData happens first and does not block the getPreviousApplicationVersions to happen, hence both are already concurrent.
Promise.all will then raise one more query and ensure both promises are resolved. What would be the benefit of the proposed change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It may be my feeling but i felt little more convenient to see both parallel requests one after the other instead of one awaiting request in the middle that's all. As I mentioned earlier it is not a blocker as there is no difference overall from execution perspective.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will make the change believing that it will be easier to understand in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Code is updated.

// If there is a previous application, generate its read-only data.
const previousReadOnlyDataPromise =
previousApplicationVersion &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

this.applicationControllerService.generateApplicationFormData(
previousApplicationVersion.data,
);
// Wait for both promises to resolve.
[currentReadOnlyData, previousReadOnlyData] = await Promise.all([
currentReadOnlyDataPromise,
previousReadOnlyDataPromise,
]);
application.data = currentReadOnlyData;
}

return this.applicationControllerService.transformToApplicationDTO(
application,
{ previousData: previousReadOnlyData },
);
}

Original file line number Diff line number Diff line change
@@ -26,6 +26,7 @@ import {
ApplicationProgressDetailsAPIOutDTO,
CompletedApplicationDetailsAPIOutDTO,
InProgressApplicationDetailsAPIOutDTO,
ApplicationDataChangeAPIOutDTO,
} from "./models/application.dto";
import {
credentialTypeToDisplay,
@@ -35,7 +36,11 @@ import {
getPIRDeniedReason,
getUserFullName,
} from "../../utilities";
import { getDateOnlyFormat } from "@sims/utilities";
import {
ApplicationDataChange,
getDateOnlyFormat,
compareApplicationData,
} from "@sims/utilities";
import {
Application,
ApplicationData,
@@ -440,12 +445,26 @@ export class ApplicationControllerService {

/**
* Transformation util for Application.
* @param application
* @returns Application DTO
* @param application application to be converted to the DTO.
* @param options additional options.
* - `previousData` previous application to allow changes detection.
* @returns application DTO.
*/
async transformToApplicationDTO(
application: Application,
options?: { previousData?: unknown },
): Promise<ApplicationSupplementalDataAPIOutDTO> {
let changes: ApplicationDataChangeAPIOutDTO[];
if (options?.previousData) {
const applicationDataChanges = compareApplicationData(
application.data,
options.previousData,
);
if (applicationDataChanges.length) {
changes = [];
this.transformToApplicationChangesDTO(applicationDataChanges, changes);
}
}
return {
data: application.data,
id: application.id,
@@ -461,9 +480,41 @@ export class ApplicationControllerService {
applicationEndDate: application.currentAssessment?.offering?.studyEndDate,
applicationInstitutionName:
application.location?.institution.legalOperatingName,
changes,
};
}

/**
* Recursively converts the {@link ApplicationDataChange} service model to the
* DTO model {@link ApplicationDataChangeAPIOutDTO} which will ensure
* that only required properties will be returned from the API also preventing
* that future changes in the service model will not be directly returned.
* @param applicationDataChanges service model application changes.
* @param applicationDataChangeAPIOutDTO converted API DTO model.
*/
transformToApplicationChangesDTO(
applicationDataChanges: ApplicationDataChange[],
applicationDataChangeAPIOutDTO: ApplicationDataChangeAPIOutDTO[],
) {
applicationDataChanges.forEach((dataChange) => {
const dataChangeDTO: ApplicationDataChangeAPIOutDTO = {
key: dataChange.key,
index: dataChange.index,
changeType: dataChange.changeType,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion:

add changes to the const dataChangeDTO:

changes: dataChange.changes.length ? [] : undefined

and do the below changes,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not able to see the benefit. Would it be to improve the code readability or something else?
Unless there is a stronger reason I am keeping the current way.

};
applicationDataChangeAPIOutDTO.push(dataChangeDTO);
// Check if there are nested changes
if (!dataChange.changes.length) {
return;
}
dataChangeDTO.changes = [];
this.transformToApplicationChangesDTO(
dataChange.changes,
dataChangeDTO.changes,
);
});
}

/**
* Convert disbursements into the enrolment DTO with information
* about first and second disbursements.
Original file line number Diff line number Diff line change
@@ -19,6 +19,7 @@ import {
import { JsonMaxSize } from "../../../utilities/class-validation";
import { JSON_20KB } from "../../../constants";
import { ECertFailedValidation } from "@sims/integrations/services/disbursement-schedule/disbursement-schedule.models";
import { ChangeTypes } from "@sims/utilities";

export class SaveApplicationAPIInDTO {
/**
@@ -113,12 +114,20 @@ export class ApplicationDataAPIOutDTO extends ApplicationBaseAPIOutDTO {
submittedDate?: Date;
}

export class ApplicationDataChangeAPIOutDTO {
key?: string;
index?: number;
changeType: ChangeTypes;
changes?: ApplicationDataChangeAPIOutDTO[];
}

export class ApplicationSupplementalDataAPIOutDTO extends ApplicationBaseAPIOutDTO {
studentFullName: string;
applicationOfferingIntensity?: OfferingIntensity;
applicationStartDate?: string;
applicationEndDate?: string;
applicationInstitutionName?: string;
changes?: ApplicationDataChangeAPIOutDTO[];
}

export class ApplicationWithProgramYearAPIOutDTO {
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
import { Injectable } from "@nestjs/common";
import { DataSource, In, Not, Brackets, EntityManager } from "typeorm";
import {
DataSource,
In,
Not,
Brackets,
EntityManager,
LessThan,
} from "typeorm";
import { LoggerService, InjectLogger } from "@sims/utilities/logger";
import {
RecordDataModelService,
@@ -1837,6 +1844,39 @@ export class ApplicationService extends RecordDataModelService<Application> {
});
}

/**
* Get previous application versions for an application.
* @param applicationId application id.
* @param options method options.
* - `limit` number of previous application versions to be returned.
* - `loadDynamicData` optionally loads the dynamic data.
* @returns previous application versions.
*/
async getPreviousApplicationVersions(
applicationId: number,
options?: { loadDynamicData?: boolean; limit?: number },
): Promise<Application[]> {
const application = await this.repo.findOne({
select: { applicationNumber: true, submittedDate: true },
where: { id: applicationId },
});
return this.repo.find({
select: {
id: true,
submittedDate: true,
data: !!options?.loadDynamicData as unknown,
},
where: {
submittedDate: LessThan(application.submittedDate),
applicationNumber: application.applicationNumber,
},
order: {
submittedDate: "DESC",
},
take: options?.limit,
});
}

@InjectLogger()
logger: LoggerService;
}
Loading