Add AFS e2e tests to cover update scenario #1810

pbn4 · 2021-09-09T15:27:42Z

Pull Request Template

Issue

Addresses # (#1367)

This change addresses the issue in full
This change addresses only certain aspects of the issue
This change is a dependency for another issue
This change has a dependency from another issue

Description

This change makes application flagged sets module take applications edits into account (e.g. a leasing agent changes something in the application).

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Prototype/POC (not to merge)
This change is a refactor/address technical debt
This change requires a documentation update
This change requires a SQL Script

How Can This Be Tested/Reviewed?

Please describe the tests that you ran to verify your changes. Provide instructions so we can review. Please also list any relevant details for your test configuration

Desktop View
Mobile View
Test A
Test B

Checklist:

netlify · 2021-09-09T15:27:48Z

✔️ Deploy Preview for dev-partners-bloom ready!

🔨 Explore the source changes: 1007ffc

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-partners-bloom/deploys/61573cb2ac9a9f0008b9831d

😎 Browse the preview: https://deploy-preview-1810--dev-partners-bloom.netlify.app

netlify · 2021-09-09T15:27:49Z

✔️ Deploy Preview for dev-storybook-bloom ready!

🔨 Explore the source changes: 1007ffc

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-storybook-bloom/deploys/61573cb2dd59d400071d2ada

😎 Browse the preview: https://deploy-preview-1810--dev-storybook-bloom.netlify.app

netlify · 2021-09-09T15:27:50Z

✔️ Deploy Preview for dev-bloom ready!

🔨 Explore the source changes: 1007ffc

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-bloom/deploys/61573cb2b3dfc200074b951a

😎 Browse the preview: https://deploy-preview-1810--dev-bloom.netlify.app

…s closed

seanmalbert

@pbn4 ,
Please see my questions around onApplicationUpdate.

seanmalbert · 2021-09-22T17:56:31Z

backend/core/src/application-flagged-sets/application-flagged-sets.service.ts

+    afses = afses.filter(
+      (afs) => afs.applications.findIndex((app) => app.id === newApplication.id) !== -1
+    )


Can't we filter this as part of the query, instead of getting them all back and filtering?

Also, do we really need to select all of applications and listing to get what we need for the below operations?

I tried doing this but with the following query builder instructions it does not work:

const qb = transAfsRepository .createQueryBuilder("afs") .leftJoinAndSelect("afs.applications", "applications") .where("afs. = :applicationId", { applicationId: newApplication.id })

because the SQL query result contains only an AFS with single (matching id) application. The resulting query looks like this:

"SELECT "afs"."id" AS "afs_id", "afs"."created_at" AS "afs_created_at", "afs"."updated_at" AS "afs_updated_at", "afs"."RULE" AS "afs_rule", "afs"."resolved_time" AS "afs_resolved_time", "afs"."status" AS "afs_status", "afs"."listing_id" AS "afs_listing_id", "afs"."resolving_user_id" AS "afs_resolving_user_id", "applications"."id" AS "applications_id", "applications"."created_at" AS "applications_created_at", "applications"."updated_at" AS "applications_updated_at", "applications"."deleted_at" AS "applications_deleted_at", "applications"."app_url" AS "applications_app_url", "applications"."additional_phone" AS "applications_additional_phone", "applications"."additional_phone_number" AS "applications_additional_phone_number", "applications"."additional_phone_number_type" AS "applications_additional_phone_number_type", "applications"."contact_preferences" AS "applications_contact_preferences", "applications"."household_size" AS "applications_household_size", "applications"."housing_status" AS "applications_housing_status", "applications"."send_mail_to_mailing_address" AS "applications_send_mail_to_mailing_address", "applications"."income_vouchers" AS "applications_income_vouchers", "applications"."income" AS "applications_income", "applications"."income_period" AS "applications_income_period", "applications"."preferences" AS "applications_preferences", "applications"."status" AS "applications_status", "applications"."LANGUAGE" AS "applications_language", "applications"."submission_type" AS "applications_submission_type", "applications"."accepted_terms" AS "applications_accepted_terms", "applications"."submission_date" AS "applications_submission_date", "applications"."marked_as_duplicate" AS "applications_marked_as_duplicate", "applications"."user_id" AS "applications_user_id", "applications"."listing_id" AS "applications_listing_id", "applications"."applicant_id" AS "applications_applicant_id", "applications"."mailing_address_id" AS "applications_mailing_address_id", "applications"."alternate_address_id" AS "applications_alternate_address_id", "applications"."alternate_contact_id" AS "applications_alternate_contact_id", "applications"."accessibility_id" AS "applications_accessibility_id", "applications"."demographics_id" AS "applications_demographics_id" FROM "application_flagged_set" "afs" LEFT JOIN "application_flagged_set_applications_applications" "afs_applications" ON "afs_applications"."application_flagged_set_id"="afs"."id" LEFT JOIN "applications" "applications" ON "applications"."id"="afs_applications"."applications_id" AND ("applications"."deleted_at" IS NULL) WHERE "applications"."id" = $1"

