-
Notifications
You must be signed in to change notification settings - Fork 3
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
[Client Profiles] HRM Migrations and save automation #469
Conversation
…given contact number (identifier)
hrm-domain/hrm-service/migrations/20230926214506-create-client-profiles-tables.js
Outdated
Show resolved
Hide resolved
hrm-domain/hrm-service/migrations/20230926214506-create-client-profiles-tables.js
Show resolved
Hide resolved
hrm-domain/hrm-service/migrations/20230926214506-create-client-profiles-tables.js
Show resolved
Hide resolved
hrm-domain/hrm-service/migrations/20230926214506-create-client-profiles-tables.js
Outdated
Show resolved
Hide resolved
hrm-domain/hrm-service/src/client-profile/sql/client-profile-insert-sql.ts
Outdated
Show resolved
Hide resolved
Is the intention for ProfileFlags to always be a predefined list managed from the HRM codebase, or is it the intention to have user definable flags in the future? I ask because if a lookup table is a predefined set of values that only changes if the application is updated, I prefer to key them on a human readable string 'code' rather than an integer or UUID, it can save lots of time debugging in the future! |
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, couple of Qs - thanks for the comprehensive service tests!
console.log("Table 'Identifiers' ownership altered."); | ||
await queryInterface.sequelize.query(` | ||
CREATE INDEX IF NOT EXISTS "Identifiers_identifier_accountSid" ON public."Identifiers" USING btree | ||
("identifier" ASC NULLS LAST, "accountSid" COLLATE pg_catalog."default" ASC NULLS LAST) |
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.
Do we want this table to permit duplicate identifiers for the same account or should we add a unique constraint to this index?
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.
accountSid: string, | ||
): Promise<TResult<{ identifier: Identifier; profile: Profile }>> => { | ||
try { | ||
if (!idx) { |
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.
Do we care about the get / create logic being atomic? At the moment, with no unique constraint being enforced there's a small chance we could duplicate identifiers here
@@ -267,6 +268,18 @@ export const createContact = async ( | |||
conversationMediaPayload, | |||
} = getNewContactPayload(newContact); | |||
|
|||
const profileResult = await getOrCreateProfileWithIdentifier(conn)( |
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.
Absolutely fine for now but I wonder in the future if profile creation should be tied to contact creation? We've separated transcript creation now, and in the future profiles might be auto-generated on the back end?
There is also the case of offline contacts, presumably those will have their identifier added manually by the counsellor after they are created, so will need to be linked up after the fact.
I know this is all out of scope for this PR and this totally makes sense for now, just thinking aloud
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.
The initial work is basically to get to feature parity with the current "previous contacts" banner, but using robust backend systems that will allow for expansion. Profile creation will definitely not be tied one to one like this with contact creation in the future... exactly what it will look like is a bit up in the air still. ;)
FOR identifier IN (SELECT * FROM "Identifiers") LOOP | ||
-- Create a "blank" Profile | ||
INSERT INTO "Profiles"("name", "accountSid", "createdAt", "updatedAt") | ||
VALUES (NULL, identifier."accountSid", current_timestamp, current_timestamp) |
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.
Would some sort of name not be preferable if only to make debugging easier? Or does your logic require these autogenerated profiles to have a null 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.
We haven't decided on how to populate the name field yet. It is null temporarily until those choices are made. We know it will be needed, but for now there are still discussions around what to populate it with.
DO $$ | ||
DECLARE tmp RECORD; | ||
BEGIN | ||
FOR tmp IN ( |
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.
The FROM
clause in the UPDATE
statement can be used to update tables with data from other tables (or the same table with a self join): https://www.postgresql.org/docs/current/sql-update.html
In this case, instead of using a loop you could do this update in a single query, something like:
UPDATE "Contacts" c SET "profileId" = tmp."profileId", "identifierId" = tmp."identifierId"
FROM "ProfilesToIdentifiers" p2i
LEFT JOIN "Identifiers" ids ON p2i."identifierId" = ids.id
WHERE c."number" = ids."identifier" AND c."accountSid" = ids."accountSid";
Obvs loops are fine for migrations, but it's good practice to try to avoid them anyway, so you know how to tackle similar issues in places where you should be avoiding SQL loops if at all possible (i.e. production code).
Don't change this code if you have it working, just FYI for the future
Description
This PR:
number
.number
is already saved.The tables structure is as in the following diagram.
![Screenshot from 2023-10-05 13-09-29](https://private-user-images.githubusercontent.com/15805319/272968882-b19b6493-6f0d-4614-b503-9a11071fe034.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU2NTc4NzksIm5iZiI6MTcyNTY1NzU3OSwicGF0aCI6Ii8xNTgwNTMxOS8yNzI5Njg4ODItYjE5YjY0OTMtNmYwZC00NjE0LWI1MDMtOWExMTA3MWZlMDM0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MDYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTA2VDIxMTkzOVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTQzOGVmNzcxZGM3M2E4MTZhYTBmNjYwOWQ4YWRiMTM1MzRiZmZkNmU4ZWZmZTBiMDRlMjA3NGMwZjdiODNhNjMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.ecZQt0UxfNVrpM-GG-zQac5Dco8l7kWU4_ok1G-xrl0)
For further details is refer to the below linked ticket.
Checklist
Verification steps
master
branch, and save a few "live" contacts and a few "offline contacts".Identifiers
andProfiles
tables are populated with the presentnumber
s in theContacts
table.Contacts
table is updated:profileId
andidentifierId
are populated ONLY for those records withnumber
(no offline contacts).