Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved TypeScript compilation blockers and missing API metadata and test coverage remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds computed transferValue to registration views and the API, calculated as paymentAmountMultiplier × fixedTransferValue.
Changes:
- Adds the database view column and migration.
- Exposes filtering, sorting, enum, and Swagger support.
- Updates tests and expected response attributes.
File summaries
| File | Reviewed change | Final review notes |
|---|---|---|
services/121-service/test/registrations/pagination/pagination-data.ts |
Updates expected response attributes. | No final comments. |
services/121-service/test/registrations/calculate-payment-amount-multiplier.test.ts |
Tests the calculation. | No final comments. |
services/121-service/swagger.json |
Documents filter parameters. | No final comments. |
services/121-service/src/registration/enum/registration-attribute.enum.ts |
Adds the new attribute. | Critical (1 vote): Add the missing portal ATTRIBUTE_LABELS entry to avoid a TypeScript build failure. |
services/121-service/src/registration/entities/registration-view.entity.ts |
Defines the computed column. | Critical (3 votes): Update registrationViewAttributeNamesRecord to avoid a service compilation failure. |
services/121-service/src/registration/const/filter-operation.const.ts |
Enables filtering and sorting. | Moderate (1 vote): Update advertised program filter metadata. Nit (1 vote): Add filter and sort test coverage. |
services/121-service/src/migration/1789485881583-add-transfer-value-to-registration-view.ts |
Recreates the database view. | No final comments. |
Review details
Suppressed comments (2)
services/121-service/src/registration/const/filter-operation.const.ts:58
- This makes
transferValuefilterable at the registrations endpoint, but the program API's advertisedfilterableAttributesis built from the hard-codedgenericPaAttributeFiltersinProgramRegistrationAttributesService, which is not updated here. Clients that discover filters from/programs/:programIdtherefore will not see this new filter; add it to that metadata list (and cover it) or remove the filter exposure.
transferValue: AllowedFiltersNumber,
services/121-service/src/registration/const/filter-operation.const.ts:38
- The new sortable/filterable API paths are not covered by the added test, which only checks the default response field. The pagination suite already tests the analogous
paymentAmountMultiplierbehavior; addtransferValuefilter and sort cases so this newly exposed API contract is verified.
'transferValue',
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
0042c76 to
12aacbe
Compare
12aacbe to
1d048fc
Compare
1d048fc to
ccb5718
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Add transferValue as a read-only portal generic attribute so the registrations table displays the computed column.
Review details
Suppressed comments (2)
interfaces/portal/src/app/domains/program/program-attribute.helpers.ts:10
- This label is never enough to create the new table column:
createPaAttributeColumnsonly emits fields found inregistrationAttributes, butgetGenericAttributesdoes not addtransferValueto that list. As a result, the backend exposes the filter and value while the portal silently omits the Transfer value column. Add it as a read-only generic attribute (and keep it out of the update form) so the computed value can actually be displayed.
transferValue: $localize`:@@attribute-label-transferValue:Transfer value`,
services/121-service/src/program-registration-attributes/program-registration-attributes.service.ts:47
- Adding
transferValuehere makes the backend advertise apaAttributesfilter to the portal, butRegistrationAttributeService.getGenericAttributes()still omitsGenericRegistrationAttributes.transferValue.RegistrationsTableColumnService.createPaAttributeColumns()only creates a column when it finds a matching normalized attribute, so the new field/filter is silently absent from the registrations table. Add it to the portal's generic attributes and mark it non-editable because it is computed.
'transferValue',
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
ccb5718 to
866df6e
Compare
|
Re: the "Nit" suggestion on Reasoning:
What still covers this newly-exposed API surface without duplicating existing coverage:
Happy to add the dedicated tests back if there's a scenario I'm missing, but as-is they felt like pure duplication of the |
Add a computed transferValue column (paymentAmountMultiplier * program fixedTransferValue) to the registration view, exposed as a sortable and filterable attribute alongside the existing pagination fields. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
866df6e to
b5d2397
Compare
AB#44658
Describe your changes
Also use this when creating transactionshowever I decided against thisChecklist before requesting a code review
Portal preview-deployment
https://happy-rock-0411d2003-8857.westeurope.3.azurestaticapps.net