seanmalbert · 2021-09-22T18:04:52Z

backend/core/src/application-flagged-sets/application-flagged-sets.service.ts

+      afs.applications.splice(applicationIndex, 1)
+      if (afs.applications.length > 1) {
+        afsesToBeSaved.push(afs)
+      } else {
+        afsesToBeRemoved.push(afs)
+      }
+    }


We're removing newApplication from the flagged and if there's one still in the flagged set, we're removing that one from the set, leaving only newApplication in the flagged set. Is that what we want?

This part of the code is reverting any AFS logic for given application id (as if it was never added to any AFS) and it's also removing logically incorrect AFSes. Once AFS containing application with given appId is found, the application is removed from it and since AFS with just 1 application left does not make sense, it's removed too. It will probably be recreated again by onApplicationSave logic but I just wanted it to be trivial and reuse what already works.

…s closed

…om:bloom-housing/bloom into 1367/update_application_duplicates_on_edit

…application_duplicates_on_edit

seanmalbert

@pbn4 ,
This looks good, but please see my comment on the query before merging. If I'm mistaken go ahead and merge, but if I'm not, please update before merging.

seanmalbert · 2021-09-28T23:45:20Z

backend/core/src/application-flagged-sets/application-flagged-sets.service.ts

+    const query = `
+      SELECT DISTINCT afs.id FROM ${afsMetadata.tableName} afs 
+      INNER JOIN ( 
+          SELECT DISTINCT application_flagged_set_id, applications_id FROM ${applicationsJunctionTableName} 
+          WHERE ${applicationsJunctionTableName}.applications_id = $1
+      ) matching_afs ON afs.id = matching_afs.application_flagged_set_id
+      LEFT JOIN ${applicationsJunctionTableName} ON matching_afs.application_flagged_set_id = ${applicationsJunctionTableName}.application_flagged_set_id
+      LEFT JOIN ${applicationsMetadata.tableName} applications ON applications.id = ${applicationsJunctionTableName}.applications_id


@pbn4 ,
I'm probably missing somethings but does the query here return the same as querying applicationsJunctionTableName directly?

If I take what's here and compare to the below query, they appear to return the same result.

select distinct application_flagged_set_id from ${applicationsJunctionTableName} where applications_id = $1

Good point, simplified. Thanks

* Add AFS e2e tests to cover update scenario * Update CHANGELOG.md * Add constraint that leasing agent can only resolve AFS when listing is closed * Make application updates trigger AFS recalculations * Add AFS e2e tests to cover update scenario * Update CHANGELOG.md * Add constraint that leasing agent can only resolve AFS when listing is closed * Make application updates trigger AFS recalculations * Improve query efficiency in onApplicationUpdate AFS module * refactor(backend): simplify AFS filtering by applicationId query Co-authored-by: Sean Albert <[email protected]>

pbn4 requested a review from seanmalbert September 9, 2021 15:27

pbn4 added ready for review and removed ready for review labels Sep 9, 2021

pbn4 removed the request for review from seanmalbert September 9, 2021 15:52

pbn4 added the wip This PR is not ready for review, do not review it's a “Work In Progress” label Sep 9, 2021

pbn4 added 3 commits September 14, 2021 16:13

Add AFS e2e tests to cover update scenario

019133e

Update CHANGELOG.md

6bf0a41

Add constraint that leasing agent can only resolve AFS when listing i…

fd7daca

…s closed

pbn4 force-pushed the 1367/update_application_duplicates_on_edit branch from 9117bb4 to feeaba7 Compare September 15, 2021 13:41

Make application updates trigger AFS recalculations

8d6ac3d

pbn4 force-pushed the 1367/update_application_duplicates_on_edit branch from f0798c9 to 8d6ac3d Compare September 16, 2021 14:43

pbn4 requested a review from seanmalbert September 16, 2021 14:43

pbn4 added ready for review and removed wip This PR is not ready for review, do not review it's a “Work In Progress” labels Sep 16, 2021

merge upstream

46fab10

seanmalbert reviewed Sep 22, 2021

View reviewed changes

pbn4 and others added 7 commits September 23, 2021 13:30

Add AFS e2e tests to cover update scenario

06dafc6

Update CHANGELOG.md

ed7e4ca

Add constraint that leasing agent can only resolve AFS when listing i…

b98daa9

…s closed

Make application updates trigger AFS recalculations

eaa959a

Merge branch '1367/update_application_duplicates_on_edit' of github.c…

9bcd1ab

…om:bloom-housing/bloom into 1367/update_application_duplicates_on_edit

Improve query efficiency in onApplicationUpdate AFS module

87d4ff1

Merge remote-tracking branch 'remotes/upstream/dev' into 1367/update_…

afbc79a

…application_duplicates_on_edit

seanmalbert approved these changes Sep 28, 2021

View reviewed changes

pbn4 added 2 commits September 29, 2021 16:55

refactor(backend): simplify AFS filtering by applicationId query

38ecb81

feat(backend): make AFS module take applications updates into account

1007ffc

seanmalbert merged commit e67582a into dev Oct 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AFS e2e tests to cover update scenario #1810

Add AFS e2e tests to cover update scenario #1810

pbn4 commented Sep 9, 2021 •

edited

Loading

netlify bot commented Sep 9, 2021 •

edited

Loading

netlify bot commented Sep 9, 2021 •

edited

Loading

netlify bot commented Sep 9, 2021 •

edited

Loading

seanmalbert left a comment

seanmalbert Sep 22, 2021

seanmalbert Sep 22, 2021

pbn4 Sep 23, 2021

seanmalbert Sep 22, 2021

pbn4 Sep 23, 2021

seanmalbert left a comment

seanmalbert Sep 28, 2021

pbn4 Sep 29, 2021

Add AFS e2e tests to cover update scenario #1810

Add AFS e2e tests to cover update scenario #1810

Conversation

pbn4 commented Sep 9, 2021 • edited Loading

Pull Request Template

Issue

Description

Type of change

How Can This Be Tested/Reviewed?

Checklist:

netlify bot commented Sep 9, 2021 • edited Loading

netlify bot commented Sep 9, 2021 • edited Loading

netlify bot commented Sep 9, 2021 • edited Loading

seanmalbert left a comment

Choose a reason for hiding this comment

seanmalbert Sep 22, 2021

Choose a reason for hiding this comment

seanmalbert Sep 22, 2021

Choose a reason for hiding this comment

pbn4 Sep 23, 2021

Choose a reason for hiding this comment

seanmalbert Sep 22, 2021

Choose a reason for hiding this comment

pbn4 Sep 23, 2021

Choose a reason for hiding this comment

seanmalbert left a comment

Choose a reason for hiding this comment

seanmalbert Sep 28, 2021

Choose a reason for hiding this comment

pbn4 Sep 29, 2021

Choose a reason for hiding this comment

pbn4 commented Sep 9, 2021 •

edited

Loading

netlify bot commented Sep 9, 2021 •

edited

Loading

netlify bot commented Sep 9, 2021 •

edited

Loading

netlify bot commented Sep 9, 2021 •

edited

Loading