-
Notifications
You must be signed in to change notification settings - Fork 6
Add migration to create department id blacklist #3560
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
base: develop
Are you sure you want to change the base?
Conversation
4924f3f
to
d51a6e4
Compare
@Migration('2025-09-17T09:00:00') | ||
export class CreateUsedDeptIdListMigration extends BaseMigration { | ||
async up() { | ||
const filePath = new URL( |
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.
@CarsonF - What's the best way to pass this filePath
in instead of hardcoding it? I tried using the constructor, but then I'd have to use a useValue
in the provider in the module, or maybe have it as an env var or something. Is there a better way?
I probably wouldn't bother with a neo4j migration. It's not like each env has its own data that needs its own manipulation. and this is really only needed in prod. Also we don't want the full list from this file, since it has IDs of our own projects. We'll need to exclude those. |
I wasn't sure about the migration, so that's helpful. Now about the list, I wasn't thinking we had to account for Cord Ids because of Seth's comment yesterday:
I took that to mean that Cord Ids were already removed from that list. I'll follow-up in Slack on that. |
d51a6e4
to
38c4bdf
Compare
1301286
to
3881c54
Compare
3881c54
to
470a8ae
Compare
@CarsonF - I ran this locally and received the expected results:
Could you give this a look before we actually modify Prod? Also, I'm unclear on what to put as the value of (Also, also, we're not going to check this code in as a migration; this is just allowing easy testing and code review.) |
// Get blacklisted IDs | ||
.subQuery((sub) => | ||
sub | ||
.match(node('blacklist', 'BlacklistDepartmentId')) |
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'd prefer the label ExternalDepartmentId
to better represent the positive nature of work done externally, rather than coupling it to the need to exclude/"blacklist" these from our own generation.
) | ||
// Distill to available (excluding both used AND blacklisted) | ||
.with( | ||
'[id in enumerated where not id in used and not id in blacklisted][0] as next', |
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'd prefer to merge our own used and the externally used IDs into one list first, before this logic happens.
// Get used IDs
.subQuery((sub) => sub
.subQuery((sub2) => sub2
.match([
node('', 'Project'),
relation('out', '', 'departmentId', ACTIVE),
node('deptIdNode', 'Property'),
])
.where({ 'deptIdNode.value': not(isNull()) })
.return('deptIdNode.value as id')
.union()
.match(node('external', 'ExternalDepartmentId'))
.return('external.departmentId as id'))
.return(collect('id').as('used'))
)
src/components/project/migrations/create-used-dept-id-list.migration.ts
Outdated
Show resolved
Hide resolved
src/components/project/migrations/create-used-dept-id-list.migration.ts
Outdated
Show resolved
Hide resolved
src/components/project/migrations/create-used-dept-id-list.migration.ts
Outdated
Show resolved
Hide resolved
Thanks @CarsonF. @rdonigian, I'll let you make the changes in the handler and I'll make the changes in the migration script. |
470a8ae
to
a996c2c
Compare
Closes #3559.