From 553c154e468e7fc2387d9b1b282b1791c8f2d4ef Mon Sep 17 00:00:00 2001 From: Patrick Fic Date: Wed, 27 Aug 2025 11:50:00 -0700 Subject: [PATCH] IO-3255 Review comments. --- .../endpoints/lib/extractPartsTaxRates.js | 4 +- .../partsManagementDeprovisioning.js | 2 +- .../endpoints/partsManagementProvisioning.js | 10 +++-- .../endpoints/vehicleDamageEstimateAddRq.js | 43 ++++++++++--------- .../endpoints/vehicleDamageEstimateChgRq.js | 7 ++- 5 files changed, 38 insertions(+), 28 deletions(-) diff --git a/server/integrations/partsManagement/endpoints/lib/extractPartsTaxRates.js b/server/integrations/partsManagement/endpoints/lib/extractPartsTaxRates.js index f5e4cb757..cefe09c58 100644 --- a/server/integrations/partsManagement/endpoints/lib/extractPartsTaxRates.js +++ b/server/integrations/partsManagement/endpoints/lib/extractPartsTaxRates.js @@ -23,6 +23,8 @@ const KNOWN_PART_RATE_TYPES = [ * @returns {object} The parts tax rates object. */ +//PF: Major validation would be required on this - EMS files are inconsistent with things like 5% being passed as 5.0 or .05. +//PF: Is this data being sent by them now? const extractPartsTaxRates = (profile = {}) => { const rateInfos = Array.isArray(profile.RateInfo) ? profile.RateInfo : [profile.RateInfo || {}]; const partsTaxRates = {}; @@ -31,7 +33,7 @@ const extractPartsTaxRates = (profile = {}) => { const rateTypeRaw = typeof r?.RateType === "string" ? r.RateType - : typeof r?.RateType === "object" && r?.RateType._ + : typeof r?.RateType === "object" && r?.RateType._ //PF: what does _ align to? ? r.RateType._ : ""; const rateType = (rateTypeRaw || "").toUpperCase(); diff --git a/server/integrations/partsManagement/endpoints/partsManagementDeprovisioning.js b/server/integrations/partsManagement/endpoints/partsManagementDeprovisioning.js index 122a35d17..69a6c9379 100644 --- a/server/integrations/partsManagement/endpoints/partsManagementDeprovisioning.js +++ b/server/integrations/partsManagement/endpoints/partsManagementDeprovisioning.js @@ -83,7 +83,7 @@ const deleteJobsByIds = async (jobIds) => { */ const partsManagementDeprovisioning = async (req, res) => { const { logger } = req; - const p = req.body; + const p = req.body; //Same as other file, can p be renamed to be more descriptive? if (process.env.NODE_ENV === "production") { return res.status(403).json({ error: "Deprovisioning not allowed in production environment." }); diff --git a/server/integrations/partsManagement/endpoints/partsManagementProvisioning.js b/server/integrations/partsManagement/endpoints/partsManagementProvisioning.js index 4b919978f..3cd8077a9 100644 --- a/server/integrations/partsManagement/endpoints/partsManagementProvisioning.js +++ b/server/integrations/partsManagement/endpoints/partsManagementProvisioning.js @@ -123,7 +123,7 @@ const insertUserAssociation = async (uid, email, shopId) => { authid: uid, validemail: true, associations: { - data: [{ shopid: shopId, authlevel: 80, active: true }] + data: [{ shopid: shopId, authlevel: 99, active: true }] } } }; @@ -140,6 +140,7 @@ const insertUserAssociation = async (uid, email, shopId) => { const partsManagementProvisioning = async (req, res) => { const { logger } = req; const p = { ...req.body, userEmail: req.body.userEmail?.toLowerCase() }; + // Can p be renamed to be more descriptive? try { await ensureEmailNotRegistered(p.userEmail); @@ -191,7 +192,7 @@ const partsManagementProvisioning = async (req, res) => { md_messaging_presets: DefaultNewShop.md_messaging_presets, md_rbac: DefaultNewShop.md_rbac, md_classes: DefaultNewShop.md_classes, - md_ins_cos: DefaultNewShop.md_ins_cos, // TODO need? + md_ins_cos: DefaultNewShop.md_ins_cos, // TODO need? //PF: Can ignore from here down for shop settings. Doesn't hurt to prepopulate. Required for graduation path. md_categories: DefaultNewShop.md_categories, // TODO need? md_labor_rates: DefaultNewShop.md_labor_rates, // TODO need? md_payment_types: DefaultNewShop.md_payment_types, // TODO need? @@ -200,7 +201,7 @@ const partsManagementProvisioning = async (req, res) => { appt_alt_transport: DefaultNewShop.appt_alt_transport, // TODO need? md_jobline_presets: DefaultNewShop.md_jobline_presets, // TODO need? vendors: { - data: p.vendors.map((v) => ({ + data: p.vendors.map((v) => ({ //Many of these will be empty, but good to call out explicitly to self document. name: v.name, street1: v.street1 || null, street2: v.street2 || null, @@ -226,13 +227,14 @@ const partsManagementProvisioning = async (req, res) => { if (!p.userPassword) resetLink = await generateResetLink(p.userEmail); const createdUser = await insertUserAssociation(userRecord.uid, p.userEmail, newShopId); + //Association can be included in shop creation call, but this is also prescriptive and fine. return res.status(200).json({ shop: { id: newShopId, shopname: p.shopname }, user: { id: createdUser.id, email: createdUser.email, - resetLink: resetLink || undefined + resetLink: resetLink || undefined //Does WE know to expect this? } }); } catch (err) { diff --git a/server/integrations/partsManagement/endpoints/vehicleDamageEstimateAddRq.js b/server/integrations/partsManagement/endpoints/vehicleDamageEstimateAddRq.js index cf1c63014..ed7084953 100644 --- a/server/integrations/partsManagement/endpoints/vehicleDamageEstimateAddRq.js +++ b/server/integrations/partsManagement/endpoints/vehicleDamageEstimateAddRq.js @@ -25,7 +25,7 @@ const FALLBACK_DEFAULT_ORDER_STATUS = "Open"; const getDefaultOrderStatus = async (shopId, logger) => { try { const { bodyshop_by_pk } = await client.request(GET_BODYSHOP_STATUS, { id: shopId }); - return bodyshop_by_pk?.md_order_statuses?.default_open || FALLBACK_DEFAULT_ORDER_STATUS; + return bodyshop_by_pk?.md_order_statuses?.default_open || FALLBACK_DEFAULT_ORDER_STATUS; //I think this is intended to be called job status, not order status. } catch (err) { logger.log("parts-bodyshop-fetch-failed", "warn", shopId, null, { error: err }); return FALLBACK_DEFAULT_ORDER_STATUS; @@ -101,14 +101,14 @@ const extractOwnerData = (rq, shopId) => { const personName = personInfo.PersonName || {}; const address = personInfo.Communications?.Address || {}; - let ownr_ph1, ownr_ph2, ownr_ea, ownr_alt_ph; + let ownr_ph1, ownr_ph2, ownr_ea, ownr_alt_ph; const comms = Array.isArray(ownerOrClaimant.ContactInfo?.Communications) ? ownerOrClaimant.ContactInfo.Communications : [ownerOrClaimant.ContactInfo?.Communications || {}]; for (const c of comms) { - if (c.CommQualifier === "CP") ownr_ph1 = c.CommPhone; + if (c.CommQualifier === "CP") ownr_ph1 = c.CommPhone; //PF: Should document this logic. 1 and 2 don't typically indicate type in EMS. This makes sense, but good to document. if (c.CommQualifier === "WP") ownr_ph2 = c.CommPhone; if (c.CommQualifier === "EM") ownr_ea = c.CommEmail; if (c.CommQualifier === "AL") ownr_alt_ph = c.CommPhone; @@ -128,7 +128,7 @@ const extractOwnerData = (rq, shopId) => { ownr_ph1, ownr_ph2, ownr_ea, - ownr_alt_ph + ownr_alt_ph //PF: This is not in the DB, if this object is inserted in place, this will fail. // ownr_id_qualifier: ownerOrClaimant.IDInfo?.IDQualifierCode || null // New // ownr_id_num: ownerOrClaimant.IDInfo?.IDNum || null, // New // ownr_preferred_contact: ownerOrClaimant.PreferredContactMethod || null // New @@ -166,7 +166,7 @@ const extractAdjusterData = (rq) => { : [adjParty.ContactInfo?.Communications || {}]; return { - agt_ct_fn: adjParty.PersonInfo?.PersonName?.FirstName || null, + agt_ct_fn: adjParty.PersonInfo?.PersonName?.FirstName || null, //PF: I dont think we display agt_ct_* fields in app. Have they typically been sending data here? agt_ct_ln: adjParty.PersonInfo?.PersonName?.LastName || null, agt_ct_ph: adjComms.find((c) => c.CommQualifier === "CP")?.CommPhone || null, agt_ea: adjComms.find((c) => c.CommQualifier === "EM")?.CommEmail || null @@ -185,7 +185,8 @@ const extractRepairFacilityData = (rq) => { : [rfParty.ContactInfo?.Communications || {}]; return { - servicing_dealer: rfParty.OrgInfo?.CompanyName || null, + servicing_dealer: rfParty.OrgInfo?.CompanyName || null, //PF: The servicing dealer fields are a relic from synergy for a few folks + //PF: I suspect RF data could be ignored since they are the RF. servicing_dealer_contact: rfComms.find((c) => c.CommQualifier === "WP" || c.CommQualifier === "FX")?.CommPhone || null }; @@ -203,10 +204,10 @@ const extractLossInfo = (rq) => { loss_date: loss.LossDateTime || null, loss_type: custom.LossTypeCode || null, loss_desc: custom.LossTypeDesc || null - // primary_poi: loss.PrimaryPOI?.POICode || null, + // primary_poi: loss.PrimaryPOI?.POICode || null, //PF: These map back to area_of_impact.impact_# // secondary_poi: loss.SecondaryPOI?.POICode || null, // damage_memo: loss.DamageMemo || null, //(maybe ins_memo) - // total_loss_ind: rq.ClaimInfo?.LossInfo?.TotalLossInd || null // New + // total_loss_ind: rq.ClaimInfo?.LossInfo?.TotalLossInd || null // New //PF: tlosind i believe is our field. }; }; @@ -288,9 +289,9 @@ const extractVehicleData = (rq, shopId) => { v_color: exterior.Color?.ColorName || null, v_bstyle: desc.BodyStyle || null, v_engine: desc.EngineDesc || null, - v_options: desc.SubModelDesc || null, + v_options: desc.SubModelDesc || null, //PF: Need to confirm with exact data, but this is typically a list of options. Not used AFAIK. v_type: desc.FuelType || null, - v_cond: rq.VehicleInfo?.Condition?.DrivableInd, + v_cond: rq.VehicleInfo?.Condition?.DrivableInd, //PF: there is a separate driveable flag on the job. v_trimcode: desc.TrimCode || null, v_tone: exterior.Tone || null, v_stage: exterior.RefinishStage || rq.VehicleInfo?.Paint?.RefinishStage || null, @@ -342,7 +343,7 @@ const extractJobLines = (rq) => { line.ManualLineInd === true || line.ManualLineInd === 1 || line.ManualLineInd === "1" || - (typeof line.ManualLineInd === "string" && line.ManualLineInd.toUpperCase() === "Y"); + (typeof line.ManualLineInd === "string" && line.ManualLineInd.toUpperCase() === "Y"); //PF: manual line tracks manual in IO or not, this woudl presumably always be false } else { lineOut.manual_line = null; } @@ -355,8 +356,8 @@ const extractJobLines = (rq) => { const price = parseFloat(partInfo.PartPrice || partInfo.ListPrice || 0); lineOut.part_type = partInfo.PartType || null ? String(partInfo.PartType).toUpperCase() : null; lineOut.part_qty = parseFloat(partInfo.Quantity || 0) || 1; - lineOut.oem_partno = partInfo.OEMPartNum || partInfo.PartNum || null; - lineOut.db_price = isNaN(price) ? 0 : price; + lineOut.oem_partno = partInfo.OEMPartNum || partInfo.PartNum || null; //PF: if aftermarket part, we have alt_part_no to capture. + lineOut.db_price = isNaN(price) ? 0 : price; //PF: the Db and act price often are different. These should map back to their EMS equivalents. lineOut.act_price = isNaN(price) ? 0 : price; // Tax flag from PartInfo.TaxableInd when provided @@ -373,7 +374,7 @@ const extractJobLines = (rq) => { (typeof partInfo.TaxableInd === "string" && partInfo.TaxableInd.toUpperCase() === "Y"); } } else if (hasSublet) { - const amt = parseFloat(subletInfo.SubletAmount || 0); + const amt = parseFloat(subletInfo.SubletAmount || 0); //PF: Some nuance here. Usually a part and sublet amount shouldnt be on the same line, but they theoretically could. May require additional discussion. lineOut.part_type = "PAS"; // Sublet as parts-as-service lineOut.part_qty = 1; lineOut.act_price = isNaN(amt) ? 0 : amt; @@ -389,10 +390,12 @@ const extractJobLines = (rq) => { if (hasLabor) { lineOut.mod_lbr_ty = laborInfo.LaborType || null; lineOut.mod_lb_hrs = isNaN(hrs) ? 0 : hrs; - lineOut.lbr_op = laborInfo.LaborOperation || null; + lineOut.lbr_op = laborInfo.LaborOperation || null; //PF: can add lbr_op_desc according to mapping available in new partner. lineOut.lbr_amt = isNaN(amt) ? 0 : amt; } + //PF: what's the BMS logic for this? Body and refinish operations can often happen to the same part, but most systems output a second line for the refinish labor. + //PF: 2nd line may include a duplicate of the part price, but that can be removed. This is the case for CCC. // Refinish labor (if present) recorded on the same line using secondary labor fields const rHrs = parseFloat(refinishInfo.LaborHours || 0); const rAmt = parseFloat(refinishInfo.LaborAmt || 0); @@ -403,9 +406,9 @@ const extractJobLines = (rq) => { !isNaN(rAmt) || !!refinishInfo.LaborOperation); if (hasRefinish) { - lineOut.lbr_typ_j = refinishInfo.LaborType || "LAR"; - lineOut.lbr_hrs_j = isNaN(rHrs) ? 0 : rHrs; - lineOut.lbr_op_j = refinishInfo.LaborOperation || null; + lineOut.lbr_typ_j = refinishInfo.LaborType || "LAR"; //PF: _j fields indicate judgement, and are bool type. + lineOut.lbr_hrs_j = isNaN(rHrs) ? 0 : rHrs;//PF: _j fields indicate judgement, and are bool type. + lineOut.lbr_op_j = refinishInfo.LaborOperation || null; //PF: _j fields indicate judgement, and are bool type. // Aggregate refinish labor amount into the total labor amount for the line if (!isNaN(rAmt)) { lineOut.lbr_amt = (Number.isFinite(lineOut.lbr_amt) ? lineOut.lbr_amt : 0) + rAmt; @@ -474,7 +477,7 @@ const computeLinesTotal = (joblines = []) => { labor += jl.lbr_amt; } } - const total = parts + labor; + const total = parts + labor; //PF: clm_total is the 100% full amount of the repair including deductible, betterment and taxes. Typically provided by the source system. return Number.isFinite(total) && total > 0 ? total : 0; }; @@ -523,7 +526,7 @@ const vehicleDamageEstimateAddRq = async (req, res) => { } // Get default status - const defaultStatus = await getDefaultOrderStatus(shopId, logger); + const defaultStatus = await getDefaultOrderStatus(shopId, logger); //This likely should be get default job status, not order. // Extract additional data const parts_tax_rates = extractPartsTaxRates(rq.ProfileInfo); diff --git a/server/integrations/partsManagement/endpoints/vehicleDamageEstimateChgRq.js b/server/integrations/partsManagement/endpoints/vehicleDamageEstimateChgRq.js index c174e3a62..62acac4dd 100644 --- a/server/integrations/partsManagement/endpoints/vehicleDamageEstimateChgRq.js +++ b/server/integrations/partsManagement/endpoints/vehicleDamageEstimateChgRq.js @@ -38,7 +38,7 @@ const findJob = async (shopId, jobId, logger) => { const extractUpdatedJobData = (rq) => { const doc = rq.DocumentInfo || {}; const claim = rq.ClaimInfo || {}; - + //PF: In the full BMS world, much more can change. const policyNo = claim.PolicyInfo?.PolicyInfo?.PolicyNum || claim.PolicyInfo?.PolicyNum || null; const out = { @@ -239,6 +239,7 @@ const partsManagementVehicleDamageEstimateChgRq = async (req, res) => { } } // --- End fetch current notes --- + //PF: These are several different calls that can be pulled together to make the operation faster. const updatedJobData = extractUpdatedJobData(rq); const updatedLines = extractUpdatedJobLines(rq.AddsChgs, job.id, currentJobLineNotes); @@ -246,10 +247,12 @@ const partsManagementVehicleDamageEstimateChgRq = async (req, res) => { await client.request(UPDATE_JOB_BY_ID, { id: job.id, job: updatedJobData }); + //PF: for changed lines, are they deleted and then reinserted? + //PF: Updated lines should get an upsert to update things like desc, price, etc. if (deletedLineIds?.length || updatedSeqs?.length) { const allToDelete = Array.from(new Set([...(deletedLineIds || []), ...(updatedSeqs || [])])); if (allToDelete.length) { - await client.request(SOFT_DELETE_JOBLINES_BY_IDS, { jobid: job.id, unqSeqs: allToDelete }); + await client.request(SOFT_DELETE_JOBLINES_BY_IDS, { jobid: job.id, unqSeqs: allToDelete }); //PF: appears to soft delete updated lines as well. } }