Conversation
cielbellerose
requested review from
Steph375,
chpy04 and
wavehassman
and removed request for
Steph375,
chpy04 and
wavehassman
wavehassman
left a comment
wavehassman
left a comment
There was a problem hiding this comment.
Great job Kinsey! I'm fine with taking subtotal out of the backend endpoint and calculating it in the service function, but if we're going to do that, I want to take it out everywhere in the frontend and other places. If someone were to go back later, it would be pretty confusing to have it in some places but not others.
| manufacturer = await BillOfMaterialsService.getSingleManufacturerWithQueryArgs(manufacturerName, organization); | ||
| } | ||
|
|
||
| // recalculate subtotal on edits |
There was a problem hiding this comment.
if we're going to take subtotal out of the endpoints in the backend, we should take it out of the frontend hooks too since now it's being passed and then dropped
|
|
||
| const processRowUpdate = async (newRow: BomRow, oldRow: BomRow): Promise<BomRow> => { | ||
| // assemblies are not editable | ||
| if (String(newRow.id).startsWith('assembly')) return newRow; |
There was a problem hiding this comment.
id is already a string in the BomRow type so casting it is unecessary
| renderCell: renderStatusBOM, | ||
| renderCell: (params) => { | ||
| // assemblies are not editable | ||
| if (!params.value || String(params.row.id).startsWith('assembly')) return null; |
| const { mutateAsync: deleteAssemblyMutateAsync, isLoading: deleteAssemblyIsLoading } = useDeleteAssembly(project.wbsNum); | ||
| const { mutateAsync: assignMaterialToAssembly } = useAssignMaterialToAssembly(); | ||
| const { mutateAsync: editMaterial } = useEditMaterialById(project.wbsNum); | ||
| const { data: materialTypes } = useGetAllMaterialTypes(); |
There was a problem hiding this comment.
Need isloading iserror and error on both this and getAllManufacturers
| quantity: material.quantity + (material.unitName ? ' ' + material.unitName : ''), | ||
| quantityRaw: material.quantity !== undefined ? Number(material.quantity) : undefined, | ||
| price: material.price !== undefined ? `$${centsToDollar(material.price)}` : '', | ||
| priceRaw: material.price !== undefined ? material.price / 100 : undefined, |
There was a problem hiding this comment.
quantityRaw and priceRaw feels very very weird, why do we need these?
| if (!perms) throw new AccessDeniedException('create materials'); | ||
|
|
||
| const computedSubtotal = | ||
| price !== undefined && quantity !== undefined ? Math.round(price * Number(quantity)) : undefined; |
There was a problem hiding this comment.
Is this not what Wave was talking about with removing from backend everywhere
There was a problem hiding this comment.
she mentioned not passing it to the backend and just calculating it in the service function. it is also still in MaterialFormView in the frontend but thats bc the modal shows a preview of what the subtotal will be before submitting
| manufacturer = await BillOfMaterialsService.getSingleManufacturerWithQueryArgs(manufacturerName, organization); | ||
| } | ||
|
|
||
| // recalculate subtotal on edits |
3 participants
Changes
BOM inline editing
Notes
Removed the 'None' special handling for PDM file name to make inline editing more intuitive (None vs empty it was just a bit odd to handle inline)
Test Cases
Screenshots
Untitled.mov
Checklist
It can be helpful to check the
ChecksandFiles changedtabs.Please review the contributor guide and reach out to your Tech Lead if anything is unclear.
Please request reviewers and ping on slack only after you've gone through this whole checklist.
yarn.lockchanges (unless dependencies have changed)Closes #4107