#2001 - Changes to handle PD PPD Part 2 #2121

dheepak-aot · 2023-07-20T05:41:37Z

Handle PD PPD requests Part 2

Renamed PD status to disability in all possible occurrence
Updated ATBC service and ATBC Response processing service as per the new ATBC response API
As of now we are not sure on how to read the declined status and hence we are not updating the declined status for student who applied for disability. Updated the same to the ticket.
Created E2E tests.

After cleaning up the local database with data seed cleanup script, migrations fail to run due to pg_trgm package already exists. Hence in data seed cleanup added a script to remove the package during delete database.

guru-aot · 2023-07-20T21:11:15Z

.../schedulers/atbc-respone-integration/_tests_/atbc-response-integration.scheduler.e2e-spec.ts

+    expect(ppdReceivedStudent.disabilityStatus).toBe(DisabilityStatus.PPD);
+    expect(
+      formatDate(ppdReceivedStudent.studentPDUpdateAt, ATBC_DATE_FORMAT),
+    ).toBe(ppdResponse.D8Y_DTE);


Nicely done 👍

guru-aot · 2023-07-20T21:16:00Z

sources/packages/backend/apps/test-db-seeding/src/clean-db/clean-db.ts

@@ -8,5 +8,6 @@ export class CleanDatabase {
  async cleanDatabase(): Promise<void> {
    // Drops the database and all its data.It will erase all your database tables and their data.
    await this.dataSource.dropDatabase();
+    await this.dataSource.query("DROP EXTENSION pg_trgm");


Do we need to have something like this below ? to avoid errors if the extension is not exists?
await dataSource.query("DROP EXTENSION IF EXISTS pg_trgm");

I get your point @guru-aot . But we devs recently decided to stop using the IF EXISTS and ID NOT EXISTS to be aware of the side effects and handle them. That was the reason why I avoided.

I get that @dheepak-aot, I am not going to stress on this but for this exceptional scenario, I was validating if we need it or not

There was a decision to stop using the IF EXISTS for migrations because it is a controlled script execution where the up/down must be precise and if it is changing something the changed item (column/index/whatever) should be there.
For this situation, the same may not be applied, for instance, if executing the clean database twice will make the second execution fail.

.../schedulers/atbc-respone-integration/_tests_/atbc-response-integration.scheduler.e2e-spec.ts

guru-aot · 2023-07-20T21:23:50Z

...ces/packages/backend/libs/integrations/src/atbc-integration/models/atbc-integration.model.ts

+  lastName: string;
+  birthDate: Date;
+  disabilityStatus: DisabilityStatus;
+  disabilityStatusUpdatedDate: Date;


Asking this again for clarification, the disabilityStatusUpdatedDate is an replacement of both created and updated date right?

No. The disabilityStatusUpdatedDate is the property which is intended to update the pd_date_update eventually.
Both created and updated date still exist and we are not removing them now as a part of the ticket.

.../schedulers/atbc-respone-integration/_tests_/atbc-response-integration.scheduler.e2e-spec.ts

ann-aot · 2023-07-20T21:28:55Z

sources/packages/backend/apps/test-db-seeding/src/clean-db/clean-db.ts

@@ -8,5 +8,6 @@ export class CleanDatabase {
  async cleanDatabase(): Promise<void> {
    // Drops the database and all its data.It will erase all your database tables and their data.
    await this.dataSource.dropDatabase();
+    await this.dataSource.query("DROP EXTENSION pg_trgm");


sources/packages/backend/libs/integrations/src/services/atbc/atbc.service.ts

guru-aot · 2023-07-20T21:31:43Z

...ckages/backend/libs/integrations/src/atbc-integration/atbc-integration.processing.service.ts

+      }));
+    let updatedDisabilityStatusCount = 0;
+    this.logger.log(
+      `Processing disability status requests for ${studentsToUpdate.length} students`,


As a suggestion, not a show stopper or part of your PR, to handle, both the variety of logs generated below:

Processing disability status requests for 1 student

Processing disability status requests for 2 students

It would be better to have the student(s) in the logger.

sources/packages/backend/libs/integrations/src/services/atbc/atbc.service.ts

guru-aot · 2023-07-20T21:36:39Z

sources/packages/backend/libs/integrations/src/services/atbc/models/atbc.model.ts

-  id: number;
-  sin: string;
-}
+export const ATBC_DATE_FORMAT = "YYYY/MM/DD";


Can you please explain, why we are not having the ATBC_DATE_FORMAT const in the backend/libs/utilities/src/date-utils.ts? Is there a reason, that it is created here?

Because we never store the integration/external application specific date formats in date-utils.

for e.g. ECE integration model

...ckages/backend/libs/integrations/src/atbc-integration/atbc-integration.processing.service.ts

guru-aot · 2023-07-20T21:42:04Z

sources/packages/backend/libs/integrations/src/services/student/student.service.ts

+   * @param birthDate birth date.
+   * @returns student.
+   */
+  async getStudentBySINAndLastNameAndBirthDate(


IMO just a suggestion, the method name can be called as findStudentByPersonalInfo or getStudentByPersonalInfo. seems to be a big method name with 2 ANDs

guru-aot · 2023-07-20T21:44:33Z

sources/packages/backend/libs/test-utils/src/factories/student.ts

-export function createFakeStudent(user?: User): Student {
+export function createFakeStudent(
+  user?: User,
+  options?: { initialValue: Partial<Student> },


great to see the methods createFakeStudent and saveFakeStudent are enhancing like anything with the functionalities we create.

guru-aot

Done with review, very minor comments and doubts. Please have a look.

sources/packages/backend/libs/integrations/src/services/student/student.service.ts

...ckages/backend/libs/integrations/src/atbc-integration/atbc-integration.processing.service.ts

...ces/packages/backend/libs/integrations/src/atbc-integration/models/atbc-integration.model.ts

sources/packages/backend/libs/integrations/src/services/atbc/atbc.service.ts

ann-aot

Good work @dheepak-aot. added some comments

sh16011993 · 2023-07-20T22:30:36Z

...rs/src/processors/schedulers/atbc-respone-integration/atbc-response-integration.scheduler.ts

@@ -37,17 +36,11 @@ export class ATBCResponseIntegrationScheduler extends BaseScheduler<void> {
      appLogger: this.logger,
      jobLogger: job,
    });
-    await summary.info("Processing PD status for students.");
+    await summary.info("Processing disability status for students.");


sonarqubecloud · 2023-07-20T23:59:05Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

ann-aot

Thanks for doing the changes @dheepak-aot 👍 Looks good to me

github-actions · 2023-07-21T00:00:16Z

Backend Unit Tests Coverage Report

Totals
Statements:	17.87% ( 2134 / 11940 )
Methods:	8.29% ( 126 / 1520 )
Lines:	20.67% ( 1870 / 9047 )
Branches:	10.05% ( 138 / 1373 )

github-actions · 2023-07-21T00:01:12Z

E2E Workflow Workers Coverage Report

Totals
Statements:	49.81% ( 267 / 536 )
Methods:	41.56% ( 32 / 77 )
Lines:	55.33% ( 218 / 394 )
Branches:	26.15% ( 17 / 65 )

github-actions · 2023-07-21T00:01:13Z

E2E Queue Consumers Coverage Report

Totals
Statements:	72.5% ( 406 / 560 )
Methods:	63.38% ( 45 / 71 )
Lines:	74.53% ( 357 / 479 )
Branches:	40% ( 4 / 10 )

github-actions · 2023-07-21T00:07:22Z

E2E SIMS API Coverage Report

Totals
Statements:	52.23% ( 3716 / 7115 )
Methods:	48.59% ( 448 / 922 )
Lines:	57.36% ( 3045 / 5309 )
Branches:	25.23% ( 223 / 884 )

guru-aot

LGTM, nice work @dheepak-aot

dheepak-aot added 5 commits July 18, 2023 15:14

initial commit

02f586e

atbc response parsing

6c07a0b

atbc response parsing

3e9e210

atbc refactor

a424eab

E2E tests

c68a735

dheepak-aot added Queue Consumers E2E/Unit tests labels Jul 20, 2023

dheepak-aot self-assigned this Jul 20, 2023

dheepak-aot marked this pull request as ready for review July 20, 2023 17:32

dheepak-aot requested review from ann-aot, guru-aot, JasonCTang and sh16011993 July 20, 2023 17:33

guru-aot reviewed Jul 20, 2023

View reviewed changes