Skip to content

[TM-2084] Move demographics new controller and support querying #194

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

Conversation

Scriptmatico
Copy link
Collaborator

@Scriptmatico Scriptmatico requested a review from roguenet June 12, 2025 12:17
where: { id: demographic.demographicalId },
attributes: ["id", "uuid"]
});
const demographicDto = new DemographicDto(demographic, demographicData);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This demographicData isn't the right shape. I'm a little surprised it compiles... it's been tricky getting the type safety guards right on those DTOs. It should look like this (example from the association processor that is also creating this DTO):

const additionalProps = { entityType: this.entityType, entityUuid: this.entityUuid };

I'm also surprised it's not complaining that model could be null. In this case, something like this should be right:

const { demographicalType: entityType, demographicalId } = demographic;
const model = LARAVEL_MODELS[entityType];
if (model == null) {
  this.logger.error('Unknown model type', entityType);
  throw new InternalServerErrorException('Unexpected demographic association type');
}
const entity = await model.findOne({ where: { id: demographicalId }, attributes: ["uuid"] });
if (entity == null) {
  this.logger.error('Demographic parent entity not found', { entityType, id: demographicalId });
  throw new NotFoundException();
}
const additionalProps = { entityType, entityUuid: entity.uuid };
const demographicDto = new DemographicDto(demographic, additionalProps);

@Scriptmatico Scriptmatico requested a review from roguenet June 18, 2025 19:49
@Scriptmatico Scriptmatico merged commit 7deff84 into staging Jun 18, 2025
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants