feat: dynamic required fields for listings #4942
Conversation
…-fields-implementation
… fields - Updated ListingService - Enhanced ListingCreateUpdateValidationPipe - Adjusted integration tests to reflect changes in DTOs and validation logic
…ings based on jurisdiction
…th random names in tests
✅ Deploy Preview for bloom-public-seeds ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for partners-bloom-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for bloom-exygy-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for partners-bloom-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
api/prisma/migrations/20250304175121_add_required_listing_fields/migration.sql
Outdated
Show resolved
Hide resolved
api/prisma/migrations/20250304175121_add_required_listing_fields/migration.sql
Outdated
Show resolved
Hide resolved
| [ListingsStatusEnum.changesRequested]: ListingCreate, | ||
| }; | ||
| // Default required fields if jurisdiction doesn't specify any | ||
| private defaultRequiredFields = [ |
There was a problem hiding this comment.
issue: these aren't the default fields we would want. I can compile the list that we should have and report back
There was a problem hiding this comment.
@ludtkemorgan of course - if you can provide valid list i will replace it
There was a problem hiding this comment.
I think this is the list that I'd recommend for the default:
'listingsBuildingAddress',
'name',
'developer',
'listingImages',
'leasingAgentEmail',
'leasingAgentName',
'leasingAgentPhone',
'jurisdictions',
'units',
'unitGroups',
One call out is that units and unitGroups are required because otherwise the ValidateUnitsRequired is not done. So a user would have to pass in an empty array to the field that they don't want as only 1 can be used on a jurisdiction
| let jurisdictionA: IdDTO = { id: '' }; | ||
|
|
||
| if (jurisdictionId) { | ||
| jurisdictionA.id = jurisdictionId; | ||
| } else { | ||
| jurisdictionA = await prisma.jurisdictions.create({ | ||
| data: jurisdictionFactory(), | ||
| data: jurisdictionFactory(randomName()), |
There was a problem hiding this comment.
question: Are these randomName necessary? The jurisdictionFactory does the same function if nothing is passed in.
There was a problem hiding this comment.
@ludtkemorgan you are right it should not be necessary but for some reason without passing it explicitly generated names was often not unique and i had lot of database unique constraints errors.
TBH im not sure why (maybe testing env evaluate default values differently?) but i also did nit dived into this problem too deeply since passing randomName() as a param looks like a working workaround.
The other option is to just use faker lib.
There was a problem hiding this comment.
We run into this issue sometimes on the circle-ci and github actions runs of the tests. The problem is the randomName takes from a set list of options in word-generator.ts so duplicates will happen. One option is to add a prefix here to randomName. listing-${randomName()}
There was a problem hiding this comment.
we can also use a name here and just pass along a name that would be unique. We probably should do one of those options to help reduce flakiness in the suite
…y if it doesn't exist
There was a problem hiding this comment.
Thanks for making those changes. I have added the list of default values I'd like to see so assuming those are a updated I can approve this.
Still want @YazeedLoonat to take a review pass before it's ready to merge
| [ListingsStatusEnum.changesRequested]: ListingCreate, | ||
| }; | ||
| // Default required fields if jurisdiction doesn't specify any | ||
| private defaultRequiredFields = [ |
There was a problem hiding this comment.
I think this is the list that I'd recommend for the default:
'listingsBuildingAddress',
'name',
'developer',
'listingImages',
'leasingAgentEmail',
'leasingAgentName',
'leasingAgentPhone',
'jurisdictions',
'units',
'unitGroups',
One call out is that units and unitGroups are required because otherwise the ValidateUnitsRequired is not done. So a user would have to pass in an empty array to the field that they don't want as only 1 can be used on a jurisdiction
| let jurisdictionA: IdDTO = { id: '' }; | ||
|
|
||
| if (jurisdictionId) { | ||
| jurisdictionA.id = jurisdictionId; | ||
| } else { | ||
| jurisdictionA = await prisma.jurisdictions.create({ | ||
| data: jurisdictionFactory(), | ||
| data: jurisdictionFactory(randomName()), |
There was a problem hiding this comment.
We run into this issue sometimes on the circle-ci and github actions runs of the tests. The problem is the randomName takes from a set list of options in word-generator.ts so duplicates will happen. One option is to add a prefix here to randomName. listing-${randomName()}
| @@ -93,6 +93,7 @@ export const stagingSeed = async ( | |||
| FeatureFlagEnum.enableCompanyWebsite, | |||
| FeatureFlagEnum.disableCommonApplication, | |||
| ], | |||
| requiredListingFields: ['name', 'description'], | |||
There was a problem hiding this comment.
nit: "description" isn't a real field for listings, can you change it to "listingsBuildingAddress"
There was a problem hiding this comment.
Bump since it still should get addressed
There was a problem hiding this comment.
I think there is a bug that is allowing a field that is required (name) but has no value to pass the check
to test this I did the following:
stand up api and partners -> go to the partner site -> Add Listing -> hit Publish
we do get an error but that error is happening from the service level and not getting stopped in the dto level for missing a name
Also with just a name we are still failing because a jurisdiction is missing which to me points to a need for us to require the jurisdiction as part of our minimum fields
| -- CreateIndex | ||
|
|
||
| CREATE INDEX "applications_user_id_idx" ON "applications"("user_id"); | ||
| -- CreateIndex (only if it doesn't exist) |
There was a problem hiding this comment.
issue: I don't think you need this file at all, can drop
| @@ -93,6 +93,7 @@ export const stagingSeed = async ( | |||
| FeatureFlagEnum.enableCompanyWebsite, | |||
| FeatureFlagEnum.disableCommonApplication, | |||
| ], | |||
| requiredListingFields: ['name', 'description'], | |||
There was a problem hiding this comment.
Bump since it still should get addressed
| } | ||
|
|
||
| static isPublishing(status?: ListingsStatusEnum | ''): boolean { | ||
| /* |
There was a problem hiding this comment.
nitpick: we use jsdoc comment notation usually on the API
you can peep the script-runner.service.ts to see examples
| let jurisdictionA: IdDTO = { id: '' }; | ||
|
|
||
| if (jurisdictionId) { | ||
| jurisdictionA.id = jurisdictionId; | ||
| } else { | ||
| jurisdictionA = await prisma.jurisdictions.create({ | ||
| data: jurisdictionFactory(), | ||
| data: jurisdictionFactory(randomName()), |
There was a problem hiding this comment.
we can also use a name here and just pass along a name that would be unique. We probably should do one of those options to help reduce flakiness in the suite
This PR addresses #4630
Description
This PR implements dynamic required listing fields functionality that allows jurisdictions to configure their own set of required fields for listing publication, addressing the need for different validation requirements across jurisdictions.
Key changes:
requiredFieldscolumn tojurisdictionstable via migrationListingCreateUpdateValidationPipethat dynamically fetches jurisdiction-specific required fieldsrequiredFieldsproperty to listing DTOs with proper validation decoratorsrequiredFieldsmetadata before database persistence@ValidateListingPublishdecorator to handle dynamic field requirementsHow Can This Be Tested/Reviewed?
Local Testing:
yarn generate:clientand apply migrationsyarn testyarn test:e2e ./listing.e2e-spec.tsto test full validation flowsAuthor Checklist:
yarn generate:clientand/or created a migration when requiredReview Process: